| Message ID | 20210924170546.805663-4-f.fainelli@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| 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 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/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!
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
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(+)