Re: [RFC PATCH net-next 1/5] ethtool: Allow network drivers to dump arbitrary EEPROM data

Re: [RFC PATCH net-next 1/5] ethtool: Allow network drivers to dump arbitrary EEPROM data

[Andrew Lunn]

On Wed, Feb 24, 2021 at 05:41:10PM +0200, Moshe Shemesh wrote:
> From: Vladyslav Tarasiuk <vladyslavt@nvidia.com>
> 
> Define get_module_eeprom_data_by_page() ethtool callback and implement
> netlink infrastructure.
> 
> get_module_eeprom_data_by_page() allows network drivers to dump a part
> of module's EEPROM specified by page and bank numbers along with offset
> and length. It is effectively a netlink replacement for
> get_module_info() and get_module_eeprom() pair, which is needed due to
> emergence of complex non-linear EEPROM layouts.
> 
> Signed-off-by: Vladyslav Tarasiuk <vladyslavt@nvidia.com>
> ---
>  include/linux/ethtool.h              |   5 +
>  include/uapi/linux/ethtool.h         |  25 +++++
>  include/uapi/linux/ethtool_netlink.h |  19 ++++
>  net/ethtool/Makefile                 |   2 +-
>  net/ethtool/eeprom.c                 | 149 +++++++++++++++++++++++++++
>  net/ethtool/netlink.c                |  10 ++
>  net/ethtool/netlink.h                |   2 +
>  7 files changed, 211 insertions(+), 1 deletion(-)
>  create mode 100644 net/ethtool/eeprom.c
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index ec4cd3921c67..6032313fa914 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -410,6 +410,9 @@ struct ethtool_pause_stats {
>   * @get_ethtool_phy_stats: Return extended statistics about the PHY device.
>   *	This is only useful if the device maintains PHY statistics and
>   *	cannot use the standard PHY library helpers.
> + * @get_module_eeprom_data_by_page: Get a region of plug-in module EEPROM data
> + *	from specified page. Updates length to the amount actually read.
> + *	Returns a negative error code or zero.
>   *
>   * All operations are optional (i.e. the function pointer may be set
>   * to %NULL) and callers must take this into account.  Callers must
> @@ -515,6 +518,8 @@ struct ethtool_ops {
>  				   const struct ethtool_tunable *, void *);
>  	int	(*set_phy_tunable)(struct net_device *,
>  				   const struct ethtool_tunable *, const void *);
> +	int	(*get_module_eeprom_data_by_page)(struct net_device *dev,
> +						  struct ethtool_eeprom_data *page);

Hi Moshe, Vladyslav

I think it would be good to pass an extack here, so we can report more
useful errors.

> +/**
> + * struct ethtool_eeprom_data - EEPROM dump from specified page
> + * @offset: Offset within the specified EEPROM page to begin read, in bytes.
> + * @length: Number of bytes to read. On successful return, number of bytes
> + *	actually read.
> + * @page: Page number to read from.
> + * @bank: Page bank number to read from, if applicable by EEPROM spec.
> + * @i2c_address: I2C address of a page. Zero indicates a driver should choose
> + *	by itself.

I don't particularly like the idea of the driver deciding what to
read. User space should really be passing 0x50 or 0x51 for the normal
case. And we need to make it clear what these addresses mean, since
they are often referred to as 0xA0 and 0xA2, due to addresses being
shifted one bit left and a r/w bit added.

I also don't think the in place length should be modified. It would be
better to follow the use semantics of returning a negative value on
error, or a positive value for the length actually
read. ethtool_eeprom_data can then be passed as a const.

> + * @data: Pointer to buffer with EEPROM data of @length size.
> + *
> + * This can be used to manage pages during EEPROM dump in ethtool and pass
> + * required information to the driver.
> + */
> +struct ethtool_eeprom_data {
> +	__u32	offset;
> +	__u32	length;
> +	__u32	page;
> +	__u32	bank;
> +	__u32	i2c_address;
> +	__u8	*data;
> +};
> +
>  /**
>   * struct ethtool_eee - Energy Efficient Ethernet information
>   * @cmd: ETHTOOL_{G,S}EEE
> @@ -1865,6 +1888,8 @@ static inline int ethtool_validate_duplex(__u8 duplex)
>  #define ETH_MODULE_SFF_8636_MAX_LEN     640
>  #define ETH_MODULE_SFF_8436_MAX_LEN     640
>  
> +#define ETH_MODULE_EEPROM_MAX_LEN	640

I'm surprised such a high value is allowed. 128 seems more
appropriate, given the size of 1/2 pages.

> +
>  /* Reset flags */
>  /* The reset() operation must clear the flags for the components which
>   * were actually reset.  On successful return, the flags indicate the
> diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
> index a286635ac9b8..60dd848d0b54 100644
> --- a/include/uapi/linux/ethtool_netlink.h
> +++ b/include/uapi/linux/ethtool_netlink.h
> @@ -42,6 +42,7 @@ enum {
>  	ETHTOOL_MSG_CABLE_TEST_ACT,
>  	ETHTOOL_MSG_CABLE_TEST_TDR_ACT,
>  	ETHTOOL_MSG_TUNNEL_INFO_GET,
> +	ETHTOOL_MSG_EEPROM_DATA_GET,
>  
>  	/* add new constants above here */
>  	__ETHTOOL_MSG_USER_CNT,
> @@ -80,6 +81,7 @@ enum {
>  	ETHTOOL_MSG_CABLE_TEST_NTF,
>  	ETHTOOL_MSG_CABLE_TEST_TDR_NTF,
>  	ETHTOOL_MSG_TUNNEL_INFO_GET_REPLY,
> +	ETHTOOL_MSG_EEPROM_DATA_GET_REPLY,
>  
>  	/* add new constants above here */
>  	__ETHTOOL_MSG_KERNEL_CNT,
> @@ -629,6 +631,23 @@ enum {
>  	ETHTOOL_A_TUNNEL_INFO_MAX = (__ETHTOOL_A_TUNNEL_INFO_CNT - 1)
>  };
>  
> +/* MODULE EEPROM DATA */
> +
> +enum {
> +	ETHTOOL_A_EEPROM_DATA_UNSPEC,
> +	ETHTOOL_A_EEPROM_DATA_HEADER,
> +
> +	ETHTOOL_A_EEPROM_DATA_OFFSET,
> +	ETHTOOL_A_EEPROM_DATA_LENGTH,
> +	ETHTOOL_A_EEPROM_DATA_PAGE,
> +	ETHTOOL_A_EEPROM_DATA_BANK,
> +	ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS,
> +	ETHTOOL_A_EEPROM_DATA,
> +
> +	__ETHTOOL_A_EEPROM_DATA_CNT,
> +	ETHTOOL_A_EEPROM_DATA_MAX = (__ETHTOOL_A_EEPROM_DATA_CNT - 1)
> +};
> +
>  /* generic netlink info */
>  #define ETHTOOL_GENL_NAME "ethtool"
>  #define ETHTOOL_GENL_VERSION 1
> diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile
> index 7a849ff22dad..d604346bc074 100644
> --- a/net/ethtool/Makefile
> +++ b/net/ethtool/Makefile
> @@ -7,4 +7,4 @@ obj-$(CONFIG_ETHTOOL_NETLINK)	+= ethtool_nl.o
>  ethtool_nl-y	:= netlink.o bitset.o strset.o linkinfo.o linkmodes.o \
>  		   linkstate.o debug.o wol.o features.o privflags.o rings.o \
>  		   channels.o coalesce.o pause.o eee.o tsinfo.o cabletest.o \
> -		   tunnels.o
> +		   tunnels.o eeprom.o
> diff --git a/net/ethtool/eeprom.c b/net/ethtool/eeprom.c
> new file mode 100644
> index 000000000000..51a2ed81a273
> --- /dev/null
> +++ b/net/ethtool/eeprom.c
> @@ -0,0 +1,149 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/ethtool.h>
> +#include "netlink.h"
> +#include "common.h"
> +
> +struct eeprom_data_req_info {
> +	struct ethnl_req_info	base;
> +	u32			offset;
> +	u32			length;
> +	u32			page;
> +	u32			bank;
> +	u32			i2c_address;
> +};
> +
> +struct eeprom_data_reply_data {
> +	struct ethnl_reply_data base;
> +	u32			length;
> +	u32			i2c_address;
> +	u8			*data;
> +};
> +
> +#define EEPROM_DATA_REQINFO(__req_base) \
> +	container_of(__req_base, struct eeprom_data_req_info, base)
> +
> +#define EEPROM_DATA_REPDATA(__reply_base) \
> +	container_of(__reply_base, struct eeprom_data_reply_data, base)
> +
> +static int eeprom_data_prepare_data(const struct ethnl_req_info *req_base,
> +				    struct ethnl_reply_data *reply_base,
> +				    struct genl_info *info)
> +{
> +	struct eeprom_data_reply_data *reply = EEPROM_DATA_REPDATA(reply_base);
> +	struct eeprom_data_req_info *request = EEPROM_DATA_REQINFO(req_base);
> +	struct ethtool_eeprom_data page_data = {0};
> +	struct net_device *dev = reply_base->dev;
> +	int err;
> +
> +	if (!dev->ethtool_ops->get_module_eeprom_data_by_page)
> +		return -EOPNOTSUPP;
> +	err = ethnl_ops_begin(dev);
> +	if (err)
> +		return err;
> +
> +	page_data.offset = request->offset;
> +	page_data.length = request->length;
> +	page_data.i2c_address = request->i2c_address;
> +	page_data.page = request->page;
> +	page_data.bank = request->bank;
> +	page_data.data = kmalloc(page_data.length, GFP_KERNEL);
> +	if (!page_data.data)
> +		return -ENOMEM;

Isn't an ethnl_ops_complete(dev); needed here? Maybe postpone the
ethnl_ops_begin(dev) call until after the memory allocation?

> +
> +	err = dev->ethtool_ops->get_module_eeprom_data_by_page(dev, &page_data);
> +	if (err)
> +		goto err_out;
> +
> +	reply->length = page_data.length;
> +	reply->i2c_address = page_data.i2c_address;
> +	reply->data = page_data.data;
> +
> +	ethnl_ops_complete(dev);
> +	return 0;
> +
> +err_out:
> +	kfree(page_data.data);
> +	ethnl_ops_complete(dev);
> +	return err;
> +}
> +
> +static int eeprom_data_parse_request(struct ethnl_req_info *req_info, struct nlattr **tb,
> +				     struct netlink_ext_ack *extack)
> +{
> +	struct eeprom_data_req_info *request = EEPROM_DATA_REQINFO(req_info);
> +
> +	if (!tb[ETHTOOL_A_EEPROM_DATA_OFFSET] ||
> +	    !tb[ETHTOOL_A_EEPROM_DATA_LENGTH] ||
> +	    !tb[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS])
> +		return -EINVAL;
> +
> +	request->length = nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_LENGTH]);
> +	if (request->length > ETH_MODULE_EEPROM_MAX_LEN)
> +		return -EINVAL;
> +
> +	request->offset = nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_OFFSET]);

