| 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_s3en3Lgjxu6u1ii254vow-0CkHWk0j3jhBzY2MHCcLUw@mail.gmail.com>
Date: Thu, 29 Sep 2022 11:05:15 +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 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
> > +#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 || \
> > + (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.
> > +
> > #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.
>
> 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.
>
> > +
> > + 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.
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