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

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

[Saravana Kannan]

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.

Thanks,
Saravana

 drivers/irqchip/irqchip.c | 20 ++++++++++++++++++++
 include/linux/irqchip.h   | 30 +++++++++++++++++++++++++++++-
 2 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irqchip.c b/drivers/irqchip/irqchip.c
index 2b35e68bea82..191b605c72ef 100644
--- a/drivers/irqchip/irqchip.c
+++ b/drivers/irqchip/irqchip.c
@@ -10,8 +10,10 @@
 
 #include <linux/acpi.h>
 #include <linux/init.h>
+#include <linux/of_device.h>
 #include <linux/of_irq.h>
 #include <linux/irqchip.h>
+#include <linux/platform_device.h>
 
 /*
  * This special of_device_id is the sentinel at the end of the
@@ -29,3 +31,21 @@ void __init irqchip_init(void)
 	of_irq_init(__irqchip_of_table);
 	acpi_probe_device_table(irqchip);
 }
+
+#ifdef CONFIG_MODULES
+int platform_irqchip_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct device_node *pnp = of_irq_find_parent(np);
+	of_irq_init_cb_t irq_init_cb = of_device_get_match_data(&pdev->dev);
+
+	if (!irq_init_cb)
+		return -EINVAL;
+
+	if (pnp == np)
+		pnp = NULL;
+
+	return irq_init_cb(np, pnp);
+}
+EXPORT_SYMBOL_GPL(platform_irqchip_probe);
+#endif
diff --git a/include/linux/irqchip.h b/include/linux/irqchip.h
index 950e4b2458f0..26b62843cade 100644
--- a/include/linux/irqchip.h
+++ b/include/linux/irqchip.h
@@ -13,6 +13,7 @@
 
 #include <linux/acpi.h>
 #include <linux/of.h>
+#include <linux/platform_device.h>
 
 /*
  * This macro must be used by the different irqchip drivers to declare
@@ -24,7 +25,34 @@
  * @compstr: compatible string of the irqchip driver
  * @fn: initialization function
  */
