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

[Oscar Maes <oscmaes92@gmail.com>]

On Thu, May 28, 2026 at 04:55:55PM +0200, Alexander Lobakin wrote:
> 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

Sounds like a good idea, however, I think it's out of scope for this bug fix.