Message-ID: <CAMRc=Mf5fRWoOMsJ41vzvE=-vp3wi-Obw=j5fBk3DuQaZNQP2Q@mail.gmail.com>
Date: Tue, 27 Feb 2024 20:27:05 +0100
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Saravana Kannan <saravanak@...gle.com>
Cc: Herve Codina <herve.codina@...tlin.com>, Kent Gibson <warthog618@...il.com>, 
	Linus Walleij <linus.walleij@...aro.org>, linux-gpio@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Luca Ceresoli <luca.ceresoli@...tlin.com>, 
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH 2/2] gpiolib: cdev: release IRQs when the gpio chip device
 is removed

On Fri, Feb 23, 2024 at 12:51 AM Saravana Kannan <saravanak@...gle.com> wrote:
>

[snip]

> > > >
> > > > If you unbind the "gpio0" device after the consumer requested the interrupt,
> > > > 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 did add support to irqchip to use the driver model. See
> IRQCHIP_PLATFORM_DRIVER_BEGIN() and uses of it.  So this makes sure
> the probe ordering is correct.
>
> But when I added that support, there was some pushback on making the
> modules removable[1]. But that's why you'll see that the
> IRQCHIP_PLATFORM_DRIVER_BEGIN() macro set .suppress_bind_attrs = true.
>
> Do you have a way to unregister an interrupt controller in your
> example? If so, how do you unregister it? It shouldn't be too hard to
> extend those macros to add removal support. We could add a
> IRQCHIP_MATCH2() that also takes in an exit() function op that gets
> called on device unbind.
>

The provider in my example is a GPIO chip that registers its own IRQ
domain. The domain is destroyed when the GPIO controller is
unregistered but interrupts can be requested orthogonally to the GPIO
subsystem and we don't have the knowledge about any interrupt users as
the .to_irq() callback is never called. Let me know if anything can be
done in this case.

I used the gpio-sim testing module for it (instantiated over DT) but
any such GPIO chip that is also an interrupt-controller would behave
the same.

> [1] - https://lore.kernel.org/lkml/86sghas7so.wl-maz@kernel.org/#t
>
> > > >
> > > > I would prefer this to be fixed at a lower lever than the GPIOLIB character
> > > > 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.
>
> For your gpio0 device, can you see why __device_release_driver()
> doesn't end up calling device_links_unbind_consumers()?
>

It never gets into the while (device_links_busy(dev)) loop.

> Also, can you look at
> /sys/class/devlink/<bus:gpio0-devicename>--<consumer device name>
> folders and see what the status file says before you try to unbind the
> gpio0 device? It should say "active".
>

It says "available".

If you have some board supported upstream you could use for testing, I
think I could prepare you a DT snippet for testing.

Bart

[snip]

Powered by blists - more mailing lists