[PATCH net-next 0/3] ionic: support QSFP CMIS

[PATCH net-next 0/3] ionic: support QSFP CMIS

[Shannon Nelson]

This patchset sets up support for additional pages and better
handling of the QSFP CMIS data.

Shannon Nelson (3):
  ionic: extend the QSFP module sprom for more pages
  ionic: support ethtool get_module_eeprom_by_page
  ionic: add module eeprom channel data to ionic_if and ethtool

 .../ethernet/pensando/ionic/ionic_ethtool.c   | 119 +++++++++++++++---
 .../net/ethernet/pensando/ionic/ionic_if.h    |  17 ++-
 2 files changed, 119 insertions(+), 17 deletions(-)

-- 
2.17.1

[PATCH net-next 1/3] ionic: extend the QSFP module sprom for more pages

[Shannon Nelson]

Some QSFP modules have more eeprom to be read by ethtool than
the initial high and low page 0 that is currently available
in the DSC's ionic sprom[] buffer.  Since the current sprom[]
is baked into the middle of an existing API struct, to make
the high end of page 1 and page 2 available a block is carved
from a reserved space of the existing port_info struct and the
ionic_get_module_eeprom() service is taught how to get there.

Newer firmware writes the additional QSFP page info here,
yet this remains backward compatible because older firmware
sets this space to all 0 and older ionic drivers do not use
the reserved space.

Reviewed-by: Brett Creeley <brett.creeley@amd.com>
Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
---
 .../ethernet/pensando/ionic/ionic_ethtool.c   | 66 ++++++++++++++-----
 .../net/ethernet/pensando/ionic/ionic_if.h    |  7 +-
 2 files changed, 56 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
