Re: [PATCH 2/2] gpiolib: cdev: release IRQs when the gpio chip device is removed

[Herve Codina]

Hi Bartosz

On Thu, 22 Feb 2024 15:51:15 -0800
Saravana Kannan <saravanak@xxxxxxxxxx> wrote:

> On Thu, Feb 22, 2024 at 4:21 AM Bartosz Golaszewski <brgl@xxxxxxxx> wrote:
> >
..
> > > >
> > > > The fix for the user-space issue may be more-or-less correct but the problem is
> > > > deeper and this won't fix it for in-kernel users.
> > > >
> > > > Herve: please consider the following DT snippet:
> > > >
> > > >       gpio0 {
> > > >               compatible = "foo";
> > > >
> > > >               gpio-controller;
> > > >               #gpio-cells = <2>;
> > > >               interrupt-controller;
> > > >               #interrupt-cells = <1>;
> > > >               ngpios = <8>;
> > > >       };
> > > >
> > > >       consumer {
> > > >               compatible = "bar";
> > > >
> > > >               interrupts-extended = <&gpio0 0>;
> > > >       };
> > > >
> > > > 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.
> 
> [1] - https://lore.kernel.org/lkml/86sghas7so.wl-maz@xxxxxxxxxx/#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()?
> 
> 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".
> 
> > 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?  
> 
> Yeah, we are screwed in those cases. Ideally we are rejecting all
> submissions for irqchip drivers that use IRQCHIP_DECLARE().
> 

I have the feeling that this issue related to your gpio0 driver unbind is out of
the scope of this series.

Let move forward with the user-space fix (cdev) related to this series.
I will sent the v2 to cover the cdev case.

Regards,
Hervé

-- 
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com