Subject: Re: [PATCH net-next 5/5] net: txgbe: support getting module EEPROM by page

From: Vadim Fedorenko <[email-masked]>
Date: Wed, 12 Nov 2025 12:48:38 +0000

On 12/11/2025 05:58, Jiawen Wu wrote:
> Getting module EEPROM has been supported in TXGBE SP devices, since SFP
> driver has already implemented it.
> 
> Now add support to read module EEPROM for AML devices. Towards this, add
> a new firmware mailbox command to get the page data.
> 
> Signed-off-by: Jiawen Wu <[email-masked]>
> ---
>   .../net/ethernet/wangxun/txgbe/txgbe_aml.c    | 37 +++++++++++++++++++
>   .../net/ethernet/wangxun/txgbe/txgbe_aml.h    |  3 ++
>   .../ethernet/wangxun/txgbe/txgbe_ethtool.c    | 30 +++++++++++++++
>   .../net/ethernet/wangxun/txgbe/txgbe_type.h   | 11 ++++++
>   4 files changed, 81 insertions(+)
> 
> diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c
> index 3b6ea456fbf7..12900abfa91a 100644
> --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c
> +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c
> @@ -73,6 +73,43 @@ int txgbe_test_hostif(struct wx *wx)
>   					WX_HI_COMMAND_TIMEOUT, true);
>   }
>   
> +int txgbe_read_eeprom_hostif(struct wx *wx,
> +			     struct txgbe_hic_i2c_read *buffer,
> +			     u32 length, u8 *data)
> +{
> +	u32 buf_size = sizeof(struct txgbe_hic_i2c_read) - sizeof(u8);
> +	u32 total_len = buf_size + length;
> +	u32 dword_len, value, i;
> +	u8 local_data[256];
> +	int err;
> +
> +	if (total_len > sizeof(local_data))
> +		return -EINVAL;

if it's really possible? SFF pages are 128 bytes, you reserve 256 bytes
of local buffer. What are you protecting from?

> +
> +	buffer->hdr.cmd = FW_READ_EEPROM_CMD;
> +	buffer->hdr.buf_len = sizeof(struct txgbe_hic_i2c_read) -
> +			      sizeof(struct wx_hic_hdr);
> +	buffer->hdr.cmd_or_resp.cmd_resv = FW_CEM_CMD_RESERVED;
> +
> +	err = wx_host_interface_command(wx, (u32 *)buffer,
> +					sizeof(struct txgbe_hic_i2c_read),
> +					WX_HI_COMMAND_TIMEOUT, false);
> +	if (err != 0)
> +		return err;
> +
> +	dword_len = (total_len + 3) / 4;

round_up()?

> +
> +	for (i = 0; i < dword_len; i++) {
> +		value = rd32a(wx, WX_FW2SW_MBOX, i);
> +		le32_to_cpus(&value);
> +
> +		memcpy(&local_data[i * 4], &value, 4);
> +	}

the logic here is not clear from the first read of the code. effectively
in the reply you have the same txgbe_hic_i2c_read struct but without
data field, which is obviously VLA, but then you simply skip the result
of read of txgbe_hic_i2c_read and only provide the real data back to the
caller. Maybe you can organize the code the way it can avoid double copying?

> +
> +	memcpy(data, &local_data[buf_size], length);
> +	return 0;
> +}
> +
>   static int txgbe_identify_module_hostif(struct wx *wx,
>   					struct txgbe_hic_get_module_info *buffer)
>   {
> diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.h
> index 7c8fa48e68d3..4f6df0ee860b 100644
> --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.h
> +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.h
> @@ -7,6 +7,9 @@
>   void txgbe_gpio_init_aml(struct wx *wx);
>   irqreturn_t txgbe_gpio_irq_handler_aml(int irq, void *data);
>   int txgbe_test_hostif(struct wx *wx);
> +int txgbe_read_eeprom_hostif(struct wx *wx,
> +			     struct txgbe_hic_i2c_read *buffer,
> +			     u32 length, u8 *data);
>   int txgbe_set_phy_link(struct wx *wx);
>   int txgbe_identify_module(struct wx *wx);
>   void txgbe_setup_link(struct wx *wx);
> diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
> index f553ec5f8238..1f60121fe73c 100644
> --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
> +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
> @@ -10,6 +10,7 @@
>   #include "../libwx/wx_lib.h"
>   #include "txgbe_type.h"
>   #include "txgbe_fdir.h"
> +#include "txgbe_aml.h"
>   #include "txgbe_ethtool.h"
>   
>   int txgbe_get_link_ksettings(struct net_device *netdev,
> @@ -534,6 +535,34 @@ static int txgbe_set_rxnfc(struct net_device *dev, struct ethtool_rxnfc *cmd)
>   	return ret;
>   }
>   
> +static int
> +txgbe_get_module_eeprom_by_page(struct net_device *netdev,
> +				const struct ethtool_module_eeprom *page_data,
> +				struct netlink_ext_ack *extack)
> +{
> +	struct wx *wx = netdev_priv(netdev);
> +	struct txgbe_hic_i2c_read buffer;
> +	int err;
> +
> +	if (!test_bit(WX_FLAG_SWFW_RING, wx->flags))
> +		return -EOPNOTSUPP;
> +
> +	buffer.length = (__force u32)cpu_to_be32(page_data->length);
> +	buffer.offset = (__force u32)cpu_to_be32(page_data->offset);

maybe txgbe_hic_i2c_read::length and txgbe_hic_i2c_read::offset should
be __be32 ?

> +	buffer.page = page_data->page;
> +	buffer.bank = page_data->bank;
> +	buffer.i2c_address = page_data->i2c_address;
> +
> +	err = txgbe_read_eeprom_hostif(wx, &buffer, page_data->length,
> +				       page_data->data);
> +	if (err) {
> +		wx_err(wx, "Failed to read module EEPROM\n");
> +		return err;
> +	}
> +
> +	return page_data->length;
> +}
> +
>   static const struct ethtool_ops txgbe_ethtool_ops = {
>   	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
>   				     ETHTOOL_COALESCE_TX_MAX_FRAMES_IRQ |
> @@ -568,6 +597,7 @@ static const struct ethtool_ops txgbe_ethtool_ops = {
>   	.set_msglevel		= wx_set_msglevel,
>   	.get_ts_info		= wx_get_ts_info,
>   	.get_ts_stats		= wx_get_ptp_stats,
> +	.get_module_eeprom_by_page	= txgbe_get_module_eeprom_by_page,
>   };
>   
>   void txgbe_set_ethtool_ops(struct net_device *netdev)
> diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
> index e72edb9ef084..3d1bec39d74c 100644
> --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
> +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
> @@ -353,6 +353,7 @@ void txgbe_do_reset(struct net_device *netdev);
>   #define FW_PHY_GET_LINK_CMD             0xC0
>   #define FW_PHY_SET_LINK_CMD             0xC1
>   #define FW_GET_MODULE_INFO_CMD          0xC5
> +#define FW_READ_EEPROM_CMD              0xC6
>   
>   struct txgbe_sff_id {
>   	u8 identifier;		/* A0H 0x00 */
> @@ -394,6 +395,16 @@ struct txgbe_hic_ephy_getlink {
>   	u8 resv[6];
>   };
>   
> +struct txgbe_hic_i2c_read {
> +	struct wx_hic_hdr hdr;
> +	u32 offset;
> +	u32 length;
> +	u8 page;
> +	u8 bank;
> +	u8 i2c_address;
> +	u8 data;
> +};
you removed struct txgbe_hic_i2c_read in the patch 2 just to introduce
it in this patch?

> +
>   #define NODE_PROP(_NAME, _PROP)			\
>   	(const struct software_node) {		\
>   		.name = _NAME,			\