Re: [PATCH v3] irqchip: gic: Allow interrupt level to be set for PPIs.

[Marc Zyngier]

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 linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/