Re: [PATCH net v2 2/2] net: stmmac: Stop Tx queue when (en|dis)abling XSK pools

[Kurt Kanzenbach <kurt@linutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 3220 bytes --]

On Thu Oct 08 2026, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 (1) · Medium: 0 · Low: 0
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [High] The patch only stops the netdev Tx queue, and the driver's own
>   XDP transmit paths ignore that.
>
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but the new wake-
>   up makes it visible: if stmmac_enable_tx_queue() fails, the Tx queue…
>
> --
>
> net: stmmac: Stop Tx queue when (en|dis)abling XSK pools
>
> This patch stops the netdev Tx queue in stmmac_xdp_enable_pool() and
> stmmac_xdp_disable_pool() while the Tx DMA descriptor resources are freed
> and reallocated. The queue is woken once the ring has been rebuilt. The goal
> is to fix crashes in stmmac_xmit() when XSK pools are toggled while Tx
> traffic is running.
>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
>> index d7e4db7224b0c..883bd3fe8089d 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
>> @@ -6,6 +6,16 @@
>>  #include "stmmac.h"
>>  #include "stmmac_xdp.h"
>>  
>> +static void stmmac_xdp_stop_tx_queue(struct stmmac_priv *priv, u16 queue)
>> +{
>> +	struct netdev_queue *nq = netdev_get_tx_queue(priv->dev, queue);
>> +
>> +	__netif_tx_lock_bh(nq);
>> +	txq_trans_cond_update(nq);
>> +	netif_tx_stop_queue(nq);
>> +	__netif_tx_unlock_bh(nq);
>> +}
>
> [Severity: High]
>
> Does setting __QUEUE_STATE_DRV_XOFF here keep the driver's own XDP
> transmit paths away from the ring while it is freed and reallocated?
>
> The Tx lock is released before stmmac_disable_tx_queue() calls
> __free_dma_tx_desc_resources(). DRV_XOFF only stops callers that reach
> ndo_start_xmit through the stack.
>
> Both XDP Tx paths in stmmac_main.c choose the queue by CPU. Neither checks
> whether that queue is stopped before calling stmmac_xdp_xmit_xdpf():

Yes, that was pointed out by Nicolai as well. He proposed a fix and I'll
include in the next version. 

>
>> @@ -46,6 +57,7 @@ static int stmmac_xdp_enable_pool(struct stmmac_priv *priv,
>>  		stmmac_enable_rx_queue(priv, queue);
>>  		stmmac_enable_tx_queue(priv, queue);
>>  		napi_enable(&ch->rxtx_napi);
>> +		netif_tx_wake_queue(netdev_get_tx_queue(priv->dev, queue));
>
> [Severity: High]
>
> This is a pre-existing issue that this patch did not introduce, but the
> new wake-up makes it more visible. What happens here if
> stmmac_enable_tx_queue() failed?
>
> stmmac_enable_tx_queue() returns void and only logs the failure:
>

Yes, I know. I do have a patch to address this in the series to enable
TBS at run time. It's a separate issue.

Thanks,
Kurt

--
pw-bot: cr

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]