Add support for the newer get_module_eeprom_by_page interface.
Only the upper half of the 256 byte page is available for
reading, and the firmware puts the two sections into the
extended sprom buffer, so a union is used over the extended
sprom buffer to make clear which page is to be accessed.
Reviewed-by: Brett Creeley <brett.creeley@amd.com>
Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
---
.../ethernet/pensando/ionic/ionic_ethtool.c | 50 +++++++++++++++++++
.../net/ethernet/pensando/ionic/ionic_if.h | 12 ++++-
2 files changed, 60 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
index 66f172e28f8b..25dca4b36bcf 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
@@ -1052,6 +1052,55 @@ static int ionic_get_module_eeprom(struct net_device *netdev,
return err;
}
+static int ionic_get_module_eeprom_by_page(struct net_device *netdev,
+ const struct ethtool_module_eeprom *page_data,
+ struct netlink_ext_ack *extack)
+{
+ struct ionic_lif *lif = netdev_priv(netdev);
+ struct ionic_dev *idev = &lif->ionic->idev;
+ u32 err = -EINVAL;
+ u8 *src;
+
+ if (!page_data->length)
+ return -EINVAL;
+
+ if (page_data->bank != 0) {
+ NL_SET_ERR_MSG_MOD(extack, "Only bank 0 is supported");
+ return -EINVAL;
+ }
+
+ if (page_data->offset < 128 && page_data->page != 0) {
+ NL_SET_ERR_MSG_MOD(extack, "High side only for pages other than 0");
+ return -EINVAL;
+ }
+
+ if ((page_data->length + page_data->offset) > 256) {
+ NL_SET_ERR_MSG_MOD(extack, "Read past the end of the page");
+ return -EINVAL;
+ }
+
+ switch (page_data->page) {
+ case 0:
+ src = &idev->port_info->status.xcvr.sprom[page_data->offset];
+ break;
+ case 1:
+ src = &idev->port_info->sprom_page1[page_data->offset - 128];
+ break;
+ case 2:
+ src = &idev->port_info->sprom_page2[page_data->offset - 128];
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ memset(page_data->data, 0, page_data->length);
+ err = ionic_do_module_copy(page_data->data, src, page_data->length);
+ if (err)
+ return err;
+
+ return page_data->length;
+}
+
static int ionic_get_ts_info(struct net_device *netdev,
struct kernel_ethtool_ts_info *info)
{
@@ -1199,6 +1248,7 @@ static const struct ethtool_ops ionic_ethtool_ops = {
.set_tunable = ionic_set_tunable,
.get_module_info = ionic_get_module_info,
.get_module_eeprom = ionic_get_module_eeprom,
+ .get_module_eeprom_by_page = ionic_get_module_eeprom_by_page,
.get_pauseparam = ionic_get_pauseparam,
.set_pauseparam = ionic_set_pauseparam,
.get_fecparam = ionic_get_fecparam,
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_if.h b/drivers/net/ethernet/pensando/ionic/ionic_if.h
index 4943ebb27ab3..23218208b711 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_if.h
+++ b/drivers/net/ethernet/pensando/ionic/ionic_if.h
@@ -2839,7 +2839,9 @@ union ionic_port_identity {
* @status: Port status data
* @stats: Port statistics data
* @mgmt_stats: Port management statistics data
- * @sprom_epage: Extended Transceiver sprom, high page 1 and 2
+ * @sprom_epage: Extended Transceiver sprom
+ * @sprom_page1: Extended Transceiver sprom, page 1
+ * @sprom_page2: Extended Transceiver sprom, page 2
* @rsvd: reserved byte(s)
* @pb_stats: uplink pb drop stats
*/
@@ -2850,7 +2852,13 @@ struct ionic_port_info {
struct ionic_port_stats stats;
struct ionic_mgmt_port_stats mgmt_stats;
};
- u8 sprom_epage[256];
+ union {
+ u8 sprom_epage[256];
+ struct {
+ u8 sprom_page1[128];
+ u8 sprom_page2[128];
+ };
+ };
u8 rsvd[504];
/* pb_stats must start at 2k offset */
--
2.17.1On Fri, Apr 11, 2025 at 11:21:39AM -0700, Shannon Nelson wrote:
> Add support for the newer get_module_eeprom_by_page interface.
> Only the upper half of the 256 byte page is available for
> reading, and the firmware puts the two sections into the
> extended sprom buffer, so a union is used over the extended
> sprom buffer to make clear which page is to be accessed.
>
> Reviewed-by: Brett Creeley <brett.creeley@amd.com>
> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
> ---
> .../ethernet/pensando/ionic/ionic_ethtool.c | 50 +++++++++++++++++++
> .../net/ethernet/pensando/ionic/ionic_if.h | 12 ++++-
> 2 files changed, 60 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
> index 66f172e28f8b..25dca4b36bcf 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
> @@ -1052,6 +1052,55 @@ static int ionic_get_module_eeprom(struct net_device *netdev,
> return err;
> }
>
> +static int ionic_get_module_eeprom_by_page(struct net_device *netdev,
> + const struct ethtool_module_eeprom *page_data,
> + struct netlink_ext_ack *extack)
> +{
> + struct ionic_lif *lif = netdev_priv(netdev);
> + struct ionic_dev *idev = &lif->ionic->idev;
> + u32 err = -EINVAL;
> + u8 *src;
> +
> + if (!page_data->length)
> + return -EINVAL;
> +
> + if (page_data->bank != 0) {
> + NL_SET_ERR_MSG_MOD(extack, "Only bank 0 is supported");
> + return -EINVAL;
> + }
> +
> + if (page_data->offset < 128 && page_data->page != 0) {
> + NL_SET_ERR_MSG_MOD(extack, "High side only for pages other than 0");
> + return -EINVAL;
> + }
This is in the core already.
> +
> + if ((page_data->length + page_data->offset) > 256) {
> + NL_SET_ERR_MSG_MOD(extack, "Read past the end of the page");
> + return -EINVAL;
> + }
Also in the core.
> +
> + switch (page_data->page) {
> + case 0:
> + src = &idev->port_info->status.xcvr.sprom[page_data->offset];
> + break;
> + case 1:
> + src = &idev->port_info->sprom_page1[page_data->offset - 128];
> + break;
> + case 2:
> + src = &idev->port_info->sprom_page2[page_data->offset - 128];
> + break;
> + default:
> + return -EINVAL;
It is a valid page, your firmware just does not support it. EOPNOSUPP.
> + }
> +
> + memset(page_data->data, 0, page_data->length);
> + err = ionic_do_module_copy(page_data->data, src, page_data->length);
> + if (err)
> + return err;
> +
> + return page_data->length;
> +}
> +
> static int ionic_get_ts_info(struct net_device *netdev,
> struct kernel_ethtool_ts_info *info)
> {
> @@ -1199,6 +1248,7 @@ static const struct ethtool_ops ionic_ethtool_ops = {
> .set_tunable = ionic_set_tunable,
> .get_module_info = ionic_get_module_info,
> .get_module_eeprom = ionic_get_module_eeprom,
> + .get_module_eeprom_by_page = ionic_get_module_eeprom_by_page,
If i remember correctly, implementing .get_module_eeprom_by_page make
.get_module_info and .get_module_eeprom pointless, they will never be
used, so you can delete them.
Andrew
On 4/13/2025 1:58 PM, Andrew Lunn wrote:
>
> On Fri, Apr 11, 2025 at 11:21:39AM -0700, Shannon Nelson wrote:
>> Add support for the newer get_module_eeprom_by_page interface.
>> Only the upper half of the 256 byte page is available for
>> reading, and the firmware puts the two sections into the
>> extended sprom buffer, so a union is used over the extended
>> sprom buffer to make clear which page is to be accessed.
>>
>> Reviewed-by: Brett Creeley <brett.creeley@amd.com>
>> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
>> ---
>> .../ethernet/pensando/ionic/ionic_ethtool.c | 50 +++++++++++++++++++
>> .../net/ethernet/pensando/ionic/ionic_if.h | 12 ++++-
>> 2 files changed, 60 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
>> index 66f172e28f8b..25dca4b36bcf 100644
>> --- a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
>> @@ -1052,6 +1052,55 @@ static int ionic_get_module_eeprom(struct net_device *netdev,
>> return err;
>> }
>>
>> +static int ionic_get_module_eeprom_by_page(struct net_device *netdev,
>> + const struct ethtool_module_eeprom *page_data,
>> + struct netlink_ext_ack *extack)
>> +{
>> + struct ionic_lif *lif = netdev_priv(netdev);
>> + struct ionic_dev *idev = &lif->ionic->idev;
>> + u32 err = -EINVAL;
>> + u8 *src;
>> +
>> + if (!page_data->length)
>> + return -EINVAL;
>> +
>> + if (page_data->bank != 0) {
>> + NL_SET_ERR_MSG_MOD(extack, "Only bank 0 is supported");
>> + return -EINVAL;
>> + }
>> +
>> + if (page_data->offset < 128 && page_data->page != 0) {
>> + NL_SET_ERR_MSG_MOD(extack, "High side only for pages other than 0");
>> + return -EINVAL;
>> + }
>
> This is in the core already.
Will remove
>
>
>> +
>> + if ((page_data->length + page_data->offset) > 256) {
>> + NL_SET_ERR_MSG_MOD(extack, "Read past the end of the page");
>> + return -EINVAL;
>> + }
>
> Also in the core.
Will remove
>
>> +
>> + switch (page_data->page) {
>> + case 0:
>> + src = &idev->port_info->status.xcvr.sprom[page_data->offset];
>> + break;
>> + case 1:
>> + src = &idev->port_info->sprom_page1[page_data->offset - 128];
>> + break;
>> + case 2:
>> + src = &idev->port_info->sprom_page2[page_data->offset - 128];
>> + break;
>> + default:
>> + return -EINVAL;
>
> It is a valid page, your firmware just does not support it. EOPNOSUPP.
I can see the argument for this, but EINVAL seems to me to match the
out-of-bounds case from ionic_get_module_eprom(), as well as what other
drivers are reporting for an unsupported address. It seems to me that
passing EOPNOSUPP back to the user is telling them that they can't get
eeprom data at all, not that they asked for the wrong page.
>
>
>> + }
>> +
>> + memset(page_data->data, 0, page_data->length);
>> + err = ionic_do_module_copy(page_data->data, src, page_data->length);
>> + if (err)
>> + return err;
>> +
>> + return page_data->length;
>> +}
>> +
>> static int ionic_get_ts_info(struct net_device *netdev,
>> struct kernel_ethtool_ts_info *info)
>> {
>> @@ -1199,6 +1248,7 @@ static const struct ethtool_ops ionic_ethtool_ops = {
>> .set_tunable = ionic_set_tunable,
>> .get_module_info = ionic_get_module_info,
>> .get_module_eeprom = ionic_get_module_eeprom,
>> + .get_module_eeprom_by_page = ionic_get_module_eeprom_by_page,
>
> If i remember correctly, implementing .get_module_eeprom_by_page make
> .get_module_info and .get_module_eeprom pointless, they will never be
> used, so you can delete them.
If .get_module_eeprom_by_page() returns EOPNOSUPP then the
eeprom_prepare_data() code tries the fallback, which is to use these if
they are available.
Following the mlx and bnxt examples I left them in, but I can see how
this is essentially duplicated and unnecessary code.
Thanks,
sln
>
> Andrew
> > > +static int ionic_get_module_eeprom_by_page(struct net_device *netdev,
> > > + const struct ethtool_module_eeprom *page_data,
> > > + struct netlink_ext_ack *extack)
> > > + switch (page_data->page) {
> > > + case 0:
> > > + src = &idev->port_info->status.xcvr.sprom[page_data->offset];
> > > + break;
> > > + case 1:
> > > + src = &idev->port_info->sprom_page1[page_data->offset - 128];
> > > + break;
> > > + case 2:
> > > + src = &idev->port_info->sprom_page2[page_data->offset - 128];
> > > + break;
> > > + default:
> > > + return -EINVAL;
> >
> > It is a valid page, your firmware just does not support it. EOPNOSUPP.
>
> I can see the argument for this, but EINVAL seems to me to match the
> out-of-bounds case from ionic_get_module_eprom(), as well as what other
> drivers are reporting for an unsupported address. It seems to me that
> passing EOPNOSUPP back to the user is telling them that they can't get
> eeprom data at all, not that they asked for the wrong page.
I would disagree with at. Look at the ethtool usage:
ethtool -m|--dump-module-eeprom|--module-info devname [raw on|off]
[hex on|off] [offset N] [length N] [page N] [bank N] [i2c N]
You can ask for any page. The only validation that can be done is,
does the page number fit within the page selection register. And that
is a u8. So any value < 256 is valid for page. Some pages are
currently defined, some pages are reserved, and pages 128-255 are for
vendor specific functions.
The limitation here is your firmware, you don't support what the
specification allows. So EOPNOTSUPP for a page you don't supports
would give an indication of this.
ethtool's pretty print should handle -EOPNOTSUPP. It knows some netdev
have limits, and don't give full access to the module data. I would
not be too surprised to find ethtool actually interprets EINVAL for a
valid page to be fatal, but i've not checked. EOPNOTSUPP should just
stop it pretty printing that section of the module.
Andrew
On 4/14/2025 3:18 PM, Andrew Lunn wrote:
>
>>>> +static int ionic_get_module_eeprom_by_page(struct net_device *netdev,
>>>> + const struct ethtool_module_eeprom *page_data,
>>>> + struct netlink_ext_ack *extack)
>
>>>> + switch (page_data->page) {
>>>> + case 0:
>>>> + src = &idev->port_info->status.xcvr.sprom[page_data->offset];
>>>> + break;
>>>> + case 1:
>>>> + src = &idev->port_info->sprom_page1[page_data->offset - 128];
>>>> + break;
>>>> + case 2:
>>>> + src = &idev->port_info->sprom_page2[page_data->offset - 128];
>>>> + break;
>>>> + default:
>>>> + return -EINVAL;
>>>
>>> It is a valid page, your firmware just does not support it. EOPNOSUPP.
>>
>> I can see the argument for this, but EINVAL seems to me to match the
>> out-of-bounds case from ionic_get_module_eprom(), as well as what other
>> drivers are reporting for an unsupported address. It seems to me that
>> passing EOPNOSUPP back to the user is telling them that they can't get
>> eeprom data at all, not that they asked for the wrong page.
>
> I would disagree with at. Look at the ethtool usage:
>
> ethtool -m|--dump-module-eeprom|--module-info devname [raw on|off]
> [hex on|off] [offset N] [length N] [page N] [bank N] [i2c N]
>
> You can ask for any page. The only validation that can be done is,
> does the page number fit within the page selection register. And that
> is a u8. So any value < 256 is valid for page. Some pages are
> currently defined, some pages are reserved, and pages 128-255 are for
> vendor specific functions.
>
> The limitation here is your firmware, you don't support what the
> specification allows. So EOPNOTSUPP for a page you don't supports
> would give an indication of this.
>
> ethtool's pretty print should handle -EOPNOTSUPP. It knows some netdev
> have limits, and don't give full access to the module data. I would
> not be too surprised to find ethtool actually interprets EINVAL for a
> valid page to be fatal, but i've not checked. EOPNOTSUPP should just
> stop it pretty printing that section of the module.
>
> Andrew
I still think that EINVAL is the right answer because we are complaining
about an argument value, not an operation, but sure, EOPNOTSUPP is fine.
I'll update the patchset in a day or two.
sln
© 2016 - 2026 Red Hat, Inc.