I think you need to validate that offset + length is not passed the
end of a 1/2 page. There seems to be odd wrap around semantics, so we
probably want to avoid that.


> +	request->i2c_address = nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS]);

Maybe validate the i2c address. Most busses are limited to 7 bit
addresses, but some do allow 10 bits. I think we can probably ignore
10 bit addresses for this use case, so check that address is < 128.

   Andrew

Re: [RFC PATCH net-next 1/5] ethtool: Allow network drivers to dump arbitrary EEPROM data

[Vladyslav Tarasiuk]

On 28-Feb-21 02:45, Andrew Lunn wrote:
> On Wed, Feb 24, 2021 at 05:41:10PM +0200, Moshe Shemesh wrote:
>> From: Vladyslav Tarasiuk <vladyslavt@nvidia.com>
>>
>> Define get_module_eeprom_data_by_page() ethtool callback and implement
>> netlink infrastructure.
>>
>> get_module_eeprom_data_by_page() allows network drivers to dump a part
>> of module's EEPROM specified by page and bank numbers along with offset
>> and length. It is effectively a netlink replacement for
>> get_module_info() and get_module_eeprom() pair, which is needed due to
>> emergence of complex non-linear EEPROM layouts.
>>
>> Signed-off-by: Vladyslav Tarasiuk <vladyslavt@nvidia.com>
>> ---
>>   include/linux/ethtool.h              |   5 +
>>   include/uapi/linux/ethtool.h         |  25 +++++
>>   include/uapi/linux/ethtool_netlink.h |  19 ++++
>>   net/ethtool/Makefile                 |   2 +-
>>   net/ethtool/eeprom.c                 | 149 +++++++++++++++++++++++++++
>>   net/ethtool/netlink.c                |  10 ++
>>   net/ethtool/netlink.h                |   2 +
>>   7 files changed, 211 insertions(+), 1 deletion(-)
>>   create mode 100644 net/ethtool/eeprom.c
>>
>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
>> index ec4cd3921c67..6032313fa914 100644
>> --- a/include/linux/ethtool.h
>> +++ b/include/linux/ethtool.h
>> @@ -410,6 +410,9 @@ struct ethtool_pause_stats {
>>    * @get_ethtool_phy_stats: Return extended statistics about the PHY device.
>>    *	This is only useful if the device maintains PHY statistics and
>>    *	cannot use the standard PHY library helpers.
>> + * @get_module_eeprom_data_by_page: Get a region of plug-in module EEPROM data
>> + *	from specified page. Updates length to the amount actually read.
>> + *	Returns a negative error code or zero.
>>    *
>>    * All operations are optional (i.e. the function pointer may be set
>>    * to %NULL) and callers must take this into account.  Callers must
>> @@ -515,6 +518,8 @@ struct ethtool_ops {
>>   				   const struct ethtool_tunable *, void *);
>>   	int	(*set_phy_tunable)(struct net_device *,
>>   				   const struct ethtool_tunable *, const void *);
>> +	int	(*get_module_eeprom_data_by_page)(struct net_device *dev,
>> +						  struct ethtool_eeprom_data *page);
> Hi Moshe, Vladyslav
>
> I think it would be good to pass an extack here, so we can report more
> useful errors.

Hi Andrew!

Yes, I will add this parameter.

>> +/**
>> + * struct ethtool_eeprom_data - EEPROM dump from specified page
>> + * @offset: Offset within the specified EEPROM page to begin read, in bytes.
>> + * @length: Number of bytes to read. On successful return, number of bytes
>> + *	actually read.
>> + * @page: Page number to read from.
>> + * @bank: Page bank number to read from, if applicable by EEPROM spec.
>> + * @i2c_address: I2C address of a page. Zero indicates a driver should choose
>> + *	by itself.
> I don't particularly like the idea of the driver deciding what to
> read. User space should really be passing 0x50 or 0x51 for the normal
> case. And we need to make it clear what these addresses mean, since
> they are often referred to as 0xA0 and 0xA2, due to addresses being
> shifted one bit left and a r/w bit added.

I was thinking what address should I send from userspace by default?
Should I send 0x50 or 0xA0 and at some point to do the shift?
mlx5 uses 0x50 and 0x51, for example.

> I also don't think the in place length should be modified. It would be
> better to follow the use semantics of returning a negative value on
> error, or a positive value for the length actually
> read. ethtool_eeprom_data can then be passed as a const.
But how would userspace know how much actually was read?
Should I fail the command if only part of a data requested was read?
>> + * @data: Pointer to buffer with EEPROM data of @length size.
>> + *
>> + * This can be used to manage pages during EEPROM dump in ethtool and pass
>> + * required information to the driver.
>> + */
>> +struct ethtool_eeprom_data {
>> +	__u32	offset;
>> +	__u32	length;
>> +	__u32	page;
>> +	__u32	bank;
>> +	__u32	i2c_address;
>> +	__u8	*data;
>> +};
>> +
>>   /**
>>    * struct ethtool_eee - Energy Efficient Ethernet information
>>    * @cmd: ETHTOOL_{G,S}EEE
>> @@ -1865,6 +1888,8 @@ static inline int ethtool_validate_duplex(__u8 duplex)
>>   #define ETH_MODULE_SFF_8636_MAX_LEN     640
>>   #define ETH_MODULE_SFF_8436_MAX_LEN     640
>>   
>> +#define ETH_MODULE_EEPROM_MAX_LEN	640
> I'm surprised such a high value is allowed. 128 seems more
> appropriate, given the size of 1/2 pages.

This is done for backwards compatibility mechanism (last patch in the 
series)
to work properly. I wanted to limit the length to 128 (or 256, maybe), but
currently ethtool supports dumps of 640 bytes at most.
I guess I can add another value like ETH_MODULE_EEPROM_PAGE_MAX_LEN 256 
and if
new ndo is available or page number is specified, use it for check.
If ndo is not implemented, use 640 instead.

What do you think?

>> +
>>   /* Reset flags */
>>   /* The reset() operation must clear the flags for the components which
>>    * were actually reset.  On successful return, the flags indicate the
>> diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
>> index a286635ac9b8..60dd848d0b54 100644
>> --- a/include/uapi/linux/ethtool_netlink.h
>> +++ b/include/uapi/linux/ethtool_netlink.h
>> @@ -42,6 +42,7 @@ enum {
>>   	ETHTOOL_MSG_CABLE_TEST_ACT,
>>   	ETHTOOL_MSG_CABLE_TEST_TDR_ACT,
>>   	ETHTOOL_MSG_TUNNEL_INFO_GET,
>> +	ETHTOOL_MSG_EEPROM_DATA_GET,
>>   
>>   	/* add new constants above here */
>>   	__ETHTOOL_MSG_USER_CNT,
>> @@ -80,6 +81,7 @@ enum {
>>   	ETHTOOL_MSG_CABLE_TEST_NTF,
>>   	ETHTOOL_MSG_CABLE_TEST_TDR_NTF,
>>   	ETHTOOL_MSG_TUNNEL_INFO_GET_REPLY,
>> +	ETHTOOL_MSG_EEPROM_DATA_GET_REPLY,
>>   
>>   	/* add new constants above here */
>>   	__ETHTOOL_MSG_KERNEL_CNT,
>> @@ -629,6 +631,23 @@ enum {
>>   	ETHTOOL_A_TUNNEL_INFO_MAX = (__ETHTOOL_A_TUNNEL_INFO_CNT - 1)
>>   };
>>   
>> +/* MODULE EEPROM DATA */
>> +
>> +enum {
>> +	ETHTOOL_A_EEPROM_DATA_UNSPEC,
>> +	ETHTOOL_A_EEPROM_DATA_HEADER,
>> +
>> +	ETHTOOL_A_EEPROM_DATA_OFFSET,
>> +	ETHTOOL_A_EEPROM_DATA_LENGTH,
>> +	ETHTOOL_A_EEPROM_DATA_PAGE,
>> +	ETHTOOL_A_EEPROM_DATA_BANK,
>> +	ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS,
>> +	ETHTOOL_A_EEPROM_DATA,
>> +
>> +	__ETHTOOL_A_EEPROM_DATA_CNT,
>> +	ETHTOOL_A_EEPROM_DATA_MAX = (__ETHTOOL_A_EEPROM_DATA_CNT - 1)
>> +};
>> +
>>   /* generic netlink info */
>>   #define ETHTOOL_GENL_NAME "ethtool"
>>   #define ETHTOOL_GENL_VERSION 1
>> diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile
>> index 7a849ff22dad..d604346bc074 100644
>> --- a/net/ethtool/Makefile
>> +++ b/net/ethtool/Makefile
>> @@ -7,4 +7,4 @@ obj-$(CONFIG_ETHTOOL_NETLINK)	+= ethtool_nl.o
>>   ethtool_nl-y	:= netlink.o bitset.o strset.o linkinfo.o linkmodes.o \
>>   		   linkstate.o debug.o wol.o features.o privflags.o rings.o \
>>   		   channels.o coalesce.o pause.o eee.o tsinfo.o cabletest.o \
>> -		   tunnels.o
>> +		   tunnels.o eeprom.o
>> diff --git a/net/ethtool/eeprom.c b/net/ethtool/eeprom.c
>> new file mode 100644
>> index 000000000000..51a2ed81a273
>> --- /dev/null
>> +++ b/net/ethtool/eeprom.c
>> @@ -0,0 +1,149 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +
>> +#include <linux/ethtool.h>
>> +#include "netlink.h"
>> +#include "common.h"
>> +
>> +struct eeprom_data_req_info {
>> +	struct ethnl_req_info	base;
>> +	u32			offset;
>> +	u32			length;
>> +	u32			page;
>> +	u32			bank;
>> +	u32			i2c_address;
>> +};
>> +
>> +struct eeprom_data_reply_data {
>> +	struct ethnl_reply_data base;
>> +	u32			length;
>> +	u32			i2c_address;
>> +	u8			*data;
>> +};
>> +
>> +#define EEPROM_DATA_REQINFO(__req_base) \
>> +	container_of(__req_base, struct eeprom_data_req_info, base)
>> +
>> +#define EEPROM_DATA_REPDATA(__reply_base) \
>> +	container_of(__reply_base, struct eeprom_data_reply_data, base)
>> +
>> +static int eeprom_data_prepare_data(const struct ethnl_req_info *req_base,
>> +				    struct ethnl_reply_data *reply_base,
>> +				    struct genl_info *info)
>> +{
>> +	struct eeprom_data_reply_data *reply = EEPROM_DATA_REPDATA(reply_base);
>> +	struct eeprom_data_req_info *request = EEPROM_DATA_REQINFO(req_base);
>> +	struct ethtool_eeprom_data page_data = {0};
>> +	struct net_device *dev = reply_base->dev;
>> +	int err;
>> +
>> +	if (!dev->ethtool_ops->get_module_eeprom_data_by_page)
>> +		return -EOPNOTSUPP;
>> +	err = ethnl_ops_begin(dev);
>> +	if (err)
>> +		return err;
>> +
>> +	page_data.offset = request->offset;
>> +	page_data.length = request->length;
>> +	page_data.i2c_address = request->i2c_address;
>> +	page_data.page = request->page;
>> +	page_data.bank = request->bank;
>> +	page_data.data = kmalloc(page_data.length, GFP_KERNEL);
>> +	if (!page_data.data)
>> +		return -ENOMEM;
> Isn't an ethnl_ops_complete(dev); needed here? Maybe postpone the
> ethnl_ops_begin(dev) call until after the memory allocation?
Yes, I should just move it after kmalloc().
>> +
>> +	err = dev->ethtool_ops->get_module_eeprom_data_by_page(dev, &page_data);
>> +	if (err)
>> +		goto err_out;
>> +
>> +	reply->length = page_data.length;
>> +	reply->i2c_address = page_data.i2c_address;
>> +	reply->data = page_data.data;
>> +
>> +	ethnl_ops_complete(dev);
>> +	return 0;
>> +
>> +err_out:
>> +	kfree(page_data.data);
>> +	ethnl_ops_complete(dev);
>> +	return err;
>> +}
>> +
>> +static int eeprom_data_parse_request(struct ethnl_req_info *req_info, struct nlattr **tb,
>> +				     struct netlink_ext_ack *extack)
>> +{
>> +	struct eeprom_data_req_info *request = EEPROM_DATA_REQINFO(req_info);
>> +
>> +	if (!tb[ETHTOOL_A_EEPROM_DATA_OFFSET] ||
>> +	    !tb[ETHTOOL_A_EEPROM_DATA_LENGTH] ||
>> +	    !tb[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS])
>> +		return -EINVAL;
>> +
>> +	request->length = nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_LENGTH]);
>> +	if (request->length > ETH_MODULE_EEPROM_MAX_LEN)
>> +		return -EINVAL;
>> +
>> +	request->offset = nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_OFFSET]);
> I think you need to validate that offset + length is not passed the
> end of a 1/2 page. There seems to be odd wrap around semantics, so we
> probably want to avoid that.

