Re: [PATCH net-next v2 10/10] net: ethernet: adi: Add a driver for the ADIN1140 MACPHY

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Incorrect teardown order in `adin1140_remove` allows workqueue use-after-free.
- [High] Data race and list corruption when traversing MAC address lists without `netif_addr_lock_bh`.
- [High] Potential deadlock due to `spin_lock()` usage in process context without disabling bottom halves.
- [High] Statistics corruption: hardware counters are overwritten instead of accumulated, and zeroed on read failure.
- [High] Resource leak and invalid state due to calling `mdiobus_free()` on a registered bus.
- [High] Logic errors in MAC filter programming (`adin1140_rx_mode_work`).
- [High] Unconditional re-arming in `adin1140_stats_work` causes polling after close and use-after-free.
- [Medium] Returning `-EIO` in `adin1140_mdiobus_write` disrupts PHY framework operations.
--

commit dcc3638a661c0f596ed52cec0edc4d46a70735da
Author: Ciprian Regus <ciprian.regus@analog.com>
Subject: net: ethernet: adi: Add a driver for the ADIN1140 MACPHY

This commit adds a driver for the ADIN1140 10BASE-T1S MAC-PHY connected
over SPI. It implements the Open Alliance TC6 protocol for control and
frame transfers. The driver implements a custom mii_bus to intercept C22
accesses and uses workqueues to process statistics and rx_mode
configurations asynchronously.

> diff --git a/drivers/net/ethernet/adi/adin1140.c b/drivers/net/ethernet/adi/adin1140.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/net/ethernet/adi/adin1140.c

[ ... ]

