| Message ID | 20240722094226.21602-36-ysionneau@kalrayinc.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | None | expand |
On 22/07/2024 11:41, ysionneau@kalrayinc.com wrote: > From: Yann Sionneau <ysionneau@kalrayinc.com> > > The Inter-Processor Interrupt Controller (IPI) provides a fast > synchronization mechanism to the software. It exposes eight independent > sets of registers that can be used to notify each processor in the cluster. > > Co-developed-by: Clement Leger <clement@clement-leger.fr> > Signed-off-by: Clement Leger <clement@clement-leger.fr> > Co-developed-by: Guillaume Thouvenin <thouveng@gmail.com> > Signed-off-by: Guillaume Thouvenin <thouveng@gmail.com> > Co-developed-by: Julian Vetter <jvetter@kalrayinc.com> > Signed-off-by: Julian Vetter <jvetter@kalrayinc.com> > Co-developed-by: Luc Michel <luc@lmichel.fr> > Signed-off-by: Luc Michel <luc@lmichel.fr> > Signed-off-by: Jules Maselbas <jmaselbas@zdiv.net> > Signed-off-by: Yann Sionneau <ysionneau@kalrayinc.com> ... > + > +int __init kvx_ipi_ctrl_init(struct device_node *node, > + struct device_node *parent) > +{ > + int ret; > + unsigned int ipi_irq; > + void __iomem *ipi_base; > + > + BUG_ON(!node); Fix your code instead. > + > + ipi_base = of_iomap(node, 0); > + BUG_ON(!ipi_base); No, handle it by returning. > + > + kvx_ipi_controller.regs = ipi_base; > + > + /* Init mask for interrupts to PE0 -> PE15 */ > + writel(KVX_IPI_CPU_MASK, kvx_ipi_controller.regs + IPI_MASK_OFFSET); > + > + ipi_irq = irq_of_parse_and_map(node, 0); > + if (!ipi_irq) { > + pr_err("Failed to parse irq: %d\n", ipi_irq); > + return -EINVAL; > + } > + > + ret = request_percpu_irq(ipi_irq, ipi_irq_handler, > + "kvx_ipi", &kvx_ipi_controller); > + if (ret) { > + pr_err("can't register interrupt %d (%d)\n", > + ipi_irq, ret); > + return ret; > + } > + kvx_ipi_controller.ipi_irq = ipi_irq; > + > + ret = cpuhp_setup_state(CPUHP_AP_IRQ_KVX_STARTING, > + "kvx/ipi:online", > + kvx_ipi_starting_cpu, > + kvx_ipi_dying_cpu); > + if (ret < 0) { > + pr_err("Failed to setup hotplug state"); > + return ret; > + } > + > + set_smp_cross_call(kvx_ipi_send); > + pr_info("controller probed\n"); Drop this simple probe successes. This is not the way to trace system bootup. Keep only reasonable amount, not every driver printing that its initcall started. Best regards, Krzysztof
On Mon, Jul 22 2024 at 11:41, ysionneau@kalrayinc.com wrote: > +/* > + * IPI controller can signal RM and PE0 -> 15 > + * In order to restrict that to the PE, write the corresponding mask This comment is undecodable > + */ > +#define KVX_IPI_CPU_MASK (~0xFFFF) > + > +/* A collection of single bit ipi messages. */ > +static DEFINE_PER_CPU_ALIGNED(unsigned long, ipi_data); > + > +struct kvx_ipi_ctrl { > + void __iomem *regs; > + unsigned int ipi_irq; > +}; > + > +static struct kvx_ipi_ctrl kvx_ipi_controller; > + > +void kvx_ipi_send(const struct cpumask *mask, unsigned int operation) Why is this global? It's only used in this file, no? > +{ > + const unsigned long *maskb = cpumask_bits(mask); > + unsigned long flags; > + int cpu; > + > + /* Set operation that must be done by receiver */ > + for_each_cpu(cpu, mask) > + set_bit(operation, &per_cpu(ipi_data, cpu)); > + > + /* Commit the write before sending IPI */ > + smp_wmb(); > + > + local_irq_save(flags); > + > + WARN_ON(*maskb & KVX_IPI_CPU_MASK); > +#define KVX_IPI_CPU_MASK (~0xFFFF) This means the system is limited to 16 CPUs, right? How should a bit >= NR_CPUs be set in a valid cpu mask? Also above you happily iterate the full cpumask. This does not make sense. > + writel(*maskb, kvx_ipi_controller.regs + IPI_INTERRUPT_OFFSET); > + > + local_irq_restore(flags); > +} > + > +static int kvx_ipi_starting_cpu(unsigned int cpu) > +{ > + enable_percpu_irq(kvx_ipi_controller.ipi_irq, IRQ_TYPE_NONE); > + > + return 0; > +} > + > +static int kvx_ipi_dying_cpu(unsigned int cpu) > +{ > + disable_percpu_irq(kvx_ipi_controller.ipi_irq); > + > + return 0; > +} > + > +static irqreturn_t ipi_irq_handler(int irq, void *dev_id) > +{ > + unsigned long *pending_ipis = &per_cpu(ipi_data, smp_processor_id()); this_cpu_ptr() ? > + while (true) { > + unsigned long ops = xchg(pending_ipis, 0); > + > + if (!ops) > + return IRQ_HANDLED; > + > + handle_IPI(ops); > + } for (ops = xchg(pending_ipis, 0); ops; ops = xchg(pending_ipis, 0)) handle_IPI(ops); Hmm? > + return IRQ_HANDLED; > +} > + > +int __init kvx_ipi_ctrl_init(struct device_node *node, > + struct device_node *parent) > +{ > + int ret; > + unsigned int ipi_irq; > + void __iomem *ipi_base; > + > + BUG_ON(!node); > + > + ipi_base = of_iomap(node, 0); What's the point of this ipi_base indirection? Just use controller.regs directly. > + BUG_ON(!ipi_base); > + > + kvx_ipi_controller.regs = ipi_base; > + > + /* Init mask for interrupts to PE0 -> PE15 */ > + writel(KVX_IPI_CPU_MASK, kvx_ipi_controller.regs + IPI_MASK_OFFSET); > + > + ipi_irq = irq_of_parse_and_map(node, 0); > + if (!ipi_irq) { > + pr_err("Failed to parse irq: %d\n", ipi_irq); > + return -EINVAL; > + } > + > + ret = request_percpu_irq(ipi_irq, ipi_irq_handler, > + "kvx_ipi", &kvx_ipi_controller); > + if (ret) { > + pr_err("can't register interrupt %d (%d)\n", > + ipi_irq, ret); > + return ret; > + } > + kvx_ipi_controller.ipi_irq = ipi_irq; > + > + ret = cpuhp_setup_state(CPUHP_AP_IRQ_KVX_STARTING, > + "kvx/ipi:online", > + kvx_ipi_starting_cpu, > + kvx_ipi_dying_cpu); > + if (ret < 0) { > + pr_err("Failed to setup hotplug state"); That leaves the half initialized IPI handler around. > + return ret; Thanks, tglx
Hello Krzysztof, On 22/07/2024 14:39, Krzysztof Kozlowski wrote: > On 22/07/2024 11:41, ysionneau@kalrayinc.com wrote: >> From: Yann Sionneau <ysionneau@kalrayinc.com> >> >> [...] >> + >> +int __init kvx_ipi_ctrl_init(struct device_node *node, >> + struct device_node *parent) >> +{ >> + int ret; >> + unsigned int ipi_irq; >> + void __iomem *ipi_base; >> + >> + BUG_ON(!node); > Fix your code instead. I am not sure I understand your comment here, I don't have the control over what the kernel passes to my driver, do I? On the other hand, "node" being the node that matches the compatible, maybe it can never be NULL, is that what you're saying? After doing some archeology in our old code base it seems indeed this line is an artefact of this previous code snippet: ``` np = of_find_compatible_node(NULL, NULL, "kalray,coolidge-ipi-ctrl"); BUG_ON(!np); ``` Now that this is a real driver declared via IRQCHIP_DECLARE(), I guess that this check isn't needed anymore. > >> + >> + ipi_base = of_iomap(node, 0); >> + BUG_ON(!ipi_base); > No, handle it by returning. Ack > >> + >> + kvx_ipi_controller.regs = ipi_base; >> + >> + /* Init mask for interrupts to PE0 -> PE15 */ >> + writel(KVX_IPI_CPU_MASK, kvx_ipi_controller.regs + IPI_MASK_OFFSET); >> + >> + ipi_irq = irq_of_parse_and_map(node, 0); >> + if (!ipi_irq) { >> + pr_err("Failed to parse irq: %d\n", ipi_irq); >> + return -EINVAL; >> + } >> + >> + ret = request_percpu_irq(ipi_irq, ipi_irq_handler, >> + "kvx_ipi", &kvx_ipi_controller); >> + if (ret) { >> + pr_err("can't register interrupt %d (%d)\n", >> + ipi_irq, ret); >> + return ret; >> + } >> + kvx_ipi_controller.ipi_irq = ipi_irq; >> + >> + ret = cpuhp_setup_state(CPUHP_AP_IRQ_KVX_STARTING, >> + "kvx/ipi:online", >> + kvx_ipi_starting_cpu, >> + kvx_ipi_dying_cpu); >> + if (ret < 0) { >> + pr_err("Failed to setup hotplug state"); >> + return ret; >> + } >> + >> + set_smp_cross_call(kvx_ipi_send); >> + pr_info("controller probed\n"); > Drop this simple probe successes. This is not the way to trace system > bootup. Keep only reasonable amount, not every driver printing that its > initcall started. Ack. Thanks for the review! Regards,
On 23/08/2024 16:46, Yann Sionneau wrote: > Hello Krzysztof, > > On 22/07/2024 14:39, Krzysztof Kozlowski wrote: >> On 22/07/2024 11:41, ysionneau@kalrayinc.com wrote: >>> From: Yann Sionneau <ysionneau@kalrayinc.com> >>> >>> [...] >>> + >>> +int __init kvx_ipi_ctrl_init(struct device_node *node, >>> + struct device_node *parent) >>> +{ >>> + int ret; >>> + unsigned int ipi_irq; >>> + void __iomem *ipi_base; >>> + >>> + BUG_ON(!node); >> Fix your code instead. > > I am not sure I understand your comment here, I don't have the control over what the kernel passes to my driver, do I? In general you have. Investigate the path and check whether NULL is allowed. If it is allowed, then this should be handled correctly and gracefully. If it is not allowed, then BUG_ON() is not welcomed in general. > > On the other hand, "node" being the node that matches the compatible, maybe it can never be NULL, is that what you're saying? I don't remember the context anymore. You responded one month after my review. But if this is about matching, then obviously this cannot happen for DT platforms. If this can be matched via different methods then it should not be BUG_ON... Best regards, Krzysztof
diff --git a/arch/kvx/include/asm/ipi.h b/arch/kvx/include/asm/ipi.h new file mode 100644 index 0000000000000..a23275d19d225 --- /dev/null +++ b/arch/kvx/include/asm/ipi.h @@ -0,0 +1,16 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Copyright (C) 2017-2023 Kalray Inc. + * Author(s): Clement Leger + */ + +#ifndef _ASM_KVX_IPI_H +#define _ASM_KVX_IPI_H + +#include <linux/irqreturn.h> + +int kvx_ipi_ctrl_init(struct device_node *node, struct device_node *parent); + +void kvx_ipi_send(const struct cpumask *mask, unsigned int operation); + +#endif /* _ASM_KVX_IPI_H */ diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index da1dbd79dab54..65db9990cf475 100644 --- a/drivers/irqchip/Kconfig +++ b/drivers/irqchip/Kconfig @@ -337,6 +337,10 @@ config KVX_CORE_INTC depends on KVX select IRQ_DOMAIN +config KVX_IPI_CTRL + bool + depends on KVX + config KVX_APIC_GIC bool depends on KVX diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index 30b69db8789f7..f8fa246df74d2 100644 --- a/drivers/irqchip/Makefile +++ b/drivers/irqchip/Makefile @@ -69,6 +69,7 @@ obj-$(CONFIG_BRCMSTB_L2_IRQ) += irq-brcmstb-l2.o obj-$(CONFIG_KEYSTONE_IRQ) += irq-keystone.o obj-$(CONFIG_MIPS_GIC) += irq-mips-gic.o obj-$(CONFIG_KVX_CORE_INTC) += irq-kvx-core-intc.o +obj-$(CONFIG_KVX_IPI_CTRL) += irq-kvx-ipi-ctrl.o obj-$(CONFIG_KVX_APIC_GIC) += irq-kvx-apic-gic.o obj-$(CONFIG_KVX_ITGEN) += irq-kvx-itgen.o obj-$(CONFIG_KVX_APIC_MAILBOX) += irq-kvx-apic-mailbox.o diff --git a/drivers/irqchip/irq-kvx-ipi-ctrl.c b/drivers/irqchip/irq-kvx-ipi-ctrl.c new file mode 100644 index 0000000000000..09d955a5c109a --- /dev/null +++ b/drivers/irqchip/irq-kvx-ipi-ctrl.c @@ -0,0 +1,143 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (C) 2017-2024 Kalray Inc. + * + * Author(s): Clement Leger + * Jonathan Borne + * Luc Michel + */ + +#define pr_fmt(fmt) "kvx_ipi_ctrl: " fmt + +#include <linux/smp.h> +#include <linux/io.h> +#include <linux/slab.h> +#include <linux/types.h> +#include <linux/module.h> +#include <linux/irqchip.h> +#include <linux/of_irq.h> +#include <linux/cpumask.h> +#include <linux/interrupt.h> +#include <linux/cpuhotplug.h> +#include <linux/of_address.h> +#include <linux/of_platform.h> + +#include <asm/ipi.h> + +#define IPI_INTERRUPT_OFFSET 0x0 +#define IPI_MASK_OFFSET 0x20 + +/* + * IPI controller can signal RM and PE0 -> 15 + * In order to restrict that to the PE, write the corresponding mask + */ +#define KVX_IPI_CPU_MASK (~0xFFFF) + +/* A collection of single bit ipi messages. */ +static DEFINE_PER_CPU_ALIGNED(unsigned long, ipi_data); + +struct kvx_ipi_ctrl { + void __iomem *regs; + unsigned int ipi_irq; +}; + +static struct kvx_ipi_ctrl kvx_ipi_controller; + +void kvx_ipi_send(const struct cpumask *mask, unsigned int operation) +{ + const unsigned long *maskb = cpumask_bits(mask); + unsigned long flags; + int cpu; + + /* Set operation that must be done by receiver */ + for_each_cpu(cpu, mask) + set_bit(operation, &per_cpu(ipi_data, cpu)); + + /* Commit the write before sending IPI */ + smp_wmb(); + + local_irq_save(flags); + + WARN_ON(*maskb & KVX_IPI_CPU_MASK); + writel(*maskb, kvx_ipi_controller.regs + IPI_INTERRUPT_OFFSET); + + local_irq_restore(flags); +} + +static int kvx_ipi_starting_cpu(unsigned int cpu) +{ + enable_percpu_irq(kvx_ipi_controller.ipi_irq, IRQ_TYPE_NONE); + + return 0; +} + +static int kvx_ipi_dying_cpu(unsigned int cpu) +{ + disable_percpu_irq(kvx_ipi_controller.ipi_irq); + + return 0; +} + +static irqreturn_t ipi_irq_handler(int irq, void *dev_id) +{ + unsigned long *pending_ipis = &per_cpu(ipi_data, smp_processor_id()); + + while (true) { + unsigned long ops = xchg(pending_ipis, 0); + + if (!ops) + return IRQ_HANDLED; + + handle_IPI(ops); + } + + return IRQ_HANDLED; +} + +int __init kvx_ipi_ctrl_init(struct device_node *node, + struct device_node *parent) +{ + int ret; + unsigned int ipi_irq; + void __iomem *ipi_base; + + BUG_ON(!node); + + ipi_base = of_iomap(node, 0); + BUG_ON(!ipi_base); + + kvx_ipi_controller.regs = ipi_base; + + /* Init mask for interrupts to PE0 -> PE15 */ + writel(KVX_IPI_CPU_MASK, kvx_ipi_controller.regs + IPI_MASK_OFFSET); + + ipi_irq = irq_of_parse_and_map(node, 0); + if (!ipi_irq) { + pr_err("Failed to parse irq: %d\n", ipi_irq); + return -EINVAL; + } + + ret = request_percpu_irq(ipi_irq, ipi_irq_handler, + "kvx_ipi", &kvx_ipi_controller); + if (ret) { + pr_err("can't register interrupt %d (%d)\n", + ipi_irq, ret); + return ret; + } + kvx_ipi_controller.ipi_irq = ipi_irq; + + ret = cpuhp_setup_state(CPUHP_AP_IRQ_KVX_STARTING, + "kvx/ipi:online", + kvx_ipi_starting_cpu, + kvx_ipi_dying_cpu); + if (ret < 0) { + pr_err("Failed to setup hotplug state"); + return ret; + } + + set_smp_cross_call(kvx_ipi_send); + pr_info("controller probed\n"); + + return 0; +} +IRQCHIP_DECLARE(kvx_ipi_ctrl, "kalray,coolidge-ipi-ctrl", kvx_ipi_ctrl_init);