| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHLZf_uCFDLNxTD_p-NM-6VKZFQ+wMiVCM7qgj_hWLtvbgwbOw@mail.gmail.com>
Date: Fri, 30 Sep 2022 15:35:39 +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 5/6] bnxt_en: add .get_module_eeprom_by_page() support
Hi Ido,
On Thu, Sep 29, 2022 at 1:02 PM Ido Schimmel <idosch@...sch.org> wrote:
>
> On Thu, Sep 29, 2022 at 11:05:15AM +0530, Vikas Gupta wrote:
> > On Wed, Sep 28, 2022 at 3:04 PM Ido Schimmel <idosch@...sch.org> wrote:
> > >
> > > On Tue, Sep 27, 2022 at 08:58:43PM -0400, Michael Chan wrote:
> > > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> > > > index 0209f7caf490..03b1a0301a46 100644
> > > > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> > > > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> > > > @@ -2207,6 +2207,15 @@ struct bnxt {
> > > > #define SFF_MODULE_ID_QSFP 0xc
> > > > #define SFF_MODULE_ID_QSFP_PLUS 0xd
> > > > #define SFF_MODULE_ID_QSFP28 0x11
> > > > +#define SFF_MODULE_ID_QSFP_DD 0x18
> > > > +#define SFF_MODULE_ID_DSFP 0x1b
>
> Does not seem to be used.
>
> > > > +#define SFF_MODULE_ID_QSFP_PLUS_CMIS 0x1e
> > > > +
> > > > +#define BNXT_SFF_MODULE_BANK_SUPPORTED(module_id) \
> > > > + ((module_id) == SFF_MODULE_ID_QSFP_DD || \
> > > > + (module_id) == SFF_MODULE_ID_QSFP28 || \
>
> Did you mean DSFP here? QSFP28 is SFF-8636, not CMIS.
>
> > > > + (module_id) == SFF_MODULE_ID_QSFP_PLUS_CMIS)
> > >
> > > I suggest dropping this check unless you have a good reason to keep it.
> > > There are other modules out there that implement CMIS (e.g., OSFP) and
> > > given bnxt implements ethtool_ops::get_module_eeprom_by_page, it should
> > > be able to support them without kernel changes.
> > >
> > > See:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=970adfb76095fa719778d70a6b86030d2feb88dd
> > >
> > > The problem there was more severe because the driver returned '-EINVAL'
> > > instead of '-EOPNOTSUPP'.
> > >
> > We want to fallback on get_module_eeprom callback in case modules do
> > not implement CMIS and we pass the bank parameter accordingly to the
> > firmware.
>
> ethtool_ops::get_module_eeprom_by_page has nothing to do with CMIS. It
> is a generic operation to retrieve module EEPROM data based on a "3D
> address": bank, page and offset.
>
> The driving motivation behind it was CMIS modules, but it must be
> implemented in a way that it can retrieve information from modules that
> implement a different management interface such as SFF-8636.
>
> Let's say that tomorrow a user asks to retrieve pages 20h-21h from a
> QSFP module that implements SFF-8636, how will you support it? You can't
> extend the legacy ethtool::get_module_eeprom operation and your current
> implementation of ethtool_ops::get_module_eeprom_by_page has an
> artificial limitation to support only CMIS modules.
>
> By making sure that your implementation is generic as possible you will
> be able to support all possible requests and will not need to
> continuously patch the kernel (and users will not need to continuously
> upgrade).
Thanks for the detailed description. We`ll work on a simpler implementation.
Thanks,
Vikas
>
> >
> > > > +
> > > > #define SFF8636_FLATMEM_OFFSET 0x2
> > > > #define SFF8636_FLATMEM_MASK 0x4
> > > > #define SFF8636_OPT_PAGES_OFFSET 0xc3
> > > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> > > > index 379afa670494..2b18af95aacb 100644
> > > > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> > > > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> > > > @@ -3363,6 +3363,60 @@ static int bnxt_get_module_eeprom(struct net_device *dev,
> > > > return 0;
> > > > }
> > > >
> > > > +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);
> > > > + u16 length = page_data->length;
> > > > + u8 *data = page_data->data;
> > > > + u8 page = page_data->page;
> > > > + u8 bank = page_data->bank;
> > > > + u16 bytes_copied = 0;
> > > > + u8 module_id;
> > > > + int rc;
> > > > +
> > > > + /* Return -EOPNOTSUPP to fallback on .get_module_eeprom */
> > > > + if (!(bp->phy_flags & BNXT_PHY_FL_BANK_SEL))
> > > > + return -EOPNOTSUPP;
> > >
> > > Maybe:
> > >
> > > if (bank && !(bp->phy_flags & BNXT_PHY_FL_BANK_SEL)) {
> > > // extack
> > > return -EINVAL;
> > > }
> >
> > BNXT_PHY_FL_BANK_SEL is a firmware capability to handle CMIS/bank
> > parameters. It does not tell whether the hooked module is CMIS
> > compliant.
> > I think EOPNOTSUPP is an appropriate choice here.
>
> See my comment above. This operation needs to be able to retrieve EEPROM
> data from non-CMIS modules as well.
>
> >
> >
> > >
> > > This should allow you to get rid of patch #2.
> >
> > I am not sure how we can get rid of patch #2. Patch #2 handles non CMIS modules.
> > Can you please elaborate more.
>
> All the complicated parsing performed in patch #2 is already performed
> in ethtool(8) that knows to request the available pages. The kernel
> will first try to retrieve these pages using
> ethtool_ops::get_module_eeprom_by_page and fallback to
> ethtool::get_module_eeprom in case of '-EOPNOTSUPP'.
>
> By adjusting your implementation of
> ethtool_ops::get_module_eeprom_by_page to handle non-CMIS modules you
> will be able to avoid extending the legacy operation with all this
> complex parsing.
>
> >
> > >
> > > > +
> > > > + rc = bnxt_module_status_check(bp);
> > > > + if (rc)
> > > > + return rc;
> > >
> > > You can add extack here. I see that this function always returns
> > > '-EOPNOTSUPP' in case of errors, even when it does not make sense to
> > > fallback to ethtool_ops::get_module_eeprom.
> > Maybe -EIO can be used in one of these cases. I`ll check.
> >
> > >
> > > > +
> > > > + rc = bnxt_read_sfp_module_eeprom_info(bp, I2C_DEV_ADDR_A0, 0, 0, false,
> > > > + 0, 1, &module_id);
> > > > + if (rc)
> > > > + return rc;
> > > > +
> > > > + if (!BNXT_SFF_MODULE_BANK_SUPPORTED(module_id))
> > > > + return -EOPNOTSUPP;
> > >
> > > I believe this hunk can be removed given the first comment.
> > As I mentioned above we need this here to fallback.
>
> No need to fallback, simply avoid making this operation CMIS specific.
>
> >
> > Thanks,
> > Vikas
> > >
> > > > +
> > > > + while (length > 0) {
> > > > + u16 chunk = ETH_MODULE_EEPROM_PAGE_LEN;
> > > > + int rc;
> > > > +
> > > > + /* Do not access more than required */
> > > > + if (length < ETH_MODULE_EEPROM_PAGE_LEN)
> > > > + chunk = length;
> > > > +
> > > > + rc = bnxt_read_sfp_module_eeprom_info(bp,
> > > > + I2C_DEV_ADDR_A0,
> > >
> > > page_data->i2c_address
> > >
> > > > + page, bank,
> > > > + true, page_data->offset,
> > > > + chunk, data);
> > > > + if (rc)
> > >
> > > You can add a meaningful extack here according to the return code.
> > >
> > > > + return rc;
> > > > +
> > > > + data += chunk;
> > > > + length -= chunk;
> > > > + bytes_copied += chunk;
> > > > + page++;
> > > > + }
> > >
> > > I'm not sure why the loop is required? It seems
> > > bnxt_read_sfp_module_eeprom_info() is able to read
> > > 'ETH_MODULE_EEPROM_PAGE_LEN' bytes at once, which is the maximum:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/ethtool/eeprom.c#n234
> > >
> > > > +
> > > > + return bytes_copied;
> > > > +}
> > > > +
> > > > static int bnxt_nway_reset(struct net_device *dev)
> > > > {
> > > > int rc = 0;
> > > > @@ -4172,6 +4226,7 @@ const struct ethtool_ops bnxt_ethtool_ops = {
> > > > .set_eee = bnxt_set_eee,
> > > > .get_module_info = bnxt_get_module_info,
> > > > .get_module_eeprom = bnxt_get_module_eeprom,
> > > > + .get_module_eeprom_by_page = bnxt_get_module_eeprom_by_page,
> > > > .nway_reset = bnxt_nway_reset,
> > > > .set_phys_id = bnxt_set_phys_id,
> > > > .self_test = bnxt_self_test,
> > > > --
> > > > 2.18.1
> > > >
>
>
Download attachment "smime.p7s" of type "application/pkcs7-signature" (4206 bytes)
Powered by blists - more mailing lists