All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] gpio: vf610: always set GPIO to input mode when used as interrupt source
@ 2024-04-23  2:28 haibo.chen
  2024-04-23 10:56 ` Bartosz Golaszewski
  2024-04-23 11:40 ` Linus Walleij
  0 siblings, 2 replies; 8+ messages in thread
From: haibo.chen @ 2024-04-23  2:28 UTC (permalink / raw)
  To: linus.walleij, brgl; +Cc: linux-gpio, linux-kernel, imx, haibo.chen

From: Haibo Chen <haibo.chen@nxp.com>

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 <haibo.chen@nxp.com>
---
 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


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] gpio: vf610: always set GPIO to input mode when used as interrupt source
  2024-04-23  2:28 [PATCH] gpio: vf610: always set GPIO to input mode when used as interrupt source haibo.chen
@ 2024-04-23 10:56 ` Bartosz Golaszewski
  2024-04-23 11:40 ` Linus Walleij
  1 sibling, 0 replies; 8+ messages in thread
From: Bartosz Golaszewski @ 2024-04-23 10:56 UTC (permalink / raw)
  To: haibo.chen; +Cc: linus.walleij, linux-gpio, linux-kernel, imx

On Tue, Apr 23, 2024 at 4:28 AM <haibo.chen@nxp.com> wrote:
>
> From: Haibo Chen <haibo.chen@nxp.com>
>
> 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 <haibo.chen@nxp.com>
> ---
>  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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] gpio: vf610: always set GPIO to input mode when used as interrupt source
  2024-04-23  2:28 [PATCH] gpio: vf610: always set GPIO to input mode when used as interrupt source haibo.chen
  2024-04-23 10:56 ` Bartosz Golaszewski
@ 2024-04-23 11:40 ` Linus Walleij
  2024-04-23 12:20   ` Bough Chen
  1 sibling, 1 reply; 8+ messages in thread
From: Linus Walleij @ 2024-04-23 11:40 UTC (permalink / raw)
  To: haibo.chen; +Cc: brgl, linux-gpio, linux-kernel, imx

On Tue, Apr 23, 2024 at 4:28 AM <haibo.chen@nxp.com> wrote:

> From: Haibo Chen <haibo.chen@nxp.com>
>
> 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 <haibo.chen@nxp.com>
> ---
>  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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH] gpio: vf610: always set GPIO to input mode when used as interrupt source
  2024-04-23 11:40 ` Linus Walleij
@ 2024-04-23 12:20   ` Bough Chen
  2024-04-24 11:01     ` Bough Chen
  0 siblings, 1 reply; 8+ messages in thread
From: Bough Chen @ 2024-04-23 12:20 UTC (permalink / raw)
  To: Linus Walleij; +Cc: brgl, linux-gpio, linux-kernel, imx

> -----Original Message-----
> From: Linus Walleij <linus.walleij@linaro.org>
> Sent: 2024年4月23日 19:41
> To: Bough Chen <haibo.chen@nxp.com>
> Cc: brgl@bgdev.pl; linux-gpio@vger.kernel.org; linux-kernel@vger.kernel.org;
> imx@lists.linux.dev
> 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 <haibo.chen@nxp.com> wrote:
> 
> > From: Haibo Chen <haibo.chen@nxp.com>
> >
> > 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 <haibo.chen@nxp.com>
> > ---
> >  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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH] gpio: vf610: always set GPIO to input mode when used as interrupt source
  2024-04-23 12:20   ` Bough Chen
@ 2024-04-24 11:01     ` Bough Chen
  2024-04-25  8:49       ` Bough Chen
  2024-04-25 13:06       ` Bartosz Golaszewski
  0 siblings, 2 replies; 8+ messages in thread
From: Bough Chen @ 2024-04-24 11:01 UTC (permalink / raw)
  To: Linus Walleij; +Cc: brgl, linux-gpio, linux-kernel, imx

