| 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
| ||
|
Date: Thu, 29 Sep 2022 10:32:14 +0300
From: Ido Schimmel <idosch@...sch.org>
To: Vikas Gupta <vikas.gupta@...adcom.com>
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
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).
>
> > > +
> > > #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
> > >
Powered by blists - more mailing lists