Received-SPF: pass (google.com: domain of linux-kernel+bounces-76500-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240220111019.133697-1-herve.codina@bootlin.com>
 <20240220111019.133697-3-herve.codina@bootlin.com> <20240220142959.GA244726@rigel>
 <20240222005744.GA3603@rigel> <20240222010530.GA11949@rigel>
 <CAMRc=MdCm4UXMkzvG17Vd=6ajE+feihgYc66qUNTTKXhN0--dA@mail.gmail.com> <20240222123615.2cbada98@bootlin.com>
In-Reply-To: <20240222123615.2cbada98@bootlin.com>
From: Bartosz Golaszewski <brgl@bgdev.pl>
Date: Thu, 22 Feb 2024 13:21:15 +0100
Message-ID: <CAMRc=McTkVPD-1_5vvPcM2Q15=w+ocki2xTmvQzU-o395A930w@mail.gmail.com>
Subject: Re: [PATCH 2/2] gpiolib: cdev: release IRQs when the gpio chip device
 is removed
To: Herve Codina <herve.codina@bootlin.com>
Cc: Kent Gibson <warthog618@gmail.com>, Linus Walleij <linus.walleij@linaro.org>, 
	linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org, 
	Luca Ceresoli <luca.ceresoli@bootlin.com>, Thomas Petazzoni <thomas.petazzoni@bootlin.com>, 
	Saravana Kannan <saravanak@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Feb 22, 2024 at 12:36=E2=80=AFPM Herve Codina <herve.codina@bootlin=
com> wrote:
>
> Hi Bartosz,
>
> On Thu, 22 Feb 2024 00:31:08 -0800
> Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> > On Thu, 22 Feb 2024 02:05:30 +0100, Kent Gibson <warthog618@gmail.com> =
said:
> > > On Thu, Feb 22, 2024 at 08:57:44AM +0800, Kent Gibson wrote:
> > >> On Tue, Feb 20, 2024 at 10:29:59PM +0800, Kent Gibson wrote:
> > >> > On Tue, Feb 20, 2024 at 12:10:18PM +0100, Herve Codina wrote:
> > >>
> > >> ...
> > >>
> > >> > >  }
> > >> > >
> > >> > > +static int linereq_unregistered_notify(struct notifier_block *n=
b,
> > >> > > +                                     unsigned long action, void=
 *data)
> > >> > > +{
> > >> > > +      struct linereq *lr =3D container_of(nb, struct linereq,
> > >> > > +                                        device_unregistered_nb)=
;
> > >> > > +      int i;
> > >> > > +
> > >> > > +      for (i =3D 0; i < lr->num_lines; i++) {
> > >> > > +              if (lr->lines[i].desc)
> > >> > > +                      edge_detector_stop(&lr->lines[i]);
> > >> > > +      }
> > >> > > +
> > >> >
> > >> > Firstly, the re-ordering in the previous patch creates a race,
> > >> > as the NULLing of the gdev->chip serves to numb the cdev ioctls, s=
o
> > >> > there is now a window between the notifier being called and that n=
umbing,
> > >> > during which userspace may call linereq_set_config() and re-reques=
t
> > >> > the irq.
> > >> >
> > >> > There is also a race here with linereq_set_config().  That can be =
prevented
> > >> > by holding the lr->config_mutex - assuming the notifier is not bei=
ng called
> > >> > from atomic context.
> > >> >
> > >>
> > >> It occurs to me that the fixed reordering in patch 1 would place
> > >> the notifier call AFTER the NULLing of the ioctls, so there will no =
longer
> > >> be any chance of a race with linereq_set_config() - so holding the
> > >> config_mutex semaphore is not necessary.
> > >>
> > >
> > > NULLing -> numbing
> > >
> > > The gdev->chip is NULLed, so the ioctls are numbed.
> > > And I need to let the coffee soak in before sending.
> > >
> > >> In which case this patch is fine - it is only patch 1 that requires
> > >> updating.
> > >>
> > >> Cheers,
> > >> Kent.
> > >
> >
> > The fix for the user-space issue may be more-or-less correct but the pr=
oblem is
> > deeper and this won't fix it for in-kernel users.
> >
> > Herve: please consider the following DT snippet:
> >
> >       gpio0 {
> >               compatible =3D "foo";
> >
> >               gpio-controller;
> >               #gpio-cells =3D <2>;
> >               interrupt-controller;
> >               #interrupt-cells =3D <1>;
> >               ngpios =3D <8>;
> >       };
> >
> >       consumer {
> >               compatible =3D "bar";
> >
> >               interrupts-extended =3D <&gpio0 0>;
> >       };
> >
> > If you unbind the "gpio0" device after the consumer requested the inter=
rupt,
> > you'll get the same splat. And device links will not help you here (on =
that
> > note: Saravana: is there anything we could do about it? Have you even
> > considered making the irqchip subsystem use the driver model in any way=
? Is it
> > even feasible?).
> >
> > I would prefer this to be fixed at a lower lever than the GPIOLIB chara=
cter
> > device.
>
> I think this use case is covered.
> When the consumer device related to the consumer DT node is added, a
> consumer/supplier relationship is created:
> parse_interrupts() parses the 'interrups-extended' property
>   https://elixir.bootlin.com/linux/v6.8-rc1/source/drivers/of/property.c#=
L1316
> and so, of_link_to_phandle() creates the consumer/supplier link.
>   https://elixir.bootlin.com/linux/v6.8-rc1/source/drivers/of/property.c#=
L1316
>
> We that link present, if the supplier is removed, the consumer is removed
> before.
> The consumer should release the interrupt during its remove process (i.e
> explicit in its .remove() or explicit because of a devm_*() call).
>
> At least, it is my understanding.

Well, then it doesn't work, because I literally just tried it before
sending my previous email.

Please try it yourself, you'll see.

Also: an interrupt controller may not even have a device consuming its
DT node (see IRQCHIP_DECLARE()), what happens then?

Bart

>
> Best regards,
> Herv=C3=A9