Date:   Mon, 3 Oct 2022 14:55:13 +0530
From:   Vikas Gupta <vikas.gupta@...adcom.com>
To:     Ido Schimmel <idosch@...sch.org>
Cc:     Michael Chan <michael.chan@...adcom.com>, davem@...emloft.net,
        netdev@...r.kernel.org, kuba@...nel.org, edumazet@...gle.com,
        pabeni@...hat.com, gospo@...adcom.com
Subject: Re: [PATCH net-next v2 2/3] bnxt_en: add .get_module_eeprom_by_page() support

Hi Ido,

On Mon, Oct 3, 2022 at 12:49 PM Ido Schimmel <idosch@...sch.org> wrote:
>
> On Sun, Oct 02, 2022 at 09:51:10PM +0530, Vikas Gupta wrote:
> > On Sun, Oct 2, 2022 at 9:04 PM Ido Schimmel <idosch@...sch.org> wrote:
> > >
> > > On Sat, Oct 01, 2022 at 02:27:10PM -0400, Michael Chan wrote:
> > > > +static int bnxt_get_module_eeprom_by_page(struct net_device *dev,
> > > > +                                       const struct ethtool_module_eeprom *page_data,
> > > > +                                       struct netlink_ext_ack *extack)
> > > > +{
> > > > +     struct bnxt *bp = netdev_priv(dev);
> > > > +     int rc;
> > > > +
> > > > +     if (bp->link_info.module_status >
> > > > +         PORT_PHY_QCFG_RESP_MODULE_STATUS_WARNINGMSG) {
> > > > +             NL_SET_ERR_MSG_MOD(extack, "Phy status unknown");
> > >
> > > Can you make this more helpful to users? The comment above this check in
> > > bnxt_get_module_info() suggests that it is possible:
> >
> > Do you mean that we should elaborate something like
> > NL_SET_ERR_MSG_MOD(extack, "Module may be not inserted or powered down
> > or 10G Base-T");
>
> Yes, but even then the exact error is unknown and you would need
> something like drgn / kprobes to retrieve the specific module_state for
> debug. You can do something like the following (in a separate function):
>
> if (bp->link_info.module_status <=
>     PORT_PHY_QCFG_RESP_MODULE_STATUS_WARNINGMSG)
>         return 0;
>
> switch (bp->link_info.module_status) {
> case PORT_PHY_QCFG_RESP_MODULE_STATUS_PWRDOWN:
>         NL_SET_ERR_MSG_MOD(extack, "Transceiver module is powering down");
>         break;
> case PORT_PHY_QCFG_RESP_MODULE_STATUS_NOTINSERTED:
>         NL_SET_ERR_MSG_MOD(extack, "Transceiver module not inserted");
>         break;
> case PORT_PHY_QCFG_RESP_MODULE_STATUS_CURRENTFAULT:
>         NL_SET_ERR_MSG_MOD(extack, "... something that explains what this means ...");
>         break;
> default:
>         NL_SET_ERR_MSG_MOD(extack, "Unknown error");
>         break;
> }

We`ll try to provide more appropriate information above.

>
> return -EINVAL;
>
> >
> > >
> > > /* No point in going further if phy status indicates
> > >  * module is not inserted or if it is powered down or
> > >  * if it is of type 10GBase-T
> > >  */
> > > if (bp->link_info.module_status >
> > >         PORT_PHY_QCFG_RESP_MODULE_STATUS_WARNINGMSG)
> > >
> > > > +             return -EIO;
> > > > +     }
> > > > +
> > > > +     if (bp->hwrm_spec_code < 0x10202) {
> > > > +             NL_SET_ERR_MSG_MOD(extack, "Unsupported hwrm spec");
> > >
> > > Likewise. As a user I do not know what "hwrm spec" means... Maybe:
> > >
> > > NL_SET_ERR_MSG_MOD(extack, "Firmware version too old");
> > >
> > >
> > > > +             return -EOPNOTSUPP;
> > > > +     }
> > > > +
> > > > +     if (page_data->bank && !(bp->phy_flags & BNXT_PHY_FL_BANK_SEL)) {
> > > > +             NL_SET_ERR_MSG_MOD(extack, "Firmware not capable for bank selection");
> > > > +             return -EOPNOTSUPP;
> > >
> > > What happens if you have an old firmware that does not support this
> > > functionality and user space tries to dump page 10h from bank 1 of a
> > > CMIS module that supports multiple banks?
> > >
> > > I wanted to say that you would see the wrong information (from bank 0)
> > > because the legacy operations do not support banks and bank 0 is
> > > assumed. However, because only pages 10h-ffh are banked, user space will
> > > get an error from the following check in fallback_set_params():
> > >
> > > if (request->page)
> > >         offset = request->page * ETH_MODULE_EEPROM_PAGE_LEN + offset;
> > >
> > > [...]
> > >
> > > if (offset >= modinfo->eeprom_len)
> > >         return -EINVAL;
> > >
> > > I believe it makes sense to be more explicit about it and return an
> > > error in fallback_set_params() in case bank is not 0. Please follow up
> > > if the above analysis is correct.
> >
> > So older firmware do not understand bank > 0 and hence it returns to
> > EOPNOTSUPP, which goes to fallback_set_params() and fails for
> > if (offset >= modinfo->eeprom_len)
> >         return -EINVAL
> > As we are not setting modinfo->eeprom_len for CMIS modules in get_module_eeprom.
> > With the above said userspace gets EINVAL.
> > Let me know if my understanding is correct?
>
> Yes. Basically there is no point for ethtool to even try to invoke the
> legacy operations when bank is not zero:
>
> diff --git a/net/ethtool/eeprom.c b/net/ethtool/eeprom.c
> index 1c94bb8ea03f..1d6a35c8b6f0 100644
> --- a/net/ethtool/eeprom.c
> +++ b/net/ethtool/eeprom.c
> @@ -60,6 +60,9 @@ static int eeprom_fallback(struct eeprom_req_info *request,
>         u8 *data;
>         int err;
>
> +       if (request->bank)
> +               return -EINVAL;
> +
>         modinfo.cmd = ETHTOOL_GMODULEINFO;
>         err = ethtool_get_module_info_call(dev, &modinfo);
>         if (err < 0)
>
> Not sure how many will actually hit it. I expect drivers supporting
> modules with banked pages to implement the new ethtool operation.

We may return with -EINVAL so it wont fallback but I like your
suggestion for bank check in eeprom_fallback
 if (request->bank)
         return -EINVAL;
seems to be a good idea as the bank parameter is not propagating
further to the drivers.
I believe your new operation means that "drivers need to implement
get_module_eeprom_by_page" if they want to access banked pages. Am I
right?

Thanks,
Vikas

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4206 bytes)

Powered by blists - more mailing lists