I think I will add this check in case a page was specified and this is not
a case ofbackwards compatible flow.

>> +	request->i2c_address = nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS]);
> Maybe validate the i2c address. Most busses are limited to 7 bit
> addresses, but some do allow 10 bits. I think we can probably ignore
> 10 bit addresses for this use case, so check that address is < 128.

Yes, this check is possible, if I expect 0x50 or 0x51 in request. I will 
add it.

Re: [RFC PATCH net-next 1/5] ethtool: Allow network drivers to dump arbitrary EEPROM data

[Andrew Lunn]

> > > + * struct ethtool_eeprom_data - EEPROM dump from specified page
> > > + * @offset: Offset within the specified EEPROM page to begin read, in bytes.
> > > + * @length: Number of bytes to read. On successful return, number of bytes
> > > + *	actually read.
> > > + * @page: Page number to read from.
> > > + * @bank: Page bank number to read from, if applicable by EEPROM spec.
> > > + * @i2c_address: I2C address of a page. Zero indicates a driver should choose
> > > + *	by itself.
> > I don't particularly like the idea of the driver deciding what to
> > read. User space should really be passing 0x50 or 0x51 for the normal
> > case. And we need to make it clear what these addresses mean, since
> > they are often referred to as 0xA0 and 0xA2, due to addresses being
> > shifted one bit left and a r/w bit added.
> 
> I was thinking what address should I send from userspace by default?
> Should I send 0x50 or 0xA0 and at some point to do the shift?
> mlx5 uses 0x50 and 0x51, for example.

