netdev - Re: [PATCH net-next 5/6] bnxt_en: add .get_module_eeprom_by

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YzQVDXDTRnM/Oz4z@shredder>
Date:   Wed, 28 Sep 2022 12:34:05 +0300
From:   Ido Schimmel <idosch@...sch.org>
To:     Michael Chan <michael.chan@...adcom.com>
Cc:     davem@...emloft.net, netdev@...r.kernel.org, kuba@...nel.org,
        edumazet@...gle.com, pabeni@...hat.com, gospo@...adcom.com,
        vikas.gupta@...adcom.com
Subject: Re: [PATCH net-next 5/6] bnxt_en: add .get_module_eeprom_by_page()
 support

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
>