[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAHLZf_tE-fUz2wmaA8=GeqfZMev3Br1M6A316ZJL=CB_KdZxew@mail.gmail.com>
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