netdev - Re: [net,PATCH v2] net: ks8851: Queue RX packets in IRQ handler instead of disabling BHs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANn89iLKQjD1bxbirwtvzxsfOa-i2qRTGHYH_8_8O-xCusypQQ@mail.gmail.com>
Date: Wed, 1 May 2024 04:39:16 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Marek Vasut <marex@...x.de>, netdev@...r.kernel.org, 
	Ronald Wahl <ronald.wahl@...itan.com>, "David S. Miller" <davem@...emloft.net>, 
	Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>
Subject: Re: [net,PATCH v2] net: ks8851: Queue RX packets in IRQ handler
 instead of disabling BHs

On Wed, May 1, 2024 at 4:10 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Tue, 30 Apr 2024 21:43:34 +0200 Marek Vasut wrote:
> > diff --git a/drivers/net/ethernet/micrel/ks8851.h b/drivers/net/ethernet/micrel/ks8851.h
> > index 31f75b4a67fd7..f311074ea13bc 100644
> > --- a/drivers/net/ethernet/micrel/ks8851.h
> > +++ b/drivers/net/ethernet/micrel/ks8851.h
> > @@ -399,6 +399,7 @@ struct ks8851_net {
> >
> >       struct work_struct      rxctrl_work;
> >
> > +     struct sk_buff_head     rxq;
> >       struct sk_buff_head     txq;
> >       unsigned int            queued_len;
>
> One more round, sorry, this structure has a kdoc, please fill in
> the description for the new member.

Alternative is to use a local (automatic variable on the stack) struct
sk_buff_head,
no need for permanent storage in struct ks8851_net

>
>
> > @@ -408,7 +406,8 @@ static irqreturn_t ks8851_irq(int irq, void *_ks)
> >       if (status & IRQ_LCI)
> >               mii_check_link(&ks->mii);
> >
> > -     local_bh_enable();
> > +     while (!skb_queue_empty(&ks->rxq))
> > +             netif_rx(__skb_dequeue(&ks->rxq));
>
> Personal preference and probably not worth retesting but FWIW I'd write
> this as:
>
>         while ((skb = __skb_dequeue(&ks->rxq)))
>                 netif_rx(skb);
> --
> pw-bot: cr