diff mbox series

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

Message ID 1664326724-1415-6-git-send-email-michael.chan@broadcom.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series bnxt_en: Driver updates | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 5 this patch: 5
netdev/cc_maintainers success CCed 5 of 5 maintainers
netdev/build_clang success Errors and warnings before: 2 this patch: 2
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 4 this patch: 4
netdev/checkpatch warning WARNING: line length of 88 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Michael Chan Sept. 28, 2022, 12:58 a.m. UTC 
From: Vikas Gupta <vikas.gupta@broadcom.com>

Add support for .get_module_eeprom_by_page() callback
which enables CMIS for pluggable modules.
In the case that bank select is not enabled by f/w then
all the requests fallback to .get_module_eeprom() callback.

Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
Signed-off-by: Vikas Gupta <vikas.gupta@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.h     |  9 +++
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 55 +++++++++++++++++++
 2 files changed, 64 insertions(+)

Comments

Ido Schimmel Sept. 28, 2022, 9:34 a.m. UTC | #1 
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'.

> +
>  #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;
}

This should allow you to get rid of patch #2.

> +
> +	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.

> +
> +	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.

> +
> +	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
>
Vikas Gupta Sept. 29, 2022, 5:35 a.m. UTC | #2 
Hi Ido,

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
> > +#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
> >
Ido Schimmel Sept. 29, 2022, 7:32 a.m. UTC | #3 
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).

> 
> > > +
> > >  #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
> > >
Vikas Gupta Sept. 30, 2022, 10:05 a.m. UTC | #4 
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
> > > >
>
>
diff mbox series

Patch

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)
+
 #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;
+
+	rc = bnxt_module_status_check(bp);
+	if (rc)
+		return rc;
+
+	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;
+
+	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, bank,
+						      true, page_data->offset,
+						      chunk, data);
+		if (rc)
+			return rc;
+
+		data += chunk;
+		length -= chunk;
+		bytes_copied += chunk;
+		page++;
+	}
+
+	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,