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@xxxxxxx>
> > Signed-off-by: Shannon Nelson <shannon.nelson@xxxxxxx>
> > ---
> >   .../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