I would use the same meaning of i2c address as the i2c subsystem. So
addresses in the range of 0 to 0x7f. And please add a comment about
this here.

> > I also don't think the in place length should be modified. It would be
> > better to follow the use semantics of returning a negative value on
> > error, or a positive value for the length actually
> > read. ethtool_eeprom_data can then be passed as a const.
> But how would userspace know how much actually was read?

You are going to put the data into a netlink attribute. That attribute
has a length. So userspace gets to know that way.

> Should I fail the command if only part of a data requested was read?

That is not the usual POSIX semantics. I would return whatever is
available.

> > > @@ -1865,6 +1888,8 @@ static inline int ethtool_validate_duplex(__u8 duplex)
> > >   #define ETH_MODULE_SFF_8636_MAX_LEN     640
> > >   #define ETH_MODULE_SFF_8436_MAX_LEN     640
> > > +#define ETH_MODULE_EEPROM_MAX_LEN	640
> > I'm surprised such a high value is allowed. 128 seems more
> > appropriate, given the size of 1/2 pages.
> 
> This is done for backwards compatibility mechanism (last patch in the
> series)
> to work properly. I wanted to limit the length to 128 (or 256, maybe), but
> currently ethtool supports dumps of 640 bytes at most.
> I guess I can add another value like ETH_MODULE_EEPROM_PAGE_MAX_LEN 256 and
> if
> new ndo is available or page number is specified, use it for check.
> If ndo is not implemented, use 640 instead.

I need to look at the last patch. But for backwards compatibility with
the old IOCTL call, we might want the glue code to read each 1/2 page
individually, and combine them. That way we keep the new API uniform,
reads only 1/2 pages.

      Andrew