RE: [PATCH] gpio: vf610: always set GPIO to input mode when used as interrupt source

[Bough Chen <haibo.chen@xxxxxxx>]

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