netdev - Re: [PATCH 02/12 net-next] qca_spi: Improve SPI IRQ handling

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: <badc6cff-7f16-49eb-a600-d99b0e01b92f@gmx.net>
Date: Mon, 18 Dec 2023 11:23:27 +0100
From: Stefan Wahren <wahrenst@....net>
To: Andrew Lunn <andrew@...n.ch>
Cc: "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 02/12 net-next] qca_spi: Improve SPI IRQ handling

Hi Andrew,

Am 17.12.23 um 23:48 schrieb Andrew Lunn:
> On Sun, Dec 17, 2023 at 08:17:56PM +0100, Stefan Wahren wrote:
>> Hi Andrew,
>>
>> Am 17.12.23 um 19:14 schrieb Andrew Lunn:
>>> On Thu, Dec 14, 2023 at 04:09:34PM +0100, Stefan Wahren wrote:
>>>> The functions qcaspi_netdev_open/close are responsible of request &
>>>> free of the SPI interrupt, which wasn't the best choice because
>>>> allocation problems are discovered not during probe. So let us split
>>>> IRQ allocation & enabling, so we can take advantage of a device
>>>> managed IRQ.
>>> Could you replace the kernel thread with a threaded interrupt handler?
>> the kernel thread is responsible for receiving, transmitting and reset
>> handling (there is no GPIO reset in this driver) which must be
>> synchronized along the same SPI interface. The interrupt just signalize
>> a chip reset or a received packet is available.
>>
>> Could you please elaborate this request more in detail:
>> What is the problem with the kernel thread?
>> Why should i use the threaded interrupt as a replacement instead of e.g.
>> workqueue?
>>
>> Please don't get me wrong, but i need to convince my employer for such a
>> big rewrite.
> I don't know this driver, which is why i asked the question. Its just
> a suggestion. Maybe it makes no sense. But there have been other SPI
> based Ethernet drivers which have been simplified by using threaded
> interrupts rather than a kernel thread or a work queue, since the
> interrupt core does all the thread management, and in particular the
> creating and destroying of the thread which drivers often get wrong.
thanks for the explanation. I guess you refer to enc28j60 and i had a
look at commit 995585ecdf42 ("net: enc28j60: Use threaded interrupt
instead of workqueue"). The fact that the qca_spi driver has a kernel
thread which is kind of persistent (see patch 1 of this series) and the
request to change this confused me. So your suggestion is more about the
interrupt handling and not about the kernel thread which handles the SPI
communication.

Yes the usage of threaded IRQ makes sense especially this would allow to
support level triggered interrupts. But i think this complex change
should be a separate series.
> 	 Andrew