> +static void adin1140_rx_mode_work(struct work_struct *work)
> +{
> +	struct adin1140_priv *priv = container_of(work, struct adin1140_priv,
> +						  rx_mode_work);
> +	struct netdev_hw_addr *ha;

[ ... ]

> +	if (priv->netdev->flags & IFF_PROMISC) {
> +		promisc = true;
> +		all_multi = false;
> +	} else if (priv->netdev->flags & IFF_ALLMULTI) {
> +		promisc = false;
> +		all_multi = true;

[Severity: High]
If the interface is configured with IFF_ALLMULTI, does this skip programming
the secondary unicast MAC addresses? It looks like the if/else structure
bypasses the unicast address loop entirely in this state, which might cause
the device to silently drop traffic destined for those secondary addresses.

> +	} else if (mac_addrs <= ADIN1140_MAC_FILT_AVAIL) {
> +		promisc = false;
> +		all_multi = false;
> +
> +		slot = ADIN1140_MAC_FILT_UC_SLOT + 1;
> +		memset(mask, 0xFF, ETH_ALEN);
> +
> +		netdev_for_each_uc_addr(ha, priv->netdev) {

[Severity: High]
Is a lock needed when traversing these address lists? Since this executes 
asynchronously in a workqueue, the core network stack could concurrently 
modify the lists. Without holding netif_addr_lock_bh(), could this lead 
to list corruption or a use-after-free?

Also, if the number of unicast or multicast addresses has shrunk since
the last update, should the driver clear the remaining unused filter slots?
Without clearing them, the hardware might continue matching and forwarding
packets for MAC addresses that were already deleted.

> +			ret = adin1140_mac_filter_set(priv, ha->addr, mask,
> +						      slot);

[ ... ]

> +static void adin1140_stats_work(struct work_struct *work)
> +{
> +	struct delayed_work *dwork = to_delayed_work(work);
> +	u64 stat_buff[ADIN1140_STATS_CNT] = {};
> +	struct adin1140_priv *priv;
> +	u32 reg_val;
> +	int ret;
> +	u32 i;
> +
> +	priv = container_of(dwork, struct adin1140_priv, stats_work);
> +
> +	for (i = 0; i < ARRAY_SIZE(adin1140_stat_regs); i++) {
> +		ret = oa_tc6_read_register(priv->tc6, adin1140_stat_regs[i],
> +					   &reg_val);
> +		if (ret)
> +			break;
> +
> +		stat_buff[i] = reg_val;
> +	}
> +
> +	spin_lock(&priv->stat_lock);
> +	memcpy(&priv->stats, stat_buff, sizeof(priv->stats));

[Severity: High]
Could this lead to statistics corruption? If oa_tc6_read_register() fails
transiently, the loop breaks early leaving the rest of stat_buff as zeroes,
which are then copied over the entire priv->stats array.

Additionally, by replacing the values instead of accumulating them (using +=),
won't the 64-bit software counters just mirror the 32-bit hardware counters
and lose their history once the hardware registers wrap around?

[Severity: High]
Since adin1140_ndo_get_stats64() can be called from softirq/BH context
and acquires priv->stat_lock using spin_lock(), is there a risk of an
ABBA deadlock here? If a softirq preempts this process context workqueue
while the lock is held, it might spin forever trying to acquire it. Should
this use spin_lock_bh() instead?

> +	spin_unlock(&priv->stat_lock);
> +
> +	schedule_delayed_work(dwork, ADIN1140_STATS_CHECK_DELAY);

[Severity: High]
Should this check if the interface is still running before re-arming?
When the device is closed, adin1140_close() calls cancel_delayed_work_sync().
If the work is running during that call, it will wait for it to finish, but
then re-arm itself here right before returning. This could cause the work to
keep polling after the interface is down, eventually leading to a 
use-after-free when the module is removed and the netdev is freed.

> +}

[ ... ]

> +static int adin1140_mdiobus_write(struct mii_bus *bus, int addr, int regnum,
> +				  u16 val)
> +{
> +	return -EIO;
> +}

[Severity: Medium]
The commit message mentions that the C22 write function is a no-op to
protect against the ioctl MDIO access path. However, returning -EIO will
propagate an error. Could this disrupt standard PHY framework operations
like genphy_suspend() or genphy_resume() by making them abort under the
assumption that the MDIO bus is broken? Would returning 0 be more
appropriate for a true no-op?

[ ... ]

> +static int adin1140_phy_init(struct adin1140_priv *priv,
> +			     struct spi_device *spidev)
> +{
> +	int ret;
> +
> +	ret = adin1140_mdio_register(priv, spidev);
> +	if (ret)
> +		return ret;
> +
> +	priv->phydev = phy_find_first(priv->mdiobus);
> +	if (!priv->phydev) {
> +		netdev_err(priv->netdev, "No PHY found\n");
> +		ret = -ENODEV;
> +		goto free_mdio;

[Severity: High]
If adin1140_mdio_register() succeeds, the MDIO bus is fully registered.
Jumping to free_mdio will call mdiobus_free() on a registered bus. Doesn't
this violate the MDIO subsystem API, trigger a kernel warning, and leak
resources? Should this jump to unregister_mdio instead?

> +	}

[ ... ]

> +unregister_mdio:
> +	mdiobus_unregister(priv->mdiobus);
> +free_mdio:
> +	mdiobus_free(priv->mdiobus);
> +
> +	return ret;
> +}

[ ... ]

> +static void adin1140_remove(struct spi_device *spi)
> +{
> +	struct adin1140_priv *priv = spi_get_drvdata(spi);
> +
> +	cancel_work_sync(&priv->rx_mode_work);
> +	unregister_netdev(priv->netdev);

[Severity: High]
Is this the correct teardown order? During unregister_netdev(), the network
core drops device state (like clearing IFF_PROMISC), which will trigger
ndo_set_rx_mode and schedule rx_mode_work again. The priv memory is then
freed by free_netdev(). If the re-queued work executes, won't it access
the freed priv pointer and cause a kernel panic? Should unregister_netdev()
be called before cancelling the workqueue?

> +	adin1140_phy_remove(priv);
> +	oa_tc6_exit(priv->tc6);
> +	free_netdev(priv->netdev);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527-adin1140-driver-v2-0-37e5c8d4e0a0@analog.com?part=10