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  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).

Powered by blists - more mailing lists