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@xxxxxxx>
>>> ---
>>>
>>> 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 devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
