linux-kernel - Re: [PATCH 1/2] regmap: Add hardware spinlock support

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: <CAMz4kuK3C3KOAQ9oUPnV7CWisKLPf5ktP9+a+fmPMLk+ZDkRpA@mail.gmail.com>
Date:   Tue, 31 Oct 2017 19:31:57 +0800
From:   Baolin Wang <baolin.wang@...aro.org>
To:     Mark Brown <broonie@...nel.org>
Cc:     Greg KH <gregkh@...uxfoundation.org>,
        Lee Jones <lee.jones@...aro.org>,
        Arnd Bergmann <arnd@...db.de>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] regmap: Add hardware spinlock support

Hi Mark,

On 31 October 2017 at 18:38, Mark Brown <broonie@...nel.org> wrote:
> On Tue, Oct 31, 2017 at 02:35:55PM +0800, Baolin Wang wrote:
>
> This looks mostly good, a few small things:
>
>> +static void regmap_lock_hwlock(void *__map)
>> +{
>> +     struct regmap *map = __map;
>> +     int ret;
>> +
>> +     if (map->hwlock_timeout)
>> +             ret = hwspin_lock_timeout(map->hwlock, map->hwlock_timeout);
>> +     else
>> +             ret = hwspin_trylock(map->hwlock);
>> +
>> +     if (ret)
>> +             dev_err(map->dev, "Failed to get hwlock %d\n", ret);
>> +}
>
> Given that we have no error handling path on the locks should we be
> supporting timeout mode at all?  Otherwise we should probably add a
> set of error handling paths whenever we take the lock...

It will be more helpful to use the timeout to try more times to get
the hwlock, and we usually do not use hwspin_trylock_xxx(), so we can
remove hwspin_trylock_xxx() support and set timeout as MAX value as
default to avoid adding 'hwlock_timeout' config,
is this OK for you?

>
>> +             if (config->hwlock_mode == HWLOCK_IRQSTATE) {
>> +                     map->lock = regmap_lock_hwlock_irqsave;
>> +                     map->unlock = regmap_unlock_hwlock_irqrestore;
>> +             } else if (config->hwlock_mode == HWLOCK_IRQ) {
>> +                     map->lock = regmap_lock_hwlock_irq;
>> +                     map->unlock = regmap_unlock_hwlock_irq;
>> +             } else {
>> +                     map->lock = regmap_lock_hwlock;
>> +                     map->unlock = regmap_unlock_hwlock;
>> +             }
>
> This should be a switch statement.

Yes, will fix in next version.

>
>> --- a/include/linux/regmap.h
>> +++ b/include/linux/regmap.h
>> @@ -19,6 +19,7 @@
>>  #include <linux/err.h>
>>  #include <linux/bug.h>
>>  #include <linux/lockdep.h>
>> +#include <linux/hwspinlock.h>
>
> We don't actually use hwspinlock.h in the header (the config options are
> all just unsigned ints) so this could be moved to regmap.c with a
> forward declaration of the struct in internal.h.  That way we don't
> force a rebuild of every regmap user when hwspinlock changes.

You are right. I will more the hwspinlock.h to regmap.c file. Very
appreciated for your comments.

-- 
Baolin.wang
Best Regards