netdev - Re: [EXTERNAL] Re: [PATCH 2/2] net: ks8851: Handle softirqs at the end of IRQ thread to fix hang

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]

Date: Tue, 2 Apr 2024 19:29:38 +0200
From: Marek Vasut <marex@...x.de>
To: Ratheesh Kannoth <rkannoth@...vell.com>
Cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
 "David S. Miller" <davem@...emloft.net>,
 Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
 Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 Dmitry Torokhov <dmitry.torokhov@...il.com>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Mark Brown <broonie@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Ronald Wahl <ronald.wahl@...itan.com>, Simon Horman <horms@...nel.org>
Subject: Re: [EXTERNAL] Re: [PATCH 2/2] net: ks8851: Handle softirqs at the
 end of IRQ thread to fix hang

On 4/1/24 4:13 PM, Ratheesh Kannoth wrote:
>> From: Marek Vasut <marex@...x.de>
>> To: Ratheesh Kannoth <rkannoth@...vell.com>
>> Cc: netdev@...r.kernel.org; David S. Miller <davem@...emloft.net>; Uwe
>> This test here has been taken from net/core/dev.c netif_rx() , it is the same
>> one used there around __netif_rx() invocation.
>>
>>>>    	struct ks8851_net *ks = _ks;
>>>>    	unsigned handled = 0;
>>>>    	unsigned long flags;
>>>>    	unsigned int status;
>>>>
>>>> +	if (need_bh_off)
>>>> +		local_bh_disable();
>>> This threaded irq's thread function (ks8851_irq()) will always run in process
>> context, right ?
>>
>> I think so.
>>
>>> Do you need "if(need_bh_off)" loop?
> My bad. Typo. I meant "if (need_bh_off) statement"; not "loop".
> 
>> It is not a loop, it is invoked once. It is here to disable BHs so that the
>> net_rx_action BH wouldn't run until after the spinlock protected section of the
>> IRQ handler. Te net_rx_action may end up calling ks8851_start_xmit_par,
>> which must be called with the spinlock released, otherwise the system would
>> lock up.
> I understand that. My question - will there be a case (currently, without this patch)  ks8851_irq()
> Is called after disabling local BH.  If it is always called without disabled, can we avoid "if" statement.
> altogether?

Aha, I think that makes sense and yes, we can drop the if statement 
altogether. I'll add that into V2.