Re: [PATCH net-next v2] net: fec: support RX flushing via an ethtool private flag

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] fec_enet_get_sset_count() triggers WARN_ON_ONCE by returning 0 instead of -EOPNOTSUPP for ETH_SS_PRIV_FLAGS on unsupported hardware.
- [Medium] Data race on fep->rx_flush_mask due to lockless write in fec_enet_set_priv_flags().
--

commit 0779caeb36f03b70108effeec07b7cd6055f53f4
Author: Alexander Sverdlin <alexander.sverdlin@siemens.com>

net: fec: support RX flushing via an ethtool private flag

This commit adds support for flushing the RX FIFO of an individual receive
queue via an ethtool private flag. It exposes a per-queue rx-flush-qN capability
for controllers with the FEC_QUIRK_HAS_MULTI_QUEUES quirk.

> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 794ec427b0ee6..e5c1d04a665c4 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
[ ... ]
> @@ -3576,11 +3603,67 @@ static int fec_enet_get_sset_count(struct net_device *dev, int sset)
>  
>  	case ETH_SS_TEST:
>  		return net_selftest_get_count();
> +	case ETH_SS_PRIV_FLAGS:
> +		if (fep->quirks & FEC_QUIRK_HAS_MULTI_QUEUES)
> +			return min(fep->num_rx_queues, fep->num_tx_queues);
> +		return 0;

[Severity: High]
Should this return -EOPNOTSUPP instead of 0 when the hardware doesn't support
multi-queues? Returning 0 for unsupported private flags violates the ethtool API
contract and can trigger a WARN_ON_ONCE(!ret) in ethtool_get_strings().

>  	default:
>  		return -EOPNOTSUPP;
>  	}
>  }
[ ... ]
> +static int fec_enet_set_priv_flags(struct net_device *ndev, u32 flags)
> +{
> +	struct fec_enet_private *fep = netdev_priv(ndev);
> +	u32 mask = 0;
> +	int i;
[ ... ]
> +	for (i = 0; i < fep->num_rx_queues; i++)
> +		if (flags & BIT(i))
> +			mask |= QOS_RX_FLUSH(i);
> +
> +	if (mask == fep->rx_flush_mask)
> +		return 0;
> +
> +	fep->rx_flush_mask = mask;

[Severity: Medium]
Does this assignment need to be protected by fep->qos_lock?

It appears fep->rx_flush_mask is read concurrently under fep->qos_lock in
fec_enet_set_rx_flush():

	guard(spinlock)(&fep->qos_lock);
	val = readl(fep->hwp + FEC_QOS_SCHEME);
	val &= ~QOS_RX_FLUSH_MASK;
	writel(val | fep->rx_flush_mask, fep->hwp + FEC_QOS_SCHEME);

Could this lockless write lead to a data race if userspace modifies the flags
while the PHY state machine adjusts the link in the background?

> +
> +	if (!netif_running(ndev))
> +		return 0;
> +
> +	fec_enet_set_rx_flush(fep);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901074732.113791-1-alexander.sverdlin@siemens.com?part=1