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

[Marc Zyngier <maz@xxxxxxxxxx>]

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?

> 
>  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).

> +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").

>  	.irq_set_type	= max77620_gpio_set_irq_type,
>  	.irq_bus_lock	= max77620_gpio_bus_lock,
>  	.irq_bus_sync_unlock = max77620_gpio_bus_sync_unlock,
> -	.flags		= IRQCHIP_MASK_ON_SUSPEND,
> +	.irq_print_chip	= max77620_gpio_irq_print_chip,
> +	.flags		= IRQCHIP_IMMUTABLE | IRQCHIP_MASK_ON_SUSPEND,
> +	GPIOCHIP_IRQ_RESOURCE_HELPERS,
>  };
>  
>  static int max77620_gpio_dir_input(struct gpio_chip *gc, unsigned int offset)
> @@ -318,7 +327,7 @@ static int max77620_gpio_probe(struct platform_device *pdev)
>  	mgpio->gpio_chip.base = -1;
>  
>  	girq = &mgpio->gpio_chip.irq;
> -	girq->chip = &max77620_gpio_irqchip;
> +	gpio_irq_chip_set_chip(girq, &max77620_gpio_irqchip);
>  	/* This will let us handle the parent IRQ in the driver */
>  	girq->parent_handler = NULL;
>  	girq->num_parents = 0;
> -- 
> 2.25.1
> 
> 

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.