| Message ID | 1417437909-20923-1-git-send-email-Liviu.Dudau@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Dec 01, 2014 at 12:45:09PM +0000, Liviu Dudau wrote: > During a recent cleanup of the arm64 DTs it has become clear that > the handling of PPIs in xxxx_set_type() is incorrect. The ARM TRMs > for GICv2 and later allow for "implementation defined" support for > setting the edge or level type of the PPI interrupts and don't restrict > the activation level of the signal. Current ARM implementations > do restrict the PPI level type to IRQ_TYPE_LEVEL_LOW, but licensees > of the IP can decide to shoot themselves in the foot at any time. > > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com> > --- > > Made a stupid mistake in v2 and got my bit test confused with logical test. Gentle ping! Any comments here? Russell, are you OK with this approach? Best regards, Liviu > > Documentation/devicetree/bindings/arm/gic.txt | 8 ++++++-- > drivers/irqchip/irq-gic-common.c | 21 +++++++++++++-------- > drivers/irqchip/irq-gic-common.h | 2 +- > drivers/irqchip/irq-gic-v3.c | 8 ++++---- > drivers/irqchip/irq-gic.c | 9 ++++++--- > drivers/irqchip/irq-hip04.c | 9 ++++++--- > 6 files changed, 36 insertions(+), 21 deletions(-) > > diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt > index 8112d0c..c97484b 100644 > --- a/Documentation/devicetree/bindings/arm/gic.txt > +++ b/Documentation/devicetree/bindings/arm/gic.txt > @@ -32,12 +32,16 @@ Main node required properties: > The 3rd cell is the flags, encoded as follows: > bits[3:0] trigger type and level flags. > 1 = low-to-high edge triggered > - 2 = high-to-low edge triggered > + 2 = high-to-low edge triggered (invalid for SPIs) > 4 = active high level-sensitive > - 8 = active low level-sensitive > + 8 = active low level-sensitive (invalid for SPIs). > bits[15:8] PPI interrupt cpu mask. Each bit corresponds to each of > the 8 possible cpus attached to the GIC. A bit set to '1' indicated > the interrupt is wired to that CPU. Only valid for PPI interrupts. > + Also note that the configurability of PPI interrupts is IMPLEMENTATION > + DEFINED and as such not guaranteed to be present (most SoC available > + in 2014 seem to ignore the setting of this flag and use the hardware > + default value). > > - reg : Specifies base physical address(s) and size of the GIC registers. The > first region is the GIC distributor register base and size. The 2nd region is > diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c > index 61541ff..ffee521 100644 > --- a/drivers/irqchip/irq-gic-common.c > +++ b/drivers/irqchip/irq-gic-common.c > @@ -21,7 +21,7 @@ > > #include "irq-gic-common.h" > > -void gic_configure_irq(unsigned int irq, unsigned int type, > +int gic_configure_irq(unsigned int irq, unsigned int type, > void __iomem *base, void (*sync_access)(void)) > { > u32 enablemask = 1 << (irq % 32); > @@ -29,16 +29,17 @@ void gic_configure_irq(unsigned int irq, unsigned int type, > u32 confmask = 0x2 << ((irq % 16) * 2); > u32 confoff = (irq / 16) * 4; > bool enabled = false; > - u32 val; > + u32 val, oldval; > + int ret = 0; > > /* > * Read current configuration register, and insert the config > * for "irq", depending on "type". > */ > - val = readl_relaxed(base + GIC_DIST_CONFIG + confoff); > - if (type == IRQ_TYPE_LEVEL_HIGH) > + val = oldval = readl_relaxed(base + GIC_DIST_CONFIG + confoff); > + if (type & IRQ_TYPE_LEVEL_MASK) > val &= ~confmask; > - else if (type == IRQ_TYPE_EDGE_RISING) > + else if (type & IRQ_TYPE_EDGE_BOTH) > val |= confmask; > > /* > @@ -54,15 +55,19 @@ void gic_configure_irq(unsigned int irq, unsigned int type, > > /* > * Write back the new configuration, and possibly re-enable > - * the interrupt. > + * the interrupt. If we tried to write a new configuration and failed, > + * return an error. > */ > writel_relaxed(val, base + GIC_DIST_CONFIG + confoff); > - > - if (enabled) > + if (readl_relaxed(base + GIC_DIST_CONFIG + confoff) != val && val != oldval) > + ret = -EINVAL; > + else if (enabled) > writel_relaxed(enablemask, base + GIC_DIST_ENABLE_SET + enableoff); > > if (sync_access) > sync_access(); > + > + return ret; > } > > void __init gic_dist_config(void __iomem *base, int gic_irqs, > diff --git a/drivers/irqchip/irq-gic-common.h b/drivers/irqchip/irq-gic-common.h > index b41f024..35a9884 100644 > --- a/drivers/irqchip/irq-gic-common.h > +++ b/drivers/irqchip/irq-gic-common.h > @@ -20,7 +20,7 @@ > #include <linux/of.h> > #include <linux/irqdomain.h> > > -void gic_configure_irq(unsigned int irq, unsigned int type, > +int gic_configure_irq(unsigned int irq, unsigned int type, > void __iomem *base, void (*sync_access)(void)); > void gic_dist_config(void __iomem *base, int gic_irqs, > void (*sync_access)(void)); > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c > index 1a146cc..6e50803 100644 > --- a/drivers/irqchip/irq-gic-v3.c > +++ b/drivers/irqchip/irq-gic-v3.c > @@ -238,7 +238,9 @@ static int gic_set_type(struct irq_data *d, unsigned int type) > if (irq < 16) > return -EINVAL; > > - if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING) > + /* SPIs have restrictions on the supported types */ > + if (irq >= 32 && type != IRQ_TYPE_LEVEL_HIGH && > + type != IRQ_TYPE_EDGE_RISING) > return -EINVAL; > > if (gic_irq_in_rdist(d)) { > @@ -249,9 +251,7 @@ static int gic_set_type(struct irq_data *d, unsigned int type) > rwp_wait = gic_dist_wait_for_rwp; > } > > - gic_configure_irq(irq, type, base, rwp_wait); > - > - return 0; > + return gic_configure_irq(irq, type, base, rwp_wait); > } > > static u64 gic_mpidr_to_affinity(u64 mpidr) > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c > index d617ee5..4634cf7 100644 > --- a/drivers/irqchip/irq-gic.c > +++ b/drivers/irqchip/irq-gic.c > @@ -188,12 +188,15 @@ static int gic_set_type(struct irq_data *d, unsigned int type) > { > void __iomem *base = gic_dist_base(d); > unsigned int gicirq = gic_irq(d); > + int ret; > > /* Interrupt configuration for SGIs can't be changed */ > if (gicirq < 16) > return -EINVAL; > > - if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING) > + /* SPIs have restrictions on the supported types */ > + if (gicirq >= 32 && type != IRQ_TYPE_LEVEL_HIGH && > + type != IRQ_TYPE_EDGE_RISING) > return -EINVAL; > > raw_spin_lock(&irq_controller_lock); > @@ -201,11 +204,11 @@ static int gic_set_type(struct irq_data *d, unsigned int type) > if (gic_arch_extn.irq_set_type) > gic_arch_extn.irq_set_type(d, type); > > - gic_configure_irq(gicirq, type, base, NULL); > + ret = gic_configure_irq(gicirq, type, base, NULL); > > raw_spin_unlock(&irq_controller_lock); > > - return 0; > + return ret; > } > > static int gic_retrigger(struct irq_data *d) > diff --git a/drivers/irqchip/irq-hip04.c b/drivers/irqchip/irq-hip04.c > index 29b8f21..d059e66 100644 > --- a/drivers/irqchip/irq-hip04.c > +++ b/drivers/irqchip/irq-hip04.c > @@ -120,21 +120,24 @@ static int hip04_irq_set_type(struct irq_data *d, unsigned int type) > { > void __iomem *base = hip04_dist_base(d); > unsigned int irq = hip04_irq(d); > + int ret; > > /* Interrupt configuration for SGIs can't be changed */ > if (irq < 16) > return -EINVAL; > > - if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING) > + /* SPIs have restrictions on the supported types */ > + if (irq >= 32 && type != IRQ_TYPE_LEVEL_HIGH && > + type != IRQ_TYPE_EDGE_RISING) > return -EINVAL; > > raw_spin_lock(&irq_controller_lock); > > - gic_configure_irq(irq, type, base, NULL); > + ret = gic_configure_irq(irq, type, base, NULL); > > raw_spin_unlock(&irq_controller_lock); > > - return 0; > + return ret; > } > > #ifdef CONFIG_SMP > -- > 2.1.3 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On Tue, Dec 09, 2014 at 12:29:19PM +0000, Liviu Dudau wrote: > On Mon, Dec 01, 2014 at 12:45:09PM +0000, Liviu Dudau wrote: > > During a recent cleanup of the arm64 DTs it has become clear that > > the handling of PPIs in xxxx_set_type() is incorrect. The ARM TRMs > > for GICv2 and later allow for "implementation defined" support for > > setting the edge or level type of the PPI interrupts and don't restrict > > the activation level of the signal. Current ARM implementations > > do restrict the PPI level type to IRQ_TYPE_LEVEL_LOW, but licensees > > of the IP can decide to shoot themselves in the foot at any time. > > > > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com> > > --- > > > > Made a stupid mistake in v2 and got my bit test confused with logical test. > > Gentle ping! > > Any comments here? Russell, are you OK with this approach? Now that the holidays are over, I'd like to get some feedback from Marc at a minimum... thx, Jason.
Hi Liviu, On 01/12/14 12:45, Liviu Dudau wrote: > During a recent cleanup of the arm64 DTs it has become clear that > the handling of PPIs in xxxx_set_type() is incorrect. The ARM TRMs > for GICv2 and later allow for "implementation defined" support for > setting the edge or level type of the PPI interrupts and don't restrict > the activation level of the signal. Current ARM implementations > do restrict the PPI level type to IRQ_TYPE_LEVEL_LOW, but licensees > of the IP can decide to shoot themselves in the foot at any time. > > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com> > --- > > Made a stupid mistake in v2 and got my bit test confused with logical test. > > Documentation/devicetree/bindings/arm/gic.txt | 8 ++++++-- > drivers/irqchip/irq-gic-common.c | 21 +++++++++++++-------- > drivers/irqchip/irq-gic-common.h | 2 +- > drivers/irqchip/irq-gic-v3.c | 8 ++++---- > drivers/irqchip/irq-gic.c | 9 ++++++--- > drivers/irqchip/irq-hip04.c | 9 ++++++--- > 6 files changed, 36 insertions(+), 21 deletions(-) > > diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt > index 8112d0c..c97484b 100644 > --- a/Documentation/devicetree/bindings/arm/gic.txt > +++ b/Documentation/devicetree/bindings/arm/gic.txt > @@ -32,12 +32,16 @@ Main node required properties: > The 3rd cell is the flags, encoded as follows: > bits[3:0] trigger type and level flags. > 1 = low-to-high edge triggered > - 2 = high-to-low edge triggered > + 2 = high-to-low edge triggered (invalid for SPIs) > 4 = active high level-sensitive > - 8 = active low level-sensitive > + 8 = active low level-sensitive (invalid for SPIs). > bits[15:8] PPI interrupt cpu mask. Each bit corresponds to each of > the 8 possible cpus attached to the GIC. A bit set to '1' indicated > the interrupt is wired to that CPU. Only valid for PPI interrupts. > + Also note that the configurability of PPI interrupts is IMPLEMENTATION > + DEFINED and as such not guaranteed to be present (most SoC available > + in 2014 seem to ignore the setting of this flag and use the hardware > + default value). > > - reg : Specifies base physical address(s) and size of the GIC registers. The > first region is the GIC distributor register base and size. The 2nd region is > diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c > index 61541ff..ffee521 100644 > --- a/drivers/irqchip/irq-gic-common.c > +++ b/drivers/irqchip/irq-gic-common.c > @@ -21,7 +21,7 @@ > > #include "irq-gic-common.h" > > -void gic_configure_irq(unsigned int irq, unsigned int type, > +int gic_configure_irq(unsigned int irq, unsigned int type, > void __iomem *base, void (*sync_access)(void)) > { > u32 enablemask = 1 << (irq % 32); > @@ -29,16 +29,17 @@ void gic_configure_irq(unsigned int irq, unsigned int type, > u32 confmask = 0x2 << ((irq % 16) * 2); > u32 confoff = (irq / 16) * 4; > bool enabled = false; > - u32 val; > + u32 val, oldval; > + int ret = 0; > > /* > * Read current configuration register, and insert the config > * for "irq", depending on "type". > */ > - val = readl_relaxed(base + GIC_DIST_CONFIG + confoff); > - if (type == IRQ_TYPE_LEVEL_HIGH) > + val = oldval = readl_relaxed(base + GIC_DIST_CONFIG + confoff); > + if (type & IRQ_TYPE_LEVEL_MASK) > val &= ~confmask; > - else if (type == IRQ_TYPE_EDGE_RISING) > + else if (type & IRQ_TYPE_EDGE_BOTH) > val |= confmask; > > /* > @@ -54,15 +55,19 @@ void gic_configure_irq(unsigned int irq, unsigned int type, > > /* > * Write back the new configuration, and possibly re-enable > - * the interrupt. > + * the interrupt. If we tried to write a new configuration and failed, > + * return an error. > */ > writel_relaxed(val, base + GIC_DIST_CONFIG + confoff); > - > - if (enabled) > + if (readl_relaxed(base + GIC_DIST_CONFIG + confoff) != val && val != oldval) > + ret = -EINVAL; > + else if (enabled) > writel_relaxed(enablemask, base + GIC_DIST_ENABLE_SET + enableoff); So if you change the configuration of an enabled interrupt and fail to do so, you end-up with the interrupt disabled. This is a change in behaviour, and is likely to introduce regressions. > > if (sync_access) > sync_access(); > + > + return ret; > } > > void __init gic_dist_config(void __iomem *base, int gic_irqs, > diff --git a/drivers/irqchip/irq-gic-common.h b/drivers/irqchip/irq-gic-common.h > index b41f024..35a9884 100644 > --- a/drivers/irqchip/irq-gic-common.h > +++ b/drivers/irqchip/irq-gic-common.h > @@ -20,7 +20,7 @@ > #include <linux/of.h> > #include <linux/irqdomain.h> > > -void gic_configure_irq(unsigned int irq, unsigned int type, > +int gic_configure_irq(unsigned int irq, unsigned int type, > void __iomem *base, void (*sync_access)(void)); > void gic_dist_config(void __iomem *base, int gic_irqs, > void (*sync_access)(void)); > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c > index 1a146cc..6e50803 100644 > --- a/drivers/irqchip/irq-gic-v3.c > +++ b/drivers/irqchip/irq-gic-v3.c > @@ -238,7 +238,9 @@ static int gic_set_type(struct irq_data *d, unsigned int type) > if (irq < 16) > return -EINVAL; > > - if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING) > + /* SPIs have restrictions on the supported types */ > + if (irq >= 32 && type != IRQ_TYPE_LEVEL_HIGH && > + type != IRQ_TYPE_EDGE_RISING) > return -EINVAL; > > if (gic_irq_in_rdist(d)) { > @@ -249,9 +251,7 @@ static int gic_set_type(struct irq_data *d, unsigned int type) > rwp_wait = gic_dist_wait_for_rwp; > } > > - gic_configure_irq(irq, type, base, rwp_wait); > - > - return 0; > + return gic_configure_irq(irq, type, base, rwp_wait); > } > > static u64 gic_mpidr_to_affinity(u64 mpidr) > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c > index d617ee5..4634cf7 100644 > --- a/drivers/irqchip/irq-gic.c > +++ b/drivers/irqchip/irq-gic.c > @@ -188,12 +188,15 @@ static int gic_set_type(struct irq_data *d, unsigned int type) > { > void __iomem *base = gic_dist_base(d); > unsigned int gicirq = gic_irq(d); > + int ret; > > /* Interrupt configuration for SGIs can't be changed */ > if (gicirq < 16) > return -EINVAL; > > - if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING) > + /* SPIs have restrictions on the supported types */ > + if (gicirq >= 32 && type != IRQ_TYPE_LEVEL_HIGH && > + type != IRQ_TYPE_EDGE_RISING) > return -EINVAL; > > raw_spin_lock(&irq_controller_lock); > @@ -201,11 +204,11 @@ static int gic_set_type(struct irq_data *d, unsigned int type) > if (gic_arch_extn.irq_set_type) > gic_arch_extn.irq_set_type(d, type); > > - gic_configure_irq(gicirq, type, base, NULL); > + ret = gic_configure_irq(gicirq, type, base, NULL); > > raw_spin_unlock(&irq_controller_lock); > > - return 0; > + return ret; > } > > static int gic_retrigger(struct irq_data *d) > diff --git a/drivers/irqchip/irq-hip04.c b/drivers/irqchip/irq-hip04.c > index 29b8f21..d059e66 100644 > --- a/drivers/irqchip/irq-hip04.c > +++ b/drivers/irqchip/irq-hip04.c > @@ -120,21 +120,24 @@ static int hip04_irq_set_type(struct irq_data *d, unsigned int type) > { > void __iomem *base = hip04_dist_base(d); > unsigned int irq = hip04_irq(d); > + int ret; > > /* Interrupt configuration for SGIs can't be changed */ > if (irq < 16) > return -EINVAL; > > - if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING) > + /* SPIs have restrictions on the supported types */ > + if (irq >= 32 && type != IRQ_TYPE_LEVEL_HIGH && > + type != IRQ_TYPE_EDGE_RISING) > return -EINVAL; > > raw_spin_lock(&irq_controller_lock); > > - gic_configure_irq(irq, type, base, NULL); > + ret = gic_configure_irq(irq, type, base, NULL); > > raw_spin_unlock(&irq_controller_lock); > > - return 0; > + return ret; > } > > #ifdef CONFIG_SMP > I have exactly zero knowledge about the hip04 controller, and despite it looking like a GIC, it is quite different from the real thing. I'd like to see confirmation from the HiSilicon guys that this is the right thing to do, and possibly this to be moved to a separate patch if that requires extra changes. Thanks, M.
On Mon, Jan 05, 2015 at 09:05:06AM +0000, Marc Zyngier wrote: > Hi Liviu, Hi Marc, > > On 01/12/14 12:45, Liviu Dudau wrote: > > During a recent cleanup of the arm64 DTs it has become clear that > > the handling of PPIs in xxxx_set_type() is incorrect. The ARM TRMs > > for GICv2 and later allow for "implementation defined" support for > > setting the edge or level type of the PPI interrupts and don't restrict > > the activation level of the signal. Current ARM implementations > > do restrict the PPI level type to IRQ_TYPE_LEVEL_LOW, but licensees > > of the IP can decide to shoot themselves in the foot at any time. > > > > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com> > > --- > > > > Made a stupid mistake in v2 and got my bit test confused with logical test. > > > > Documentation/devicetree/bindings/arm/gic.txt | 8 ++++++-- > > drivers/irqchip/irq-gic-common.c | 21 +++++++++++++-------- > > drivers/irqchip/irq-gic-common.h | 2 +- > > drivers/irqchip/irq-gic-v3.c | 8 ++++---- > > drivers/irqchip/irq-gic.c | 9 ++++++--- > > drivers/irqchip/irq-hip04.c | 9 ++++++--- > > 6 files changed, 36 insertions(+), 21 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt > > index 8112d0c..c97484b 100644 > > --- a/Documentation/devicetree/bindings/arm/gic.txt > > +++ b/Documentation/devicetree/bindings/arm/gic.txt > > @@ -32,12 +32,16 @@ Main node required properties: > > The 3rd cell is the flags, encoded as follows: > > bits[3:0] trigger type and level flags. > > 1 = low-to-high edge triggered > > - 2 = high-to-low edge triggered > > + 2 = high-to-low edge triggered (invalid for SPIs) > > 4 = active high level-sensitive > > - 8 = active low level-sensitive > > + 8 = active low level-sensitive (invalid for SPIs). > > bits[15:8] PPI interrupt cpu mask. Each bit corresponds to each of > > the 8 possible cpus attached to the GIC. A bit set to '1' indicated > > the interrupt is wired to that CPU. Only valid for PPI interrupts. > > + Also note that the configurability of PPI interrupts is IMPLEMENTATION > > + DEFINED and as such not guaranteed to be present (most SoC available > > + in 2014 seem to ignore the setting of this flag and use the hardware > > + default value). > > > > - reg : Specifies base physical address(s) and size of the GIC registers. The > > first region is the GIC distributor register base and size. The 2nd region is > > diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c > > index 61541ff..ffee521 100644 > > --- a/drivers/irqchip/irq-gic-common.c > > +++ b/drivers/irqchip/irq-gic-common.c > > @@ -21,7 +21,7 @@ > > > > #include "irq-gic-common.h" > > > > -void gic_configure_irq(unsigned int irq, unsigned int type, > > +int gic_configure_irq(unsigned int irq, unsigned int type, > > void __iomem *base, void (*sync_access)(void)) > > { > > u32 enablemask = 1 << (irq % 32); > > @@ -29,16 +29,17 @@ void gic_configure_irq(unsigned int irq, unsigned int type, > > u32 confmask = 0x2 << ((irq % 16) * 2); > > u32 confoff = (irq / 16) * 4; > > bool enabled = false; > > - u32 val; > > + u32 val, oldval; > > + int ret = 0; > > > > /* > > * Read current configuration register, and insert the config > > * for "irq", depending on "type". > > */ > > - val = readl_relaxed(base + GIC_DIST_CONFIG + confoff); > > - if (type == IRQ_TYPE_LEVEL_HIGH) > > + val = oldval = readl_relaxed(base + GIC_DIST_CONFIG + confoff); > > + if (type & IRQ_TYPE_LEVEL_MASK) > > val &= ~confmask; > > - else if (type == IRQ_TYPE_EDGE_RISING) > > + else if (type & IRQ_TYPE_EDGE_BOTH) > > val |= confmask; > > > > /* > > @@ -54,15 +55,19 @@ void gic_configure_irq(unsigned int irq, unsigned int type, > > > > /* > > * Write back the new configuration, and possibly re-enable > > - * the interrupt. > > + * the interrupt. If we tried to write a new configuration and failed, > > + * return an error. > > */ > > writel_relaxed(val, base + GIC_DIST_CONFIG + confoff); > > - > > - if (enabled) > > + if (readl_relaxed(base + GIC_DIST_CONFIG + confoff) != val && val != oldval) > > + ret = -EINVAL; > > + else if (enabled) > > writel_relaxed(enablemask, base + GIC_DIST_ENABLE_SET + enableoff); > > So if you change the configuration of an enabled interrupt and fail to > do so, you end-up with the interrupt disabled. This is a change in > behaviour, and is likely to introduce regressions. Is it? Beforehand we were silently ignoring the fact that the new values haven't been set, now we return an error. Should we continue to ignore the fact that there is now a difference between what the kernel believes the setup is and what the hardware is configured to do? (Sorry for delay in responding, just returned from my extended holiday.) Best regards, Liviu > > > > > if (sync_access) > > sync_access(); > > + > > + return ret; > > } > > > > void __init gic_dist_config(void __iomem *base, int gic_irqs, > > diff --git a/drivers/irqchip/irq-gic-common.h b/drivers/irqchip/irq-gic-common.h > > index b41f024..35a9884 100644 > > --- a/drivers/irqchip/irq-gic-common.h > > +++ b/drivers/irqchip/irq-gic-common.h > > @@ -20,7 +20,7 @@ > > #include <linux/of.h> > > #include <linux/irqdomain.h> > > > > -void gic_configure_irq(unsigned int irq, unsigned int type, > > +int gic_configure_irq(unsigned int irq, unsigned int type, > > void __iomem *base, void (*sync_access)(void)); > > void gic_dist_config(void __iomem *base, int gic_irqs, > > void (*sync_access)(void)); > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c > > index 1a146cc..6e50803 100644 > > --- a/drivers/irqchip/irq-gic-v3.c > > +++ b/drivers/irqchip/irq-gic-v3.c > > @@ -238,7 +238,9 @@ static int gic_set_type(struct irq_data *d, unsigned int type) > > if (irq < 16) > > return -EINVAL; > > > > - if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING) > > + /* SPIs have restrictions on the supported types */ > > + if (irq >= 32 && type != IRQ_TYPE_LEVEL_HIGH && > > + type != IRQ_TYPE_EDGE_RISING) > > return -EINVAL; > > > > if (gic_irq_in_rdist(d)) { > > @@ -249,9 +251,7 @@ static int gic_set_type(struct irq_data *d, unsigned int type) > > rwp_wait = gic_dist_wait_for_rwp; > > } > > > > - gic_configure_irq(irq, type, base, rwp_wait); > > - > > - return 0; > > + return gic_configure_irq(irq, type, base, rwp_wait); > > } > > > > static u64 gic_mpidr_to_affinity(u64 mpidr) > > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c > > index d617ee5..4634cf7 100644 > > --- a/drivers/irqchip/irq-gic.c > > +++ b/drivers/irqchip/irq-gic.c > > @@ -188,12 +188,15 @@ static int gic_set_type(struct irq_data *d, unsigned int type) > > { > > void __iomem *base = gic_dist_base(d); > > unsigned int gicirq = gic_irq(d); > > + int ret; > > > > /* Interrupt configuration for SGIs can't be changed */ > > if (gicirq < 16) > > return -EINVAL; > > > > - if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING) > > + /* SPIs have restrictions on the supported types */ > > + if (gicirq >= 32 && type != IRQ_TYPE_LEVEL_HIGH && > > + type != IRQ_TYPE_EDGE_RISING) > > return -EINVAL; > > > > raw_spin_lock(&irq_controller_lock); > > @@ -201,11 +204,11 @@ static int gic_set_type(struct irq_data *d, unsigned int type) > > if (gic_arch_extn.irq_set_type) > > gic_arch_extn.irq_set_type(d, type); > > > > - gic_configure_irq(gicirq, type, base, NULL); > > + ret = gic_configure_irq(gicirq, type, base, NULL); > > > > raw_spin_unlock(&irq_controller_lock); > > > > - return 0; > > + return ret; > > } > > > > static int gic_retrigger(struct irq_data *d) > > diff --git a/drivers/irqchip/irq-hip04.c b/drivers/irqchip/irq-hip04.c > > index 29b8f21..d059e66 100644 > > --- a/drivers/irqchip/irq-hip04.c > > +++ b/drivers/irqchip/irq-hip04.c > > @@ -120,21 +120,24 @@ static int hip04_irq_set_type(struct irq_data *d, unsigned int type) > > { > > void __iomem *base = hip04_dist_base(d); > > unsigned int irq = hip04_irq(d); > > + int ret; > > > > /* Interrupt configuration for SGIs can't be changed */ > > if (irq < 16) > > return -EINVAL; > > > > - if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING) > > + /* SPIs have restrictions on the supported types */ > > + if (irq >= 32 && type != IRQ_TYPE_LEVEL_HIGH && > > + type != IRQ_TYPE_EDGE_RISING) > > return -EINVAL; > > > > raw_spin_lock(&irq_controller_lock); > > > > - gic_configure_irq(irq, type, base, NULL); > > + ret = gic_configure_irq(irq, type, base, NULL); > > > > raw_spin_unlock(&irq_controller_lock); > > > > - return 0; > > + return ret; > > } > > > > #ifdef CONFIG_SMP > > > > I have exactly zero knowledge about the hip04 controller, and despite it > looking like a GIC, it is quite different from the real thing. I'd like > to see confirmation from the HiSilicon guys that this is the right thing > to do, and possibly this to be moved to a separate patch if that > requires extra changes. > > Thanks, > > M. > -- > Jazz is not dead. It just smells funny... >
Hi Liviu, Welcome back! ;-) On 12/01/15 11:15, Liviu Dudau wrote: > On Mon, Jan 05, 2015 at 09:05:06AM +0000, Marc Zyngier wrote: >> Hi Liviu, > > Hi Marc, > >> >> On 01/12/14 12:45, Liviu Dudau wrote: >>> During a recent cleanup of the arm64 DTs it has become clear that >>> the handling of PPIs in xxxx_set_type() is incorrect. The ARM TRMs >>> for GICv2 and later allow for "implementation defined" support for >>> setting the edge or level type of the PPI interrupts and don't restrict >>> the activation level of the signal. Current ARM implementations >>> do restrict the PPI level type to IRQ_TYPE_LEVEL_LOW, but licensees >>> of the IP can decide to shoot themselves in the foot at any time. >>> >>> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com> >>> --- >>> >>> Made a stupid mistake in v2 and got my bit test confused with logical test. >>> >>> Documentation/devicetree/bindings/arm/gic.txt | 8 ++++++-- >>> drivers/irqchip/irq-gic-common.c | 21 +++++++++++++-------- >>> drivers/irqchip/irq-gic-common.h | 2 +- >>> drivers/irqchip/irq-gic-v3.c | 8 ++++---- >>> drivers/irqchip/irq-gic.c | 9 ++++++--- >>> drivers/irqchip/irq-hip04.c | 9 ++++++--- >>> 6 files changed, 36 insertions(+), 21 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt >>> index 8112d0c..c97484b 100644 >>> --- a/Documentation/devicetree/bindings/arm/gic.txt >>> +++ b/Documentation/devicetree/bindings/arm/gic.txt >>> @@ -32,12 +32,16 @@ Main node required properties: >>> The 3rd cell is the flags, encoded as follows: >>> bits[3:0] trigger type and level flags. >>> 1 = low-to-high edge triggered >>> - 2 = high-to-low edge triggered >>> + 2 = high-to-low edge triggered (invalid for SPIs) >>> 4 = active high level-sensitive >>> - 8 = active low level-sensitive >>> + 8 = active low level-sensitive (invalid for SPIs). >>> bits[15:8] PPI interrupt cpu mask. Each bit corresponds to each of >>> the 8 possible cpus attached to the GIC. A bit set to '1' indicated >>> the interrupt is wired to that CPU. Only valid for PPI interrupts. >>> + Also note that the configurability of PPI interrupts is IMPLEMENTATION >>> + DEFINED and as such not guaranteed to be present (most SoC available >>> + in 2014 seem to ignore the setting of this flag and use the hardware >>> + default value). >>> >>> - reg : Specifies base physical address(s) and size of the GIC registers. The >>> first region is the GIC distributor register base and size. The 2nd region is >>> diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c >>> index 61541ff..ffee521 100644 >>> --- a/drivers/irqchip/irq-gic-common.c >>> +++ b/drivers/irqchip/irq-gic-common.c >>> @@ -21,7 +21,7 @@ >>> >>> #include "irq-gic-common.h" >>> >>> -void gic_configure_irq(unsigned int irq, unsigned int type, >>> +int gic_configure_irq(unsigned int irq, unsigned int type, >>> void __iomem *base, void (*sync_access)(void)) >>> { >>> u32 enablemask = 1 << (irq % 32); >>> @@ -29,16 +29,17 @@ void gic_configure_irq(unsigned int irq, unsigned int type, >>> u32 confmask = 0x2 << ((irq % 16) * 2); >>> u32 confoff = (irq / 16) * 4; >>> bool enabled = false; >>> - u32 val; >>> + u32 val, oldval; >>> + int ret = 0; >>> >>> /* >>> * Read current configuration register, and insert the config >>> * for "irq", depending on "type". >>> */ >>> - val = readl_relaxed(base + GIC_DIST_CONFIG + confoff); >>> - if (type == IRQ_TYPE_LEVEL_HIGH) >>> + val = oldval = readl_relaxed(base + GIC_DIST_CONFIG + confoff); >>> + if (type & IRQ_TYPE_LEVEL_MASK) >>> val &= ~confmask; >>> - else if (type == IRQ_TYPE_EDGE_RISING) >>> + else if (type & IRQ_TYPE_EDGE_BOTH) >>> val |= confmask; >>> >>> /* >>> @@ -54,15 +55,19 @@ void gic_configure_irq(unsigned int irq, unsigned int type, >>> >>> /* >>> * Write back the new configuration, and possibly re-enable >>> - * the interrupt. >>> + * the interrupt. If we tried to write a new configuration and failed, >>> + * return an error. >>> */ >>> writel_relaxed(val, base + GIC_DIST_CONFIG + confoff); >>> - >>> - if (enabled) >>> + if (readl_relaxed(base + GIC_DIST_CONFIG + confoff) != val && val != oldval) >>> + ret = -EINVAL; >>> + else if (enabled) >>> writel_relaxed(enablemask, base + GIC_DIST_ENABLE_SET + enableoff); >> >> So if you change the configuration of an enabled interrupt and fail to >> do so, you end-up with the interrupt disabled. This is a change in >> behaviour, and is likely to introduce regressions. > > Is it? Beforehand we were silently ignoring the fact that the new values haven't been > set, now we return an error. Should we continue to ignore the fact that there is now > a difference between what the kernel believes the setup is and what the hardware is > configured to do? Returning the error is fine. It is the fact that you've now disabled a line that was previously enabled.I don't think that sort of side effect is acceptable. Thanks, M.
diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt index 8112d0c..c97484b 100644 --- a/Documentation/devicetree/bindings/arm/gic.txt +++ b/Documentation/devicetree/bindings/arm/gic.txt @@ -32,12 +32,16 @@ Main node required properties: The 3rd cell is the flags, encoded as follows: bits[3:0] trigger type and level flags. 1 = low-to-high edge triggered - 2 = high-to-low edge triggered + 2 = high-to-low edge triggered (invalid for SPIs) 4 = active high level-sensitive - 8 = active low level-sensitive + 8 = active low level-sensitive (invalid for SPIs). bits[15:8] PPI interrupt cpu mask. Each bit corresponds to each of the 8 possible cpus attached to the GIC. A bit set to '1' indicated the interrupt is wired to that CPU. Only valid for PPI interrupts. + Also note that the configurability of PPI interrupts is IMPLEMENTATION + DEFINED and as such not guaranteed to be present (most SoC available + in 2014 seem to ignore the setting of this flag and use the hardware + default value). - reg : Specifies base physical address(s) and size of the GIC registers. The first region is the GIC distributor register base and size. The 2nd region is diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c index 61541ff..ffee521 100644 --- a/drivers/irqchip/irq-gic-common.c +++ b/drivers/irqchip/irq-gic-common.c @@ -21,7 +21,7 @@ #include "irq-gic-common.h" -void gic_configure_irq(unsigned int irq, unsigned int type, +int gic_configure_irq(unsigned int irq, unsigned int type, void __iomem *base, void (*sync_access)(void)) { u32 enablemask = 1 << (irq % 32); @@ -29,16 +29,17 @@ void gic_configure_irq(unsigned int irq, unsigned int type, u32 confmask = 0x2 << ((irq % 16) * 2); u32 confoff = (irq / 16) * 4; bool enabled = false; - u32 val; + u32 val, oldval; + int ret = 0; /* * Read current configuration register, and insert the config * for "irq", depending on "type". */ - val = readl_relaxed(base + GIC_DIST_CONFIG + confoff); - if (type == IRQ_TYPE_LEVEL_HIGH) + val = oldval = readl_relaxed(base + GIC_DIST_CONFIG + confoff); + if (type & IRQ_TYPE_LEVEL_MASK) val &= ~confmask; - else if (type == IRQ_TYPE_EDGE_RISING) + else if (type & IRQ_TYPE_EDGE_BOTH) val |= confmask; /* @@ -54,15 +55,19 @@ void gic_configure_irq(unsigned int irq, unsigned int type, /* * Write back the new configuration, and possibly re-enable - * the interrupt. + * the interrupt. If we tried to write a new configuration and failed, + * return an error. */ writel_relaxed(val, base + GIC_DIST_CONFIG + confoff); - - if (enabled) + if (readl_relaxed(base + GIC_DIST_CONFIG + confoff) != val && val != oldval) + ret = -EINVAL; + else if (enabled) writel_relaxed(enablemask, base + GIC_DIST_ENABLE_SET + enableoff); if (sync_access) sync_access(); + + return ret; } void __init gic_dist_config(void __iomem *base, int gic_irqs, diff --git a/drivers/irqchip/irq-gic-common.h b/drivers/irqchip/irq-gic-common.h index b41f024..35a9884 100644 --- a/drivers/irqchip/irq-gic-common.h +++ b/drivers/irqchip/irq-gic-common.h @@ -20,7 +20,7 @@ #include <linux/of.h> #include <linux/irqdomain.h> -void gic_configure_irq(unsigned int irq, unsigned int type, +int gic_configure_irq(unsigned int irq, unsigned int type, void __iomem *base, void (*sync_access)(void)); void gic_dist_config(void __iomem *base, int gic_irqs, void (*sync_access)(void)); diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c index 1a146cc..6e50803 100644 --- a/drivers/irqchip/irq-gic-v3.c +++ b/drivers/irqchip/irq-gic-v3.c @@ -238,7 +238,9 @@ static int gic_set_type(struct irq_data *d, unsigned int type) if (irq < 16) return -EINVAL; - if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING) + /* SPIs have restrictions on the supported types */ + if (irq >= 32 && type != IRQ_TYPE_LEVEL_HIGH && + type != IRQ_TYPE_EDGE_RISING) return -EINVAL; if (gic_irq_in_rdist(d)) { @@ -249,9 +251,7 @@ static int gic_set_type(struct irq_data *d, unsigned int type) rwp_wait = gic_dist_wait_for_rwp; } - gic_configure_irq(irq, type, base, rwp_wait); - - return 0; + return gic_configure_irq(irq, type, base, rwp_wait); } static u64 gic_mpidr_to_affinity(u64 mpidr) diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c index d617ee5..4634cf7 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -188,12 +188,15 @@ static int gic_set_type(struct irq_data *d, unsigned int type) { void __iomem *base = gic_dist_base(d); unsigned int gicirq = gic_irq(d); + int ret; /* Interrupt configuration for SGIs can't be changed */ if (gicirq < 16) return -EINVAL; - if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING) + /* SPIs have restrictions on the supported types */ + if (gicirq >= 32 && type != IRQ_TYPE_LEVEL_HIGH && + type != IRQ_TYPE_EDGE_RISING) return -EINVAL; raw_spin_lock(&irq_controller_lock); @@ -201,11 +204,11 @@ static int gic_set_type(struct irq_data *d, unsigned int type) if (gic_arch_extn.irq_set_type) gic_arch_extn.irq_set_type(d, type); - gic_configure_irq(gicirq, type, base, NULL); + ret = gic_configure_irq(gicirq, type, base, NULL); raw_spin_unlock(&irq_controller_lock); - return 0; + return ret; } static int gic_retrigger(struct irq_data *d) diff --git a/drivers/irqchip/irq-hip04.c b/drivers/irqchip/irq-hip04.c index 29b8f21..d059e66 100644 --- a/drivers/irqchip/irq-hip04.c +++ b/drivers/irqchip/irq-hip04.c @@ -120,21 +120,24 @@ static int hip04_irq_set_type(struct irq_data *d, unsigned int type) { void __iomem *base = hip04_dist_base(d); unsigned int irq = hip04_irq(d); + int ret; /* Interrupt configuration for SGIs can't be changed */ if (irq < 16) return -EINVAL; - if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING) + /* SPIs have restrictions on the supported types */ + if (irq >= 32 && type != IRQ_TYPE_LEVEL_HIGH && + type != IRQ_TYPE_EDGE_RISING) return -EINVAL; raw_spin_lock(&irq_controller_lock); - gic_configure_irq(irq, type, base, NULL); + ret = gic_configure_irq(irq, type, base, NULL); raw_spin_unlock(&irq_controller_lock); - return 0; + return ret; } #ifdef CONFIG_SMP
During a recent cleanup of the arm64 DTs it has become clear that the handling of PPIs in xxxx_set_type() is incorrect. The ARM TRMs for GICv2 and later allow for "implementation defined" support for setting the edge or level type of the PPI interrupts and don't restrict the activation level of the signal. Current ARM implementations do restrict the PPI level type to IRQ_TYPE_LEVEL_LOW, but licensees of the IP can decide to shoot themselves in the foot at any time. Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com> --- Made a stupid mistake in v2 and got my bit test confused with logical test. Documentation/devicetree/bindings/arm/gic.txt | 8 ++++++-- drivers/irqchip/irq-gic-common.c | 21 +++++++++++++-------- drivers/irqchip/irq-gic-common.h | 2 +- drivers/irqchip/irq-gic-v3.c | 8 ++++---- drivers/irqchip/irq-gic.c | 9 ++++++--- drivers/irqchip/irq-hip04.c | 9 ++++++--- 6 files changed, 36 insertions(+), 21 deletions(-)