| Message ID | e25e0ac5-abe2-6024-f669-c5352183114e@free.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Jul 16 2017 at 7:40:06 pm BST, Mason <slash.tmp@free.fr> wrote: > Use GIC_SPI explicitly instead of an implicit 0. What bug is this fixing? What benefit does this bring? > > Signed-off-by: Mason <slash.tmp@free.fr> > --- > FWIW, 'make C=2' flagged a few lines: > > $ make C=2 drivers/irqchip/irq-gic.o > CHECK drivers/irqchip/irq-gic.c > drivers/irqchip/irq-gic.c:1079:44: warning: incorrect type in assignment (different address spaces) > drivers/irqchip/irq-gic.c:1079:44: expected void [noderef] <asn:3>*[noderef] <asn:2>*percpu_base > drivers/irqchip/irq-gic.c:1079:44: got void [noderef] <asn:2>*[noderef] <asn:3>*<noident> How is that related to this patch? M.
On 16/07/2017 22:13, Marc Zyngier wrote: > Mason wrote: > >> Use GIC_SPI explicitly instead of an implicit 0. > > What bug is this fixing? What benefit does this bring? The patch aims to replace an (implicit) literal constant with the corresponding symbolic macro. IMO, it makes the intent somewhat clearer, and, more importantly, grepping for said symbol now returns the respective file/line. (It took me a while to find the line.) Are you saying that changing the code at this point is not worth the trouble? >> FWIW, 'make C=2' flagged a few lines: >> >> $ make C=2 drivers/irqchip/irq-gic.o >> CHECK drivers/irqchip/irq-gic.c >> drivers/irqchip/irq-gic.c:1079:44: warning: incorrect type in assignment (different address spaces) >> drivers/irqchip/irq-gic.c:1079:44: expected void [noderef] <asn:3>*[noderef] <asn:2>*percpu_base >> drivers/irqchip/irq-gic.c:1079:44: got void [noderef] <asn:2>*[noderef] <asn:3>*<noident> > > How is that related to this patch? I tested the patch with 'make C=2' and reported the output FWIW. Are you saying I should have written a separate message, or not bothered altogether? Regards.
On Sun, Jul 16 2017 at 10:50:17 pm BST, Mason <slash.tmp@free.fr> wrote: > On 16/07/2017 22:13, Marc Zyngier wrote: > >> Mason wrote: >> >>> Use GIC_SPI explicitly instead of an implicit 0. >> >> What bug is this fixing? What benefit does this bring? > > The patch aims to replace an (implicit) literal constant > with the corresponding symbolic macro. IMO, it makes the > intent somewhat clearer, and, more importantly, grepping > for said symbol now returns the respective file/line. > (It took me a while to find the line.) > > Are you saying that changing the code at this point is > not worth the trouble? You're assuming that this GIC_SPI macro has anything to do with the GIC driver. It doesn't. That's just a convenience macro for people writing DT, and definitely not something I'd ever want to rely on in the Linux driver. The binding defines the raw value, and not this macro. So to sum it up, thank you, but no thank out. > >>> FWIW, 'make C=2' flagged a few lines: >>> >>> $ make C=2 drivers/irqchip/irq-gic.o >>> CHECK drivers/irqchip/irq-gic.c >>> drivers/irqchip/irq-gic.c:1079:44: warning: incorrect type in >>> assignment (different address spaces) >>> drivers/irqchip/irq-gic.c:1079:44: expected void [noderef] >>> <asn:3>*[noderef] <asn:2>*percpu_base >>> drivers/irqchip/irq-gic.c:1079:44: got void [noderef] >>> <asn:2>*[noderef] <asn:3>*<noident> >> >> How is that related to this patch? > > I tested the patch with 'make C=2' and reported the > output FWIW. Are you saying I should have written > a separate message, or not bothered altogether? An even better outcome would be analysing the issue and coming up with a patch if there is something to fix. On its own, dumping these warnings in the middle of something unrelated is a best at waste of bandwidth. M.
On 16/07/2017 23:08, Marc Zyngier wrote: > Mason wrote: > >> On 16/07/2017 22:13, Marc Zyngier wrote: >> >>> Mason wrote: >>> >>>> Use GIC_SPI explicitly instead of an implicit 0. >>> >>> What bug is this fixing? What benefit does this bring? >> >> The patch aims to replace an (implicit) literal constant >> with the corresponding symbolic macro. IMO, it makes the >> intent somewhat clearer, and, more importantly, grepping >> for said symbol now returns the respective file/line. >> (It took me a while to find the line.) >> >> Are you saying that changing the code at this point is >> not worth the trouble? > > You're assuming that this GIC_SPI macro has anything to do with the GIC > driver. It doesn't. That's just a convenience macro for people writing > DT, and definitely not something I'd ever want to rely on in the Linux > driver. The binding defines the raw value, and not this macro. AFAIU, you're referring to Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt "The 1st cell is the interrupt type; 0 for SPI interrupts, 1 for PPI interrupts." Doesn't it make sense to have symbolic constants for the 0 and 1 above? In linux/irqchip/arm-gic.h ? IIUC, headers in include/dt-bindings are just convenience macros, and are not to be included from driver code? Regards.
Hello, On Sun, 16 Jul 2017 22:08:01 +0100, Marc Zyngier wrote: > > Are you saying that changing the code at this point is > > not worth the trouble? > > You're assuming that this GIC_SPI macro has anything to do with the GIC > driver. It doesn't. That's just a convenience macro for people writing > DT, and definitely not something I'd ever want to rely on in the Linux > driver. The binding defines the raw value, and not this macro. > > So to sum it up, thank you, but no thank out. FWIW, a few drivers are already using GIC_SPI: irq-mvebu-gicp.c: fwspec.param[0] = GIC_SPI; irq-mvebu-odmi.c: fwspec.param[0] = GIC_SPI; irq-tegra.c: if (fwspec->param[0] != GIC_SPI) irq-vf610-mscm-ir.c: parent_fwspec.param[0] = GIC_SPI; For pretty much exactly the situation being patched by Mason. Best regards, Thomas
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c index 1b1df4f770bd..ae414f5223b6 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -42,6 +42,8 @@ #include <linux/irqchip/chained_irq.h> #include <linux/irqchip/arm-gic.h> +#include <dt-bindings/interrupt-controller/arm-gic.h> + #include <asm/cputype.h> #include <asm/irq.h> #include <asm/exception.h> @@ -990,7 +992,7 @@ static int gic_irq_domain_translate(struct irq_domain *d, * For SPIs, we need to add 16 more to get the GIC irq * ID number */ - if (!fwspec->param[0]) + if (fwspec->param[0] == GIC_SPI) *hwirq += 16; *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
Use GIC_SPI explicitly instead of an implicit 0. Signed-off-by: Mason <slash.tmp@free.fr> --- FWIW, 'make C=2' flagged a few lines: $ make C=2 drivers/irqchip/irq-gic.o CHECK drivers/irqchip/irq-gic.c drivers/irqchip/irq-gic.c:1079:44: warning: incorrect type in assignment (different address spaces) drivers/irqchip/irq-gic.c:1079:44: expected void [noderef] <asn:3>*[noderef] <asn:2>*percpu_base drivers/irqchip/irq-gic.c:1079:44: got void [noderef] <asn:2>*[noderef] <asn:3>*<noident> drivers/irqchip/irq-gic.c:1080:43: warning: incorrect type in assignment (different address spaces) drivers/irqchip/irq-gic.c:1080:43: expected void [noderef] <asn:3>*[noderef] <asn:2>*percpu_base drivers/irqchip/irq-gic.c:1080:43: got void [noderef] <asn:2>*[noderef] <asn:3>*<noident> drivers/irqchip/irq-gic.c:1091:26: warning: incorrect type in initializer (different address spaces) drivers/irqchip/irq-gic.c:1091:26: expected void const [noderef] <asn:3>*__vpp_verify drivers/irqchip/irq-gic.c:1091:26: got void [noderef] <asn:3>*[noderef] <asn:2>*<noident> drivers/irqchip/irq-gic.c:1091:71: warning: incorrect type in assignment (different address spaces) drivers/irqchip/irq-gic.c:1091:71: expected void [noderef] <asn:3>*<noident> drivers/irqchip/irq-gic.c:1091:71: got void [noderef] <asn:2>* drivers/irqchip/irq-gic.c:1093:26: warning: incorrect type in initializer (different address spaces) drivers/irqchip/irq-gic.c:1093:26: expected void const [noderef] <asn:3>*__vpp_verify drivers/irqchip/irq-gic.c:1093:26: got void [noderef] <asn:3>*[noderef] <asn:2>*<noident> drivers/irqchip/irq-gic.c:1093:70: warning: incorrect type in assignment (different address spaces) drivers/irqchip/irq-gic.c:1093:70: expected void [noderef] <asn:3>*<noident> drivers/irqchip/irq-gic.c:1093:70: got void [noderef] <asn:2>* drivers/irqchip/irq-gic.c:1167:43: warning: incorrect type in argument 1 (different address spaces) drivers/irqchip/irq-gic.c:1167:43: expected void [noderef] <asn:3>*__pdata drivers/irqchip/irq-gic.c:1167:43: got void [noderef] <asn:3>*[noderef] <asn:2>*percpu_base drivers/irqchip/irq-gic.c:1168:42: warning: incorrect type in argument 1 (different address spaces) drivers/irqchip/irq-gic.c:1168:42: expected void [noderef] <asn:3>*__pdata drivers/irqchip/irq-gic.c:1168:42: got void [noderef] <asn:3>*[noderef] <asn:2>*percpu_base --- drivers/irqchip/irq-gic.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)