Re: [PATCH net] net: stmmac: ethtool: validate TX coalesce before reprogramming RX

[Andrew Lunn <andrew@lunn.ch>]

On Thu, Sep 17, 2026 at 09:12:35PM +0800, Linkui Xiao wrote:
> From: Linkui Xiao <xiaolinkui@kylinos.cn>
> 
> __stmmac_set_coalesce() applies the RX part of the request first and
> only afterwards checks the TX parameters. The RX path already calls
> stmmac_rx_watchdog() and stores rx_riwt[] and rx_coal_frames[], so when
> the TX check rejects the request the driver returns -EINVAL after having
> silently changed the hardware. A following ethtool -c then reports the
> new RX values even though the command failed.
> 
> This became easy to hit once the per-queue interface was added.
> __stmmac_get_coalesce() reports tx-usecs and tx-frames as 0 for a queue
> index that is RX-only, and ethtool applies per-queue coalesce by reading
> the current values first and sending them straight back. The next set is
> therefore guaranteed to trip the test for both TX fields being zero,
> right after the RX watchdog has been reprogrammed.
> 
> Move both TX checks in front of the RX block so a request is either
> applied completely or rejected without touching the device.
> 
> Fixes: db2f2842e6f5 ("net: stmmac: add per-queue TX & RX coalesce ethtool support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
> ---
>  .../ethernet/stmicro/stmmac/stmmac_ethtool.c  | 21 ++++++++++++-------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> index 154cc0c7623d..325db062f72a 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> @@ -850,6 +850,19 @@ static int __stmmac_set_coalesce(struct net_device *dev,
>  	else if (queue >= max_cnt)
>  		return -EINVAL;
>  
> +	/* Check the TX parameters before anything is applied: the RX part
> +	 * below already writes to the hardware, so rejecting the request
> +	 * afterwards would leave the device with only half of the settings
> +	 * the caller asked for while reporting a failure.
> +	 */

Why such a verbose comment? Look at the rest of the code and make your
comments similar in verbosity.

    Andrew

---
pw-bot: cr