Re: [PATCH] gpiolib: Avoid side effects in gpio_is_visible()

[Chris Packham <Chris.Packham@xxxxxxxxxxxxxxxxxxx>]

On 12/05/23 19:24, Johan Hovold wrote:
> On Fri, May 12, 2023 at 04:28:06PM +1200, Chris Packham wrote:
>> Calling gpiod_to_irq() creates an irq_desc for the GPIO. This is not
>> something that should happen when just exporting the GPIO via sysfs. The
>> effect of this was observed as triggering a warning in
>> gpiochip_disable_irq() when kexec()ing after exporting a GPIO.
> You need a better explanation as to why this is an issue. What does the
> warning look like for example?

Ironically I had that in my first attempt to address the issue but was 
told it was too much detail. So now I've gone too far the other way. 
I'll include it in the response I'm about to send to LinusW.

>> Remove the call to gpiod_to_irq() from gpio_is_visible(). The actual
>> intended creation of the irq_desc comes via edge_store() when requested
>> by the user.
> And why does that avoid whatever problem you're seeing?
>
>> Fixes: ebbeba120ab2 ("gpio: sysfs: fix gpio attribute-creation race")
> This is clearly not the right Fixes tag. The above commit only moved the
> creation of the attribute to before registering the sysfs device and
> specifically gpiod_to_irq() was used also prior to that commit.
>
> As a matter of fact, back then there was no call to
> irq_create_mapping() in gpiod_to_irq() either. That was added years
> later by commit
>
> 	dc749a09ea5e ("gpiolib: allow gpio irqchip to map irqs dynamically")

OK thanks for doing better research. I know this is a problem at least 
as far back as 5.15 but it's hard to track down exactly how far back it 
goes as there appears to be multiple changes involved. I should probably 
leave out the fixes tag until I've actually convinced people there is a 
problem to be fixed.

>
>> Signed-off-by: Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx>
>> ---
>>
>> Notes:
>>      This is technically a v2 of
>>      https://scanmail.trustwave.com/?c=20988&d=lund5IJBEmmGjG6d8Os5a2IYlSQ938RfCAuZWmdeyA&u=https%3a%2f%2flore%2ekernel%2eorg%2flkml%2f20230510001151%2e3946931-1-chris%2epackham%40alliedtelesis%2eco%2enz%2f
>>      but the solution is so different it's probably best to treat it as a new
>>      patch.
>>
>>   drivers/gpio/gpiolib-sysfs.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
>> index 530dfd19d7b5..f859dcd1cbf3 100644
>> --- a/drivers/gpio/gpiolib-sysfs.c
>> +++ b/drivers/gpio/gpiolib-sysfs.c
>> @@ -362,8 +362,6 @@ static umode_t gpio_is_visible(struct kobject *kobj, struct attribute *attr,
>>   		if (!show_direction)
>>   			mode = 0;
>>   	} else if (attr == &dev_attr_edge.attr) {
>> -		if (gpiod_to_irq(desc) < 0)
>> -			mode = 0;
>>   		if (!show_direction && test_bit(FLAG_IS_OUT, &desc->flags))
>>   			mode = 0;
>>   	}
> Johan