Re: [RFC PATCH V2 2/8] irqdomain: Don't set type when mapping an IRQ

Re: [RFC PATCH V2 2/8] irqdomain: Don't set type when mapping an IRQ

[Grygorii Strashko]

On 12/17/2015 12:48 PM, Jon Hunter wrote:
> Some IRQ chips, such as GPIO controllers or secondary level interrupt
> controllers, may require require additional runtime power management
> control to ensure they are accessible. For such IRQ chips, it makes sense
> to enable the IRQ chip when interrupts are requested and disabled them
> again once all interrupts have been freed.
> 
> When mapping an IRQ, the IRQ type settings are read and then programmed.
> The mapping of the IRQ happens before the IRQ is requested and so the
> programming of the type settings occurs before the IRQ is requested. This
> is a problem for IRQ chips that require additional power management
> control because they may not be accessible yet. Therefore, when mapping
> the IRQ, don't program the type settings, just save them and then program
> these saved settings when the IRQ is requested (so long as if they are not
> overridden via the call to request the IRQ).
> 
> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>   kernel/irq/irqdomain.c | 18 ++++++++++++++----
>   kernel/irq/manage.c    |  7 +++++++
>   2 files changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index eae31e978ab2..4322d6fd0b8f 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -570,6 +570,7 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>   {
>   	struct device_node *of_node;
>   	struct irq_domain *domain;
> +	struct irq_data *irq_data;
>   	irq_hw_number_t hwirq;
>   	unsigned int cur_type = IRQ_TYPE_NONE;
>   	unsigned int type = IRQ_TYPE_NONE;
> @@ -634,10 +635,19 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>   		return 0;
>   	}
>   
> -	/* Set type if specified and different than the current one */
> -	if (type != IRQ_TYPE_NONE &&
> -	    type != irq_get_trigger_type(virq))
> -		irq_set_irq_type(virq, type);

^^^ note: Return code hes never ever been checked here ;)

This patch has side effect - some boards which have
ARM TWD timer will produce below backtrace on boot:

 genirq: Setting trigger mode 4 for irq 17 failed (gic_set_type+0x0/0x58)
 twd: can't register interrupt 17 (-22)
------------[ cut here ]------------
WARNING: CPU: 0 PID: 0 at arch/arm/kernel/smp_twd.c:405 twd_local_timer_of_register+0x68/0x7c()
twd_local_timer_of_register failed (-22)
Modules linked in:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.1.13-01909-g59e42df #53
Hardware name: Generic AM43 (Flattened Device Tree)
Backtrace: 
[<c00130dc>] (dump_backtrace) from [<c00132fc>] (show_stack+0x18/0x1c)
 r7:c0797cec r6:00000000 r5:c08fa7bc r4:00000000
[<c00132e4>] (show_stack) from [<c0639334>] (dump_stack+0x98/0xe0)
[<c063929c>] (dump_stack) from [<c0044928>] (warn_slowpath_common+0x7c/0xb8)
 r7:c0797cec r6:00000195 r5:00000009 r4:c08b3f30
[<c00448ac>] (warn_slowpath_common) from [<c004499c>] (warn_slowpath_fmt+0x38/0x40)
 r8:c0932000 r7:c08b48c0 r6:ffffffff r5:ee5c9fb4 r4:c0797cc0
[<c0044968>] (warn_slowpath_fmt) from [<c085c440>] (twd_local_timer_of_register+0x68/0x7c)
 r3:ffffffea r2:c0797cc0
 r4:c09322c4
[<c085c3d8>] (twd_local_timer_of_register) from [<c0889d74>] (clocksource_of_init+0x50/0x90)
 r5:00000001 r4:ee5c9fb4
[<c0889d24>] (clocksource_of_init) from [<c0864224>] (omap4_local_timer_init+0x34/0x68)
 r5:c0932000 r4:00000000
