[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.

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 19:07:47 +0200

> 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.

Up to you, you might want to improve these sections later and send to
net-next, since (as far as I can see) they didn't cause any issues
except this one.

Just keep in mind in general that NAPI callbacks should in general
either fully avoid irqoff sections or limit them to the minimum, and
especially should not try to pass skbs to the netstack with the IRQs
disabled and in general not call any NAPI/networking-related functions
under irqoff.

For this particular fix:

Reviewed-by: Alexander Lobakin <aleksander.lobakin@intel.com>

How long has this issue been present in the kernel? You may want to add
"Cc: stable@vger.kernel.org" after the "Fixes:" tag to be marked as a
candidate for backporting to LTSes.

Thanks,
Olek