Re: [PATCH net v2] net: atlantic: fix reading SFP module info on some AQC100 cards

[Jakub Kicinski <kuba@kernel.org>]

On Mon, 16 Feb 2026 16:49:12 -0600 Tiernan Hubble wrote:
> Commit 853a2944aaf3 ("net: atlantic: support reading SFP module info")
> added support for reading SFP module info on AQC100-based cards. However,
> it only supports reading directly from the controller's hardware
> registers, and this does not seem to be supported on certain cards,
> including my TRENDnet TEG-10GECSFP V3. "ethtool -m" times out when reading
> certain registers, even when I increase the read poll timeout values.
> 
> The DPDK "atlantic" driver reads module info via firmware calls instead of
> directly reading the hardware registers, provided that the NIC's firmware
> version supports it.
> 
> This change adapts the DPDK firmware call code to the kernel driver. It
> preserves the old hardware-based module read code as a fallback when the
> firmware does not support it, to avoid breaking cards that are currently
> working.
> 
> Tested on 2 different TRENDnet TEG-10GECSFP V3 cards, both with firmware
> version 3.1.121 (current at the time of this patch). Both cards correctly
> reported module info for a passive DAC cable and 2 different 10G optical
> transceivers.
> 
> Fixes: 853a2944aaf3 ("net: atlantic: support reading SFP module info")
> Signed-off-by: Tiernan Hubble <thubble@thubble.ca>

Overall looks quite nice, but I don't think it qualifies as a fix.
Fix means something regressed from previous release, crashes, etc
For the adapters in question IIUC ethtool -m has never worked before.
Please drop the Fixes tag and repost next week for net-next.

Some drive by comments below..

> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c b/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c
> index a6e1826dd5d7..5be9e9ee7e9f 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c
> @@ -983,6 +983,36 @@ static int aq_ethtool_set_phy_tunable(struct net_device *ndev,
>  	return err;
>  }
>  
> +static bool aq_ethtool_can_read_module_eeprom(struct aq_nic_s *aq_nic)
> +{
> +	return aq_nic->aq_fw_ops->read_module_eeprom ||
> +		aq_nic->aq_hw_ops->hw_read_module_eeprom;
> +}
> +
> +static int aq_ethtool_read_module_eeprom(struct aq_nic_s *aq_nic, u8 dev_addr,
> +					 u8 reg_start_addr, int len, u8 *data)
> +{
> +	int err = -EOPNOTSUPP;

maybe add a temp variable:

	const struct aq_fw_ops *ops = aq_nic->aq_fw_ops;

> +	if (aq_nic->aq_fw_ops->read_module_eeprom) {
> +		err = aq_nic->aq_fw_ops->read_module_eeprom(aq_nic->aq_hw,

to avoid the long lines and awkward wrapping ?

> +			dev_addr, reg_start_addr, len, data);
> +
> +		/* If the only error is that the firmware version doesn't
> +		 * support reading EEPROM, we can still attempt to read it
> +		 * directly from the hardware if supported.
> +		 */
> +		if (err != -EOPNOTSUPP)
> +			return err;
> +	}
> +
> +	if (aq_nic->aq_hw_ops->hw_read_module_eeprom)
> +		err = aq_nic->aq_hw_ops->hw_read_module_eeprom(aq_nic->aq_hw,

and here

> +			dev_addr, reg_start_addr, len, data);
> +
> +	return err;
> +}

> +static int aq_fw2x_read_module_eeprom(struct aq_hw_s *self, u8 dev_addr,
> +				      u8 reg_start_addr, int len, u8 *data)
> +{
> +	u32 low_status, orig_low_status, low_req = 0;
> +	u32 res_bytes_remain_cnt = len % sizeof(u32);
> +	u32 res_dword_cnt = len / sizeof(u32);
> +	struct smbus_request request = { 0 };
> +	u32 req_dword_cnt;
> +	u32 result = 0;
> +	u32 caps_lo;
> +	u32 offset;
> +	int err;
> +
> +	caps_lo = aq_fw2x_get_link_capabilities(self);
> +	if (!(caps_lo & BIT(CAPS_LO_SMBUS_READ)))
> +		return -EOPNOTSUPP;
> +
> +	request.msg_id = 0;
> +	request.device_id = dev_addr;
> +	request.address = reg_start_addr;
> +	request.length = len;
> +
> +	/* Write SMBUS request to cfg memory */
> +	req_dword_cnt = (sizeof(request) + sizeof(u32) - 1) / sizeof(u32);

DIV_ROUND_UP()

> +	err = hw_atl_write_fwcfg_dwords(self, (void *)&request, req_dword_cnt);
> +	if (err < 0)
> +		return err;
> +
> +	/* Toggle 0x368.CAPS_LO_SMBUS_READ bit */
> +	low_req = aq_hw_read_reg(self, HW_ATL_FW2X_MPI_CONTROL_ADDR);
> +	orig_low_status = low_req & BIT(CAPS_LO_SMBUS_READ);

the mixing of BIT(CAPS_LO_SMBUS_READ)..

> +	low_req ^= HW_ATL_FW2X_CAP_SMBUS_READ;

.. and HW_ATL_FW2X_CAP_SMBUS_READ which is defined to

+#define HW_ATL_FW2X_CAP_SMBUS_READ       BIT(CAPS_LO_SMBUS_READ)

seems unnecessary and confusing?
-- 
pw-bot: cr