Re: [patch] net, bcmgenet: Fix locking of netpoll facing functions

[Florian Fainelli]

On 8/26/25 01:24, Mike Galbraith wrote:
> Lockdep reports ring->lock to not be irq safe during netpoll/netconsole
> session, resulting in a potential deadlock scenario.
>
>   Chain exists of:
>     &host->lock --> target_list_lock --> &ring->lock
>    Possible interrupt unsafe locking scenario:
>          CPU0                    CPU1
>          ----                    ----
>     lock(&ring->lock);
>                                  local_irq_disable();
>                                  lock(&host->lock);
>                                  lock(target_list_lock);
>     <Interrupt>
>       lock(&host->lock);
>    *** DEADLOCK ***
>
> Prevent that via use of irqsave/restore spinlock variant when polling.
>
> Signed-off-by: Mike Galbraith <efault@xxxxxx>

Your patch did not make it to the adequate mailing list which should be 
at least netdev@xxxxxxxxxxxxxxx. This is effectively a partial revert of 
b0447ecb533270cf857ebee1133cb8ff67115423 ("net: bcmgenet: relax lock 
constraints to reduce IRQ latency") therefore I would want Doug to chime 
in and review this.

Thanks!

> ---
>   drivers/net/ethernet/broadcom/genet/bcmgenet.c |   10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
>
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -2022,14 +2022,15 @@ static int bcmgenet_tx_poll(struct napi_
>   		container_of(napi, struct bcmgenet_tx_ring, napi);
>   	unsigned int work_done = 0;
>   	struct netdev_queue *txq;
> +	unsigned long flags;
>   
> -	spin_lock(&ring->lock);
> +	spin_lock_irqsave(&ring->lock, flags);
>   	work_done = __bcmgenet_tx_reclaim(ring->priv->dev, ring);
>   	if (ring->free_bds > (MAX_SKB_FRAGS + 1)) {
>   		txq = netdev_get_tx_queue(ring->priv->dev, ring->index);
>   		netif_tx_wake_queue(txq);
>   	}
> -	spin_unlock(&ring->lock);
> +	spin_unlock_irqrestore(&ring->lock, flags);
>   
>   	if (work_done == 0) {
>   		napi_complete(napi);
> @@ -2128,6 +2129,7 @@ static netdev_tx_t bcmgenet_xmit(struct
>   	struct bcmgenet_tx_ring *ring = NULL;
>   	struct enet_cb *tx_cb_ptr;
>   	struct netdev_queue *txq;
> +	unsigned long flags;
>   	int nr_frags, index;
>   	dma_addr_t mapping;
>   	unsigned int size;
> @@ -2149,7 +2151,7 @@ static netdev_tx_t bcmgenet_xmit(struct
>   
>   	nr_frags = skb_shinfo(skb)->nr_frags;
>   
> -	spin_lock(&ring->lock);
> +	spin_lock_irqsave(&ring->lock, flags);
>   	if (ring->free_bds <= (nr_frags + 1)) {
>   		if (!netif_tx_queue_stopped(txq))
>   			netif_tx_stop_queue(txq);
> @@ -2239,7 +2241,7 @@ static netdev_tx_t bcmgenet_xmit(struct
>   		bcmgenet_tdma_ring_writel(priv, ring->index,
>   					  ring->prod_index, TDMA_PROD_INDEX);
>   out:
> -	spin_unlock(&ring->lock);
> +	spin_unlock_irqrestore(&ring->lock, flags);
>   
>   	return ret;
>   


--
Florian