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

[Marc Zyngier <maz@kernel.org>]

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