Re: [PATCH net] pcnet32: stop holding device spin lock during napi_complete_done

[Alexander Lobakin <aleksander.lobakin@intel.com>]

From: Oscar Maes <oscmaes92@gmail.com>
Date: Thu, 28 May 2026 16:03:20 +0200

> napi_complete_done may call gro_flush_normal (though not currently, as GRO
> is unsupported at the moment), which may result in packet TX. This will
> eventually result in calling pcnet32_start_xmit - resulting in a deadlock
> while trying to re-acquire the already locked spin lock.
> 
> It is safe to split the spinlock block into two, because the hardware
> registers are still protected from concurrent access, and the two blocks
> perform unrelated operations that don't need to happen atomically.
> 
> Fixes: 5b2ec6f2be51 ("pcnet32: use napi_complete_done()")
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Oscar Maes <oscmaes92@gmail.com>
> ---
> NOTE: This patch was a part of the following net-next series:
> https://lore.kernel.org/netdev/20260525125437.4061-2-oscmaes92@gmail.com/
> 
>  drivers/net/ethernet/amd/pcnet32.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/amd/pcnet32.c b/drivers/net/ethernet/amd/pcnet32.c
> index 911808ab13a7..4f3076d4ea34 100644
> --- a/drivers/net/ethernet/amd/pcnet32.c
> +++ b/drivers/net/ethernet/amd/pcnet32.c
> @@ -1407,8 +1407,10 @@ static int pcnet32_poll(struct napi_struct *napi, int budget)
>  		pcnet32_restart(dev, CSR0_START);
>  		netif_wake_queue(dev);
>  	}
> +	spin_unlock_irqrestore(&lp->lock, flags);
>  
>  	if (work_done < budget && napi_complete_done(napi, work_done)) {
> +		spin_lock_irqsave(&lp->lock, flags);
>  		/* clear interrupt masks */
>  		val = lp->a->read_csr(ioaddr, CSR3);
>  		val &= 0x00ff;
> @@ -1416,9 +1418,9 @@ static int pcnet32_poll(struct napi_struct *napi, int budget)
>  
>  		/* Set interrupt enable. */
>  		lp->a->write_csr(ioaddr, CSR0, CSR0_INTEN);
> +		spin_unlock_irqrestore(&lp->lock, flags);
>  	}
>  
> -	spin_unlock_irqrestore(&lp->lock, flags);
>  	return work_done;

While this fix is valid, I'm wondering whether this needs a deeper
rework as it's generally a very bad idea to have IRQs disabled when
NAPI-polling (except for every short sections).

Could these irqoff sections get narrowed down to reading/writing
interrupt registers only?

Thanks,
Olek