| Message ID | 20210924170546.805663-1-f.fainelli@gmail.com |
|---|---|
| Headers | show |
| Series | Modular Broadcom irqchip drivers | expand |
On Fri, 24 Sep 2021 18:05:38 +0100, Florian Fainelli <f.fainelli@gmail.com> wrote: > > irq-bcm7038-l1 uses that symbol and we want to make it a loadable module > in subsequent changes. > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > kernel/irq/manage.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c > index 7405e384e5ed..e0c573e5d249 100644 > --- a/kernel/irq/manage.c > +++ b/kernel/irq/manage.c > @@ -369,6 +369,7 @@ int irq_set_affinity_locked(struct irq_data *data, const struct cpumask *mask, > > return ret; > } > +EXPORT_SYMBOL_GPL(irq_set_affinity_locked); > > /** > * irq_update_affinity_desc - Update affinity management for an interrupt This doesn't seem right. This driver seem to try and move interrupts on its own when the CPU goes down. Why can't it rely on the normal CPU hotplug infrastructure to do so like all the other drivers (bar some Cavium driver that does the same thing)? I'd rather you take this opportunity to move these drivers into the 21st century, so that we can kill irq_cpu_offline() and co altogether. Thanks, M.
On Fri, 24 Sep 2021 18:05:45 +0100, Florian Fainelli <f.fainelli@gmail.com> wrote: > > Now that the various second level interrupt controllers have been moved > to IRQCHIP_PLATFORM_DRIVER and they do default to ARCH_BRCMSTB and > ARCH_BCM2835 where relevant, remove their forced selection from the > machine entry to allow an user to build them as modules. > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > arch/arm64/Kconfig.platforms | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms > index b0ce18d4cc98..2e9440f2da22 100644 > --- a/arch/arm64/Kconfig.platforms > +++ b/arch/arm64/Kconfig.platforms > @@ -44,7 +44,6 @@ config ARCH_BCM2835 > select ARM_AMBA > select ARM_GIC > select ARM_TIMER_SP804 > - select BRCMSTB_L2_IRQ > help > This enables support for the Broadcom BCM2837 and BCM2711 SoC. > These SoCs are used in the Raspberry Pi 3 and 4 devices. > @@ -82,8 +81,6 @@ config ARCH_BITMAIN > config ARCH_BRCMSTB > bool "Broadcom Set-Top-Box SoCs" > select ARCH_HAS_RESET_CONTROLLER > - select BCM7038_L1_IRQ > - select BRCMSTB_L2_IRQ > select GENERIC_IRQ_CHIP > select PINCTRL > help How does the user know about that? People will build a kernel selecting their platform, and find out it doesn't work. This seems terribly counter-productive to me. M.
On Sat, Sep 25, 2021 at 2:10 PM Marc Zyngier <maz@kernel.org> wrote: > On Fri, 24 Sep 2021 18:05:45 +0100, Florian Fainelli <f.fainelli@gmail.com> wrote: > > How does the user know about that? People will build a kernel > selecting their platform, and find out it doesn't work. This seems > terribly counter-productive to me. It's default-enabled on the platforms that need it, see one of the earlier patches. Having the option to make it a loadable module is a definite benefit as far as I'm concerned, and I generally like the idea of having individually selectable symbols for consistency as that is what we have in other subsystems as well. Ideally I'd do away with all the 'select' statements for the platforms and only have them control dependencies as we do for most other subsystems. irqchip is one of the few exceptions here, though I understand the reason for having the most important drivers tied to the platform more closely. Arnd
On Fri, Sep 24 2021 at 10:05, Florian Fainelli wrote: > In order to build drivers/irqchip/irq-bcm7038-l1.c as a module (for use > in GKI), we need to export_to_desc() which is used in this snippet of > code: > > irqd_set_single_target(irq_desc_get_irq_data(irq_to_desc(virq))); > > This effectively reverts 64a1b95bb9fe ("genirq: Restrict export of > irq_to_desc()"). No. I'm not reexporting this. We've spent quite some time to prevent all kind of drivers for fiddle with irq descriptors and I'm not going to reopen that can of worms. irq_get_irq_data() is exported and provides you what you need. Thanks, tglx
On Sat, Sep 25 2021 at 12:48, Marc Zyngier wrote: > On Fri, 24 Sep 2021 18:05:38 +0100, Florian Fainelli <f.fainelli@gmail.com> wrote: >> } >> +EXPORT_SYMBOL_GPL(irq_set_affinity_locked); > > This doesn't seem right. > > This driver seem to try and move interrupts on its own when the CPU > goes down. Why can't it rely on the normal CPU hotplug infrastructure > to do so like all the other drivers (bar some Cavium driver that does > the same thing)? > > I'd rather you take this opportunity to move these drivers into the > 21st century, so that we can kill irq_cpu_offline() and co altogether. I wanted to kill these callbacks years ago. Cavium has two variants of those offline/online callbacks: 1) octeon_irq_cpu_offline_ciu() which is doing the same as that BCM driver. These really can go away. Just remove the callback and everything just works. 2) Two other variants to fiddle with chip internals, but those chips do not have an irq_affinity() callback which makes it more interesting. I don't see a proper way to solve that except for removing Cavium alltogether, but once the BCM one is gone, we just can make this muck depend on CAVIUM and be done with it. And I mean depend and not select. Thanks, tglx
On Sat, Sep 25 2021 at 23:21, Thomas Gleixner wrote: > On Sat, Sep 25 2021 at 12:48, Marc Zyngier wrote: >> On Fri, 24 Sep 2021 18:05:38 +0100, Florian Fainelli <f.fainelli@gmail.com> wrote: >>> } >>> +EXPORT_SYMBOL_GPL(irq_set_affinity_locked); >> >> This doesn't seem right. >> >> This driver seem to try and move interrupts on its own when the CPU >> goes down. Why can't it rely on the normal CPU hotplug infrastructure >> to do so like all the other drivers (bar some Cavium driver that does >> the same thing)? >> >> I'd rather you take this opportunity to move these drivers into the >> 21st century, so that we can kill irq_cpu_offline() and co altogether. > > I wanted to kill these callbacks years ago. Cavium has two variants of > those offline/online callbacks: > > 1) octeon_irq_cpu_offline_ciu() which is doing the same as that BCM > driver. These really can go away. Just remove the callback and > everything just works. For BCM this works today when that chip is used on ARM[64] simply because the only architecture which invokes irq_cpu_offline() is MIPS. Thanks, tglx
On 9/25/2021 2:00 PM, Thomas Gleixner wrote: > On Fri, Sep 24 2021 at 10:05, Florian Fainelli wrote: >> In order to build drivers/irqchip/irq-bcm7038-l1.c as a module (for use >> in GKI), we need to export_to_desc() which is used in this snippet of >> code: >> >> irqd_set_single_target(irq_desc_get_irq_data(irq_to_desc(virq))); >> >> This effectively reverts 64a1b95bb9fe ("genirq: Restrict export of >> irq_to_desc()"). > > No. I'm not reexporting this. We've spent quite some time to prevent all > kind of drivers for fiddle with irq descriptors and I'm not going > to reopen that can of worms. > > irq_get_irq_data() is exported and provides you what you need. That is exactly what I was looking for and somehow missed it during my search the other day, thanks!
On 9/24/2021 10:05 AM, Florian Fainelli wrote: > In order to allow drivers/irqchip/irq-brcmstb-l2.c to be built as a > module we need to export: irq_gc_unmask_enable_reg() and > irq_gc_mask_disable_reg(). Note to self: this needs to come before patch 5 to avoid a modular build linking failure.
On 9/25/21 2:37 PM, Thomas Gleixner wrote: > On Sat, Sep 25 2021 at 23:21, Thomas Gleixner wrote: > >> On Sat, Sep 25 2021 at 12:48, Marc Zyngier wrote: >>> On Fri, 24 Sep 2021 18:05:38 +0100, Florian Fainelli <f.fainelli@gmail.com> wrote: >>>> } >>>> +EXPORT_SYMBOL_GPL(irq_set_affinity_locked); >>> >>> This doesn't seem right. >>> >>> This driver seem to try and move interrupts on its own when the CPU >>> goes down. Why can't it rely on the normal CPU hotplug infrastructure >>> to do so like all the other drivers (bar some Cavium driver that does >>> the same thing)? >>> >>> I'd rather you take this opportunity to move these drivers into the >>> 21st century, so that we can kill irq_cpu_offline() and co altogether. >> >> I wanted to kill these callbacks years ago. Cavium has two variants of >> those offline/online callbacks: >> >> 1) octeon_irq_cpu_offline_ciu() which is doing the same as that BCM >> driver. These really can go away. Just remove the callback and >> everything just works. > > For BCM this works today when that chip is used on ARM[64] simply > because the only architecture which invokes irq_cpu_offline() is MIPS. That is correct. How would you recommend addressing that? In premise when this driver is used on ARM[64] it is used as a second level interrupt controller hanging off the ARM GIC (or another ARM CPU interrupt controller), so in that case I suppose I could make the irq_set_cpu_offline be dependent upon CONFIG_SMP and CONFIG_MIPS, would that be acceptable?
On Mon, Sep 27 2021 at 10:47, Florian Fainelli wrote: > On 9/25/21 2:37 PM, Thomas Gleixner wrote: >>> I wanted to kill these callbacks years ago. Cavium has two variants of >>> those offline/online callbacks: >>> >>> 1) octeon_irq_cpu_offline_ciu() which is doing the same as that BCM >>> driver. These really can go away. Just remove the callback and >>> everything just works. >> >> For BCM this works today when that chip is used on ARM[64] simply >> because the only architecture which invokes irq_cpu_offline() is MIPS. > > That is correct. How would you recommend addressing that? In premise > when this driver is used on ARM[64] it is used as a second level > interrupt controller hanging off the ARM GIC (or another ARM CPU > interrupt controller), so in that case I suppose I could make the > irq_set_cpu_offline be dependent upon CONFIG_SMP and CONFIG_MIPS, would > that be acceptable? Why? Just get rid of the callback in that driver and ensure that irq_migrate_all_off_this_cpu() is invoked when the CPU dies. arch/mips/kernel/smp-cps.c already does that, but I don't know whether your MIPS platform uses those SMP ops. If not you surely have a template there. Thanks, tglx
On 9/27/21 11:18 AM, Thomas Gleixner wrote: > On Mon, Sep 27 2021 at 10:47, Florian Fainelli wrote: >> On 9/25/21 2:37 PM, Thomas Gleixner wrote: >>>> I wanted to kill these callbacks years ago. Cavium has two variants of >>>> those offline/online callbacks: >>>> >>>> 1) octeon_irq_cpu_offline_ciu() which is doing the same as that BCM >>>> driver. These really can go away. Just remove the callback and >>>> everything just works. >>> >>> For BCM this works today when that chip is used on ARM[64] simply >>> because the only architecture which invokes irq_cpu_offline() is MIPS. >> >> That is correct. How would you recommend addressing that? In premise >> when this driver is used on ARM[64] it is used as a second level >> interrupt controller hanging off the ARM GIC (or another ARM CPU >> interrupt controller), so in that case I suppose I could make the >> irq_set_cpu_offline be dependent upon CONFIG_SMP and CONFIG_MIPS, would >> that be acceptable? > > Why? Just get rid of the callback in that driver and ensure that > irq_migrate_all_off_this_cpu() is invoked when the CPU dies. > > arch/mips/kernel/smp-cps.c already does that, but I don't know whether > your MIPS platform uses those SMP ops. If not you surely have a template > there. We use arch/mips/kernel/smp-bmips.c but I do see the path forward, thanks!
On Fri, Sep 24, 2021 at 12:06 PM Florian Fainelli <f.fainelli@gmail.com> wrote: > > In order to allow drivers/irqchip/irq-bcm7038-l1.c to be built as a > module and usable in GKI, export cpu_logical_map or __cpu_logical_map > towards the modules. This is the usage: #ifdef CONFIG_SMP cpu = intc->cpus[cpu_logical_map(smp_processor_id())]; #else cpu = intc->cpus[0]; #endif This is totally broken! cpu_logical_map() takes the logical cpu number, 0-N, and returns the MPIDR which you then use as an array index. Rob
On 9/27/21 12:32 PM, Rob Herring wrote: > On Fri, Sep 24, 2021 at 12:06 PM Florian Fainelli <f.fainelli@gmail.com> wrote: >> >> In order to allow drivers/irqchip/irq-bcm7038-l1.c to be built as a >> module and usable in GKI, export cpu_logical_map or __cpu_logical_map >> towards the modules. > > This is the usage: > > #ifdef CONFIG_SMP > cpu = intc->cpus[cpu_logical_map(smp_processor_id())]; > #else > cpu = intc->cpus[0]; > #endif > > This is totally broken! cpu_logical_map() takes the logical cpu > number, 0-N, and returns the MPIDR which you then use as an array > index. There is no MPIDR on MIPS, which is where this code is being primarily used as-is. On ARM/ARM64 the driver is used as a second level interrupt controller with only a single "bank" of registers as opposed to one per-CPU, meaning that we would always use intc->cpus[0] because you cannot change the interrupt affinity of a second level interrupt controller AFAICT. Maybe the above deserves to be made CONFIG_SMP && CONFIG_MIPS somehow.
Hi Thomas, Marc, This patch series aims at allowing the 3 interrupt controller drivers used on Broadcom STB platforms to be built as modules in order for those to be shipped in a GKI enabled system (Android). The irq-bcm7038-l1 requires us to export a number of symbols, which is not great, but there are not obvious solutions other than adding accessor functions to get the same information. Assuming you are happy with the changes though, please do take the last two changes as well through your tree. Thanks! Florian Fainelli (11): arch: Export cpu_logical_map to modules genirq: Export irq_to_desc() again to modules genirq: Export irq_set_affinity_locked() irqchip/irq-bcm7038-l1: Switch to IRQCHIP_PLATFORM_DRIVER irqchip/irq-brcmstb-l2: Switch to IRQCHIP_PLATFORM_DRIVER genirq: Export irq_gc_{unmask_enable,mask_disable}_reg of/irq: Export of_irq_count to drivers genirq: Export irq_gc_noop() irqchip/irq-bcm7120-l2: Switch to IRQCHIP_PLATFORM_DRIVER arm64: broadcom: Removed forced select of interrupt controllers ARM: bcm: Removed forced select of interrupt controllers arch/arm/kernel/setup.c | 1 + arch/arm/mach-bcm/Kconfig | 4 ---- arch/arm64/Kconfig.platforms | 3 --- arch/arm64/kernel/setup.c | 1 + arch/sh/kernel/smp.c | 1 + drivers/irqchip/Kconfig | 12 +++++++++--- drivers/irqchip/irq-bcm7038-l1.c | 6 +++++- drivers/irqchip/irq-bcm7120-l2.c | 11 ++++++----- drivers/irqchip/irq-brcmstb-l2.c | 16 +++++++++------- drivers/of/irq.c | 1 + kernel/irq/generic-chip.c | 3 +++ kernel/irq/irqdesc.c | 2 -- kernel/irq/manage.c | 1 + 13 files changed, 37 insertions(+), 25 deletions(-)