linux-kernel - Re: [PATCH] pinctrl: ingenic: Convert to immutable irq chip

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, 07 Jun 2022 17:47:19 +0100
From:   Aidan MacDonald <aidanmacdonald.0x0@...il.com>
To:     Paul Cercueil <paul@...pouillou.net>
Cc:     linus.walleij@...aro.org, linux-mips@...r.kernel.org,
        linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] pinctrl: ingenic: Convert to immutable irq chip


Paul Cercueil <paul@...pouillou.net> writes:

> Hi Aidan,
>
> Le mar., juin 7 2022 at 12:05:25 +0100, Aidan MacDonald
> <aidanmacdonald.0x0@...il.com> a écrit :
>> Update the driver to use an immutable IRQ chip to fix this warning:
>>     "not an immutable chip, please consider fixing it!"
>> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@...il.com>
>> ---
>>  drivers/pinctrl/pinctrl-ingenic.c | 33 ++++++++++++++++++-------------
>>  1 file changed, 19 insertions(+), 14 deletions(-)
>> diff --git a/drivers/pinctrl/pinctrl-ingenic.c
>> b/drivers/pinctrl/pinctrl-ingenic.c
>> index 1ca11616db74..37258fb05be3 100644
>> --- a/drivers/pinctrl/pinctrl-ingenic.c
>> +++ b/drivers/pinctrl/pinctrl-ingenic.c
>> @@ -135,7 +135,6 @@ struct ingenic_pinctrl {
>>  struct ingenic_gpio_chip {
>>  	struct ingenic_pinctrl *jzpc;
>>  	struct gpio_chip gc;
>> -	struct irq_chip irq_chip;
>>  	unsigned int irq, reg_base;
>>  };
>> @@ -3419,6 +3418,8 @@ static void ingenic_gpio_irq_enable(struct irq_data
>> *irqd)
>>  	struct ingenic_gpio_chip *jzgc = gpiochip_get_data(gc);
>>  	int irq = irqd->hwirq;
>> +	gpiochip_enable_irq(gc, irq);
>> +
>>  	if (is_soc_or_above(jzgc->jzpc, ID_JZ4770))
>>  		ingenic_gpio_set_bit(jzgc, JZ4770_GPIO_INT, irq, true);
>>  	else if (is_soc_or_above(jzgc->jzpc, ID_JZ4740))
>> @@ -3443,6 +3444,8 @@ static void ingenic_gpio_irq_disable(struct irq_data
>> *irqd)
>>  		ingenic_gpio_set_bit(jzgc, JZ4740_GPIO_SELECT, irq, false);
>>  	else
>>  		ingenic_gpio_set_bit(jzgc, JZ4730_GPIO_GPIER, irq, false);
>> +
>> +	gpiochip_disable_irq(gc, irq);
>>  }
>>  static void ingenic_gpio_irq_ack(struct irq_data *irqd)
>> @@ -3684,6 +3687,20 @@ static void ingenic_gpio_irq_release(struct irq_data
>> *data)
>>  	return gpiochip_relres_irq(gpio_chip, data->hwirq);
>>  }
>> +static const struct irq_chip ingenic_gpio_irqchip = {
>> +	.name			= "gpio",
>> +	.irq_enable		= ingenic_gpio_irq_enable,
>> +	.irq_disable		= ingenic_gpio_irq_disable,
>> +	.irq_unmask		= ingenic_gpio_irq_unmask,
>> +	.irq_mask		= ingenic_gpio_irq_mask,
>> +	.irq_ack		= ingenic_gpio_irq_ack,
>> +	.irq_set_type		= ingenic_gpio_irq_set_type,
>> +	.irq_set_wake		= ingenic_gpio_irq_set_wake,
>> +	.irq_request_resources	= ingenic_gpio_irq_request,
>> +	.irq_release_resources	= ingenic_gpio_irq_release,
>> +	.flags			= IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_IMMUTABLE,
>> +};
>> +
>>  static int ingenic_pinmux_set_pin_fn(struct ingenic_pinctrl *jzpc,
>>  		int pin, int func)
>>  {
>> @@ -4172,20 +4189,8 @@ static int __init ingenic_gpio_probe(struct
>> ingenic_pinctrl *jzpc,
>>  	if (!jzgc->irq)
>>  		return -EINVAL;
>> -	jzgc->irq_chip.name = jzgc->gc.label;
>> -	jzgc->irq_chip.irq_enable = ingenic_gpio_irq_enable;
>> -	jzgc->irq_chip.irq_disable = ingenic_gpio_irq_disable;
>> -	jzgc->irq_chip.irq_unmask = ingenic_gpio_irq_unmask;
>> -	jzgc->irq_chip.irq_mask = ingenic_gpio_irq_mask;
>> -	jzgc->irq_chip.irq_ack = ingenic_gpio_irq_ack;
>> -	jzgc->irq_chip.irq_set_type = ingenic_gpio_irq_set_type;
>> -	jzgc->irq_chip.irq_set_wake = ingenic_gpio_irq_set_wake;
>> -	jzgc->irq_chip.irq_request_resources = ingenic_gpio_irq_request;
>> -	jzgc->irq_chip.irq_release_resources = ingenic_gpio_irq_release;
>> -	jzgc->irq_chip.flags = IRQCHIP_MASK_ON_SUSPEND;
>> -
>>  	girq = &jzgc->gc.irq;
>> -	girq->chip = &jzgc->irq_chip;
>> +	gpio_irq_chip_set_chip(girq, &ingenic_gpio_irqchip);
>
> This will change each irq_chip's name to "gpio", do we want that?
>
> You didn't remove jzgc->irq_chip, so maybe what you could do is
> jzgc->irq_chip = ingenic_gpio_irqchip;
> jzgc->irq_chip.name = jzgc->gc.label;
> gpio_irq_chip_set_chip(girq, &jzgc->irq_chip);
>
> Thoughts?
>
> Cheers,
> -Paul
>

I wondered that myself, but it doesn't seem to affect anything except
what is displayed in /proc/interrupts. Is the name used anywhere else
where it might cause confusion?

The only similar case I could find was pinctrl-microchip-sgpio.c where
microchip_sgpio_register_bank() is called in a loop and registers the
same irq chip repeatedly, so it's probably(?) okay to do this here. It
seems to defeat the point of immutable irqchips if they just have to be
copied anyway...

(btw, I did remove jzgc->irq_chip -- or did I miss something?)

Best regards,
Aidan

>>  	girq->parent_handler = ingenic_gpio_irq_handler;
>>  	girq->num_parents = 1;
>>  	girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents),
>> --
>> 2.35.1
>>