[<c08641f0>] (omap4_local_timer_init) from [<c085b7f4>] (time_init+0x24/0x38)
[<c085b7d0>] (time_init) from [<c0857b84>] (start_kernel+0x248/0x3e4)
[<c085793c>] (start_kernel) from [<8000807c>] (0x8000807c)
 r10:00000000 r9:412fc09a r8:80004059 r7:c08b8884 r6:c089934c r5:c08b4970
 r4:c0932214
---[ end trace cb88537fdc8fa200 ]---

This happens, most probably, because TWD IRQs definitions in DT
do not corresponds HW and gic_configure_irq() will return -EINVAL
example (am4372.dtsi):
	local_timer: timer@48240600 {
		compatible = "arm,cortex-a9-twd-timer";
		reg = <0x48240600 0x100>;
		interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_HIGH>;
		interrupt-parent = <&gic>;
		clocks = <&mpu_periphclk>;
		status = "disabled";
	};


So, some additional fixes might be required. Potentially problematic files are:
arch/arm/boot/dts/bcm5301x.dtsi
arch/arm/boot/dts/bcm63138.dtsi
arch/arm/boot/dts/berlin2cd.dtsi
arch/arm/boot/dts/berlin2.dtsi
arch/arm/boot/dts/berlin2q.dtsi
arch/arm/boot/dts/hip01.dtsi
arch/arm/boot/dts/omap4.dtsi
arch/arm/boot/dts/r8a7779.dts
arch/arm/boot/dts/rk3xxx.dtsi
arch/arm/boot/dts/sh73a0.dtsi
arch/arm/boot/dts/socfpga_arria10.dtsi
arch/arm/boot/dts/socfpga.dtsi
arch/arm/boot/dts/spear13xx.dtsi
arch/arm/boot/dts/ste-dbx5x0.dtsi
arch/arm/boot/dts/tegra20.dtsi
arch/arm/boot/dts/tegra30.dtsi
arch/arm/boot/dts/uniphier-ph1-ld4.dtsi
arch/arm/boot/dts/uniphier-ph1-XXXX.dtsi
arch/arm/boot/dts/uniphier-proxstream2.dtsi
arch/arm/boot/dts/vexpress-v2p-ca5s.dts
arch/arm/boot/dts/vexpress-v2p-ca9.dts


Any way, It seems working for me, but I've back-ported and tested
it on kernel 4.1 and will try to do more tests this week.

Thanks a lot.
-- 
regards,
-grygorii

Re: [RFC PATCH V2 2/8] irqdomain: Don't set type when mapping an IRQ

[Jon Hunter]

