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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Deadlock vector in stats array protection due to mixed locking contexts without softirq disabling.
- [Medium] Lost software statistics (like tx_dropped) in the `ndo_get_stats64` implementation.
- [Medium] Hardware MAC filters are not logically disabled in `adin1140_mac_filter_clear()`, creating unintended match rules for the all-zero MAC address.
--

--- Patch [13]: [PATCH net-next v5 13/13] net: ethernet: adi: Add a driver for the ADIN1140 MACPHY ---
commit 05f4df01760b18265ccbc2e4743e13ea5b0d6714
Author: Ciprian Regus <ciprian.regus@analog.com>
Subject: net: ethernet: adi: Add a driver for the ADIN1140 MACPHY

This commit introduces a driver for the ADIN1140 10BASE-T1S MAC-PHY.
The device connects over an SPI bus using the Open Alliance TC6 protocol.
It implements custom mii_bus methods to handle specific hardware limitations
with C22 register access during active reception.

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

[ ... ]

> +static int adin1140_mac_filter_set(struct adin1140_priv *priv,
> +				   const u8 *addr, const u8 *mask,
> +				   u8 slot)
> +{
> +	u32 reg_address;
> +	u32 val;
> +	int ret;
> +
> +	if (slot >= ADIN1140_MAC_FILT_MAX_SLOT)
> +		return -ENOSPC;
> +
> +	reg_address = ADIN1140_MAC_ADDR_FILT_UPR_REG + 2 * slot;
> +
> +	ret = oa_tc6_write_register_mms(priv->tc6, OA_TC6_MAC_MMS1,
> +					reg_address,
> +					get_unaligned_be16(&addr[0]) |
> +					ADIN1140_MAC_ADDR_FILT_APPLY2PORT1 |
> +					ADIN1140_MAC_ADDR_FILT_TO_HOST);

[Severity: Medium]
Does this unintentionally create an active filter for the all-zero MAC
address?

When adin1140_mac_filter_clear() passes an all-zero MAC address here to
disable an unused slot, the ADIN1140_MAC_ADDR_FILT_APPLY2PORT1 and
ADIN1140_MAC_ADDR_FILT_TO_HOST bits are still unconditionally applied.

This seems to leave the hardware rule active to match 00:00:00:00:00:00
and forward those frames to the host, instead of disabling the filter
slot entirely.

[ ... ]

> +static void adin1140_stats_work(struct work_struct *work)
> +{
> +	struct delayed_work *dwork = to_delayed_work(work);
> +	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_mms(priv->tc6, OA_TC6_MAC_MMS1,
> +					       adin1140_stat_regs[i],
> +					       &reg_val);
> +		if (ret)
> +			goto out;
> +
> +		scoped_guard(spinlock, &priv->stat_lock)

[Severity: High]
Can this cause a deadlock on the stat_lock?

This delayed work executes in process context and acquires the spinlock
without disabling softirqs.

The core network stack can concurrently invoke the .ndo_get_stats64
callback, which calls adin1140_ndo_get_stats64() from softirq context.

If the process-context thread is preempted by the softirq on the same
CPU, the softirq could spin forever attempting to acquire the same lock.

Should this use a softirq-safe guard like scoped_guard(spinlock_bh, ...)
to protect against softirq preemptions?

> +			priv->stats[i] += reg_val;
> +	}
> +
> +out:
> +	schedule_delayed_work(dwork, ADIN1140_STATS_CHECK_DELAY);
> +}

[ ... ]

> +static void __adin1140_ndo_get_stats64(struct adin1140_priv *priv,
> +				       struct rtnl_link_stats64 *storage)
> +{
> +	storage->rx_errors = priv->stats[rx_crc_errors] +
> +			     priv->stats[rx_align_errors] +
> +			     priv->stats[rx_preamble_errors] +
> +			     priv->stats[rx_short_frame_errors] +
> +			     priv->stats[rx_long_frame_errors] +
> +			     priv->stats[rx_phy_errors] +
> +			     priv->stats[rx_ifg_errors];
> +
> +	storage->tx_errors = priv->stats[tx_excess_collision] +
> +			     priv->stats[tx_underrun];
> +
> +	storage->rx_dropped = priv->stats[rx_fifo_full_dropped] +
> +			      priv->stats[rx_addr_filter_dropped];

[Severity: Medium]
Will this overwrite existing software drop counters instead of aggregating
them?

The rx_dropped counter from the core network stack may already have values,
but this directly assigns the hardware drop counters to storage->rx_dropped,
overwriting any software drops that might have been recorded.

> +
> +	storage->multicast = priv->stats[rx_mc_frames];

[ ... ]

> +static void adin1140_ndo_get_stats64(struct net_device *dev,
> +				     struct rtnl_link_stats64 *storage)
> +{
> +	struct adin1140_priv *priv = netdev_priv(dev);
> +
> +	storage->rx_packets = priv->netdev->stats.rx_packets;
> +	storage->tx_packets = priv->netdev->stats.tx_packets;
> +
> +	storage->rx_bytes = priv->netdev->stats.rx_bytes;
> +	storage->tx_bytes = priv->netdev->stats.tx_bytes;

[Severity: Medium]
Are software-maintained statistics like tx_dropped intentionally omitted
here?

The oa_tc6 framework maintains drop statistics such as
netdev->stats.tx_dropped when skb_linearize() fails in oa_tc6_start_xmit().
However, this function only copies rx/tx_packets and rx/tx_bytes, leaving
tx_dropped and other core netdev stats unreported to userspace.

> +
> +	scoped_guard(spinlock, &priv->stat_lock)
> +		__adin1140_ndo_get_stats64(priv, storage);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-adin1140-driver-v5-0-4aca7b51a58b@analog.com?part=13