Received-SPF: pass (google.com: domain of linux-kernel+bounces-158570-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240423022814.3951048-1-haibo.chen@nxp.com> <CACRpkdaRxoEZT1_KyJ3QMDgBcciw1XUXKr=cEiPxbcwSnpmyiA@mail.gmail.com>
 <AS1PR04MB9502BE89834E7F9DA3E8D70B90112@AS1PR04MB9502.eurprd04.prod.outlook.com>
 <DU0PR04MB949691D7F68E2F32371B604F90102@DU0PR04MB9496.eurprd04.prod.outlook.com>
In-Reply-To: <DU0PR04MB949691D7F68E2F32371B604F90102@DU0PR04MB9496.eurprd04.prod.outlook.com>
From: Bartosz Golaszewski <brgl@bgdev.pl>
Date: Thu, 25 Apr 2024 15:06:45 +0200
Message-ID: <CAMRc=McQtohbuUSrhH8LiF6EWUzRKyHSObBv4D6VeE87NmdZQQ@mail.gmail.com>
Subject: Re: [PATCH] gpio: vf610: always set GPIO to input mode when used as
 interrupt source
To: Bough Chen <haibo.chen@nxp.com>
Cc: Linus Walleij <linus.walleij@linaro.org>, 
	"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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Apr 24, 2024 at 1:01=E2=80=AFPM Bough Chen <haibo.chen@nxp.com> wro=
te:
>
> > -----Original Message-----
> > From: Bough Chen
> > Sent: 2024=E5=B9=B44=E6=9C=8823=E6=97=A5 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 us=
ed as
> > interrupt source
> >
> > > -----Original Message-----
> > > From: Linus Walleij <linus.walleij@linaro.org>
> > > Sent: 2024=E5=B9=B44=E6=9C=8823=E6=97=A5 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=E2=80=AFAM <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 fo=
r 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 a=
s 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: tri=
ed 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) o=
n 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