Re: [PATCH] gpio: max77620: Make the irqchip immutable

[Jon Hunter <jonathanh@nvidia.com>]

On 04/05/2022 12:35, Marc Zyngier wrote:
> Hi Jon,
> 
> On Wed, 04 May 2022 12:19:36 +0100,
> Jon Hunter <jonathanh@nvidia.com> wrote:
>>
>> Commit 6c846d026d49 ("gpio: Don't fiddle with irqchips marked as
>> immutable") added a warning to indicate if the gpiolib is altering the
>> internals of irqchips. Following this change the following warning is
>> now observed for the max77620 gpio driver ...
>>
>>   WARNING KERN gpio gpiochip0: (max77620-gpio): not an immutable chip,
>>   	please consider fixing it!
>>
>> Fix the above warning by making the max77620 gpio driver immutable.
> 
> Thanks for looking into this. Comments below.
> 
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>> This warning is observed with v5.18-rc5 and so it would be great if
>> we can fix for v5.18. This is triggering a kernel warning test failure
>> on one of our platforms.
> 
> I'm surprised. This is definitely *not* 5.18 material, and I can't see
> the patches in Linus' tree. Are you sure you're not running -next
> instead?

Ah, you are correct, it was -next and not v5.18-rc5!

>>
>>   drivers/gpio/gpio-max77620.c | 15 ++++++++++++---
>>   1 file changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-max77620.c b/drivers/gpio/gpio-max77620.c
>> index ebf9dea6546b..aa92658780d6 100644
>> --- a/drivers/gpio/gpio-max77620.c
>> +++ b/drivers/gpio/gpio-max77620.c
>> @@ -119,14 +119,23 @@ static void max77620_gpio_bus_sync_unlock(struct irq_data *data)
>>   	mutex_unlock(&gpio->buslock);
>>   }
>>   
>> -static struct irq_chip max77620_gpio_irqchip = {
>> +static void max77620_gpio_irq_print_chip(struct irq_data *data, struct seq_file *p)
>> +{
>> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
>> +
>> +	seq_printf(p, dev_name(gc->parent));
>> +}
>> +
> 
> I'd rather you don't do this. This was added as a band-aid for drivers
> that display a device-specific string in /proc/interrupts, and that we
> cannot fix because the string is in effect an ABI.
> 
> Here, you're going in the opposite direction (making the name
> device-specific while it was constant so far). This has the same
> ABI-breaking effect, and we shouldn't do that. It is also rather
> pointless, as we already have all the required debugging information
> in debugfs (and I assume that this is the reason this is added).


OK, I will drop this.

>> +static const struct irq_chip max77620_gpio_irqchip = {
>>   	.name		= "max77620-gpio",
>>   	.irq_mask	= max77620_gpio_irq_mask,
>>   	.irq_unmask	= max77620_gpio_irq_unmask,
> 
> You seem to be missing the updates for these two functions. Please see
> the updated documentation in commit 5644b66a9c63 ("Documentation:
> Update the recommended pattern for GPIO irqchips").

Yes I see. OK, I will add that.

Thanks! Jon

-- 
nvpublic