Re: [net,PATCH v2] net: ks8851: Reinstate disabling of BHs around IRQ handler

[Sebastian Andrzej Siewior]

On 2026-04-13 17:31:34 [+0200], Marek Vasut wrote:
> > I don't see why it needs to disable interrupts.
> 
> Because when the lock is held, the PAR code shouldn't be interrupted by an
> interrupt, otherwise it would completely mess up the state of the KS8851
> MAC. The spinlock does not protect only the IRQ handler, it protects also
> ks8851_start_xmit_par() and ks8851_write_mac_addr() and
> ks8851_read_mac_addr() and ks8851_net_open() and ks8851_net_stop() and other
> sites which call ks8851_lock()/ks8851_unlock() which cannot be executed
> concurrently, but where BHs can be enabled.

I need check this once brain is at full power again. But which
interrupt? Your interrupt is threaded. So that should be okay.

> > ? This seems to be used by
> > the _par driver and the _common part. The comments refer to DMA but I
> > see only FIFO access.
> 
> The KS8851 does its own internal DMA into the SRAM, from which the data are
> copied by the driver into system DRAM.

So this no interrupt involved as "dma completed" and you do your manual
"memcpy".

> > And while at it, I would recommend to
> > 
> > diff --git a/drivers/net/ethernet/micrel/ks8851_common.c b/drivers/net/ethernet/micrel/ks8851_common.c
> > index 8048770958d60..f1c662887646c 100644
> > --- a/drivers/net/ethernet/micrel/ks8851_common.c
> > +++ b/drivers/net/ethernet/micrel/ks8851_common.c
> > @@ -378,9 +378,12 @@ static irqreturn_t ks8851_irq(int irq, void *_ks)
> >   	if (status & IRQ_LCI)
> >   		mii_check_link(&ks->mii);
> > -	if (status & IRQ_RXI)
> > +	if (status & IRQ_RXI) {
> > +		local_bh_disable();
> >   		while ((skb = __skb_dequeue(&rxq)))
> >   			netif_rx(skb);
> > +		local_bh_enable();
> > +	}
> >   	return IRQ_HANDLED;
> >   }
> > 
> > Because otherwise it will kick-off backlog NAPI after every packet if
> > multiple packets are available.
> I think this patch will do the same, but the above should be done for the
> SPI part ?

Yes, both. This the SPI/ Mutex part does not matter. You inject one
packet into netif_rx() then if will add it to its internal NAPI and
schedule a softirq, process it. It would be more efficient to queue
multiple packets and process them all at the local_bh_enable() time.

Sebastian