diff mbox series

[1/1] gpio: vf610: make irq_chip immutable

Message ID 20230214073638.571417-1-alexander.stein@ew.tq-group.com
State New
Headers
Series [1/1] gpio: vf610: make irq_chip immutable |

Commit Message

Alexander Stein Feb. 14, 2023, 7:36 a.m. UTC 
  Since recently, the kernel is nagging about mutable irq_chips:

    "not an immutable chip, please consider fixing it!"

Drop the unneeded copy, flag it as IRQCHIP_IMMUTABLE, add the new
helper functions and call the appropriate gpiolib functions.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
The overall changes are based on commit f1138dacb7ff
("gpio: sch: make irq_chip immutable")

 drivers/gpio/gpio-vf610.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

Comments

Andy Shevchenko Feb. 14, 2023, 10:52 a.m. UTC | #1 
Tue, Feb 14, 2023 at 08:36:38AM +0100, Alexander Stein kirjoitti:
> Since recently, the kernel is nagging about mutable irq_chips:
> 
>     "not an immutable chip, please consider fixing it!"
> 
> Drop the unneeded copy, flag it as IRQCHIP_IMMUTABLE, add the new
> helper functions and call the appropriate gpiolib functions.

...

> The overall changes are based on commit f1138dacb7ff
> ("gpio: sch: make irq_chip immutable")

Nice, but you forgot one crucial detail. You need to mark GPIO resuested
whenever it's locked as IRQ and otherwise when unlocked.
Linus Walleij Feb. 15, 2023, 10:18 a.m. UTC | #2 
On Tue, Feb 14, 2023 at 11:52 AM <andy.shevchenko@gmail.com> wrote:
> Tue, Feb 14, 2023 at 08:36:38AM +0100, Alexander Stein kirjoitti:
> > Since recently, the kernel is nagging about mutable irq_chips:
> >
> >     "not an immutable chip, please consider fixing it!"
> >
> > Drop the unneeded copy, flag it as IRQCHIP_IMMUTABLE, add the new
> > helper functions and call the appropriate gpiolib functions.
>
> ...
>
> > The overall changes are based on commit f1138dacb7ff
> > ("gpio: sch: make irq_chip immutable")
>
> Nice, but you forgot one crucial detail. You need to mark GPIO resuested
> whenever it's locked as IRQ and otherwise when unlocked.

