Re: [PATCH net-next v11 10/14] net: ethernet: onsemi: s2500: Add driver support for S2500

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] net: s2500: ethtool -S rx_mismatch_drops duplicates
  rtnl_link_stats64
- [Medium] net: s2500: S2500_PTP_MAX_ADJ_PPB overflows 32-bit long
  scaled_ppm
- [Medium] net: s2500: s2500_set_multicast_mode() enables MCSF filter
  under IFF_PROMISC
--

commit 98b1718831c733b46bf9d80e2766e580982ab454
Author: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>

net: ethernet: onsemi: s2500: Add driver support for S2500

This commit adds driver support for the onsemi S2500 10BASE-T1S MAC-PHY,
including ethtool statistics, multicast filtering, and PTP hardware clock
operations, along with RX cut-through and TX credit threshold helpers in
the OA 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
> index 0000000000000..2fc09107aed3e
> --- /dev/null
> +++ b/drivers/net/ethernet/onsemi/s2500/s2500_ethtool.c
[ ... ]
> +static const char s2500_mac_stat_strings[][ETH_GSTRING_LEN] = {
> +	"rx_mismatch_drops",
> +};
> +
> +static const u8 s2500_mac_stat_map[S2500_NUM_MAC_STATS] = {
> +	S2500_MSTRXDROPPED_IDX,
> +};
[ ... ]
> +static void s2500_get_ethtool_stats(struct net_device *ndev,
> +				    struct ethtool_stats *stats, u64 *data)
> +{
> +	struct s2500_info *priv = netdev_priv(ndev);
> +	int i;
> +
> +	s2500_update_stats(priv);
> +	for (i = 0; i < S2500_NUM_MAC_STATS; i++)
> +		data[i] = priv->stats_data[s2500_mac_stat_map[i]];
> +}

[Severity: Medium]
Should S2500_MSTRXDROPPED_IDX be reported via rx_otherhost_dropped in
struct rtnl_link_stats64 instead of a custom "rx_mismatch_drops" counter in
ethtool -S?

Frames dropped due to destination MAC address filter mismatch match the
standard rx_otherhost_dropped counter (see
Documentation/networking/statistics.rst), and exposing them only as a
driver-private ethtool -S string hides them from standard netlink tools
like ip -s link.

> diff --git a/drivers/net/ethernet/onsemi/s2500/s2500_hw_def.h b/drivers/net/ethernet/onsemi/s2500/s2500_hw_def.h
> new file mode 100644
> index 0000000000000..cb9c53b936b83
> --- /dev/null
> +++ b/drivers/net/ethernet/onsemi/s2500/s2500_hw_def.h
[ ... ]
> +/* Frequency adjust magnitude, 1 LSB = 0.000125 ppm = 1/8 ppb */
> +#define S2500_PTP_ADJ_MAX		GENMASK(27, 0)
> +#define S2500_PTP_ADJ_BASE		8000000000ULL
> +#define S2500_PTP_MAX_ADJ_PPB		(S2500_PTP_ADJ_MAX / 8)

[Severity: Medium]
Can S2500_PTP_MAX_ADJ_PPB (33,554,431 ppb) overflow the 32-bit signed long
scaled_ppm parameter on 32-bit architectures?

ptp_clock_adjtime() passes tx->freq as a signed long to
scaled_ppm_to_ppb() and info->adjfine().  On 32-bit kernels, LONG_MAX
holds at most 32,767,999 ppb, so requesting an adjustment between
+32,768,000 and +33,554,431 ppb wraps scaled_ppm to a negative value that
passes the max_adj check and programs S2500_REG_VS_PTP_ADJ in the wrong
direction.  When scaled_ppm is LONG_MIN (-32,768,000 ppb), negating it in
diff_by_scaled_ppm() also overflows and fails with -ERANGE.

> diff --git a/drivers/net/ethernet/onsemi/s2500/s2500_main.c b/drivers/net/ethernet/onsemi/s2500/s2500_main.c
> new file mode 100644
> index 0000000000000..f176475cfd7ed
> --- /dev/null
> +++ b/drivers/net/ethernet/onsemi/s2500/s2500_main.c
[ ... ]
> +static int s2500_set_promiscuous_mode(struct s2500_info *priv,
> +				      unsigned int rx_flags)
> +{
> +	u32 val = S2500_MAC_CTRL_ADRF;
> +	bool clr = false;
> +
> +	if (rx_flags & IFF_PROMISC)
> +		clr = true;
> +	return s2500_mac_ctrl_modify_bits(priv, val, clr);
> +}
> +
> +static int s2500_set_multicast_mode(struct s2500_info *priv,
> +				    unsigned int rx_flags,
> +				    struct netdev_hw_addr_list *mc)
> +{
> +	struct netdev_hw_addr *ha;
> +	u8 mms = OA_TC6_MAC_MMS1;
> +	int i, ret = 0;
> +	int mc_count;
> +	u16 addr;
> +	u32 val;
> +
> +	mc_count = netdev_hw_addr_list_count(mc);
> +
> +	if ((rx_flags & IFF_ALLMULTI) ||
> +	    mc_count > S2500_N_MCAST_FILTERS) {
> +		/* Disable multicast filter */
> +		ret = s2500_mac_ctrl_modify_bits(priv,
> +						 S2500_MAC_CTRL_MCSF,
> +						 true);
[ ... ]
> +	} else if (mc_count == 0) {
> +		/* Enable multicast filter */
> +		ret = s2500_mac_ctrl_modify_bits(priv,
> +						 S2500_MAC_CTRL_MCSF,
> +						 false);
[ ... ]
> +static int s2500_set_rx_mode(struct net_device *ndev,
> +			     struct netdev_hw_addr_list *uc,
> +			     struct netdev_hw_addr_list *mc)
> +{
> +	struct s2500_info *priv = netdev_priv(ndev);
> +	int ret;
> +
> +	ret = s2500_set_promiscuous_mode(priv, ndev->flags);
> +	if (ret)
> +		return ret;
> +
> +	return s2500_set_multicast_mode(priv, ndev->flags, mc);
> +}

[Severity: Medium]
Does s2500_set_multicast_mode() leave S2500_MAC_CTRL_MCSF enabled when
IFF_PROMISC is set?

s2500_set_rx_mode() calls s2500_set_multicast_mode() after
s2500_set_promiscuous_mode(), and s2500_set_multicast_mode() only checks
IFF_ALLMULTI.  When IFF_PROMISC is set without IFF_ALLMULTI and mc_count
is 0, the else-if branch sets S2500_MAC_CTRL_MCSF and disables the address
filter slots, which can filter out multicast traffic while in promiscuous
mode.

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