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

Marek Vasut
2026-04-13T15:31:39Z

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 ?