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

[Marc Zyngier]

Hi Saravana,

Sorry for the delay replying.

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?

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. How do you
propose we handle these dependencies, both between irqchip drivers and
client drivers?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...