On Wed, 18 Dec 2019, Olof Johansson wrote:
> Commit a4c3733d32a7 ("riscv: abstract out CSR names for supervisor vs
> machine mode") introduced new non-S/M-specific defines for some of the
> CSRs and fields that differ for when you run the kernel in machine or
> supervisor mode.
>
> One of those was "IRQ_TIMER" (instead of IRQ_S_TIMER/IRQ_M_MTIMER),
> which was generic enough to cause conflicts with other defines in
> drivers. Since it was in csr.h, it ended up getting pulled in through
> fairly generic include files, etc.
>
> I looked at just renaming those, but for consistency I chose to rename all
> M/S symbols to using 'X' instead of 'S'/'M' in the identifiers instead,
> which gives them all less generic names.
>
> This is pretty churny, and I'm sure there'll be opinions on naming, but
> I figured I'd do the busywork of fixing it up instead of just pointing
> out the conflicts.
>
> Fixes: a4c3733d32a7 ("riscv: abstract out CSR names for supervisor vs machine mode")
> Cc: Christoph Hellwig <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Signed-off-by: Olof Johansson <[email protected]>
Thanks for taking a stab at fixing the issue. I queued the following
minimal fix has been queued for v5.5-rc, adding an RV_ prefix to the IRQ_*
macros. It may be that we need to do the same thing to the rest of the
CSRs. But, based on a quick look, I think we should be OK for the moment.
Let us know if you have a different point of view.
- Paul
From: Paul Walmsley <[email protected]>
Date: Fri, 20 Dec 2019 03:09:49 -0800
Subject: [PATCH] riscv: prefix IRQ_ macro names with an RV_ namespace
"IRQ_TIMER", used in the arch/riscv CSR header file, is a sufficiently
generic macro name that it's used by several source files across the
Linux code base. Some of these other files ultimately include the
arch/riscv CSR include file, causing collisions. Fix by prefixing the
RISC-V csr.h IRQ_ macro names with an RV_ prefix.
Fixes: a4c3733d32a72 ("riscv: abstract out CSR names for supervisor vs machine mode")
Reported-by: Olof Johansson <[email protected]>
Signed-off-by: Paul Walmsley <[email protected]>
---
arch/riscv/include/asm/csr.h | 18 +++++++++---------
arch/riscv/kernel/irq.c | 6 +++---
drivers/irqchip/irq-sifive-plic.c | 2 +-
3 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h
index 0a62d2d68455..435b65532e29 100644
--- a/arch/riscv/include/asm/csr.h
+++ b/arch/riscv/include/asm/csr.h
@@ -116,9 +116,9 @@
# define SR_PIE SR_MPIE
# define SR_PP SR_MPP
-# define IRQ_SOFT IRQ_M_SOFT
-# define IRQ_TIMER IRQ_M_TIMER
-# define IRQ_EXT IRQ_M_EXT
+# define RV_IRQ_SOFT IRQ_M_SOFT
+# define RV_IRQ_TIMER IRQ_M_TIMER
+# define RV_IRQ_EXT IRQ_M_EXT
#else /* CONFIG_RISCV_M_MODE */
# define CSR_STATUS CSR_SSTATUS
# define CSR_IE CSR_SIE
@@ -133,15 +133,15 @@
# define SR_PIE SR_SPIE
# define SR_PP SR_SPP
-# define IRQ_SOFT IRQ_S_SOFT
-# define IRQ_TIMER IRQ_S_TIMER
-# define IRQ_EXT IRQ_S_EXT
+# define RV_IRQ_SOFT IRQ_S_SOFT
+# define RV_IRQ_TIMER IRQ_S_TIMER
+# define RV_IRQ_EXT IRQ_S_EXT
#endif /* CONFIG_RISCV_M_MODE */
/* IE/IP (Supervisor/Machine Interrupt Enable/Pending) flags */
-#define IE_SIE (_AC(0x1, UL) << IRQ_SOFT)
-#define IE_TIE (_AC(0x1, UL) << IRQ_TIMER)
-#define IE_EIE (_AC(0x1, UL) << IRQ_EXT)
+#define IE_SIE (_AC(0x1, UL) << RV_IRQ_SOFT)
+#define IE_TIE (_AC(0x1, UL) << RV_IRQ_TIMER)
+#define IE_EIE (_AC(0x1, UL) << RV_IRQ_EXT)
#ifndef __ASSEMBLY__
diff --git a/arch/riscv/kernel/irq.c b/arch/riscv/kernel/irq.c
index 3f07a91d5afb..345c4f2eba13 100644
--- a/arch/riscv/kernel/irq.c
+++ b/arch/riscv/kernel/irq.c
@@ -23,11 +23,11 @@ asmlinkage __visible void __irq_entry do_IRQ(struct pt_regs *regs)
irq_enter();
switch (regs->cause & ~CAUSE_IRQ_FLAG) {
- case IRQ_TIMER:
+ case RV_IRQ_TIMER:
riscv_timer_interrupt();
break;
#ifdef CONFIG_SMP
- case IRQ_SOFT:
+ case RV_IRQ_SOFT:
/*
* We only use software interrupts to pass IPIs, so if a non-SMP
* system gets one, then we don't know what to do.
@@ -35,7 +35,7 @@ asmlinkage __visible void __irq_entry do_IRQ(struct pt_regs *regs)
riscv_software_interrupt();
break;
#endif
- case IRQ_EXT:
+ case RV_IRQ_EXT:
handle_arch_irq(regs);
break;
default:
diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index 8df547d2d935..0aca5807a119 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -256,7 +256,7 @@ static int __init plic_init(struct device_node *node,
* Skip contexts other than external interrupts for our
* privilege level.
*/
- if (parent.args[0] != IRQ_EXT)
+ if (parent.args[0] != RV_IRQ_EXT)
continue;
hartid = plic_find_hart_id(parent.np);
--
2.24.0
On Fri, Jan 3, 2020 at 5:28 PM Paul Walmsley <[email protected]> wrote:
>
> On Wed, 18 Dec 2019, Olof Johansson wrote:
>
> > Commit a4c3733d32a7 ("riscv: abstract out CSR names for supervisor vs
> > machine mode") introduced new non-S/M-specific defines for some of the
> > CSRs and fields that differ for when you run the kernel in machine or
> > supervisor mode.
> >
> > One of those was "IRQ_TIMER" (instead of IRQ_S_TIMER/IRQ_M_MTIMER),
> > which was generic enough to cause conflicts with other defines in
> > drivers. Since it was in csr.h, it ended up getting pulled in through
> > fairly generic include files, etc.
> >
> > I looked at just renaming those, but for consistency I chose to rename all
> > M/S symbols to using 'X' instead of 'S'/'M' in the identifiers instead,
> > which gives them all less generic names.
> >
> > This is pretty churny, and I'm sure there'll be opinions on naming, but
> > I figured I'd do the busywork of fixing it up instead of just pointing
> > out the conflicts.
> >
> > Fixes: a4c3733d32a7 ("riscv: abstract out CSR names for supervisor vs machine mode")
> > Cc: Christoph Hellwig <[email protected]>
> > Cc: Thomas Gleixner <[email protected]>
> > Signed-off-by: Olof Johansson <[email protected]>
>
> Thanks for taking a stab at fixing the issue. I queued the following
> minimal fix has been queued for v5.5-rc, adding an RV_ prefix to the IRQ_*
> macros. It may be that we need to do the same thing to the rest of the
> CSRs. But, based on a quick look, I think we should be OK for the moment.
> Let us know if you have a different point of view.
Sure, this does the job. I'd personally prefer consistent prefixes but
that's just bikeshed color preferences -- this is fine.
Acked-by: Olof Johansson <[email protected]>
(Builds are still failing for some configs, but will be fixed if/when
you pick up https://lore.kernel.org/linux-riscv/[email protected]/)
-Olof
On Fri, 3 Jan 2020, Olof Johansson wrote:
> Sure, this does the job. I'd personally prefer consistent prefixes but
> that's just bikeshed color preferences -- this is fine.
Thanks for the ack. For what it's worth, we're in agreement that we
should prophylactically place RV_ prefixes on the rest of the CSR_ macro
names. I just would prefer that it's done outside the late -rc series,
since it's not technically a fix.
> (Builds are still failing for some configs, but will be fixed if/when
> you pick up https://lore.kernel.org/linux-riscv/[email protected]/)
That one is on my radar - just haven't had a chance to review/test it
yet. Thanks for sending that one too!
- Paul
On Tue, 07 Jan 2020 03:15:56 PST (-0800), Paul Walmsley wrote:
> On Fri, 3 Jan 2020, Olof Johansson wrote:
>
>> Sure, this does the job. I'd personally prefer consistent prefixes but
>> that's just bikeshed color preferences -- this is fine.
>
> Thanks for the ack. For what it's worth, we're in agreement that we
> should prophylactically place RV_ prefixes on the rest of the CSR_ macro
> names. I just would prefer that it's done outside the late -rc series,
> since it's not technically a fix.
Olof: are you going to send a v2 of this patch that converts everything else
over or do you want me to? I think we all agree it's the right way to go, Paul
was just trying to limit the scope of the change late in the RC cycle. I'd
like to get this on for-next sooner rather than later as it'll probably
conflict with everything.
>> (Builds are still failing for some configs, but will be fixed if/when
>> you pick up https://lore.kernel.org/linux-riscv/[email protected]/)
>
> That one is on my radar - just haven't had a chance to review/test it
> yet. Thanks for sending that one too!
>
>
> - Paul