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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
- [High] PTP `adjtime` callback incorrectly blocks the caller by sleeping for up to 60 seconds to emulate hardware slewing.
- [High] Early exit of the background kthread on transient errors causes a Use-After-Free when `kthread_stop()` is later called.
- [High] Missing cleanup in `s2500_open()` error paths leaks the kthread and leaves an `ERR_PTR` that causes crashes.
- [High] Missing cleanup in `s2500_probe()` error paths leaks `oa_tc6` and the PTP clock device.
- [High] The frequency adjustment calculation in `s2500_ptp_adjtime` is off by a factor of 1000.
- [High] Unprotected read-modify-write on `S2500_REG_MAC_CTRL` leads to torn updates, potentially leaving the MAC enabled after the interface is stopped.
- [High] Lockless iteration of the network device multicast address list in the background kthread.
- [Medium] Standard network statistics are incorrectly duplicated as private `ethtool -S` strings instead of implementing standard APIs.
- [Medium] Missing state barrier in the kthread wait loop allows `kthread_stop()` wakeups to be lost, hanging `ndo_stop` for 5 seconds.
- [Medium] `s2500_init` masks the failure of `s2500_set_mac_filter`, allowing initialization to succeed even if the hardware MAC filter cannot be programmed.
- [Medium] `s2500_set_multicast_mode` forgets to re-enable the hardware multicast filter after programming specific MACs.
--

--- Patch [14]: [PATCH net-next v7 14/15] onsemi: s2500: Add driver support for TS2500 MAC-PHY ---
commit 6987fd19b1f59d7fe0c41efd95d6c4fa14e869c0
Author: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>

onsemi: s2500: Add driver support for TS2500 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
> index 0000000000000..7e96d9ad563f8
> --- /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",

[Severity: Medium]
Should these standard network statistics be duplicated as private ethtool -S
strings? 

Standard metrics like tx_bytes, tx_frames, and rx_crc_errors are typically
exposed through standard network APIs such as ndo_get_stats64 rather than
added as custom strings.

> +	"tx_broadcast_frames",
> +	"tx_multicast_frames",
> +	"tx_underflow_errors",
[ ... ]
> diff --git a/drivers/net/ethernet/onsemi/s2500/s2500_main.c b/drivers/net/ethernet/onsemi/s2500/s2500_main.c
> index 0000000000000..2d81b44f8c76c
> --- /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)
> +{
> +	u16 mms = OA_TC6_PHY_C45_MAC_MMS1;
> +	u32 reg = S2500_REG_MAC_CTRL;
> +	u32 rval = 0;
> +	u32 wval = 0;
> +	int ret;
> +
> +	ret = oa_tc6_read_register_mms(priv->tc6, reg, mms, &rval);
> +	if (ret)
> +		return ret;
> +
> +	if (clr)
> +		wval = rval & ~in_bits;
> +	else
> +		wval = rval | in_bits;
> +	if (rval != wval)
> +		ret = oa_tc6_write_register_mms(priv->tc6, reg,
> +						mms, wval);

[Severity: High]
Can this sequence cause torn updates if called concurrently?

s2500_mac_ctrl_modify_bits() performs a lockless read-modify-write on
S2500_REG_MAC_CTRL over SPI. 

If the shutdown logic in s2500_shutdown() races with the kthread updating
promiscuous or multicast flags, could the kthread overwrite the register and
inadvertently resurrect TX/RX after they were cleared?

> +	return ret;
> +}
> +
> +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]
Is it intentional to overwrite the error status from s2500_set_mac_filter()
here? 

If writing the MAC filter fails due to an SPI error, the next line overwrites
the ret variable, allowing initialization to succeed even if the hardware
filter cannot be programmed.

> +}
> +
> +static void s2500_shutdown(struct s2500_info *priv)
[ ... ]
> +static int s2500_set_multicast_mode(struct s2500_info *priv,
> +				    unsigned int rx_flags)
> +{
> +	u16 mms = OA_TC6_PHY_C45_MAC_MMS1;
> +	int i, ret = 0;
> +	u16 addr;
> +	u32 val;
> +
> +	if ((rx_flags & IFF_ALLMULTI) ||
> +	    (netdev_mc_count(priv->ndev) > S2500_N_MCAST_FILTERS)) {
> +		/* Disable multicast filter */
> +		ret = s2500_mac_ctrl_modify_bits(priv,
> +						 S2500_MAC_CTRL_MCSF_BIT,
> +						 true);
> +		if (ret)
> +			return ret;
> +
> +		/* Accept all multicasts */
[ ... ]
> +	} 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 */
> +		for (i = 1; i <= S2500_N_MCAST_FILTERS; i++) {
> +			addr = S2500_REG_MAC_ADDRFILTH(i);
> +			ret = oa_tc6_write_register_mms(priv->tc6,
> +							addr, mms, 0);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		i = 1;
> +		netdev_for_each_mc_addr(ha, priv->ndev) {

[Severity: High]
Is it safe for the background kthread to iterate the multicast address list
without holding netif_addr_lock_bh()? 

If the network stack concurrently adds or removes multicast addresses, could
this lockless traversal of priv->ndev->mc cause list corruption or a
use-after-free?

> +			if (i > S2500_N_MCAST_FILTERS)
> +				break;
> +
> +			addrh = S2500_MAC_ADDRFILT_EN_BIT |
[ ... ]
> +			addr = S2500_REG_MAC_ADDRMASKH(i);
> +			ret = oa_tc6_write_register_mms(priv->tc6,
> +							addr, mms,
> +							0xffff);
> +			if (ret)
> +				return ret;
> +			i++;
> +		}
> +	}
> +	return ret;

[Severity: Medium]
Does s2500_set_multicast_mode() leave the global multicast filter disabled
after programming specific MACs? 

The code disables the filter by clearing S2500_MAC_CTRL_MCSF_BIT, but there
doesn't appear to be a corresponding call to modify_bits(..., false) to
re-enable it before returning.

> +}
> +
> +/* Deferred function for applying RX mode flags in non-atomic context */
[ ... ]
> +static int s2500_thread_fun(void *data)
> +{
> +	struct s2500_info *priv = data;
> +	bool update_rx_mode = false;
> +	unsigned long flags;
> +	signed long tout;
> +	int ret = 0;
> +
> +	tout = priv->poll_jiff;
> +
> +	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]
Could this break statement cause a use-after-free during driver unload?

