From nobody Thu Oct 24 18:35:36 2024 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4XZF3W00k8z5Zkdw for ; Thu, 24 Oct 2024 18:35:50 +0000 (UTC) (envelope-from jrtc27@jrtc27.com) Received: from mail-ed1-f46.google.com (mail-ed1-f46.google.com [209.85.208.46]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "WR4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4XZF3V3Dm2z4kMn for ; Thu, 24 Oct 2024 18:35:50 +0000 (UTC) (envelope-from jrtc27@jrtc27.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-ed1-f46.google.com with SMTP id 4fb4d7f45d1cf-5c9709c9b0cso1627020a12.1 for ; Thu, 24 Oct 2024 11:35:50 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1729794949; x=1730399749; h=to:references:message-id:content-transfer-encoding:cc:date :in-reply-to:from:subject:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=g30avWxoLGH9f+nr+KtIxvR6qgx1yh47UaZigo5V7Ng=; b=i+NzQot9evGQjNtlSM/OBfuxXIkbh1GDS8j0TR6pcXxgM/G6nV/dhwtw5TdjbJl4HM aYRfGFu8veKXfLsQ/WeXNCQ8VMeGld0h/28P8rDo2cb/2Sj3ZDlqUDkabqL7rc9FXP23 8fpc2TBnEKsK6z37ffWwNBrkbFOdHqvkgQINkgPAm45MkyrUonIidvxHP+1ZflowML9J jSVo7eRdpD7f2Uyj/PEwylHoMXGf0QcaaTixTsujAW/UUv9nuRGEhTr9TOazMX2rtMha nAl6uWtI2Yivw+tn1jjNMkLaCfyeWuBv2DUpGCeYdVf//1ZsjLSEX1mOuoG3qor1uAal rGEA== X-Forwarded-Encrypted: i=1; AJvYcCUlDzc4R9yl8XTlzhJCPkukNXK0epNZB53eY98O+FcBewnpLdBBqYhmQRO1uw79G+dlPmo/TJn61iPXH9eAizWAqCmQ@freebsd.org X-Gm-Message-State: AOJu0YwAjh16jA7MBvFFiKHm2TKWD4ByRe9kdN4+xJxQl9KmmEoPcSrQ vz5T0NafbCvuL8XTvs/C6ql02m8OxWQUBHMUIrxK30Fr8vJVqBLfeMW60oLJ7ls= X-Google-Smtp-Source: AGHT+IGXoxETyBW4KQcQ/iNdE2FosWcNC4dicHHjo3PDHzW96qNvU3XhoXAfwMe++O0m76Ow0J5+NQ== X-Received: by 2002:a05:6402:278c:b0:5c9:709c:247f with SMTP id 4fb4d7f45d1cf-5cb8ac380damr5504059a12.2.1729794948520; Thu, 24 Oct 2024 11:35:48 -0700 (PDT) Received: from smtpclient.apple ([131.111.5.201]) by smtp.gmail.com with ESMTPSA id 4fb4d7f45d1cf-5cb66c6b9d1sm6219157a12.70.2024.10.24.11.35.47 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Thu, 24 Oct 2024 11:35:47 -0700 (PDT) Content-Type: text/plain; charset=utf-8 List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-all@freebsd.org Sender: owner-dev-commits-src-all@FreeBSD.org Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3818.100.11.1.3\)) Subject: Re: git: 536c8d948e85 - main - intrng: change multi-interrupt root support type to enum From: Jessica Clarke In-Reply-To: <202410240358.49O3wKVY018297@gitrepo.freebsd.org> Date: Thu, 24 Oct 2024 19:35:36 +0100 Cc: "src-committers@freebsd.org" , "dev-commits-src-all@freebsd.org" , "dev-commits-src-main@freebsd.org" Content-Transfer-Encoding: quoted-printable Message-Id: References: <202410240358.49O3wKVY018297@gitrepo.freebsd.org> To: Kyle Evans X-Mailer: Apple Mail (2.3818.100.11.1.3) X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:15169, ipnet:209.85.128.0/17, country:US] X-Rspamd-Queue-Id: 4XZF3V3Dm2z4kMn X-Spamd-Bar: ---- On 24 Oct 2024, at 04:58, Kyle Evans 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 > AuthorDate: 2024-10-24 03:55:21 +0000 > Commit: Kyle Evans > 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 > #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 > #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 >=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 */