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