If the kthread encounters an error during s2500_rx_mode_update() and breaks
out of its loop, it terminates itself. Later, when s2500_stop() is called,
it invokes kthread_stop(priv->thread) on the freed task_struct pointer.

> +			}
> +		}
> +
> +		if (tout == 0) {
[ ... ]
> +		s2500_process_events(priv);
> +
> +		spin_lock_irqsave(&priv->lock, flags);
> +		__set_current_state(TASK_INTERRUPTIBLE);

[Severity: Medium]
Can a wakeup from kthread_stop() be lost here? 

Setting the task state to TASK_INTERRUPTIBLE without re-checking
kthread_should_stop() after the lock is released might cause the loop to sleep
in schedule_timeout(), stalling the network stack module unload for up to 5
seconds.

> +
> +		update_rx_mode = priv->rx_flags_upd;
> +		ret = update_rx_mode;
[ ... ]
> +static int s2500_open(struct net_device *ndev)
> +{
> +	struct s2500_info *priv = netdev_priv(ndev);
> +	int ret = 0;
> +	u32 val;
> +
> +	dev_dbg(&ndev->dev, "%s", "s2500_open");
> +	phy_start(priv->ndev->phydev);
> +
> +	priv->thread = kthread_run(s2500_thread_fun, priv,
> +				   DRV_NAME "/%s:%d",
> +				   dev_name(&priv->spi->dev),
> +				   spi_get_chipselect(priv->spi, 0));
> +
> +	if (IS_ERR(priv->thread)) {
> +		ret = PTR_ERR(priv->thread);
> +	} else {

[Severity: High]
Does this error path leak the started PHY or leave an invalid pointer in
priv->thread? 

If kthread_run() fails, returning here leaves priv->thread as an ERR_PTR.
Later calls to s2500_set_rx_mode() could attempt to call wake_up_process()
on this ERR_PTR.

> +		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;
> +}
[ ... ]
> +static int s2500_probe(struct spi_device *spi)
> +{
[ ... ]
> +	ret = register_netdev(ndev);
> +	if (ret) {
> +		dev_err(&spi->dev, "failed to register netdev\n");
> +		ret = -ENODEV;
> +
> +		goto err_reg_read;
> +	}
> +	return 0;
> +
> +err_reg_read:
> +	dev_err(&spi->dev, "could not initialize macphy");
> +	return ret;

[Severity: High]
Does this error path need to call oa_tc6_exit() or oa_tc6_ptp_unregister()?

If probe fails during s2500_init, s2500_ptp_register, or register_netdev,
returning directly leaks the oa_tc6 instance and the PTP clock device, which
could lead to a use-after-free since priv is devres-managed.

> +}
> +
> +static void s2500_remove(struct spi_device *spi)
> diff --git a/drivers/net/ethernet/onsemi/s2500/s2500_ptp.c b/drivers/net/ethernet/onsemi/s2500/s2500_ptp.c
> index 0000000000000..7da660cb5b190
> --- /dev/null
> +++ b/drivers/net/ethernet/onsemi/s2500/s2500_ptp.c
[ ... ]
> +static int s2500_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
> +{
> +	struct s2500_info *priv = container_of(ptp, struct s2500_info,
> +					       ptp_clock_info);
> +	struct ptp_system_timestamp sts;
> +	struct timespec64 target;
> +	unsigned int period_ms;
> +	struct timespec64 now;
> +	s64 scaled_ppm, rem;
> +	int max_iters = 3;
> +	s64 target_ns;
> +	int ret = 0;
> +	s64 now_ns;
> +	s64 num;
> +	s64 den;
[ ... ]
> +		/* 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 = rem * 65536LL;
> +		den = (s64)period_ms * 1000LL;

[Severity: High]
Is this calculation off by a factor of 1000? 

Scaling period_ms by 1000 instead of shifting it to calculate
parts-per-million could result in an adjustment rate that is drastically
lower than requested by the servo.

> +
> +		/* 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 -EINTR.
> +		 */
> +		if (msleep_interruptible(period_ms)) {

[Severity: High]
Is it safe to block the caller by sleeping for up to 60 seconds in the
PTP adjtime callback? 

The adjtime callback is expected to execute atomically or return quickly.
Sleeping here blocks the ioctl and exposes a use-after-free if the device
is unbound while the thread is sleeping.

> +			/* Clear adjfine */
> +			ptp->adjfine(ptp, 0);
> +			ret = -EINTR;
> +			break;
> +		}
> +
> +		/* Clear adjfine and measure remaining offset */
> +		ptp->adjfine(ptp, 0);
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-s2500-mac-phy-support-v7-0-478c877aa1a9@onsemi.com?part=14