From: Haibo Chen <[email protected]>
Though the default pin configuration is INPUT, but if the prior stage does
configure the pins as OUTPUT, then Linux will not reconfigure the pin as
INPUT.
e.g. When use one pin as interrupt source, and set as low level trigger,
if prior stage already set this pin as OUTPUT low, then will meet interrupt
storm.
So always set GPIO to input mode when used as interrupt source to fix above
case.
Signed-off-by: Haibo Chen <[email protected]>
---
drivers/gpio/gpio-vf610.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c
index 07e5e6323e86..305b0bcdee6f 100644
--- a/drivers/gpio/gpio-vf610.c
+++ b/drivers/gpio/gpio-vf610.c
@@ -214,7 +214,7 @@ static int vf610_gpio_irq_set_type(struct irq_data *d, u32 type)
else
irq_set_handler_locked(d, handle_edge_irq);
- return 0;
+ return port->gc.direction_input(&port->gc, d->hwirq);
}
static void vf610_gpio_irq_mask(struct irq_data *d)
--
2.34.1
On Tue, Apr 23, 2024 at 4:28 AM <[email protected]> wrote:
>
> From: Haibo Chen <[email protected]>
>
> Though the default pin configuration is INPUT, but if the prior stage does
> configure the pins as OUTPUT, then Linux will not reconfigure the pin as
> INPUT.
>
> e.g. When use one pin as interrupt source, and set as low level trigger,
> if prior stage already set this pin as OUTPUT low, then will meet interrupt
> storm.
>
> So always set GPIO to input mode when used as interrupt source to fix above
> case.
>
> Signed-off-by: Haibo Chen <[email protected]>
> ---
> drivers/gpio/gpio-vf610.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c
> index 07e5e6323e86..305b0bcdee6f 100644
> --- a/drivers/gpio/gpio-vf610.c
> +++ b/drivers/gpio/gpio-vf610.c
> @@ -214,7 +214,7 @@ static int vf610_gpio_irq_set_type(struct irq_data *d, u32 type)
> else
> irq_set_handler_locked(d, handle_edge_irq);
>
> - return 0;
> + return port->gc.direction_input(&port->gc, d->hwirq);
> }
>
> static void vf610_gpio_irq_mask(struct irq_data *d)
> --
> 2.34.1
>
Can you use gpiod_direction_output()? Otherwise the flags of the
descriptor will tell a different story.
Also: this doesn't matter here as it's a built-in driver but irq
callbacks accessing gpio_chip is a thing that still needs addressing
as it doesn't use SRCU. :(
Bart
On Tue, Apr 23, 2024 at 4:28 AM <[email protected]> wrote:
> From: Haibo Chen <[email protected]>
>
> Though the default pin configuration is INPUT, but if the prior stage does
> configure the pins as OUTPUT, then Linux will not reconfigure the pin as
> INPUT.
>
> e.g. When use one pin as interrupt source, and set as low level trigger,
> if prior stage already set this pin as OUTPUT low, then will meet interrupt
> storm.
>
> So always set GPIO to input mode when used as interrupt source to fix above
> case.
>
> Signed-off-by: Haibo Chen <[email protected]>
> ---
> drivers/gpio/gpio-vf610.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c
> index 07e5e6323e86..305b0bcdee6f 100644
> --- a/drivers/gpio/gpio-vf610.c
> +++ b/drivers/gpio/gpio-vf610.c
> @@ -214,7 +214,7 @@ static int vf610_gpio_irq_set_type(struct irq_data *d, u32 type)
> else
> irq_set_handler_locked(d, handle_edge_irq);
>
> - return 0;
> + return port->gc.direction_input(&port->gc, d->hwirq);
Just call vf610_gpio_direction_input() instead of indirecting through
gc->direction_input(), no need to jump through the vtable and as
Bartosz says: it just makes that struct vulnerable.
Second:
In this patch also implement gc->get_direction() which is currently
unimplemented. If you are going to change the direction of a
GPIO randomly at runtime then the framework really likes to
have a chance to know the current direction for obvious reasons.
Yours,
Linus Walleij
> -----Original Message-----
> From: Linus Walleij <[email protected]>
> Sent: 2024年4月23日 19:41
> To: Bough Chen <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH] gpio: vf610: always set GPIO to input mode when used as
> interrupt source
>
> On Tue, Apr 23, 2024 at 4:28 AM <[email protected]> wrote:
>
> > From: Haibo Chen <[email protected]>
> >
> > Though the default pin configuration is INPUT, but if the prior stage
> > does configure the pins as OUTPUT, then Linux will not reconfigure the
> > pin as INPUT.
> >
> > e.g. When use one pin as interrupt source, and set as low level
> > trigger, if prior stage already set this pin as OUTPUT low, then will
> > meet interrupt storm.
> >
> > So always set GPIO to input mode when used as interrupt source to fix
> > above case.
> >
> > Signed-off-by: Haibo Chen <[email protected]>
> > ---
> > drivers/gpio/gpio-vf610.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c
> > index 07e5e6323e86..305b0bcdee6f 100644
> > --- a/drivers/gpio/gpio-vf610.c
> > +++ b/drivers/gpio/gpio-vf610.c
> > @@ -214,7 +214,7 @@ static int vf610_gpio_irq_set_type(struct irq_data *d,
> u32 type)
> > else
> > irq_set_handler_locked(d, handle_edge_irq);
> >
> > - return 0;
> > + return port->gc.direction_input(&port->gc, d->hwirq);
>
> Just call vf610_gpio_direction_input() instead of indirecting through
> gc->direction_input(), no need to jump through the vtable and as
> Bartosz says: it just makes that struct vulnerable.
Thanks for your quick review, I will do that in V2.
>
> Second:
>
> In this patch also implement gc->get_direction() which is currently
> unimplemented. If you are going to change the direction of a GPIO randomly at
> runtime then the framework really likes to have a chance to know the current
> direction for obvious reasons.
Yes, will implement gc->get_direction(), if we did this before, then for this case we meet, framework will print out error log, save much debug time.
Best Regards
Haibo Chen
>
> Yours,
> Linus Walleij
> -----Original Message-----
> From: Bough Chen
> Sent: 2024年4月23日 20:21
> To: Linus Walleij <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: RE: [PATCH] gpio: vf610: always set GPIO to input mode when used as
> interrupt source
>
> > -----Original Message-----
> > From: Linus Walleij <[email protected]>
> > Sent: 2024年4月23日 19:41
> > To: Bough Chen <[email protected]>
> > Cc: [email protected]; [email protected];
> > [email protected]; [email protected]
> > Subject: Re: [PATCH] gpio: vf610: always set GPIO to input mode when
> > used as interrupt source
> >
> > On Tue, Apr 23, 2024 at 4:28 AM <[email protected]> wrote:
> >
> > > From: Haibo Chen <[email protected]>
> > >
> > > Though the default pin configuration is INPUT, but if the prior
> > > stage does configure the pins as OUTPUT, then Linux will not
> > > reconfigure the pin as INPUT.
> > >
> > > e.g. When use one pin as interrupt source, and set as low level
> > > trigger, if prior stage already set this pin as OUTPUT low, then
> > > will meet interrupt storm.
> > >
> > > So always set GPIO to input mode when used as interrupt source to
> > > fix above case.
> > >
> > > Signed-off-by: Haibo Chen <[email protected]>
> > > ---
> > > drivers/gpio/gpio-vf610.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c
> > > index 07e5e6323e86..305b0bcdee6f 100644
> > > --- a/drivers/gpio/gpio-vf610.c
> > > +++ b/drivers/gpio/gpio-vf610.c
> > > @@ -214,7 +214,7 @@ static int vf610_gpio_irq_set_type(struct
> > > irq_data *d,
> > u32 type)
> > > else
> > > irq_set_handler_locked(d, handle_edge_irq);
> > >
> > > - return 0;
> > > + return port->gc.direction_input(&port->gc, d->hwirq);
> >
> > Just call vf610_gpio_direction_input() instead of indirecting through
> > gc->direction_input(), no need to jump through the vtable and as
> > Bartosz says: it just makes that struct vulnerable.
>
> Thanks for your quick review, I will do that in V2.
>
> >
> > Second:
> >
> > In this patch also implement gc->get_direction() which is currently
> > unimplemented. If you are going to change the direction of a GPIO
> > randomly at runtime then the framework really likes to have a chance
> > to know the current direction for obvious reasons.
>
> Yes, will implement gc->get_direction(), if we did this before, then for this case
> we meet, framework will print out error log, save much debug time.
Hi Linus,
I implement gc->get_direction() today, for the case to request one gpio as irq, gpio architecture will first
call gpiochip_reqres_irq(), if the ROM or Uboot already config this gpio pin as OUTPUT mode, will get
the following log:
[ 2.714889] gpio gpiochip3: (43850000.gpio): gpiochip_lock_as_irq: tried to flag a GPIO set as output for IRQ
[ 2.724816] gpio gpiochip3: (43850000.gpio): unable to lock HW IRQ 11 for IRQ
[ 2.731972] genirq: Failed to request resources for 2-0050 (irq 211) on irqchip gpio-vf610
Any suggested method to avoid this case? My previous method works because driver lack of gc->get_direction().
Best Regards
Haibo Chen
>
> Best Regards
> Haibo Chen
> >
> > Yours,
> > Linus Walleij
> -----Original Message-----
> From: Bough Chen
> Sent: 2024年4月24日 19:01
> To: 'Linus Walleij' <[email protected]>
> Cc: '[email protected]' <[email protected]>; '[email protected]'
> <[email protected]>; '[email protected]'
> <[email protected]>; '[email protected]' <[email protected]>
> Subject: RE: [PATCH] gpio: vf610: always set GPIO to input mode when used as
> interrupt source
>
> > -----Original Message-----
> > From: Bough Chen
> > Sent: 2024年4月23日 20:21
> > To: Linus Walleij <[email protected]>
> > Cc: [email protected]; [email protected];
> > [email protected]; [email protected]
> > Subject: RE: [PATCH] gpio: vf610: always set GPIO to input mode when
> > used as interrupt source
> >
> > > -----Original Message-----
> > > From: Linus Walleij <[email protected]>
> > > Sent: 2024年4月23日 19:41
> > > To: Bough Chen <[email protected]>
> > > Cc: [email protected]; [email protected];
> > > [email protected]; [email protected]
> > > Subject: Re: [PATCH] gpio: vf610: always set GPIO to input mode when
> > > used as interrupt source
> > >
> > > On Tue, Apr 23, 2024 at 4:28 AM <[email protected]> wrote:
> > >
> > > > From: Haibo Chen <[email protected]>
> > > >
> > > > Though the default pin configuration is INPUT, but if the prior
> > > > stage does configure the pins as OUTPUT, then Linux will not
> > > > reconfigure the pin as INPUT.
> > > >
> > > > e.g. When use one pin as interrupt source, and set as low level
> > > > trigger, if prior stage already set this pin as OUTPUT low, then
> > > > will meet interrupt storm.
> > > >
> > > > So always set GPIO to input mode when used as interrupt source to
> > > > fix above case.
> > > >
> > > > Signed-off-by: Haibo Chen <[email protected]>
> > > > ---
> > > > drivers/gpio/gpio-vf610.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c
> > > > index 07e5e6323e86..305b0bcdee6f 100644
> > > > --- a/drivers/gpio/gpio-vf610.c
> > > > +++ b/drivers/gpio/gpio-vf610.c
> > > > @@ -214,7 +214,7 @@ static int vf610_gpio_irq_set_type(struct
> > > > irq_data *d,
> > > u32 type)
> > > > else
> > > > irq_set_handler_locked(d, handle_edge_irq);
> > > >
> > > > - return 0;
> > > > + return port->gc.direction_input(&port->gc, d->hwirq);
> > >
> > > Just call vf610_gpio_direction_input() instead of indirecting
> > > through
> > > gc->direction_input(), no need to jump through the vtable and as
> > > Bartosz says: it just makes that struct vulnerable.
> >
> > Thanks for your quick review, I will do that in V2.
> >
> > >
> > > Second:
> > >
> > > In this patch also implement gc->get_direction() which is currently
> > > unimplemented. If you are going to change the direction of a GPIO
> > > randomly at runtime then the framework really likes to have a chance
> > > to know the current direction for obvious reasons.
> >
> > Yes, will implement gc->get_direction(), if we did this before, then
> > for this case we meet, framework will print out error log, save much debug
> time.
>
> Hi Linus,
>
> I implement gc->get_direction() today, for the case to request one gpio as irq,
> gpio architecture will first call gpiochip_reqres_irq(), if the ROM or Uboot
> already config this gpio pin as OUTPUT mode, will get the following log:
>
> [ 2.714889] gpio gpiochip3: (43850000.gpio): gpiochip_lock_as_irq: tried to
> flag a GPIO set as output for IRQ
> [ 2.724816] gpio gpiochip3: (43850000.gpio): unable to lock HW IRQ 11 for
> IRQ
> [ 2.731972] genirq: Failed to request resources for 2-0050 (irq 211) on
> irqchip gpio-vf610
>
> Any suggested method to avoid this case? My previous method works because
> driver lack of gc->get_direction().
Hi Linus,
I find gpio-hog can work around this case.
I add the following change, config this gpio as input in gpio-hog, so that when this gpio controller probe, gpiolib will setting the pin to input mode.
+&gpio5 {
+ ptn5110-int-hog {
+ gpio-hog;
+ gpios = <11 GPIO_ACTIVE_LOW>;
+ input;
};
Not sure whether this is an acceptable method. If yes, what I need to do in the gpio-vf610 driver is to implement gc->get_direction() call back.
Best Regards
Haibo Chen
>
> Best Regards
> Haibo Chen
>
> >
> > Best Regards
> > Haibo Chen
> > >
> > > Yours,
> > > Linus Walleij
On Wed, Apr 24, 2024 at 1:01 PM Bough Chen <[email protected]> wrote:
>
> > -----Original Message-----
> > From: Bough Chen
> > Sent: 2024年4月23日 20:21
> > To: Linus Walleij <[email protected]>
> > Cc: [email protected]; [email protected]; [email protected];
> > [email protected]
> > Subject: RE: [PATCH] gpio: vf610: always set GPIO to input mode when used as
> > interrupt source
> >
> > > -----Original Message-----
> > > From: Linus Walleij <[email protected]>
> > > Sent: 2024年4月23日 19:41
> > > To: Bough Chen <[email protected]>
> > > Cc: [email protected]; [email protected];
> > > [email protected]; [email protected]
> > > Subject: Re: [PATCH] gpio: vf610: always set GPIO to input mode when
> > > used as interrupt source
> > >
> > > On Tue, Apr 23, 2024 at 4:28 AM <[email protected]> wrote:
> > >
> > > > From: Haibo Chen <[email protected]>
> > > >
> > > > Though the default pin configuration is INPUT, but if the prior
> > > > stage does configure the pins as OUTPUT, then Linux will not
> > > > reconfigure the pin as INPUT.
> > > >
> > > > e.g. When use one pin as interrupt source, and set as low level
> > > > trigger, if prior stage already set this pin as OUTPUT low, then
> > > > will meet interrupt storm.
> > > >
> > > > So always set GPIO to input mode when used as interrupt source to
> > > > fix above case.
> > > >
> > > > Signed-off-by: Haibo Chen <[email protected]>
> > > > ---
> > > > drivers/gpio/gpio-vf610.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c
> > > > index 07e5e6323e86..305b0bcdee6f 100644
> > > > --- a/drivers/gpio/gpio-vf610.c
> > > > +++ b/drivers/gpio/gpio-vf610.c
> > > > @@ -214,7 +214,7 @@ static int vf610_gpio_irq_set_type(struct
> > > > irq_data *d,
> > > u32 type)
> > > > else
> > > > irq_set_handler_locked(d, handle_edge_irq);
> > > >
> > > > - return 0;
> > > > + return port->gc.direction_input(&port->gc, d->hwirq);
> > >
> > > Just call vf610_gpio_direction_input() instead of indirecting through
> > > gc->direction_input(), no need to jump through the vtable and as
> > > Bartosz says: it just makes that struct vulnerable.
> >
> > Thanks for your quick review, I will do that in V2.
> >
> > >
> > > Second:
> > >
> > > In this patch also implement gc->get_direction() which is currently
> > > unimplemented. If you are going to change the direction of a GPIO
> > > randomly at runtime then the framework really likes to have a chance
> > > to know the current direction for obvious reasons.
> >
> > Yes, will implement gc->get_direction(), if we did this before, then for this case
> > we meet, framework will print out error log, save much debug time.
>
> Hi Linus,
>
> I implement gc->get_direction() today, for the case to request one gpio as irq, gpio architecture will first
> call gpiochip_reqres_irq(), if the ROM or Uboot already config this gpio pin as OUTPUT mode, will get
> the following log:
>
> [ 2.714889] gpio gpiochip3: (43850000.gpio): gpiochip_lock_as_irq: tried to flag a GPIO set as output for IRQ
> [ 2.724816] gpio gpiochip3: (43850000.gpio): unable to lock HW IRQ 11 for IRQ
> [ 2.731972] genirq: Failed to request resources for 2-0050 (irq 211) on irqchip gpio-vf610
>
> Any suggested method to avoid this case? My previous method works because driver lack of gc->get_direction().
>
Can you make the driver default all lines to input when the device is
being registered? Possibly also revert to input when the line is being
freed?
Bartosz
> -----Original Message-----
> From: Bartosz Golaszewski <[email protected]>
> Sent: 2024年4月25日 21:07
> To: Bough Chen <[email protected]>
> Cc: Linus Walleij <[email protected]>; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH] gpio: vf610: always set GPIO to input mode when used as
> interrupt source
>
> On Wed, Apr 24, 2024 at 1:01 PM Bough Chen <[email protected]> wrote:
> >
> > > -----Original Message-----
> > > From: Bough Chen
> > > Sent: 2024年4月23日 20:21
> > > To: Linus Walleij <[email protected]>
> > > Cc: [email protected]; [email protected];
> > > [email protected]; [email protected]
> > > Subject: RE: [PATCH] gpio: vf610: always set GPIO to input mode when
> > > used as interrupt source
> > >
> > > > -----Original Message-----
> > > > From: Linus Walleij <[email protected]>
> > > > Sent: 2024年4月23日 19:41
> > > > To: Bough Chen <[email protected]>
> > > > Cc: [email protected]; [email protected];
> > > > [email protected]; [email protected]
> > > > Subject: Re: [PATCH] gpio: vf610: always set GPIO to input mode
> > > > when used as interrupt source
> > > >
> > > > On Tue, Apr 23, 2024 at 4:28 AM <[email protected]> wrote:
> > > >
> > > > > From: Haibo Chen <[email protected]>
> > > > >
> > > > > Though the default pin configuration is INPUT, but if the prior
> > > > > stage does configure the pins as OUTPUT, then Linux will not
> > > > > reconfigure the pin as INPUT.
> > > > >
> > > > > e.g. When use one pin as interrupt source, and set as low level
> > > > > trigger, if prior stage already set this pin as OUTPUT low, then
> > > > > will meet interrupt storm.
> > > > >
> > > > > So always set GPIO to input mode when used as interrupt source
> > > > > to fix above case.
> > > > >
> > > > > Signed-off-by: Haibo Chen <[email protected]>
> > > > > ---
> > > > > drivers/gpio/gpio-vf610.c | 2 +-
> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/gpio/gpio-vf610.c
> > > > > b/drivers/gpio/gpio-vf610.c index 07e5e6323e86..305b0bcdee6f
> > > > > 100644
> > > > > --- a/drivers/gpio/gpio-vf610.c
> > > > > +++ b/drivers/gpio/gpio-vf610.c
> > > > > @@ -214,7 +214,7 @@ static int vf610_gpio_irq_set_type(struct
> > > > > irq_data *d,
> > > > u32 type)
> > > > > else
> > > > > irq_set_handler_locked(d, handle_edge_irq);
> > > > >
> > > > > - return 0;
> > > > > + return port->gc.direction_input(&port->gc, d->hwirq);
> > > >
> > > > Just call vf610_gpio_direction_input() instead of indirecting
> > > > through
> > > > gc->direction_input(), no need to jump through the vtable and as
> > > > Bartosz says: it just makes that struct vulnerable.
> > >
> > > Thanks for your quick review, I will do that in V2.
> > >
> > > >
> > > > Second:
> > > >
> > > > In this patch also implement gc->get_direction() which is
> > > > currently unimplemented. If you are going to change the direction
> > > > of a GPIO randomly at runtime then the framework really likes to
> > > > have a chance to know the current direction for obvious reasons.
> > >
> > > Yes, will implement gc->get_direction(), if we did this before, then
> > > for this case we meet, framework will print out error log, save much debug
> time.
> >
> > Hi Linus,
> >
> > I implement gc->get_direction() today, for the case to request one
> > gpio as irq, gpio architecture will first call gpiochip_reqres_irq(),
> > if the ROM or Uboot already config this gpio pin as OUTPUT mode, will get the
> following log:
> >
> > [ 2.714889] gpio gpiochip3: (43850000.gpio): gpiochip_lock_as_irq: tried
> to flag a GPIO set as output for IRQ
> > [ 2.724816] gpio gpiochip3: (43850000.gpio): unable to lock HW IRQ 11
> for IRQ
> > [ 2.731972] genirq: Failed to request resources for 2-0050 (irq 211) on
> irqchip gpio-vf610
> >
> > Any suggested method to avoid this case? My previous method works because
> driver lack of gc->get_direction().
> >
>
> Can you make the driver default all lines to input when the device is being
> registered? Possibly also revert to input when the line is being freed?
Yes, this should be a better method, I will have a try.
Thanks
Haibo Chen
>
> Bartosz