Re: [PATCH net-next v8 10/11] onsemi: s2500: Add driver support for S2500 MAC-PHY

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] net/s2500: multicast filter remains disabled after specific MAC programming
- [Medium] net/s2500: multicast filter slot race condition due to incorrect register write order
- [Low] net/s2500: uAPI violation by exporting standard rx drops in ethtool -S
--

commit d393e2edea43d8c612d285df4be05c1d1b4fc7d4
Author: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>

onsemi: s2500: Add driver support for S2500 MAC-PHY

Support for onsemi's S2500, 802.3 cg compliant Ethernet
transceiver with integrated MAC-PHY. Works with
Open Alliance TC6 framework.

> diff --git a/drivers/net/ethernet/onsemi/s2500/s2500_ethtool.c b/drivers/net/ethernet/onsemi/s2500/s2500_ethtool.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/net/ethernet/onsemi/s2500/s2500_ethtool.c
> @@ -0,0 +1,369 @@

[ ... ]

> +static const char s2500_mac_stat_strings[][ETH_GSTRING_LEN] = {
> +	"rx_mismatch_drops",
> +};

[Severity: Low]
Is it necessary to export this standard statistic as a custom ethtool -S
string?

This counter maps to S2500_MSTRXDROPPED_IDX which is a standard rx dropped
statistic. Per the networking subsystem guidelines, statistics that have
standard interfaces (such as rtnl_link_stats64) should not be duplicated
in private ethtool strings. Could this be moved to the standard interface?

> diff --git a/drivers/net/ethernet/onsemi/s2500/s2500_main.c b/drivers/net/ethernet/onsemi/s2500/s2500_main.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/net/ethernet/onsemi/s2500/s2500_main.c
> @@ -0,0 +1,523 @@

[ ... ]

> +static int s2500_set_multicast_mode(struct s2500_info *priv,
> +				    unsigned int rx_flags,
> +				    struct netdev_hw_addr_list *mc)
> +{

[ ... ]

> +	} else {
> +		u32 addrh, addrl;
> +
> +		/* Disable multicast filter */
> +		ret = s2500_mac_ctrl_modify_bits(priv,
> +						 S2500_MAC_CTRL_MCSF,
> +						 true);
> +		if (ret)
> +			return ret;
> +
> +		/* Disable filters */
> +		for (i = 1; i <= S2500_N_MCAST_FILTERS; i++) {
> +			addr = S2500_REG_MAC_ADDRFILTH(i);
> +			ret = oa_tc6_write_register_mms(priv->tc6,
> +							mms, addr, 0);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		/* Slot 0 holds the unicast MAC, multicast starts at 1 */
> +		i = 1;
> +		netdev_hw_addr_list_for_each(ha, mc) {
> +			const u8 *mac = ha->addr;
> +			int slot = i++;
> +
> +			addrh = S2500_MAC_ADDRFILT_ENABLE |
> +				get_unaligned_be16(mac);
> +			addrl = get_unaligned_be32(&mac[2]);
> +
> +			addr = S2500_REG_MAC_ADDRFILTH(slot);
> +			ret = oa_tc6_write_register_mms(priv->tc6,
> +							mms, addr,
> +							addrh);

[Severity: Medium]
Does writing ADDRFILTH before the other filter registers introduce a race
condition where the hardware filter might incorrectly match or drop packets?

The ADDRFILTH register contains the S2500_MAC_ADDRFILT_ENABLE bit, which is
being written before the lower address bits (ADDRFILTL) and masks (ADDRMASKL,
ADDRMASKH) are configured. Should the ENABLE bit be written last, similar to
how it is handled in s2500_set_mac_filter()?

> +			if (ret)
> +				return ret;
> +
> +			addr = S2500_REG_MAC_ADDRFILTL(slot);
> +			ret = oa_tc6_write_register_mms(priv->tc6,
> +							mms, addr,
> +							addrl);
> +			if (ret)
> +				return ret;
> +
> +			addr = S2500_REG_MAC_ADDRMASKL(slot);
> +			ret = oa_tc6_write_register_mms(priv->tc6,
> +							mms, addr,
> +							0xffffffff);
> +			if (ret)
> +				return ret;
> +
> +			addr = S2500_REG_MAC_ADDRMASKH(slot);
> +			ret = oa_tc6_write_register_mms(priv->tc6,
> +							mms, addr,
> +							0xffff);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +	return ret;
> +}

[Severity: Medium]
Will this leave the multicast filter globally disabled?

When s2500_set_multicast_mode() is called to configure a specific list of
multicast addresses, the filter is explicitly disabled at the start of the
else block via s2500_mac_ctrl_modify_bits(..., S2500_MAC_CTRL_MCSF, true).
However, it appears the function completes without re-enabling the filter
after the slots are successfully programmed, potentially passing all
multicast traffic to the CPU.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928-s2500-mac-phy-support-v8-0-7e011aacc309@onsemi.com?part=10