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