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

["Nelson, Shannon" <shannon.nelson@amd.com>]

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