> -----Original Message-----
> From: Bough Chen
> Sent: 2024年4月23日 20:21
> To: Linus Walleij <linus.walleij@linaro.org>
> Cc: brgl@bgdev.pl; linux-gpio@vger.kernel.org; linux-kernel@vger.kernel.org;
> imx@lists.linux.dev
> Subject: RE: [PATCH] gpio: vf610: always set GPIO to input mode when used as
> interrupt source
> 
> > -----Original Message-----
> > From: Linus Walleij <linus.walleij@linaro.org>
> > Sent: 2024年4月23日 19:41
> > To: Bough Chen <haibo.chen@nxp.com>
> > Cc: brgl@bgdev.pl; linux-gpio@vger.kernel.org;
> > linux-kernel@vger.kernel.org; imx@lists.linux.dev
> > 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 <haibo.chen@nxp.com> wrote:
> >
> > > From: Haibo Chen <haibo.chen@nxp.com>
> > >
> > > 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 <haibo.chen@nxp.com>
> > > ---
> > >  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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH] gpio: vf610: always set GPIO to input mode when used as interrupt source
  2024-04-24 11:01     ` Bough Chen
@ 2024-04-25  8:49       ` Bough Chen
  2024-04-25 13:06       ` Bartosz Golaszewski
  1 sibling, 0 replies; 8+ messages in thread
From: Bough Chen @ 2024-04-25  8:49 UTC (permalink / raw)
  To: Linus Walleij; +Cc: brgl, linux-gpio, linux-kernel, imx

> -----Original Message-----
> From: Bough Chen
> Sent: 2024年4月24日 19:01
> To: 'Linus Walleij' <linus.walleij@linaro.org>
> Cc: 'brgl@bgdev.pl' <brgl@bgdev.pl>; 'linux-gpio@vger.kernel.org'
> <linux-gpio@vger.kernel.org>; 'linux-kernel@vger.kernel.org'
> <linux-kernel@vger.kernel.org>; 'imx@lists.linux.dev' <imx@lists.linux.dev>
> 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 <linus.walleij@linaro.org>
> > Cc: brgl@bgdev.pl; linux-gpio@vger.kernel.org;
> > linux-kernel@vger.kernel.org; imx@lists.linux.dev
> > Subject: RE: [PATCH] gpio: vf610: always set GPIO to input mode when
> > used as interrupt source
> >
> > > -----Original Message-----
> > > From: Linus Walleij <linus.walleij@linaro.org>
> > > Sent: 2024年4月23日 19:41
> > > To: Bough Chen <haibo.chen@nxp.com>
> > > Cc: brgl@bgdev.pl; linux-gpio@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; imx@lists.linux.dev
> > > 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 <haibo.chen@nxp.com> wrote:
> > >
> > > > From: Haibo Chen <haibo.chen@nxp.com>
> > > >
> > > > 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 <haibo.chen@nxp.com>
> > > > ---
> > > >  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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] gpio: vf610: always set GPIO to input mode when used as interrupt source
  2024-04-24 11:01     ` Bough Chen
  2024-04-25  8:49       ` Bough Chen
@ 2024-04-25 13:06       ` Bartosz Golaszewski
  2024-04-28  2:31         ` Bough Chen
  1 sibling, 1 reply; 8+ messages in thread
From: Bartosz Golaszewski @ 2024-04-25 13:06 UTC (permalink / raw)
  To: Bough Chen; +Cc: Linus Walleij, linux-gpio, linux-kernel, imx

On Wed, Apr 24, 2024 at 1:01 PM Bough Chen <haibo.chen@nxp.com> wrote:
>
> > -----Original Message-----
> > From: Bough Chen
> > Sent: 2024年4月23日 20:21
> > To: Linus Walleij <linus.walleij@linaro.org>
> > Cc: brgl@bgdev.pl; linux-gpio@vger.kernel.org; linux-kernel@vger.kernel.org;
> > imx@lists.linux.dev
> > Subject: RE: [PATCH] gpio: vf610: always set GPIO to input mode when used as
> > interrupt source
> >
> > > -----Original Message-----
> > > From: Linus Walleij <linus.walleij@linaro.org>
> > > Sent: 2024年4月23日 19:41
> > > To: Bough Chen <haibo.chen@nxp.com>
> > > Cc: brgl@bgdev.pl; linux-gpio@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; imx@lists.linux.dev
> > > 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 <haibo.chen@nxp.com> wrote:
> > >
> > > > From: Haibo Chen <haibo.chen@nxp.com>
> > > >
> > > > 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 <haibo.chen@nxp.com>
> > > > ---
> > > >  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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH] gpio: vf610: always set GPIO to input mode when used as interrupt source
  2024-04-25 13:06       ` Bartosz Golaszewski
@ 2024-04-28  2:31         ` Bough Chen
  0 siblings, 0 replies; 8+ messages in thread
From: Bough Chen @ 2024-04-28  2:31 UTC (permalink / raw)
  To: Bartosz Golaszewski; +Cc: Linus Walleij, linux-gpio, linux-kernel, imx

> -----Original Message-----
> From: Bartosz Golaszewski <brgl@bgdev.pl>
> Sent: 2024年4月25日 21:07
> To: Bough Chen <haibo.chen@nxp.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>; linux-gpio@vger.kernel.org;
> linux-kernel@vger.kernel.org; imx@lists.linux.dev
> 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 <haibo.chen@nxp.com> wrote:
> >
> > > -----Original Message-----
> > > From: Bough Chen
> > > Sent: 2024年4月23日 20:21
> > > To: Linus Walleij <linus.walleij@linaro.org>
> > > Cc: brgl@bgdev.pl; linux-gpio@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; imx@lists.linux.dev
> > > Subject: RE: [PATCH] gpio: vf610: always set GPIO to input mode when
> > > used as interrupt source
> > >
> > > > -----Original Message-----
> > > > From: Linus Walleij <linus.walleij@linaro.org>
> > > > Sent: 2024年4月23日 19:41
> > > > To: Bough Chen <haibo.chen@nxp.com>
> > > > Cc: brgl@bgdev.pl; linux-gpio@vger.kernel.org;
> > > > linux-kernel@vger.kernel.org; imx@lists.linux.dev
> > > > 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 <haibo.chen@nxp.com> wrote:
> > > >
> > > > > From: Haibo Chen <haibo.chen@nxp.com>
> > > > >
> > > > > 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 <haibo.chen@nxp.com>
> > > > > ---
> > > > >  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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2024-04-28  2:31 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-23  2:28 [PATCH] gpio: vf610: always set GPIO to input mode when used as interrupt source haibo.chen
2024-04-23 10:56 ` Bartosz Golaszewski
2024-04-23 11:40 ` Linus Walleij
2024-04-23 12:20   ` Bough Chen
2024-04-24 11:01     ` Bough Chen
2024-04-25  8:49       ` Bough Chen
2024-04-25 13:06       ` Bartosz Golaszewski
2024-04-28  2:31         ` Bough Chen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.