netdev - 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] [day] [month] [year] [list]

Message-ID: <a8d51f07-030c-45fe-a99e-fb290488e3a8@denx.de>
Date: Tue, 30 Apr 2024 03:18:17 +0200
From: Marek Vasut <marex@...x.de>
To: Ronald Wahl <ronald.wahl@...itan.com>, netdev@...r.kernel.org
Cc: "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>,
 Simon Horman <horms@...nel.org>
Subject: Re: [PATCH 2/2] net: ks8851: Handle softirqs at the end of IRQ thread
 to fix hang

On 4/29/24 3:50 PM, Ronald Wahl wrote:
> On 29.04.24 15:23, Marek Vasut wrote:
>> On 4/29/24 1:46 PM, Ronald Wahl wrote:
>>> Hi,
>>
>> Hi,
>>
>>> for the spi version of the chip this change now leads to
>>>
>>> [   23.793000] BUG: sleeping function called from invalid context at
>>> kernel/locking/mutex.c:283
>>> [   23.801915] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid:
>>> 857, name: irq/52-eth-link
>>> [   23.810895] preempt_count: 200, expected: 0
>>> [   23.815288] CPU: 0 PID: 857 Comm: irq/52-eth-link Not tainted
>>> 6.6.28-sama5 #1
>>> [   23.822790] Hardware name: Atmel SAMA5
>>> [   23.826717]  unwind_backtrace from show_stack+0xb/0xc
>>> [   23.831992]  show_stack from dump_stack_lvl+0x19/0x1e
>>> [   23.837433]  dump_stack_lvl from __might_resched+0xb7/0xec
>>> [   23.843122]  __might_resched from mutex_lock+0xf/0x2c
>>> [   23.848540]  mutex_lock from ks8851_irq+0x1f/0x164
>>> [   23.853525]  ks8851_irq from irq_thread_fn+0xf/0x28
>>> [   23.858776]  irq_thread_fn from irq_thread+0x93/0x130
>>> [   23.864037]  irq_thread from kthread+0x7f/0x90
>>> [   23.868699]  kthread from ret_from_fork+0x11/0x1c
>>>
>>> Actually the spi driver variant does not suffer from the issue as it has
>>> different locking so we probably should do the
>>> local_bh_disable/local_bh_enable only for the "par" version. What do
>>> you think?
>>
>> Ah sigh, sorry for the breakage. Indeed, the locking is not great here.
>>
>> I am not entirely sure about the local_bh_disable/enable being par only.
>>
>> I will try to prepare some sort of a patch, would you be willing to test
>> it on the SPI variant ?
> 
> Yes, I can help here, thanks. Meanwhile I also have some good understanding
> at least on the TX path because we had some issues here in the past.
> 
> I will come up myself with another fix in the interrupt handler later. We
> currently reset the ISR status flags too late risking a TX queue stall with
> the SPI chip variant. They must be reset immediately after reading them.
> Need
> to wait a bit for field feedback as I was not able to reproduce this
> mysqelf.

This chip really is a gift that keeps on giving ... sigh.

I just sent this patch, you are on CC, please give it a try:

https://patchwork.kernel.org/project/netdevbpf/patch/20240430011518.110416-1-marex@denx.de/