On 22/12/15 11:18, Grygorii Strashko wrote:
> On 12/17/2015 12:48 PM, Jon Hunter wrote:
>> Some IRQ chips, such as GPIO controllers or secondary level interrupt
>> controllers, may require require additional runtime power management
>> control to ensure they are accessible. For such IRQ chips, it makes sense
>> to enable the IRQ chip when interrupts are requested and disabled them
>> again once all interrupts have been freed.
>>
>> When mapping an IRQ, the IRQ type settings are read and then programmed.
>> The mapping of the IRQ happens before the IRQ is requested and so the
>> programming of the type settings occurs before the IRQ is requested. This
>> is a problem for IRQ chips that require additional power management
>> control because they may not be accessible yet. Therefore, when mapping
>> the IRQ, don't program the type settings, just save them and then program
>> these saved settings when the IRQ is requested (so long as if they are not
>> overridden via the call to request the IRQ).
>>
>> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>   kernel/irq/irqdomain.c | 18 ++++++++++++++----
>>   kernel/irq/manage.c    |  7 +++++++
>>   2 files changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
>> index eae31e978ab2..4322d6fd0b8f 100644
>> --- a/kernel/irq/irqdomain.c
>> +++ b/kernel/irq/irqdomain.c
>> @@ -570,6 +570,7 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>>   {
>>   	struct device_node *of_node;
>>   	struct irq_domain *domain;
>> +	struct irq_data *irq_data;
>>   	irq_hw_number_t hwirq;
>>   	unsigned int cur_type = IRQ_TYPE_NONE;
>>   	unsigned int type = IRQ_TYPE_NONE;
>> @@ -634,10 +635,19 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>>   		return 0;
>>   	}
>>   
>> -	/* Set type if specified and different than the current one */
>> -	if (type != IRQ_TYPE_NONE &&
>> -	    type != irq_get_trigger_type(virq))
>> -		irq_set_irq_type(virq, type);
> 
> ^^^ note: Return code hes never ever been checked here ;)
> 
> This patch has side effect - some boards which have
> ARM TWD timer will produce below backtrace on boot:
> 
>  genirq: Setting trigger mode 4 for irq 17 failed (gic_set_type+0x0/0x58)
>  twd: can't register interrupt 17 (-22)
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 0 at arch/arm/kernel/smp_twd.c:405 twd_local_timer_of_register+0x68/0x7c()
> twd_local_timer_of_register failed (-22)
> Modules linked in:
> CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.1.13-01909-g59e42df #53
> Hardware name: Generic AM43 (Flattened Device Tree)
> Backtrace: 
> [<c00130dc>] (dump_backtrace) from [<c00132fc>] (show_stack+0x18/0x1c)
>  r7:c0797cec r6:00000000 r5:c08fa7bc r4:00000000
> [<c00132e4>] (show_stack) from [<c0639334>] (dump_stack+0x98/0xe0)
> [<c063929c>] (dump_stack) from [<c0044928>] (warn_slowpath_common+0x7c/0xb8)
>  r7:c0797cec r6:00000195 r5:00000009 r4:c08b3f30
> [<c00448ac>] (warn_slowpath_common) from [<c004499c>] (warn_slowpath_fmt+0x38/0x40)
>  r8:c0932000 r7:c08b48c0 r6:ffffffff r5:ee5c9fb4 r4:c0797cc0
> [<c0044968>] (warn_slowpath_fmt) from [<c085c440>] (twd_local_timer_of_register+0x68/0x7c)
>  r3:ffffffea r2:c0797cc0
>  r4:c09322c4
> [<c085c3d8>] (twd_local_timer_of_register) from [<c0889d74>] (clocksource_of_init+0x50/0x90)
>  r5:00000001 r4:ee5c9fb4
> [<c0889d24>] (clocksource_of_init) from [<c0864224>] (omap4_local_timer_init+0x34/0x68)
>  r5:c0932000 r4:00000000
> [<c08641f0>] (omap4_local_timer_init) from [<c085b7f4>] (time_init+0x24/0x38)
> [<c085b7d0>] (time_init) from [<c0857b84>] (start_kernel+0x248/0x3e4)
> [<c085793c>] (start_kernel) from [<8000807c>] (0x8000807c)
>  r10:00000000 r9:412fc09a r8:80004059 r7:c08b8884 r6:c089934c r5:c08b4970
>  r4:c0932214
> ---[ end trace cb88537fdc8fa200 ]---
> 
> This happens, most probably, because TWD IRQs definitions in DT
> do not corresponds HW and gic_configure_irq() will return -EINVAL
> example (am4372.dtsi):
> 	local_timer: timer@48240600 {
> 		compatible = "arm,cortex-a9-twd-timer";
> 		reg = <0x48240600 0x100>;
> 		interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_HIGH>;
> 		interrupt-parent = <&gic>;
> 		clocks = <&mpu_periphclk>;
> 		status = "disabled";
> 	};


Hmm ... that's interesting. Looking at the GIC documentation, it does
say that it is "implementation defined" whether you can program the type
bit for a PPI. Therefore, I am wondering if we should convert the error
into a WARN instead? It would be hard to know which of the below would
be impacted from just looking at the DT files.

