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

["Sverdlin, Alexander" <alexander.sverdlin@siemens.com>]

On Wed, 2026-09-02 at 07:47 +0000, sashiko-bot@kernel.org wrote:
> 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().

ACK: both will be fixed in v3

> 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;
> > +}

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com