Message-ID: <CAGETcx_SYJey=a3T_PtDJ_R2KwV=vEqNMBiKeJT5W382LNb=ew@mail.gmail.com>
Date:   Thu, 16 Jul 2020 19:55:26 -0700
From:   Saravana Kannan <saravanak@...gle.com>
To:     Marc Zyngier <maz@...nel.org>
Cc:     John Stultz <john.stultz@...aro.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Jason Cooper <jason@...edaemon.net>,
        Android Kernel Team <kernel-team@...roid.com>,
        lkml <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH v1] irqchip: Add IRQCHIP_MODULE_BEGIN/END helper macros

On Wed, Jun 3, 2020 at 3:12 AM Marc Zyngier <maz@...nel.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@...nel.org> wrote:
> >>
> >> On 2020-04-29 20:04, Saravana Kannan wrote:
> >> > On Wed, Apr 29, 2020 at 2:28 AM Marc Zyngier <maz@...nel.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

Powered by blists - more mailing lists