Re: [PATCH] net: atlantic: support reading SFP module info

[Jakub Kicinski]

On Sun,  6 Oct 2024 23:50:25 +0200 Lorenz Brun wrote:
> Add support for reading SFP module info and digital diagnostic
> monitoring data if supported by the module. The only Aquantia
> controller without an integrated PHY is the AQC100 which belongs to
> the B0 revision, that's why it's only implemented there.
> 
> The register information was extracted from a diagnostic tool made
> publicly available by Dell, but all code was written from scratch by me.
> 
> This has been tested to work with a variety of both optical and direct
> attach modules I had lying around and seems to work fine with all of
> them, including the diagnostics if supported by an optical module.
> All tests have been done with an AQC100 on an TL-NT521F card on firmware
> version 3.1.121 (current at the time of this patch).

> +static int aq_ethtool_get_module_info(struct net_device *ndev,
> +				      struct ethtool_modinfo *modinfo)
> +{
> +	int err;
> +	u8 compliance_val, dom_type;
> +	struct aq_nic_s *aq_nic = netdev_priv(ndev);

nit:
Could you reverse the order of variable declarations?
We prefer longest to shortest lines.

> +
> +	/* Module EEPROM is only supported for controllers with external PHY */
> +	if (aq_nic->aq_nic_cfg.aq_hw_caps->media_type != AQ_HW_MEDIA_TYPE_FIBRE)
> +		return -EOPNOTSUPP;
> +
> +	if (!aq_nic->aq_hw_ops->hw_read_module_eeprom)
> +		return -EOPNOTSUPP;
> +
> +	err = aq_nic->aq_hw_ops->hw_read_module_eeprom(aq_nic->aq_hw,
> +		SFF_8472_ID_ADDR, SFF_8472_COMP_ADDR, 1, &compliance_val);
> +	if (err)
> +		return err;
> +
> +	err = aq_nic->aq_hw_ops->hw_read_module_eeprom(aq_nic->aq_hw,
> +		SFF_8472_ID_ADDR, SFF_8472_DOM_TYPE_ADDR, 1, &dom_type);
> +	if (err)
> +		return err;
> +
> +	if (dom_type & SFF_8472_ADDRESS_CHANGE_REQ_MASK || compliance_val == 0x00) {
> +		modinfo->type = ETH_MODULE_SFF_8079;
> +		modinfo->eeprom_len = ETH_MODULE_SFF_8079_LEN;
> +	} else {
> +		modinfo->type = ETH_MODULE_SFF_8472;
> +		modinfo->eeprom_len = ETH_MODULE_SFF_8472_LEN;
> +	}
> +	return 0;
> +}
> +
> +static int aq_ethtool_get_module_eeprom(struct net_device *ndev,
> +					struct ethtool_eeprom *ee, unsigned char *data)
> +{
> +	int err;
> +	unsigned int first, last, len;
> +		struct aq_nic_s *aq_nic = netdev_priv(ndev);

nit: extra tab here

> +	if (!aq_nic->aq_hw_ops->hw_read_module_eeprom)
> +		return -EOPNOTSUPP;
> +
> +	if (ee->len == 0)
> +		return -EINVAL;

I don't think core will let that happen, you can remove the check

> +	first = ee->offset;
> +	last = ee->offset + ee->len;
> +
> +	if (first < ETH_MODULE_SFF_8079_LEN) {
> +		len = min_t(unsigned int, last, ETH_MODULE_SFF_8079_LEN);

AFAIU pure min() may work these days

> +		len -= first;
> +
> +		err = aq_nic->aq_hw_ops->hw_read_module_eeprom(aq_nic->aq_hw,
> +			SFF_8472_ID_ADDR, first, len, data);
> +		if (err)
> +			return err;
> +
> +		first += len;
> +		data += len;
> +	}
> +	if (first < ETH_MODULE_SFF_8472_LEN && last > ETH_MODULE_SFF_8079_LEN) {
> +		len = min_t(unsigned int, last, ETH_MODULE_SFF_8472_LEN);
> +		len -= first;
> +		first -= ETH_MODULE_SFF_8079_LEN;
> +
> +		err = aq_nic->aq_hw_ops->hw_read_module_eeprom(aq_nic->aq_hw,
> +			SFF_8472_DIAGNOSTICS_ADDR, first, len, data);
> +		if (err)
> +			return err;
> +	}
> +	return 0;
> +}
> +
>  const struct ethtool_ops aq_ethtool_ops = {
>  	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
>  				     ETHTOOL_COALESCE_MAX_FRAMES,
> @@ -1014,4 +1090,6 @@ const struct ethtool_ops aq_ethtool_ops = {
>  	.get_ts_info         = aq_ethtool_get_ts_info,
>  	.get_phy_tunable     = aq_ethtool_get_phy_tunable,
>  	.set_phy_tunable     = aq_ethtool_set_phy_tunable,
> +	.get_module_info     = aq_ethtool_get_module_info,
> +	.get_module_eeprom   = aq_ethtool_get_module_eeprom,
>  };

> +static int hw_atl_b0_read_module_eeprom(struct aq_hw_s *self, u8 dev_addr,
> +					u8 reg_start_addr, int len, u8 *data)
> +{
> +	int err;
> +	int i, b;
> +	u32 val;
> +
> +	/* Wait for SMBUS0 to be idle */
> +	err = readx_poll_timeout_atomic(hw_atl_smb0_bus_busy_get, self,
> +					val, val == 0, 100U, 10000U);

Why atomic?
-- 
pw-bot: cr