-#define IRQCHIP_DECLARE(name, compat, fn) OF_DECLARE_2(irqchip, name, compat, fn)
+#ifndef MODULE
+
+#define IRQCHIP_MODULE_BEGIN
+#define IRQCHIP_DECLARE(name, compat, fn) OF_DECLARE_2(irqchip, name, compat, fn);
+#define IRQCHIP_MODULE_END(drv_name)
+
+#else
+
+extern int platform_irqchip_probe(struct platform_device *pdev);
+
+#define IRQCHIP_MODULE_BEGIN	\
+static const struct of_device_id __irqchip_match_table[] = {
+
+#define IRQCHIP_DECLARE(name, compat, fn) { .compatible = compat, .data = fn },
+
+#define IRQCHIP_MODULE_END(drv_name)			\
+	{},						\
+};							\
+MODULE_DEVICE_TABLE(of, __irqchip_match_table);		\
+static struct platform_driver drv_name##_driver = {	\
+	.probe  = platform_irqchip_probe,		\
+	.driver = {					\
+		.name = #drv_name,			\
+		.of_match_table = __irqchip_match_table,\
+	},						\
+};							\
+module_platform_driver(drv_name##_driver);
+#endif
 
 /*
  * This macro must be used by the different irqchip drivers to declare
-- 
2.26.0.110.g2183baf09c-goog

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

[Marc Zyngier]

Hi Saravana,

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?

Thanks,

	M.

-- 
Jazz is not dead, it just smells funny.

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

[John Stultz]

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

[1]: See:
  https://arstechnica.com/gadgets/2019/11/google-outlines-plans-for-mainline-linux-kernel-support-in-android/
  https://www.linuxplumbersconf.org/event/2/contributions/61/attachments/69/80/Android_and_Linux_Kernel__Herding_billions_of_penguins_one_version_at_a_time.pdf
  https://linuxplumbersconf.org/event/4/contributions/401/attachments/318/542/GKI_Progress.pdf

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

[Saravana Kannan]

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.

-Saravana

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@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?

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...

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@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

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

[Marc Zyngier]

On 2020-04-29 20:04, Saravana Kannan wrote:
> On Wed, Apr 29, 2020 at 2:28 AM Marc Zyngier <maz@kernel.org> wrote:

[...]

>> 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.

As long as you don't try to modularise a driver that does both DT and
ACPI, you'll be safe. I'm also actively trying to discourage people
from inventing custom irqchips on ACPI platforms (the spec almost
forbids them, but not quite).

>> 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.

Uhuh. I'm afraid that's not a practical solution. We need to offer the
same behaviour for both and not rely on the user to understand the
topology of the SoC.

> 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.

Seems like an interesting option. Two things then:

- Can we enforce the use of fw_devlink for modularized irqchips?
- For those irqchips that can be modularized, it is apparent that they
   should have been written as platform devices the first place. Maybe
   we should just do that (long term, though).

Thanks,

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

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

[Saravana Kannan]

On Fri, May 1, 2020 at 1:48 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On 2020-04-29 20:04, Saravana Kannan wrote:
> > On Wed, Apr 29, 2020 at 2:28 AM Marc Zyngier <maz@kernel.org> wrote:
>
> [...]
>
> >> 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.
>
> As long as you don't try to modularise a driver that does both DT and
> ACPI, you'll be safe. I'm also actively trying to discourage people
> from inventing custom irqchips on ACPI platforms (the spec almost
> forbids them, but not quite).
>
> >> 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.
>
> Uhuh. I'm afraid that's not a practical solution. We need to offer the
> same behaviour for both and not rely on the user to understand the
> topology of the SoC.
>
> > 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.
>
> Seems like an interesting option. Two things then:
>
> - Can we enforce the use of fw_devlink for modularized irqchips?

fw_devlink doesn't have any config and it's a command line option. So
not sure how you can enforce that.

> - For those irqchips that can be modularized, it is apparent that they
>    should have been written as platform devices the first place. Maybe
>    we should just do that (long term, though).

I agree. If they can be platform devices, they should be. But when
those platform device drivers are built in, you'll either need:
1) fw_devlink=on to enforce the topological init order
Or
2) have a generic irqchip probe helper function that ensures that.
My patch with some additional checks added to platform_irqchip_probe()
can provide (2).

In the short term, my patch series also makes it easier to convert
existing non-platform drivers into platform drivers.

So if I fix up platform_irqchip_probe() to also do -EPROBE_DEFER to
enforce topology, will that make this patch acceptable?

-Saravana

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

[Saravana Kannan]

On Fri, May 1, 2020 at 1:23 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Fri, May 1, 2020 at 1:48 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On 2020-04-29 20:04, Saravana Kannan wrote:
> > > On Wed, Apr 29, 2020 at 2:28 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > [...]
> >
> > >> 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.
> >
> > As long as you don't try to modularise a driver that does both DT and
> > ACPI, you'll be safe. I'm also actively trying to discourage people
> > from inventing custom irqchips on ACPI platforms (the spec almost
> > forbids them, but not quite).
> >
> > >> 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.
> >
> > Uhuh. I'm afraid that's not a practical solution. We need to offer the
> > same behaviour for both and not rely on the user to understand the
> > topology of the SoC.
> >
> > > 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.
> >
> > Seems like an interesting option. Two things then:
> >
> > - Can we enforce the use of fw_devlink for modularized irqchips?
>
> fw_devlink doesn't have any config and it's a command line option. So
> not sure how you can enforce that.
>
> > - For those irqchips that can be modularized, it is apparent that they
> >    should have been written as platform devices the first place. Maybe
> >    we should just do that (long term, though).
>
> I agree. If they can be platform devices, they should be. But when
> those platform device drivers are built in, you'll either need:
> 1) fw_devlink=on to enforce the topological init order
> Or
> 2) have a generic irqchip probe helper function that ensures that.
> My patch with some additional checks added to platform_irqchip_probe()
> can provide (2).
>
> In the short term, my patch series also makes it easier to convert
> existing non-platform drivers into platform drivers.
>
> So if I fix up platform_irqchip_probe() to also do -EPROBE_DEFER to
> enforce topology, will that make this patch acceptable?
>

Friendly reminder.

-Saravana

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

[Marc Zyngier]

Hi Saravana,

On 2020-05-01 21:23, Saravana Kannan wrote:
> On Fri, May 1, 2020 at 1:48 AM Marc Zyngier <maz@kernel.org> wrote:
>> 
>> On 2020-04-29 20:04, Saravana Kannan wrote:
>> > On Wed, Apr 29, 2020 at 2:28 AM Marc Zyngier <maz@kernel.org> wrote:
>> 
>> [...]
>> 
>> >> 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.
>> 
>> As long as you don't try to modularise a driver that does both DT and
>> ACPI, you'll be safe. I'm also actively trying to discourage people
>> from inventing custom irqchips on ACPI platforms (the spec almost
>> forbids them, but not quite).
>> 
>> >> 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.
>> 
>> Uhuh. I'm afraid that's not a practical solution. We need to offer the
>> same behaviour for both and not rely on the user to understand the
>> topology of the SoC.
>> 
>> > 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.
>> 
>> Seems like an interesting option. Two things then:
>> 
>> - Can we enforce the use of fw_devlink for modularized irqchips?
> 
> fw_devlink doesn't have any config and it's a command line option. So
> not sure how you can enforce that.

By having a config option that forces it on if that option is selected
by modular irqchips? More importantly, what is the drawback of having
fw_devlink on at all times? It definitely looks like the best thing
since sliced bread (with cheese), so what is the catch?

> 
>> - For those irqchips that can be modularized, it is apparent that they
>>    should have been written as platform devices the first place. Maybe
>>    we should just do that (long term, though).
> 
> I agree. If they can be platform devices, they should be. But when
> those platform device drivers are built in, you'll either need:
> 1) fw_devlink=on to enforce the topological init order

That would have my preference, provided that there is no drawbacks.

> Or
> 2) have a generic irqchip probe helper function that ensures that.
> My patch with some additional checks added to platform_irqchip_probe()
> can provide (2).
> 
> In the short term, my patch series also makes it easier to convert
> existing non-platform drivers into platform drivers.
> 
> So if I fix up platform_irqchip_probe() to also do -EPROBE_DEFER to
> enforce topology, will that make this patch acceptable?

That'd be a lot better. We also need some guards for things that
cannot be a driver (primary interrupt controllers don't have a struct
device).

Thanks,

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

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

[Saravana Kannan]

On Wed, Jun 3, 2020 at 3:12 AM Marc Zyngier <maz@kernel.org> wrote:
>
> Hi Saravana,
>
> On 2020-05-01 21:23, Saravana Kannan wrote:
> > On Fri, May 1, 2020 at 1:48 AM Marc Zyngier <maz@kernel.org> wrote:
> >>
> >> On 2020-04-29 20:04, Saravana Kannan wrote:
> >> > On Wed, Apr 29, 2020 at 2:28 AM Marc Zyngier <maz@kernel.org> wrote:
> >>
> >> [...]
> >>
> >> >> 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.
> >>
> >> As long as you don't try to modularise a driver that does both DT and
> >> ACPI, you'll be safe. I'm also actively trying to discourage people
> >> from inventing custom irqchips on ACPI platforms (the spec almost
> >> forbids them, but not quite).
> >>
> >> >> 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.
> >>
> >> Uhuh. I'm afraid that's not a practical solution. We need to offer the
> >> same behaviour for both and not rely on the user to understand the
> >> topology of the SoC.
> >>
> >> > 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.
> >>
> >> Seems like an interesting option. Two things then:
> >>
> >> - Can we enforce the use of fw_devlink for modularized irqchips?
> >
> > fw_devlink doesn't have any config and it's a command line option. So
> > not sure how you can enforce that.
>
> By having a config option that forces it on if that option is selected
> by modular irqchips?

Hmmm... not a bad idea. I think this could be useful in general. I'll
look into that separately.

> More importantly, what is the drawback of having
> fw_devlink on at all times? It definitely looks like the best thing
> since sliced bread (with cheese), so what is the catch?

Lol, thanks for the compliment :) My goal is to eventually enable it by default.

But there are a few corner cases on some boards where the DT has a
cycle and if we just blindly create device links for those, some of
the devices will end up not probing. I've slowly been identifying
different corner cases and making sure we drop the "bad link" that's
causing a cycle so that all the devices still probe properly.
Hopefully I can enable it by default soon.

> >
> >> - For those irqchips that can be modularized, it is apparent that they
> >>    should have been written as platform devices the first place. Maybe
> >>    we should just do that (long term, though).
> >
> > I agree. If they can be platform devices, they should be. But when
> > those platform device drivers are built in, you'll either need:
> > 1) fw_devlink=on to enforce the topological init order
>
> That would have my preference, provided that there is no drawbacks.

Since this can't happen yet I went with option 2.

> > Or
> > 2) have a generic irqchip probe helper function that ensures that.
> > My patch with some additional checks added to platform_irqchip_probe()
> > can provide (2).
> >
> > In the short term, my patch series also makes it easier to convert
> > existing non-platform drivers into platform drivers.
> >
> > So if I fix up platform_irqchip_probe() to also do -EPROBE_DEFER to
> > enforce topology, will that make this patch acceptable?
>
> That'd be a lot better. We also need some guards for things that
> cannot be a driver (primary interrupt controllers don't have a struct
> device).

Just sent out v2 of this patch.
https://lore.kernel.org/lkml/20200717024447.3128361-1-saravanak@google.com/T/#u

-Saravana