Re: [PATCH net-next 5/6] bnxt_en: add .get_module_eeprom_by_page() support

[Vikas Gupta <vikas.gupta@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 8702 bytes --]

Hi Ido,

On Thu, Sep 29, 2022 at 1:02 PM Ido Schimmel <idosch@idosch.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@idosch.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
> > > >
>
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4206 bytes --]