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 <[email protected]> >>> --- >>> >>> 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. -- Jazz is not dead. It just smells funny... -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
