Date: Thu, 24 Oct 2024 19:35:36 +0100 From: Jessica Clarke <jrtc27@freebsd.org> To: Kyle Evans <kevans@FreeBSD.org> Cc: "src-committers@freebsd.org" <src-committers@FreeBSD.org>, "dev-commits-src-all@freebsd.org" <dev-commits-src-all@FreeBSD.org>, "dev-commits-src-main@freebsd.org" <dev-commits-src-main@FreeBSD.org> Subject: Re: git: 536c8d948e85 - main - intrng: change multi-interrupt root support type to enum Message-ID: <C7870A47-0EE6-4EFD-8D4F-71834F7AE12A@freebsd.org> In-Reply-To: <202410240358.49O3wKVY018297@gitrepo.freebsd.org> References: <202410240358.49O3wKVY018297@gitrepo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 24 Oct 2024, at 04:58, Kyle Evans <kevans@FreeBSD.org> wrote: >=20 > The branch main has been updated by kevans: >=20 > URL: = https://cgit.FreeBSD.org/src/commit/?id=3D536c8d948e8563141356fd41fb8bfe65= be289385 >=20 > 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 >=20 > intrng: change multi-interrupt root support type to enum >=20 > 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. >=20 > Switch to INTR_ROOT_COUNT as this better matches the purpose of the > value. Switch to root_type, rather than rootnum for similar = reasons. >=20 > 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. >=20 > 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=E2=80=99t 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(-) >=20 > 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) >=20 > #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 =3D 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) > } >=20 > 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 >=20 > +enum root_type { Should this not have intr in the name? It=E2=80=99s quite generic. > + INTR_ROOT_IRQ =3D 0, > + > + INTR_ROOT_COUNT /* MUST BE LAST */ These comments don=E2=80=99t seem particularly useful? Anyone who knows = what they=E2=80=99re doing knows that=E2=80=99s clearly true. It definitely = doesn=E2=80=99t 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) >=20 > #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) >=20 > for (i =3D 0; i < sc->gic_nchildren; i++) { > child =3D sc->gic_children[i]; > - PIC_INIT_SECONDARY(child, rootnum); > + PIC_INIT_SECONDARY(child, root_type); > } > } >=20 > 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, >=20 > #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; >=20 > 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 >=20 > +enum root_type { > + INTR_ROOT_IRQ =3D 0, > + INTR_ROOT_FIQ =3D 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> >=20 > #ifndef NIRQ > @@ -48,7 +55,4 @@ arm_irq_memory_barrier(uintptr_t irq) > #define ACPI_GPIO_XREF 3 > #endif >=20 > -#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 { > } >=20 > static void > - null_pic_init_secondary(device_t dev, uint32_t rootnum) > + null_pic_init_secondary(device_t dev, enum root_type root_type) > { > } >=20 > @@ -157,7 +157,7 @@ METHOD void pre_ithread { >=20 > METHOD void init_secondary { > device_t dev; > - uint32_t rootnum; > + enum root_type root_type; > } DEFAULT null_pic_init_secondary; >=20 > 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 @@ >=20 > #define INTRNAME_LEN (2*MAXCOMLEN + 1) >=20 > -/* > - * 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; > }; >=20 > -static struct intr_irq_root intr_irq_roots[INTR_ROOT_NUM]; > +static struct intr_irq_root intr_irq_roots[INTR_ROOT_COUNT]; >=20 > 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=E2=80=99s 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; >=20 > - 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)); >=20 > - root =3D &intr_irq_roots[rootnum]; > + root =3D &intr_irq_roots[root_type]; > KASSERT(root->filter !=3D NULL, ("%s: no filter", __func__)); >=20 > kasan_mark(tf, sizeof(*tf), sizeof(*tf), 0); > @@ -495,11 +486,11 @@ isrc_free_irq(struct intr_irqsrc *isrc) > } >=20 > 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=E2=80=99s an enum so it=E2=80=99s an int unless = you define your enum in ways that intrng doesn=E2=80=99t have to support. Just check 0 = <=3D root_type < INTR_ROOT_COUNT and be done with it, printing with %d? Repeated below too. > + return (intr_irq_roots[root_type].dev); > } >=20 > /* > @@ -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 =3D &intr_irq_roots[rootnum]; > + KASSERT((uintmax_t)root_type < INTR_ROOT_COUNT, > + ("%s: invalid interrupt root %ju", __func__, = (uintmax_t)root_type)); > + root =3D &intr_irq_roots[root_type]; > if (root->dev !=3D 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; >=20 > /* > * QQQ: Only root PICs are aware of other CPUs ??? > */ > //mtx_lock(&isrc_table_lock); > - for (rootnum =3D 0; rootnum < INTR_ROOT_NUM; rootnum++) { > - dev =3D intr_irq_roots[rootnum].dev; > + for (root_type =3D 0; root_type < INTR_ROOT_COUNT; root_type++) { > + dev =3D intr_irq_roots[root_type].dev; > if (dev !=3D 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_ >=20 > +enum root_type { > + INTR_ROOT_IRQ =3D 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, >=20 > #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 @@ >=20 > #define INTR_IRQ_INVALID 0xFFFFFFFF >=20 > -#define INTR_ROOT_IRQ 0 > - > enum intr_map_data_type { > INTR_MAP_DATA_ACPI =3D 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 *); >=20 > -device_t intr_irq_root_device(uint32_t); > +device_t intr_irq_root_device(enum root_type); >=20 > /* Intr interface for BUS. */ >=20 > @@ -166,7 +164,7 @@ void intr_ipi_send(cpuset_t cpus, u_int ipi); > void intr_ipi_dispatch(u_int ipi); > #endif >=20 > -/* 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); >=20 > #endif /* _SYS_INTR_H */
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?C7870A47-0EE6-4EFD-8D4F-71834F7AE12A>