linux-kernel - Re: [PATCH v6 00/12] gpio: Tight IRQ chip integration

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:   Tue, 7 Nov 2017 10:49:58 -0600
From:   Grygorii Strashko <grygorii.strashko@...com>
To:     Thierry Reding <thierry.reding@...il.com>
CC:     Linus Walleij <linus.walleij@...aro.org>,
        Jonathan Hunter <jonathanh@...dia.com>,
        <linux-gpio@...r.kernel.org>, <linux-tegra@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v6 00/12] gpio: Tight IRQ chip integration



On 11/07/2017 05:52 AM, Thierry Reding wrote:
> On Tue, Nov 07, 2017 at 12:13:44PM +0100, Thierry Reding wrote:
>> On Mon, Nov 06, 2017 at 05:13:33PM -0600, Grygorii Strashko wrote:
>>> On 11/06/2017 05:18 AM, Thierry Reding wrote:
>>>> On Fri, Nov 03, 2017 at 05:30:30PM -0500, Grygorii Strashko wrote:
> [...]
>>>>> @@ -312,8 +321,29 @@ struct gpio_chip {
>>>>>    extern const char *gpiochip_is_requested(struct gpio_chip *chip,
>>>>>    			unsigned offset);
>>>>>    
>>>>> +extern int gpiochip_add_data_key(struct gpio_chip *chip, void *data,
>>>>> +				 struct  *irq_lock_key);
>>>>> +#ifdef CONFIG_LOCKDEP
>>>>> +/*
>>>>> + * Lockdep requires that each irqchip instance be created with a
>>>>> + * unique key so as to avoid unnecessary warnings. This upfront
>>>>> + * boilerplate static inlines provides such a key for each
>>>>> + * unique instance which is created now from inside gpiochip_add_data_key().
>>>>> + */
>>>>> +static inline int gpiochip_add_data(struct gpio_chip *chip, void *data)
>>>>> +{
>>>>> +	static struct lock_class_key key;
>>>>> +
>>>>> +	return gpiochip_add_data_key(chip, data, key);
>>>>> +}
>>>>
>>>> This looks like a neat improvement, but I think it can be done in a
>>>> follow-up to remove the boilerplate in drivers.
>>>
>>> Can't agree here - it better to be considered now.
>>> Now only two GPIO drivers define lock_class_key:
>>> ./drivers/gpio/gpio-bcm-kona.c:static struct lock_class_key gpio_lock_class;
>>> ./drivers/gpio/gpio-brcmstb.c:static struct lock_class_key brcmstb_gpio_irq_lock_class;
>>>
>>> and these drivers do not use gpioirq framework (your tegra driver will be the third).
>>>
>>> So, if proposed changes will be applied all drivers switched to use it will need to define
>>> its own lock_class_key again and it will be step back.
>>
>> I think this would be a minor, mostly mechanical refactoring to do as
>> follow-up. But since you feel very strongly about it, I'll add that into
>> the series.
> 
> After implementing this, I'm having second thoughts. We've got a bunch
> of drivers calling gpiochip_add_data() that never register an IRQ chip
> but which will each add a struct lock_class_key after this change, and
> it will never be used. Now, struct lock_class_key is only 8 bytes big,
> so maybe this isn't a big deal, but it still seems like a waste.

True. And this I've called my approach not ideal, but I do not see other way to do it :(
- that's price to pay for gpioirq chip initialization integration in
gpiochip_add_data() which limits APIs variation used by GPIO drivers. 

Any other opinions, thoughts?

-- 
regards,
-grygorii