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

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

[Oscar Maes]

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;
 }
 
-- 
2.39.5

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

[Alexander Lobakin]

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

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

[Oscar Maes]

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.