| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <90d5a870c46643f6b4654f9c8cbd7740@kernel.org>
Date: Fri, 17 Jul 2020 19:04:15 +0100
From: Marc Zyngier <maz@...nel.org>
To: Saravana Kannan <saravanak@...gle.com>
Cc: Thomas Gleixner <tglx@...utronix.de>,
Jason Cooper <jason@...edaemon.net>,
Matthias Brugger <matthias.bgg@...il.com>,
John Stultz <john.stultz@...aro.org>,
CC Hwang <cc.hwang@...iatek.com>,
Loda Chou <loda.chou@...iatek.com>,
Hanks Chen <hanks.chen@...iatek.com>,
Android Kernel Team <kernel-team@...roid.com>,
LKML <linux-kernel@...r.kernel.org>,
linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
"moderated list:ARM/Mediatek SoC support"
<linux-mediatek@...ts.infradead.org>
Subject: Re: [PATCH v2] irqchip: Add IRQCHIP_PLATFORM_DRIVER_BEGIN/END and
IRQCHIP_MATCH helper macros
On 2020-07-17 18:50, Saravana Kannan wrote:
> On Fri, Jul 17, 2020 at 3:49 AM Marc Zyngier <maz@...nel.org> wrote:
>>
>> Hi Saravana,
>>
>> Thanks for re-spinning this one.
>>
>> On Fri, 17 Jul 2020 03:44:47 +0100,
>> Saravana Kannan <saravanak@...gle.com> wrote:
>> >
>> > Compiling an irqchip driver as a platform driver needs to bunch of
>> > things to be done right:
>> > - Making sure the parent domain is initialized first
>> > - Making sure the device can't be unbound from sysfs
>> > - Disallowing module unload if it's built as a module
>> > - Finding the parent node
>> > - Etc.
>> >
>> > Instead of trying to make sure all future irqchip platform drivers get
>> > this right, provide boilerplate macros that take care of all of this.
>> >
>> > An example use would look something like this. Where acme_foo_init and
>> > acme_bar_init are similar to what would be passed to IRQCHIP_DECLARE.
>> >
>> > IRQCHIP_PLATFORM_DRIVER_BEGIN
>>
>> I think there is some value in having the BEGIN statement containing
>> the driver name, see below.
>>
>> > IRQCHIP_MATCH(foo, "acme,foo", acme_foo_init)
>> > IRQCHIP_MATCH(bar, "acme,bar", acme_bar_init)
>> > IRQCHIP_PLATFORM_DRIVER_END(acme_irq)
>> >
>> > Signed-off-by: Saravana Kannan <saravanak@...gle.com>
>> > ---
>> > drivers/irqchip/irqchip.c | 22 ++++++++++++++++++++++
>> > include/linux/irqchip.h | 23 +++++++++++++++++++++++
>> > 2 files changed, 45 insertions(+)
>> >
>> > diff --git a/drivers/irqchip/irqchip.c b/drivers/irqchip/irqchip.c
>> > index 2b35e68bea82..236ea793f01c 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,23 @@ void __init irqchip_init(void)
>> > of_irq_init(__irqchip_of_table);
>> > acpi_probe_device_table(irqchip);
>> > }
>> > +
>> > +int platform_irqchip_probe(struct platform_device *pdev)
>> > +{
>> > + struct device_node *np = pdev->dev.of_node;
>> > + struct device_node *par_np = 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 (par_np == np)
>> > + par_np = NULL;
>> > +
>> > + /* If there's a parent irqchip, make sure it has been initialized. */
>> > + if (par_np && !irq_find_matching_host(np, DOMAIN_BUS_ANY))
>>
>> There is no guarantee that the calling driver wants BUS_ANY as a token
>> for its parent. It may work for now, if you only have dependencies to
>> drivers that only expose a single domain, but that's not a general
>> purpose check..
>>
>> At least, please add a comment saying that the new driver may want to
>> check that the irqdomain it depends on may not be available.
>
> This is just checking if the parent interrupt controller has been
> initialized. It's just saying that if NONE of the parent irq domains
> have been registered, it's not time for this interrupt controller to
> initialize. And yes, as you said, the actual init code can do more
> checks and defer probe too. Maybe I'll just put the 2nd sentence as
> the comment.
Sure, go ahead.
>
>>
>> > + return -EPROBE_DEFER;
>> > +
>> > + return irq_init_cb(np, par_np);
>> > +}
>> > +EXPORT_SYMBOL_GPL(platform_irqchip_probe);
>> > diff --git a/include/linux/irqchip.h b/include/linux/irqchip.h
>> > index 950e4b2458f0..6d5eba7cbbb7 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
>> > @@ -26,6 +27,28 @@
>> > */
>> > #define IRQCHIP_DECLARE(name, compat, fn) OF_DECLARE_2(irqchip, name, compat, fn)
>> >
>> > +extern int platform_irqchip_probe(struct platform_device *pdev);
>> > +
>> > +#define IRQCHIP_PLATFORM_DRIVER_BEGIN \
>> > +static const struct of_device_id __irqchip_match_table[] = {
>>
>> How about:
>>
>> #define IRQCHIP_PLATFORM_DRIVER_BEGIN(drv_name) \
>> static const struct of_device_id __irqchip_match_table_##drv_name = {
>>
>> which makes it easier to debug when you want to identify specific
>> structures in an object (otherwise, they all have the same
>> name...). it is also much more pleasing aesthetically ;-).
>
> I totally agree. I wanted BEGIN to have the name and END to not have
> to specify the name. But I couldn't figure out a way to do it. I
> assumed you wouldn't want the names repeated in both BEGIN and END. If
> you are okay with that, I prefer your suggestion too.
I'm perfectly fine having the name in both the BEGIN and END tags.
It has a nice LaTeX twist to it ;-).
>
>> > +
>> > +#define IRQCHIP_MATCH(compat, fn) { .compatible = compat, .data = fn },
>> > +
>> > +#define IRQCHIP_PLATFORM_DRIVER_END(drv_name) \
>> > + {}, \
>> > +}; \
>> > +MODULE_DEVICE_TABLE(of, __irqchip_match_table); \
>> > +static struct platform_driver drv_name##_driver = { \
>>
>> const?
>
> Sure.
>
>> > + .probe = platform_irqchip_probe, \
>> > + .driver = { \
>> > + .name = #drv_name, \
>> > + .owner = THIS_MODULE, \
>> > + .of_match_table = __irqchip_match_table,\
>> > + .suppress_bind_attrs = true, \
>> > + }, \
>> > +}; \
>> > +builtin_platform_driver(drv_name##_driver)
>> > +
>> > /*
>> > * This macro must be used by the different irqchip drivers to declare
>> > * the association between their version and their initialization function.
>> > --
>> > 2.28.0.rc0.105.gf9edc3c819-goog
>> >
>> >
>>
>> Otherwise looks good. When you respin it, it would be good to also
>> submit one user of this API by converting an existing driver, as I'd
>> hate to merge something that has no user.
>
> The only one I know will work is the qcom pdc one from John. So I was
> hoping John would respin his patch if you accept this one or I was
> going to redo it after it shows up on linux-next. Maybe MTK can use
> this too for their other series?
I have queued John's PDC work in irq/irqchip-5.9, which I will get
into -next over the weekend. Feel free to post a patch reworking
his last patch, which will give a very nice overview of what we gain.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
Powered by blists - more mailing lists