Messages in this thread | | | From | Saravana Kannan <> | Date | Wed, 29 Apr 2020 12:04:55 -0700 | Subject | Re: [RFC PATCH v1] irqchip: Add IRQCHIP_MODULE_BEGIN/END helper macros |
| |
On Wed, Apr 29, 2020 at 2:28 AM Marc Zyngier <maz@kernel.org> wrote: > > Hi Saravana, > > Sorry for the delay replying.
No worries.
> On Mon, 13 Apr 2020 15:43:31 -0700 > Saravana Kannan <saravanak@google.com> wrote: > > > On Mon, Apr 13, 2020 at 3:13 PM John Stultz <john.stultz@linaro.org> wrote: > > > > > > On Sat, Apr 11, 2020 at 2:14 AM Marc Zyngier <maz@kernel.org> wrote: > > > > On Sat, 11 Apr 2020 05:59:18 +0100, > > > > Saravana Kannan <saravanak@google.com> 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@linaro.org> > > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > > > > --- > > > > > 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
| |