netdev - Re: [PATCH V5 10/19] net: ks8851: Factor out bus lock 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]

Date:   Thu, 14 May 2020 03:37:49 +0200
From:   Marek Vasut <marex@...x.de>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     netdev@...r.kernel.org, "David S . Miller" <davem@...emloft.net>,
        Lukas Wunner <lukas@...ner.de>, Petr Stetiar <ynezz@...e.cz>,
        YueHaibing <yuehaibing@...wei.com>
Subject: Re: [PATCH V5 10/19] net: ks8851: Factor out bus lock handling

On 5/14/20 3:19 AM, Andrew Lunn wrote:
> On Thu, May 14, 2020 at 02:07:38AM +0200, Marek Vasut wrote:
>> Pull out bus access locking code into separate functions, this is done
>> in preparation for unifying the driver with the parallel bus one. The
>> parallel bus driver does not need heavy mutex locking of the bus and
>> works better with spinlocks, hence prepare these locking functions to
>> be overridden then.
>>
>> Signed-off-by: Marek Vasut <marex@...x.de>
>> Cc: David S. Miller <davem@...emloft.net>
>> Cc: Lukas Wunner <lukas@...ner.de>
>> Cc: Petr Stetiar <ynezz@...e.cz>
>> Cc: YueHaibing <yuehaibing@...wei.com>
> 
>   
>> +/**
>> + * ks8851_lock - register access lock
>> + * @ks: The chip state
>> + * @flags: Spinlock flags
>> + *
>> + * Claim chip register access lock
>> + */
>> +static void ks8851_lock(struct ks8851_net *ks, unsigned long *flags)
>> +{
>> +	mutex_lock(&ks->lock);
>> +}
> 
> Do you actually need flags? It is for spin_lock_irqsave().  Which you
> use when you have a critical section inside an interrupt handler. But
> a mutex cannot protect against an interrupt handler. So there should
> be no need to use spin_lock_irqsave(), spin_lock() should be enough,
> and that does not need flags.

I do need it, the SPI variant of the device uses threaded interrupt
handler and does quite a few heavy operations there (like pumping TX
data across the SPI bus) so it needs the mutex, but the overhead of that
is too much for the parallel bus variant of the chip (which pumps the
data in the start_xmit handler directly) and so that one uses spinlock
both in ks8851_start_xmit_par() and in the IRQ handler.

I had to do it this way (spinlock + pumping data in start_xmit),
otherwise the overhead for the parallel bus option was too big (in the
20% ballpark).