index a2d4336d2766..66f172e28f8b 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
@@ -968,10 +968,13 @@ static int ionic_get_module_info(struct net_device *netdev,
 		break;
 	case SFF8024_ID_QSFP_8436_8636:
 	case SFF8024_ID_QSFP28_8636:
-	case SFF8024_ID_QSFP_PLUS_CMIS:
 		modinfo->type = ETH_MODULE_SFF_8436;
 		modinfo->eeprom_len = ETH_MODULE_SFF_8436_LEN;
 		break;
+	case SFF8024_ID_QSFP_PLUS_CMIS:
+		modinfo->type = ETH_MODULE_SFF_8472;
+		modinfo->eeprom_len = ETH_MODULE_SFF_8472_LEN;
+		break;
 	default:
 		netdev_info(netdev, "unknown xcvr type 0x%02x\n",
 			    xcvr->sprom[0]);
@@ -983,29 +986,20 @@ static int ionic_get_module_info(struct net_device *netdev,
 	return 0;
 }
 
-static int ionic_get_module_eeprom(struct net_device *netdev,
-				   struct ethtool_eeprom *ee,
-				   u8 *data)
+static int ionic_do_module_copy(u8 *dst, u8 *src, u32 len)
 {
-	struct ionic_lif *lif = netdev_priv(netdev);
-	struct ionic_dev *idev = &lif->ionic->idev;
-	struct ionic_xcvr_status *xcvr;
-	char tbuf[sizeof(xcvr->sprom)];
+	char tbuf[sizeof_field(struct ionic_xcvr_status, sprom)];
 	int count = 10;
-	u32 len;
 
 	/* The NIC keeps the module prom up-to-date in the DMA space
 	 * so we can simply copy the module bytes into the data buffer.
 	 */
-	xcvr = &idev->port_info->status.xcvr;
-	len = min_t(u32, sizeof(xcvr->sprom), ee->len);
-
 	do {
-		memcpy(data, &xcvr->sprom[ee->offset], len);
-		memcpy(tbuf, &xcvr->sprom[ee->offset], len);
+		memcpy(dst, src, len);
+		memcpy(tbuf, src, len);
 
 		/* Let's make sure we got a consistent copy */
-		if (!memcmp(data, tbuf, len))
+		if (!memcmp(dst, tbuf, len))
 			break;
 
 	} while (--count);
@@ -1016,6 +1010,48 @@ static int ionic_get_module_eeprom(struct net_device *netdev,
 	return 0;
 }
 
+static int ionic_get_module_eeprom(struct net_device *netdev,
+				   struct ethtool_eeprom *ee,
+				   u8 *data)
+{
+	struct ionic_lif *lif = netdev_priv(netdev);
+	struct ionic_dev *idev = &lif->ionic->idev;
+	u32 start = ee->offset;
+	u32 err = -EINVAL;
+	u32 size = 0;
+	u8 *src;
+
+	if (start < ETH_MODULE_SFF_8079_LEN) {
+		if (start + ee->len > ETH_MODULE_SFF_8079_LEN)
+			size = ETH_MODULE_SFF_8079_LEN - start;
+		else
+			size = ee->len;
+
+		src = &idev->port_info->status.xcvr.sprom[start];
+		err = ionic_do_module_copy(data, src, size);
+		if (err)
+			return err;
+
+		data += size;
+		start += size;
+	}
+
+	if (start >= ETH_MODULE_SFF_8079_LEN &&
+	    start < ETH_MODULE_SFF_8472_LEN) {
+		size = ee->len - size;
+		if (start + size > ETH_MODULE_SFF_8472_LEN)
+			size = ETH_MODULE_SFF_8472_LEN - start;
+
+		start -= ETH_MODULE_SFF_8079_LEN;
+		src = &idev->port_info->sprom_epage[start];
+		err = ionic_do_module_copy(data, src, size);
+		if (err)
+			return err;
+	}
+
+	return err;
+}
+
 static int ionic_get_ts_info(struct net_device *netdev,
 			     struct kernel_ethtool_ts_info *info)
 {
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_if.h b/drivers/net/ethernet/pensando/ionic/ionic_if.h
index 830c8adbfbee..4943ebb27ab3 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_if.h
+++ b/drivers/net/ethernet/pensando/ionic/ionic_if.h
@@ -2839,6 +2839,7 @@ 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
  * @rsvd:            reserved byte(s)
  * @pb_stats:        uplink pb drop stats
  */
@@ -2849,8 +2850,10 @@ struct ionic_port_info {
 		struct ionic_port_stats      stats;
 		struct ionic_mgmt_port_stats mgmt_stats;
 	};
-	/* room for pb_stats to start at 2k offset */
-	u8                          rsvd[760];
+	u8     sprom_epage[256];
+	u8     rsvd[504];
+
+	/* pb_stats must start at 2k offset */
 	struct ionic_port_pb_stats  pb_stats;
 };
 
-- 
2.17.1

[PATCH net-next 2/3] ionic: support ethtool get_module_eeprom_by_page

[Shannon Nelson]

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.1

[PATCH net-next 3/3] ionic: add module eeprom channel data to ionic_if and ethtool

[Shannon Nelson]

Make the CMIS module type's page 17 channel data available for
ethtool to request.  As done previously, carve space for this
data from the port_info reserved space.

In the future, if additional pages are needed, a new firmware
AdminQ command will be added for accessing random pages.

Reviewed-by: Brett Creeley <brett.creeley@amd.com>
Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
---
 drivers/net/ethernet/pensando/ionic/ionic_ethtool.c | 3 +++
 drivers/net/ethernet/pensando/ionic/ionic_if.h      | 6 ++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
index 25dca4b36bcf..66685cfb3571 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
@@ -1089,6 +1089,9 @@ static int ionic_get_module_eeprom_by_page(struct net_device *netdev,
 	case 2:
 		src = &idev->port_info->sprom_page2[page_data->offset - 128];
 		break;
+	case 17:
+		src = &idev->port_info->sprom_page17[page_data->offset - 128];
+		break;
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_if.h b/drivers/net/ethernet/pensando/ionic/ionic_if.h
index 23218208b711..f1ddbe9994a3 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_if.h
+++ b/drivers/net/ethernet/pensando/ionic/ionic_if.h
@@ -2842,6 +2842,7 @@ union ionic_port_identity {
  * @sprom_epage:     Extended Transceiver sprom
  * @sprom_page1:     Extended Transceiver sprom, page 1
  * @sprom_page2:     Extended Transceiver sprom, page 2
+ * @sprom_page17:    Extended Transceiver sprom, page 17
  * @rsvd:            reserved byte(s)
  * @pb_stats:        uplink pb drop stats
  */
@@ -2853,13 +2854,14 @@ struct ionic_port_info {
 		struct ionic_mgmt_port_stats mgmt_stats;
 	};
 	union {
-		u8     sprom_epage[256];
+		u8     sprom_epage[384];
 		struct {
 			u8 sprom_page1[128];
 			u8 sprom_page2[128];
+			u8 sprom_page17[128];
 		};
 	};
-	u8     rsvd[504];
+	u8     rsvd[376];
 
 	/* pb_stats must start at 2k offset */
 	struct ionic_port_pb_stats  pb_stats;
-- 
2.17.1

Re: [PATCH net-next 2/3] ionic: support ethtool get_module_eeprom_by_page

[Andrew Lunn]

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.


> +
> +	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

Re: [PATCH net-next 2/3] ionic: support ethtool get_module_eeprom_by_page

[Nelson, Shannon]

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

Re: [PATCH net-next 2/3] ionic: support ethtool get_module_eeprom_by_page

[Andrew Lunn]

> > > +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

Re: [PATCH net-next 2/3] ionic: support ethtool get_module_eeprom_by_page

[Nelson, Shannon]

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