Re: git: 536c8d948e85 - main - intrng: change multi-interrupt root support type to enum
- In reply to: Kyle Evans : "git: 536c8d948e85 - main - intrng: change multi-interrupt root support type to enum"
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Thu, 24 Oct 2024 18:35:36 UTC
On 24 Oct 2024, at 04:58, Kyle Evans <kevans@FreeBSD.org> wrote:
>
> The branch main has been updated by kevans:
>
> URL: https://cgit.FreeBSD.org/src/commit/?id=536c8d948e8563141356fd41fb8bfe65be289385
>
> commit 536c8d948e8563141356fd41fb8bfe65be289385
> Author: Elliott Mitchell <ehem+freebsd@m5p.com>
> AuthorDate: 2024-10-24 03:55:21 +0000
> Commit: Kyle Evans <kevans@FreeBSD.org>
> CommitDate: 2024-10-24 03:55:21 +0000
>
> intrng: change multi-interrupt root support type to enum
>
> uint32_t is handy for directly interfacing with assembly-language. For
> the C portion, enum is much handier. In particular there is no need to
> count the number of roots by hand. This also works better for being
> able to build kernels with varying numbers of roots.
>
> Switch to INTR_ROOT_COUNT as this better matches the purpose of the
> value. Switch to root_type, rather than rootnum for similar reasons.
>
> Remove the default from the core. Better to require the architectures
> to declare the type since they will routinely deviate and a default
> chosen now will likely be suboptimal.
>
> Leave intr_irq_handler() taking a register type as that better matches
> for interfacing with assembly-language.
Hi Kyle,
A few comments, since I didn’t realise we were going ahead with this
change, otherwise I would have left them on a review somewhere.
> ---
> sys/arm/arm/gic.c | 2 +-
> sys/arm/broadcom/bcm2835/bcm2836.c | 2 +-
> sys/arm/include/intr.h | 6 ++++++
> sys/arm64/arm64/gic_v3.c | 4 ++--
> sys/arm64/arm64/gicv3_its.c | 2 +-
> sys/arm64/include/intr.h | 10 ++++++---
> sys/kern/pic_if.m | 4 ++--
> sys/kern/subr_intr.c | 43 +++++++++++++++-----------------------
> sys/riscv/include/intr.h | 6 ++++++
> sys/riscv/riscv/intc.c | 2 +-
> sys/sys/intr.h | 10 ++++-----
> 11 files changed, 48 insertions(+), 43 deletions(-)
>
> diff --git a/sys/arm/arm/gic.c b/sys/arm/arm/gic.c
> index b1b7aacd63ab..ffce86e62128 100644
> --- a/sys/arm/arm/gic.c
> +++ b/sys/arm/arm/gic.c
> @@ -200,7 +200,7 @@ gic_cpu_mask(struct arm_gic_softc *sc)
>
> #ifdef SMP
> static void
> -arm_gic_init_secondary(device_t dev, uint32_t rootnum)
> +arm_gic_init_secondary(device_t dev, enum root_type root_type)
> {
> struct arm_gic_softc *sc = device_get_softc(dev);
> u_int irq, cpu;
> diff --git a/sys/arm/broadcom/bcm2835/bcm2836.c b/sys/arm/broadcom/bcm2835/bcm2836.c
> index 7ed9dedaa77e..fd34ff8818ad 100644
> --- a/sys/arm/broadcom/bcm2835/bcm2836.c
> +++ b/sys/arm/broadcom/bcm2835/bcm2836.c
> @@ -538,7 +538,7 @@ bcm_lintc_init_pmu_on_ap(struct bcm_lintc_softc *sc, u_int cpu)
> }
>
> static void
> -bcm_lintc_init_secondary(device_t dev, uint32_t rootnum)
> +bcm_lintc_init_secondary(device_t dev, enum root_type root_type)
> {
> u_int cpu;
> struct bcm_lintc_softc *sc;
> diff --git a/sys/arm/include/intr.h b/sys/arm/include/intr.h
> index d0d0ff9fc32a..e74be3ac548e 100644
> --- a/sys/arm/include/intr.h
> +++ b/sys/arm/include/intr.h
> @@ -43,6 +43,12 @@
> #include <dev/ofw/openfirm.h>
> #endif
>
> +enum root_type {
Should this not have intr in the name? It’s quite generic.
> + INTR_ROOT_IRQ = 0,
> +
> + INTR_ROOT_COUNT /* MUST BE LAST */
These comments don’t seem particularly useful? Anyone who knows what
they’re doing knows that’s clearly true. It definitely doesn’t need to
be shouted at the reader, at least.
Jess
> +};
> +
> #ifndef NIRQ
> #define NIRQ 1024 /* XXX - It should be an option. */
> #endif
> diff --git a/sys/arm64/arm64/gic_v3.c b/sys/arm64/arm64/gic_v3.c
> index 964a129111e2..750f734a7757 100644
> --- a/sys/arm64/arm64/gic_v3.c
> +++ b/sys/arm64/arm64/gic_v3.c
> @@ -1093,7 +1093,7 @@ gic_v3_bind_intr(device_t dev, struct intr_irqsrc *isrc)
>
> #ifdef SMP
> static void
> -gic_v3_init_secondary(device_t dev, uint32_t rootnum)
> +gic_v3_init_secondary(device_t dev, enum root_type root_type)
> {
> struct gic_v3_setup_periph_args pargs;
> device_t child;
> @@ -1140,7 +1140,7 @@ gic_v3_init_secondary(device_t dev, uint32_t rootnum)
>
> for (i = 0; i < sc->gic_nchildren; i++) {
> child = sc->gic_children[i];
> - PIC_INIT_SECONDARY(child, rootnum);
> + PIC_INIT_SECONDARY(child, root_type);
> }
> }
>
> diff --git a/sys/arm64/arm64/gicv3_its.c b/sys/arm64/arm64/gicv3_its.c
> index 5ecd9b8c0e94..31e23fc01224 100644
> --- a/sys/arm64/arm64/gicv3_its.c
> +++ b/sys/arm64/arm64/gicv3_its.c
> @@ -1293,7 +1293,7 @@ gicv3_its_setup_intr(device_t dev, struct intr_irqsrc *isrc,
>
> #ifdef SMP
> static void
> -gicv3_its_init_secondary(device_t dev, uint32_t rootnum)
> +gicv3_its_init_secondary(device_t dev, enum root_type root_type)
> {
> struct gicv3_its_softc *sc;
>
> diff --git a/sys/arm64/include/intr.h b/sys/arm64/include/intr.h
> index 99b4d15ccc1c..008c377b7a16 100644
> --- a/sys/arm64/include/intr.h
> +++ b/sys/arm64/include/intr.h
> @@ -31,6 +31,13 @@
> #include <dev/ofw/openfirm.h>
> #endif
>
> +enum root_type {
> + INTR_ROOT_IRQ = 0,
> + INTR_ROOT_FIQ = 1,
Is there a reason to be manually assigning each value rather than
letting the compiler do it for us?
> +
> + INTR_ROOT_COUNT /* MUST BE LAST */
> +};
> +
> #include <sys/intr.h>
>
> #ifndef NIRQ
> @@ -48,7 +55,4 @@ arm_irq_memory_barrier(uintptr_t irq)
> #define ACPI_GPIO_XREF 3
> #endif
>
> -#define INTR_ROOT_FIQ 1
> -#define INTR_ROOT_NUM 2
> -
> #endif /* _MACHINE_INTR_H */
> diff --git a/sys/kern/pic_if.m b/sys/kern/pic_if.m
> index f78e787594c5..2d938520b106 100644
> --- a/sys/kern/pic_if.m
> +++ b/sys/kern/pic_if.m
> @@ -74,7 +74,7 @@ CODE {
> }
>
> static void
> - null_pic_init_secondary(device_t dev, uint32_t rootnum)
> + null_pic_init_secondary(device_t dev, enum root_type root_type)
> {
> }
>
> @@ -157,7 +157,7 @@ METHOD void pre_ithread {
>
> METHOD void init_secondary {
> device_t dev;
> - uint32_t rootnum;
> + enum root_type root_type;
> } DEFAULT null_pic_init_secondary;
>
> METHOD void ipi_send {
> diff --git a/sys/kern/subr_intr.c b/sys/kern/subr_intr.c
> index a6052f28b04c..6b4ebd16675c 100644
> --- a/sys/kern/subr_intr.c
> +++ b/sys/kern/subr_intr.c
> @@ -89,15 +89,6 @@
>
> #define INTRNAME_LEN (2*MAXCOMLEN + 1)
>
> -/*
> - * Archs may define multiple roots with INTR_ROOT_NUM to support different kinds
> - * of interrupts (e.g. arm64 FIQs which use a different exception vector than
> - * IRQs).
> - */
> -#if !defined(INTR_ROOT_NUM)
> -#define INTR_ROOT_NUM 1
> -#endif
> -
> #ifdef DEBUG
> #define debugf(fmt, args...) do { printf("%s(): ", __func__); \
> printf(fmt,##args); } while (0)
> @@ -115,7 +106,7 @@ struct intr_irq_root {
> void *arg;
> };
>
> -static struct intr_irq_root intr_irq_roots[INTR_ROOT_NUM];
> +static struct intr_irq_root intr_irq_roots[INTR_ROOT_COUNT];
>
> struct intr_pic_child {
> SLIST_ENTRY(intr_pic_child) pc_next;
> @@ -337,16 +328,16 @@ isrc_release_counters(struct intr_irqsrc *isrc)
> * from the assembler, where CPU interrupt is served.
> */
> void
> -intr_irq_handler(struct trapframe *tf, uint32_t rootnum)
> +intr_irq_handler(struct trapframe *tf, u_register_t root_type)
Why u_register_t? If you make it int then it’s the same underlying type
as the enum values so no casting is needed below. Or just make it be
the enum itself. Assembly can pass those as easily as a uint32_t.
> {
> struct trapframe * oldframe;
> struct thread * td;
> struct intr_irq_root *root;
>
> - KASSERT(rootnum < INTR_ROOT_NUM,
> - ("%s: invalid interrupt root %d", __func__, rootnum));
> + KASSERT((uintmax_t)root_type < INTR_ROOT_COUNT,
> + ("%s: invalid interrupt root %ju", __func__, (uintmax_t)root_type));
>
> - root = &intr_irq_roots[rootnum];
> + root = &intr_irq_roots[root_type];
> KASSERT(root->filter != NULL, ("%s: no filter", __func__));
>
> kasan_mark(tf, sizeof(*tf), sizeof(*tf), 0);
> @@ -495,11 +486,11 @@ isrc_free_irq(struct intr_irqsrc *isrc)
> }
>
> device_t
> -intr_irq_root_device(uint32_t rootnum)
> +intr_irq_root_device(enum root_type root_type)
> {
> - KASSERT(rootnum < INTR_ROOT_NUM,
> - ("%s: invalid interrupt root %d", __func__, rootnum));
> - return (intr_irq_roots[rootnum].dev);
> + KASSERT((uintmax_t)root_type < INTR_ROOT_COUNT,
> + ("%s: invalid interrupt root %ju", __func__, (uintmax_t)root_type));
Why are we casting? It’s an enum so it’s an int unless you define your
enum in ways that intrng doesn’t have to support. Just check 0 <=
root_type < INTR_ROOT_COUNT and be done with it, printing with %d?
Repeated below too.
> + return (intr_irq_roots[root_type].dev);
> }
>
> /*
> @@ -900,7 +891,7 @@ intr_pic_deregister(device_t dev, intptr_t xref)
> */
> int
> intr_pic_claim_root(device_t dev, intptr_t xref, intr_irq_filter_t *filter,
> - void *arg, uint32_t rootnum)
> + void *arg, enum root_type root_type)
> {
> struct intr_pic *pic;
> struct intr_irq_root *root;
> @@ -925,9 +916,9 @@ intr_pic_claim_root(device_t dev, intptr_t xref, intr_irq_filter_t *filter,
> * Note that we further suppose that there is not threaded interrupt
> * routine (handler) on the root. See intr_irq_handler().
> */
> - KASSERT(rootnum < INTR_ROOT_NUM,
> - ("%s: invalid interrupt root %d", __func__, rootnum));
> - root = &intr_irq_roots[rootnum];
> + KASSERT((uintmax_t)root_type < INTR_ROOT_COUNT,
> + ("%s: invalid interrupt root %ju", __func__, (uintmax_t)root_type));
> + root = &intr_irq_roots[root_type];
> if (root->dev != NULL) {
> device_printf(dev, "another root already set\n");
> return (EBUSY);
> @@ -1580,16 +1571,16 @@ void
> intr_pic_init_secondary(void)
> {
> device_t dev;
> - uint32_t rootnum;
> + enum root_type root_type;
>
> /*
> * QQQ: Only root PICs are aware of other CPUs ???
> */
> //mtx_lock(&isrc_table_lock);
> - for (rootnum = 0; rootnum < INTR_ROOT_NUM; rootnum++) {
> - dev = intr_irq_roots[rootnum].dev;
> + for (root_type = 0; root_type < INTR_ROOT_COUNT; root_type++) {
> + dev = intr_irq_roots[root_type].dev;
> if (dev != NULL) {
> - PIC_INIT_SECONDARY(dev, rootnum);
> + PIC_INIT_SECONDARY(dev, root_type);
> }
> }
> //mtx_unlock(&isrc_table_lock);
> diff --git a/sys/riscv/include/intr.h b/sys/riscv/include/intr.h
> index ad811dcbc449..8cbb07c6be24 100644
> --- a/sys/riscv/include/intr.h
> +++ b/sys/riscv/include/intr.h
> @@ -35,6 +35,12 @@
> #ifndef _MACHINE_INTR_MACHDEP_H_
> #define _MACHINE_INTR_MACHDEP_H_
>
> +enum root_type {
> + INTR_ROOT_IRQ = 0,
> +
> + INTR_ROOT_COUNT /* MUST BE LAST */
> +};
> +
> #ifndef NIRQ
> #define NIRQ 1024
> #endif
> diff --git a/sys/riscv/riscv/intc.c b/sys/riscv/riscv/intc.c
> index 248175e8bea3..fcfc0c826fb9 100644
> --- a/sys/riscv/riscv/intc.c
> +++ b/sys/riscv/riscv/intc.c
> @@ -241,7 +241,7 @@ intc_setup_intr(device_t dev, struct intr_irqsrc *isrc,
>
> #ifdef SMP
> static void
> -intc_init_secondary(device_t dev, uint32_t rootnum)
> +intc_init_secondary(device_t dev, enum root_type root_type)
> {
> struct intc_softc *sc;
> struct intr_irqsrc *isrc;
> diff --git a/sys/sys/intr.h b/sys/sys/intr.h
> index c1e440a9fa93..0208844e90c8 100644
> --- a/sys/sys/intr.h
> +++ b/sys/sys/intr.h
> @@ -37,8 +37,6 @@
>
> #define INTR_IRQ_INVALID 0xFFFFFFFF
>
> -#define INTR_ROOT_IRQ 0
> -
> enum intr_map_data_type {
> INTR_MAP_DATA_ACPI = 0,
> INTR_MAP_DATA_FDT,
> @@ -113,12 +111,12 @@ u_int intr_irq_next_cpu(u_int current_cpu, cpuset_t *cpumask);
> struct intr_pic *intr_pic_register(device_t, intptr_t);
> int intr_pic_deregister(device_t, intptr_t);
> int intr_pic_claim_root(device_t, intptr_t, intr_irq_filter_t *, void *,
> - uint32_t);
> + enum root_type);
> int intr_pic_add_handler(device_t, struct intr_pic *,
> intr_child_irq_filter_t *, void *, uintptr_t, uintptr_t);
> bool intr_is_per_cpu(struct resource *);
>
> -device_t intr_irq_root_device(uint32_t);
> +device_t intr_irq_root_device(enum root_type);
>
> /* Intr interface for BUS. */
>
> @@ -166,7 +164,7 @@ void intr_ipi_send(cpuset_t cpus, u_int ipi);
> void intr_ipi_dispatch(u_int ipi);
> #endif
>
> -/* Main interrupt handler called from asm on most archs except riscv. */
> -void intr_irq_handler(struct trapframe *tf, uint32_t rootnum);
> +/* Main interrupt handler called from asm on many archs. */
> +void intr_irq_handler(struct trapframe *tf, u_register_t root_type);
>
> #endif /* _SYS_INTR_H */