Re: [PATCH net-next v5 12/15] onsemi: s2500: Add driver support for TS2500 MAC-PHY

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider:
- [High] Premature exit of the polling kthread leads to a Use-After-Free during driver teardown or RX mode updates.
- [High] Lockless traversal of the multicast address list in the background thread causes data races and potential Use-After-Free.
- [High] PTP adjtime blocks for up to 60 seconds and computes frequency offset incorrectly.
- [High] Failure in s2500_open() leaks the background kthread and leaves the PHY running.
- [High] Failure paths in s2500_probe() do not clean up resources allocated by oa_tc6_init().
- [High] Concurrent Read-Modify-Write of the MAC_CTRL register leads to lost updates.
- [High] The s2500_thread_fun kthread contains a lost wakeup window for kthread_stop() causing 5 second interface teardown hang.
- [Medium] Standard netdev statistics are duplicated in driver-private ethtool -S strings.
- [Medium] The multicast hardware filter is incorrectly left disabled when specific MAC addresses are configured.
- [Medium] IFF_UNICAST_FLT is set but secondary unicast MAC filtering is not implemented.
- [Medium] The initialization function silently ignores failures when configuring the device's MAC address filter.
- [Medium] Incorrect MAC address filter register write ordering for multicast addresses.
--

commit 460baa0800374b033a33a01995e62355074e779a
Author: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>

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

This commit introduces a new ethernet driver for the onsemi S2500, an 802.3 cg 
compliant transceiver with integrated MAC-PHY. It hooks into the Open Alliance 
TC6 framework and implements ethtool stats, hardware timestamping, and PTP.

> --- /dev/null
> +++ b/drivers/net/ethernet/onsemi/s2500/s2500_ethtool.c

[ ... ]

