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

[Oscar Maes <oscmaes92@gmail.com>]

On Mon, Jun 01, 2026 at 07:44:28PM -0700, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> pcnet32: stop holding device spin lock during napi_complete_done
> 
> This commit removes the lp->lock spinlock hold around napi_complete_done()
> to avoid a potential deadlock with pcnet32_start_xmit() if a GRO flush
> were to occur.
> 
> > diff --git a/drivers/net/ethernet/amd/pcnet32.c b/drivers/net/ethernet/amd/pcnet32.c
> > index 911808ab13a79..4f3076d4ea34e 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);
> 
> [Severity: High]
> Does dropping the spinlock before napi_complete_done() introduce a race
> condition with shared IRQs that breaks NAPI interrupt mitigation?
> 
> If napi_complete_done() clears the NAPIF_STATE_SCHED bit, marking the
> NAPI poll as complete, a shared IRQ could fire on another CPU.
> 
> If pcnet32_interrupt() then acquires lp->lock and checks csr0 without
> checking the INTR bit, could it process pending events and successfully
> call napi_schedule_prep()?
> 
> If pcnet32_interrupt() schedules NAPI and releases the lock, this code
> would then resume, re-acquire lp->lock, and unconditionally unmask
> device interrupts.
> 
> This could leave the system in a state where NAPI is scheduled but
> hardware interrupts are enabled, resulting in an interrupt storm on
> every subsequent packet.
> 
> >  		/* 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;
> >  }
> -- 
> pw-bot: cr

The suggested race condition is impossible, even after
calling napi_complete_done all pcnet32 interrupts are masked via CSR3.
pcnet32_interrupt would never be called until the second locked
section in pcnet32_poll, which unmasks the interrupts.