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 <[email protected]>
---
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(-)
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,
--
2.34.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.
--
With Best Regards,
Andy Shevchenko
On Tue, Feb 14, 2023 at 11:52 AM <[email protected]> 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
On Tue, Feb 14, 2023 at 8:36 AM Alexander Stein
<[email protected]> 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 <[email protected]>
Looks good to me, CC to Marc Z.
Reviewed-by: Linus Walleij <[email protected]>
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
Am Mittwoch, 15. Februar 2023, 11:18:06 CET schrieb Linus Walleij:
> On Tue, Feb 14, 2023 at 11:52 AM <[email protected]> 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
--
TQ-Systems GmbH | M?hlstra?e 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht M?nchen, HRB 105018
Gesch?ftsf?hrer: Detlef Schneider, R?diger Stahl, Stefan Schneider
http://www.tq-group.com/
On Wed, 15 Feb 2023 10:19:28 +0000,
Linus Walleij <[email protected]> wrote:
>
> On Tue, Feb 14, 2023 at 8:36 AM Alexander Stein
> <[email protected]> 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 <[email protected]>
>
> 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 <[email protected]>
>
> 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.
--
Without deviation from the norm, progress is not possible.
On Wed, Feb 15, 2023 at 12:09 PM Alexander Stein
<[email protected]> wrote:
> Am Mittwoch, 15. Februar 2023, 11:18:06 CET schrieb Linus Walleij:
> > On Tue, Feb 14, 2023 at 11:52 AM <[email protected]> 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
On Wed, Feb 15, 2023 at 12:16 PM Marc Zyngier <[email protected]> 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
On Wed, Feb 15, 2023 at 12:18 PM Linus Walleij <[email protected]> wrote:
> On Tue, Feb 14, 2023 at 11:52 AM <[email protected]> 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.
--
With Best Regards,
Andy Shevchenko