> +static const char s2500_mac_stat_strings[][ETH_GSTRING_LEN] = {
> +	"tx_bytes",
> +	"tx_frames",
> +	"tx_broadcast_frames",
> +	"tx_multicast_frames",

[Severity: Medium]
Is it necessary to duplicate standard network statistics like tx_bytes and
tx_frames here? 

Standard hardware counters should typically be exposed through ndo_get_stats64
(struct rtnl_link_stats64) rather than driver-private ethtool -S strings.

[ ... ]

> --- /dev/null
> +++ b/drivers/net/ethernet/onsemi/s2500/s2500_main.c

[ ... ]

> +static int s2500_mac_ctrl_modify_bits(struct s2500_info *priv,
> +				      u32 in_bits, bool clr)
> +{
> +	u32 reg = S2500_REG_MAC_CTRL;
> +	u32 rval = 0;
> +	int ret;
> +
> +	ret = oa_tc6_read_register_mms(priv->tc6, reg,
> +				       OA_TC6_PHY_C45_MAC_MMS1, &rval);
> +	if (!ret) {
> +		u32 wval = 0;
> +
> +		if (clr)
> +			wval = rval & ~in_bits;
> +		else
> +			wval = rval | in_bits;
> +		if (rval != wval)
> +			ret = oa_tc6_write_register_mms(priv->tc6, reg,
> +							OA_TC6_PHY_C45_MAC_MMS1, wval);

[Severity: High]
Is this read-modify-write cycle safe from concurrent execution? 

This function appears to be called from the asynchronous background thread
(s2500_rx_mode_update) as well as the networking stack callbacks (e.g.,
s2500_shutdown in the ndo_stop path). Without locking, a TOCTOU race could
cause one path's modification to overwrite the other's.

[ ... ]

> +static int s2500_init(struct s2500_info *priv)
> +{

[ ... ]

> +	/* Program the source MAC address into the device */
> +	ret = s2500_set_mac_filter(priv->ndev, priv->ndev->dev_addr);
> +
> +	val = S2500_MAC_CTRL_ADRF_BIT | S2500_MAC_CTRL_FCSA_BIT;
> +
> +	return s2500_mac_ctrl_modify_bits(priv, val, false);

[Severity: Medium]
Does this intentionally discard the return code from s2500_set_mac_filter()?

If setting the MAC address filter fails, the error in 'ret' is immediately
overwritten. This could cause the driver to silently proceed with a
misconfigured MAC address.

[ ... ]

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

[ ... ]

> +	} else {
> +		struct netdev_hw_addr *ha;
> +		u32 addrh, addrl;
> +
> +		/* Disable multicast filter */
> +		ret = s2500_mac_ctrl_modify_bits(priv,
> +						 S2500_MAC_CTRL_MCSF_BIT,
> +						 true);
> +		if (ret)
> +			return ret;
> +
> +		/* Disable filters */

[Severity: Medium]
Should the multicast filter be re-enabled after writing the specific addresses
to the hardware slots?

The code intentionally disables the global filter (S2500_MAC_CTRL_MCSF_BIT) at
the beginning of this block, but it does not invoke modify_bits(..., false) at
the end, leaving the hardware in all-multicast mode.

[ ... ]

> +		i = 1;
> +		netdev_for_each_mc_addr(ha, priv->ndev) {

[Severity: High]
Can this lockless iteration race with userspace modifications? 

Since this is executed in the background kthread asynchronously, traversing the
multicast list without holding the netif_addr_lock_bh(ndev) lock might
cause list corruption or a use-after-free if dev_mc_add or dev_mc_del runs
concurrently.

[ ... ]

> +			ret = oa_tc6_write_register_mms(priv->tc6,
> +							S2500_REG_MAC_ADDRFILTH(i),
> +							OA_TC6_PHY_C45_MAC_MMS1, addrh);
> +			if (ret)
> +				return ret;
> +
> +			ret = oa_tc6_write_register_mms(priv->tc6,
> +							S2500_REG_MAC_ADDRFILTL(i),
> +							OA_TC6_PHY_C45_MAC_MMS1, addrl);

[Severity: Medium]
Is this write order correct? 

This writes S2500_REG_MAC_ADDRFILTH(i) (which contains the 
S2500_MAC_ADDRFILT_EN_BIT enable flag) before writing the 
S2500_REG_MAC_ADDRFILTL(i) payload. Writing the enable bit before the data
payload could cause the hardware to match against a partially-updated,
stale MAC address.

[ ... ]

> +static int s2500_thread_fun(void *data)
> +{

[ ... ]

> +	do {
> +		if (update_rx_mode) {
> +			ret = s2500_rx_mode_update(priv);
> +			if (unlikely(ret)) {
> +				dev_err(&priv->spi->dev, "Failed to set new RX mode");
> +				break;
> +			}
> +		}

[Severity: High]
If s2500_rx_mode_update() fails, this break statement drops out of the loop
causing the kthread to terminate and the kernel to free its task_struct.

However, the driver does not set priv->thread to NULL. When the device is
stopped, won't kthread_stop() blindly dereference the freed task_struct
leading to a use-after-free?

[ ... ]

> +		spin_lock_irqsave(&priv->lock, flags);
> +		__set_current_state(TASK_INTERRUPTIBLE);
> +
> +		update_rx_mode = priv->rx_flags_upd;
> +		ret = update_rx_mode;
> +
> +		spin_unlock_irqrestore(&priv->lock, flags);
> +
> +		if (!ret)
> +			tout = schedule_timeout(tout);
> +		else
> +			set_current_state(TASK_RUNNING);
> +	} while (!kthread_should_stop());
> +	return 0;
> +}

[Severity: High]
Could kthread_stop() trigger a lost wakeup here? 

If kthread_stop() runs after the loop condition is evaluated but before
__set_current_state(), the thread's TASK_RUNNING state will be immediately
overwritten with TASK_INTERRUPTIBLE. The thread would then sleep in
schedule_timeout() for up to 5 seconds, ignoring the stop request.

[ ... ]

> +static int s2500_open(struct net_device *ndev)
> +{

[ ... ]

> +	if (IS_ERR(priv->thread)) {
> +		ret = PTR_ERR(priv->thread);
> +	} else {
> +		val = S2500_MAC_CTRL_TXEN_BIT | S2500_MAC_CTRL_RXEN_BIT;
> +		ret = s2500_mac_ctrl_modify_bits(priv, val, false);
> +
> +		netif_start_queue(priv->ndev);
> +	}
> +	return ret;
> +}

[Severity: High]
If s2500_mac_ctrl_modify_bits() fails here, the function returns an error
without stopping the kthread or the PHY. Since ndo_stop is not invoked
if ndo_open fails, won't this leak the kthread and leave the PHY running?

[ ... ]

> +static int s2500_stop(struct net_device *ndev)
> +{

[ ... ]

> +	s2500_shutdown(priv);
> +
> +	kthread_stop(priv->thread);
> +	priv->thread = NULL;
> +
> +	return 0;
> +}

[ ... ]

> +static int s2500_probe(struct spi_device *spi)
> +{

[ ... ]

> +	ndev->priv_flags |= IFF_UNICAST_FLT;

[Severity: Medium]
The driver advertises IFF_UNICAST_FLT, but s2500_set_rx_mode() does not
appear to process ndev->uc (the unicast address list). 

Since this flag is set, the kernel won't automatically fall back to promiscuous
mode when secondary unicast addresses are assigned, which may cause traffic for
those addresses to be dropped.

[ ... ]

> +	ret = s2500_init(priv);
> +	if (unlikely(ret)) {
> +		dev_err(&spi->dev, "failed to s2500_init the device");
> +		goto err_reg_read;
> +	}

[ ... ]

> +	ret = register_netdev(ndev);
> +	if (ret) {
> +		dev_err(&spi->dev, "failed to register the S2500 device\n");
> +		ret = -ENODEV;
> +
> +		goto err_reg_read;
> +	}
> +	return 0;
> +
> +err_reg_read:
> +	dev_err(&spi->dev, "could not initialize macphy");
> +	return ret;
> +}

[Severity: High]
If s2500_init(), s2500_ptp_register(), or register_netdev() fail, this jumps
to err_reg_read but does not clean up the TC6 framework. 

Should this call oa_tc6_exit() and oa_tc6_ptp_unregister() to prevent memory
leaks and dangling PTP registrations?

[ ... ]

> --- /dev/null
> +++ b/drivers/net/ethernet/onsemi/s2500/s2500_ptp.c

[ ... ]

> +static int s2500_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
> +{

[ ... ]

> +		/* Compute scaled_ppm (Qx.16). scaled_ppm = ppm * 2^16
> +		 * ppm = (delta_seconds / period_seconds) * 1e6
> +		 * => scaled_ppm = delta_ns * 65536 / (period_ms * 1000)
> +		 */
> +		num = remaining * 65536LL;
> +		den = (s64)period_ms * 1000LL;
> +
> +		/* Integer division rounds toward zero; keep sign in numerator */
> +		scaled_ppm = div_s64(num, den);
> +
> +		/* Apply frequency adjustment */
> +		ret = ptp->adjfine(ptp, (long)scaled_ppm);
> +		if (ret)
> +			break;
> +
> +		/* Sleep for the slew period (interruptible). If interrupted, clear
> +		 * the adjfine and return with -EINTR.
> +		 */
> +		if (msleep_interruptible(period_ms)) {

[Severity: High]
Does this extra multiplication by 1000 result in an incorrect scaled_ppm?
Dividing the denominator by 1000 again will make the frequency adjustment
1000 times too small.

Additionally, the software slewing loop calls msleep_interruptible() for
up to 60 seconds (or 180s total) while holding the ptp_adj_lock. Will this
block the calling userspace servo loop (e.g., ptp4l via clock_adjtime)
in the kernel for excessive periods?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260614-s2500-mac-phy-support-v5-0-89874b72f725@onsemi.com?part=12