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

[Marek Vasut]

On 4/13/26 2:57 PM, Sebastian Andrzej Siewior wrote:
> On 2026-04-12 10:51:25 [-0700], Jakub Kicinski wrote:
> > > Does the backtrace make the problem clearer, with the annotation above ?
> >
> > Sebastian, do you have any recommendation here? tl;dr is that the driver does
> …
>
> What about this:
>
> --- a/drivers/net/ethernet/micrel/ks8851_par.c
> +++ b/drivers/net/ethernet/micrel/ks8851_par.c
> @@ -63,7 +63,7 @@ static void ks8851_lock_par(struct ks8851_net *ks, unsigned long *flags)
>   {
>   	struct ks8851_net_par *ksp = to_ks8851_par(ks);
>   
> -	spin_lock_irqsave(&ksp->lock, *flags);
> +	spin_lock_bh(&ksp->lock);
>   }
>   
>   /**
> @@ -77,7 +77,7 @@ static void ks8851_unlock_par(struct ks8851_net *ks, unsigned long *flags)
>   {
>   	struct ks8851_net_par *ksp = to_ks8851_par(ks);
>   
> -	spin_unlock_irqrestore(&ksp->lock, *flags);
> +	spin_unlock_bh(&ksp->lock);
>   }
>   
>   /**
>
>
> 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.

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

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