> So, some additional fixes might be required. Potentially problematic files are:
> arch/arm/boot/dts/bcm5301x.dtsi
> arch/arm/boot/dts/bcm63138.dtsi
> arch/arm/boot/dts/berlin2cd.dtsi
> arch/arm/boot/dts/berlin2.dtsi
> arch/arm/boot/dts/berlin2q.dtsi
> arch/arm/boot/dts/hip01.dtsi
> arch/arm/boot/dts/omap4.dtsi
> arch/arm/boot/dts/r8a7779.dts
> arch/arm/boot/dts/rk3xxx.dtsi
> arch/arm/boot/dts/sh73a0.dtsi
> arch/arm/boot/dts/socfpga_arria10.dtsi
> arch/arm/boot/dts/socfpga.dtsi
> arch/arm/boot/dts/spear13xx.dtsi
> arch/arm/boot/dts/ste-dbx5x0.dtsi
> arch/arm/boot/dts/tegra20.dtsi
> arch/arm/boot/dts/tegra30.dtsi

So far I have not seen any issues on tegra.

> arch/arm/boot/dts/uniphier-ph1-ld4.dtsi
> arch/arm/boot/dts/uniphier-ph1-XXXX.dtsi
> arch/arm/boot/dts/uniphier-proxstream2.dtsi
> arch/arm/boot/dts/vexpress-v2p-ca5s.dts
> arch/arm/boot/dts/vexpress-v2p-ca9.dts
> 
> 
> Any way, It seems working for me, but I've back-ported and tested
> it on kernel 4.1 and will try to do more tests this week.
> 
> Thanks a lot.

Great. No problem.

Jon

Re: [RFC PATCH V2 2/8] irqdomain: Don't set type when mapping an IRQ

[Geert Uytterhoeven]

On Tue, Dec 22, 2015 at 12:56 PM, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> On 22/12/15 11:18, Grygorii Strashko wrote:
>> On 12/17/2015 12:48 PM, Jon Hunter wrote:
>>> Some IRQ chips, such as GPIO controllers or secondary level interrupt
>>> controllers, may require require additional runtime power management
>>> control to ensure they are accessible. For such IRQ chips, it makes sense
>>> to enable the IRQ chip when interrupts are requested and disabled them
>>> again once all interrupts have been freed.
>>>
>>> When mapping an IRQ, the IRQ type settings are read and then programmed.
>>> The mapping of the IRQ happens before the IRQ is requested and so the
>>> programming of the type settings occurs before the IRQ is requested. This
>>> is a problem for IRQ chips that require additional power management
>>> control because they may not be accessible yet. Therefore, when mapping
>>> the IRQ, don't program the type settings, just save them and then program
>>> these saved settings when the IRQ is requested (so long as if they are not
>>> overridden via the call to request the IRQ).
>>>
>>> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

>> This patch has side effect - some boards which have
>> ARM TWD timer will produce below backtrace on boot:
>>
>>  genirq: Setting trigger mode 4 for irq 17 failed (gic_set_type+0x0/0x58)
>>  twd: can't register interrupt 17 (-22)
>> ------------[ cut here ]------------
>> WARNING: CPU: 0 PID: 0 at arch/arm/kernel/smp_twd.c:405 twd_local_timer_of_register+0x68/0x7c()

>> This happens, most probably, because TWD IRQs definitions in DT
>> do not corresponds HW and gic_configure_irq() will return -EINVAL
>> example (am4372.dtsi):
>>       local_timer: timer@48240600 {
>>               compatible = "arm,cortex-a9-twd-timer";
>>               reg = <0x48240600 0x100>;
>>               interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_HIGH>;
>>               interrupt-parent = <&gic>;
>>               clocks = <&mpu_periphclk>;
>>               status = "disabled";
>>       };
>
>
> Hmm ... that's interesting. Looking at the GIC documentation, it does
> say that it is "implementation defined" whether you can program the type
> bit for a PPI. Therefore, I am wondering if we should convert the error
> into a WARN instead? It would be hard to know which of the below would
> be impacted from just looking at the DT files.
>
>> So, some additional fixes might be required. Potentially problematic files are:

>> arch/arm/boot/dts/r8a7779.dts
>> arch/arm/boot/dts/sh73a0.dtsi

The above two are affected. Sending patches...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds