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

[Jon Hunter]

On 04/05/2022 12:35, Marc Zyngier wrote:
> Hi Jon,
>
> On Wed, 04 May 2022 12:19:36 +0100,
> Jon Hunter <jonathanh@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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