+static const struct irq_chip vf610_irqchip = {
(...)
+       GPIOCHIP_IRQ_RESOURCE_HELPERS,

That's what this macro does ;)

Yours,
Linus Walleij
Linus Walleij Feb. 15, 2023, 10:19 a.m. UTC | #3 
On Tue, Feb 14, 2023 at 8:36 AM Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:

> Since recently, the kernel is nagging about mutable irq_chips:
>
>     "not an immutable chip, please consider fixing it!"
>
> Drop the unneeded copy, flag it as IRQCHIP_IMMUTABLE, add the new
> helper functions and call the appropriate gpiolib functions.
>
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>

Looks good to me, CC to Marc Z.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

We fixed quite a few of these now, Marc do you have an idea about
how much we have left until we can make immutable the default?

Yours,
Linus Walleij
Alexander Stein Feb. 15, 2023, 11:09 a.m. UTC | #4 
Am Mittwoch, 15. Februar 2023, 11:18:06 CET schrieb Linus Walleij:
> On Tue, Feb 14, 2023 at 11:52 AM <andy.shevchenko@gmail.com> wrote:
> > Tue, Feb 14, 2023 at 08:36:38AM +0100, Alexander Stein kirjoitti:
> > > Since recently, the kernel is nagging about mutable irq_chips:
> > >     "not an immutable chip, please consider fixing it!"
> > > 
> > > Drop the unneeded copy, flag it as IRQCHIP_IMMUTABLE, add the new
> > > helper functions and call the appropriate gpiolib functions.
> > 
> > ...
> > 
> > > The overall changes are based on commit f1138dacb7ff
> > > ("gpio: sch: make irq_chip immutable")
> > 
> > Nice, but you forgot one crucial detail. You need to mark GPIO resuested
> > whenever it's locked as IRQ and otherwise when unlocked.
> 
> +static const struct irq_chip vf610_irqchip = {
> (...)
> +       GPIOCHIP_IRQ_RESOURCE_HELPERS,
> 
> That's what this macro does ;)

Does this mean the calls to gpiochip_disable_irq/gpiochip_enable_irq in v2/v3 
are not necessary?

Best regards,
Alexander

> Yours,
> Linus Walleij
Marc Zyngier Feb. 15, 2023, 11:16 a.m. UTC | #5 
On Wed, 15 Feb 2023 10:19:28 +0000,
Linus Walleij <linus.walleij@linaro.org> wrote:
> 
> On Tue, Feb 14, 2023 at 8:36 AM Alexander Stein
> <alexander.stein@ew.tq-group.com> wrote:
> 
> > Since recently, the kernel is nagging about mutable irq_chips:
> >
> >     "not an immutable chip, please consider fixing it!"
> >
> > Drop the unneeded copy, flag it as IRQCHIP_IMMUTABLE, add the new
> > helper functions and call the appropriate gpiolib functions.
> >
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> 
> Looks good to me, CC to Marc Z.

Looks wrong to me. This is missing the explicit callbacks into gpiolib
so that it knows what gets enabled/disabled on mask/unmask.

>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> We fixed quite a few of these now, Marc do you have an idea about
> how much we have left until we can make immutable the default?

I haven't tracked that, and making it the default would probably mean
getting rid of the code that patches the irqchip structures. I'd say
that once -rc1 is out, we replace the polite nag with something
nastier (WARN_ON() of some sort), and push that into -next.

Leave the warning in place for a couple of releases (until the next
LTS), and then drop the patching code. The not-so-nice part is that
that drivers that haven't been fixed will break silently. The good
side is that these drivers will not have been touched over 2 LTS
releases, and are thus most likely abandonware.

Thanks,

	M.
Linus Walleij Feb. 15, 2023, 11:19 a.m. UTC | #6 
On Wed, Feb 15, 2023 at 12:09 PM Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:
> Am Mittwoch, 15. Februar 2023, 11:18:06 CET schrieb Linus Walleij:
> > On Tue, Feb 14, 2023 at 11:52 AM <andy.shevchenko@gmail.com> wrote:
> > > Tue, Feb 14, 2023 at 08:36:38AM +0100, Alexander Stein kirjoitti:
> > > > Since recently, the kernel is nagging about mutable irq_chips:
> > > >     "not an immutable chip, please consider fixing it!"
> > > >
> > > > Drop the unneeded copy, flag it as IRQCHIP_IMMUTABLE, add the new
> > > > helper functions and call the appropriate gpiolib functions.
> > >
> > > ...
> > >
> > > > The overall changes are based on commit f1138dacb7ff
> > > > ("gpio: sch: make irq_chip immutable")
> > >
> > > Nice, but you forgot one crucial detail. You need to mark GPIO resuested
> > > whenever it's locked as IRQ and otherwise when unlocked.
> >
> > +static const struct irq_chip vf610_irqchip = {
> > (...)
> > +       GPIOCHIP_IRQ_RESOURCE_HELPERS,
> >
> > That's what this macro does ;)
>
> Does this mean the calls to gpiochip_disable_irq/gpiochip_enable_irq in v2/v3
> are not necessary?

No I guess I just misunderstood Andy's comments about "mark GPIO requested".
The callbacks to gpiolib are needed just like pointed out by Marc Z in his
answer, these callbacks are indeed needed.

Yours,
Linus Walleij
Linus Walleij Feb. 15, 2023, 11:33 a.m. UTC | #7 
On Wed, Feb 15, 2023 at 12:16 PM Marc Zyngier <maz@kernel.org> wrote:

> > We fixed quite a few of these now, Marc do you have an idea about
> > how much we have left until we can make immutable the default?
>
> I haven't tracked that, and making it the default would probably mean
> getting rid of the code that patches the irqchip structures. I'd say
> that once -rc1 is out, we replace the polite nag with something
> nastier (WARN_ON() of some sort), and push that into -next.
>
> Leave the warning in place for a couple of releases (until the next
> LTS), and then drop the patching code. The not-so-nice part is that
> that drivers that haven't been fixed will break silently. The good
> side is that these drivers will not have been touched over 2 LTS
> releases, and are thus most likely abandonware.

Hmmm I will take a round and fix some more that are simple and
obvious, I know some that are definitely used but just sees low attention
from users.

Yours,
Linus Walleij
Andy Shevchenko Feb. 15, 2023, 11:51 a.m. UTC | #8 
On Wed, Feb 15, 2023 at 12:18 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Feb 14, 2023 at 11:52 AM <andy.shevchenko@gmail.com> wrote:
> > Tue, Feb 14, 2023 at 08:36:38AM +0100, Alexander Stein kirjoitti:
> > > Since recently, the kernel is nagging about mutable irq_chips:
> > >
> > >     "not an immutable chip, please consider fixing it!"
> > >
> > > Drop the unneeded copy, flag it as IRQCHIP_IMMUTABLE, add the new
> > > helper functions and call the appropriate gpiolib functions.
> >
> > ...
> >
> > > The overall changes are based on commit f1138dacb7ff
> > > ("gpio: sch: make irq_chip immutable")
> >
> > Nice, but you forgot one crucial detail. You need to mark GPIO resuested
> > whenever it's locked as IRQ and otherwise when unlocked.
>
> +static const struct irq_chip vf610_irqchip = {
> (...)
> +       GPIOCHIP_IRQ_RESOURCE_HELPERS,
>
> That's what this macro does ;)

Maybe I was unclear, but I meant that the above mentioned macro
requires to have the helpers to be called to enable the GPIO line.
diff mbox series

Patch

diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c
index a429176673e7..e63ca8c85bec 100644
--- a/drivers/gpio/gpio-vf610.c
+++ b/drivers/gpio/gpio-vf610.c
@@ -30,7 +30,6 @@  struct fsl_gpio_soc_data {
 
 struct vf610_gpio_port {
 	struct gpio_chip gc;
-	struct irq_chip ic;
 	void __iomem *base;
 	void __iomem *gpio_base;
 	const struct fsl_gpio_soc_data *sdata;
@@ -237,6 +236,17 @@  static int vf610_gpio_irq_set_wake(struct irq_data *d, u32 enable)
 	return 0;
 }
 
+static const struct irq_chip vf610_irqchip = {
+	.name = "gpio-vf610",
+	.irq_ack = vf610_gpio_irq_ack,
+	.irq_mask = vf610_gpio_irq_mask,
+	.irq_unmask = vf610_gpio_irq_unmask,
+	.irq_set_type = vf610_gpio_irq_set_type,
+	.irq_set_wake = vf610_gpio_irq_set_wake,
+	.flags = IRQCHIP_IMMUTABLE,
+	GPIOCHIP_IRQ_RESOURCE_HELPERS,
+};
+
 static void vf610_gpio_disable_clk(void *data)
 {
 	clk_disable_unprepare(data);
@@ -249,7 +259,6 @@  static int vf610_gpio_probe(struct platform_device *pdev)
 	struct vf610_gpio_port *port;
 	struct gpio_chip *gc;
 	struct gpio_irq_chip *girq;
-	struct irq_chip *ic;
 	int i;
 	int ret;
 
@@ -315,14 +324,6 @@  static int vf610_gpio_probe(struct platform_device *pdev)
 	gc->direction_output = vf610_gpio_direction_output;
 	gc->set = vf610_gpio_set;
 
-	ic = &port->ic;
-	ic->name = "gpio-vf610";
-	ic->irq_ack = vf610_gpio_irq_ack;
-	ic->irq_mask = vf610_gpio_irq_mask;
-	ic->irq_unmask = vf610_gpio_irq_unmask;
-	ic->irq_set_type = vf610_gpio_irq_set_type;
-	ic->irq_set_wake = vf610_gpio_irq_set_wake;
-
 	/* Mask all GPIO interrupts */
 	for (i = 0; i < gc->ngpio; i++)
 		vf610_gpio_writel(0, port->base + PORT_PCR(i));
@@ -331,7 +332,7 @@  static int vf610_gpio_probe(struct platform_device *pdev)
 	vf610_gpio_writel(~0, port->base + PORT_ISFR);
 
 	girq = &gc->irq;
-	girq->chip = ic;
+	gpio_irq_chip_set_chip(girq, &vf610_irqchip);
 	girq->parent_handler = vf610_gpio_irq_handler;
 	girq->num_parents = 1;
 	girq->parents = devm_kcalloc(&pdev->dev, 1,