Re: [RFC PATCH v1] irqchip: Add IRQCHIP_MODULE_BEGIN/END helper macros

[Saravana Kannan]

On Wed, Apr 29, 2020 at 2:28 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> Hi Saravana,
>
> Sorry for the delay replying.

No worries.

> On Mon, 13 Apr 2020 15:43:31 -0700
> Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
>
> > On Mon, Apr 13, 2020 at 3:13 PM John Stultz <john.stultz@xxxxxxxxxx> wrote:
> > >
> > > On Sat, Apr 11, 2020 at 2:14 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
> > > > On Sat, 11 Apr 2020 05:59:18 +0100,
> > > > Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
> > > > >
> > > > > Add helper macros IRQCHIP_MODULE_BEGIN and IRQCHIP_MODULE_END that add
> > > > > the boilerplate code to be able to compile an irqchip driver as a
> > > > > module.
> > > > >
> > > > > The driver developer just needs to do add IRQCHIP_MODULE_BEGIN and
> > > > > IRQCHIP_MODULE_END(driver_name) around the IRQCHIP_DECLARE macros, like
> > > > > so:
> > > > >
> > > > > IRQCHIP_MODULE_BEGIN
> > > > > IRQCHIP_DECLARE(foo, "acme,foo", acme_foo_init)
> > > > > IRQCHIP_DECLARE(bar, "acme,bar", acme_bar_init)
> > > > > IRQCHIP_MODULE_END(acme_irq)
> > > > >
> > > > > Cc: John Stultz <john.stultz@xxxxxxxxxx>
> > > > > Signed-off-by: Saravana Kannan <saravanak@xxxxxxxxxx>
> > > > > ---
> > > > > I don't expect this patch to be perfect or the final version. But I'd
> > > > > like to introduce macros like this that don't need the driver developer
> > > > > to copy/paste or repeat the same thing (compat string, function name,
> > > > > etc) in multiple places for the driver to work as a module. If the exact
> > > > > style of my macros aren't appealing, I'm open to other suggestions.
> > > > >
> > > > > There are some checkpatch warning about the > 80 columns that my patch
> > > > > doesn't add. There are also checkpatch warnings about the trailing ; in
> > > > > a macro, but I need those for IRQCHIP_DECLARE to work when the driver is
> > > > > builtin.
> > > >
> > > > I think you are looking at the problem from the wrong end, and adding
> > > > syntactic sugar should be the least of your worries. The reason for
> > > > not allowing irqchip drivers to be modular is that there is no
> > > > refcounting in place to prevent drivers from being removed whilst the
> > > > IRQ stack still has irq_desc, irq_data and various other objects
> > > > indirectly referencing the driver.
> > > >
> > > > I'm all for addressing these issues, though it begs the question of
> > > > *why* you want to do this. We have been perfectly happy with built-in
> > > > irqchips so far (they are pretty small, and there aren't millions of
> > > > them), so what changed?
> > > >
> > >
> > > I can't speak for Saravana, but my sense is that moving functionality
> > > to loadable modules is becoming more important for Android devices due
> > > to their efforts to utilize a single kernel image across various
> > > vendor devices[1].  Obviously with mobile device constraints
> > > minimizing the unused vendor code in the kernel image is important,
> > > and modules help there. While the unloading issues you raised are
> > > important, one can still benefit from having a permanent module
> > > (modules that don't have a .remove handler), as they can be only
> > > loaded on the devices that use them. You're also right that irqchip
> > > drivers are fairly small, but one issue is that any code they depend
> > > on also has to be built in if they are not able to be configured as a
> > > module, so by allowing the irqchip driver to be a module, you can also
> > > modularize whatever platform calls are made from that driver.
> >
> > Thanks John. I was planning on digging up the context for GKI and then
> > replying. Looks like you are better at finding that than me :)
> >
> > And I was also going to suggest the same "permanent" module option and
> > also setting up the driver attributes (bind something?) so that the
> > driver can't be unbound from the device either.
> >
> > Marc, does that answer your questions? Sorry for not giving enough
> > context in my original email.
>
> This makes more sense, thanks.
>
> One thing though: this seems to be exclusively DT driven. Have you
> looked into how that would look like for other firmware types such as
> ACPI?

I'm not very familiar with ACPI at all. I've just started to learn
about how it works in the past few months poking at code when I have
some time. So I haven't tried to get this to work with ACPI nor do I
think I'll be able to do that anytime in the near future. I hope that
doesn't block this from being used for DT based platforms.

> Another thing is the handling of dependencies. Statically built
> irqchips are initialized in the right order based on the topology
> described in DT, and are initialized early enough that client devices
> will find their irqchip This doesn't work here, obviously.

Yeah, I read that code thoroughly :)

> How do you
> propose we handle these dependencies, both between irqchip drivers and
> client drivers?

For client drivers, we don't need to do anything. The IRQ apis seem to
already handle -EPROBE_DEFER correctly in this case.

For irqchip drivers, the easy answer can be: Load the IRQ modules
early if you make them modules.

But in my case, I've been testing this with fw_devlink=on. The TL;DR
of "fw_devlink=on" in this context is that the IRQ devices will get
device links created based on "interrupt-parent" property. So, with
the magic of device links, these IRQ devices will probe in the right
topological order without any wasted deferred probe attempts. For
cases without fw_devlink=on, I think I can improve
platform_irqchip_probe() in my patch to check if the parent device has
probed and defer if it hasn't.

-Saravana