Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1460259 > unrolled thread
| Started by | Marc Zyngier <marc.zyngier@arm.com> |
|---|---|
| First post | 2016-08-11 10:40 +0200 |
| Last post | 2016-08-11 14:10 +0200 |
| Articles | 13 — 5 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [Regression] "irqdomain: Don't set type when mapping an IRQ" breaks nexus7 gpio buttons Marc Zyngier <marc.zyngier@arm.com> - 2016-08-11 10:40 +0200
Re: [Regression] "irqdomain: Don't set type when mapping an IRQ" breaks nexus7 gpio buttons Jon Hunter <jonathanh@nvidia.com> - 2016-08-11 11:50 +0200
Re: [Regression] "irqdomain: Don't set type when mapping an IRQ" breaks nexus7 gpio buttons Marc Zyngier <marc.zyngier@arm.com> - 2016-08-11 14:00 +0200
Re: [Regression] "irqdomain: Don't set type when mapping an IRQ" breaks nexus7 gpio buttons Marc Zyngier <marc.zyngier@arm.com> - 2016-08-11 14:00 +0200
Re: [Regression] "irqdomain: Don't set type when mapping an IRQ" breaks nexus7 gpio buttons Marc Zyngier <marc.zyngier@arm.com> - 2016-08-11 14:50 +0200
Re: [Regression] "irqdomain: Don't set type when mapping an IRQ" breaks nexus7 gpio buttons Jon Hunter <jonathanh@nvidia.com> - 2016-08-11 15:30 +0200
Re: [Regression] "irqdomain: Don't set type when mapping an IRQ" breaks nexus7 gpio buttons Marc Zyngier <marc.zyngier@arm.com> - 2016-08-11 15:40 +0200
Re: [Regression] "irqdomain: Don't set type when mapping an IRQ" breaks nexus7 gpio buttons John Stultz <john.stultz@linaro.org> - 2016-08-11 17:40 +0200
Re: [Regression] "irqdomain: Don't set type when mapping an IRQ" breaks nexus7 gpio buttons Marc Zyngier <marc.zyngier@arm.com> - 2016-08-11 18:00 +0200
Re: [Regression] "irqdomain: Don't set type when mapping an IRQ" breaks nexus7 gpio buttons Linus Walleij <linus.walleij@linaro.org> - 2016-08-11 23:10 +0200
Re: [Regression] "irqdomain: Don't set type when mapping an IRQ" breaks nexus7 gpio buttons Bjorn Andersson <bjorn.andersson@linaro.org> - 2016-08-11 23:30 +0200
Re: [Regression] "irqdomain: Don't set type when mapping an IRQ" breaks nexus7 gpio buttons Marc Zyngier <marc.zyngier@arm.com> - 2016-08-12 12:30 +0200
Re: [Regression] "irqdomain: Don't set type when mapping an IRQ" breaks nexus7 gpio buttons Linus Walleij <linus.walleij@linaro.org> - 2016-08-11 14:10 +0200
| From | Marc Zyngier <marc.zyngier@arm.com> |
|---|---|
| Date | 2016-08-11 10:40 +0200 |
| Subject | Re: [Regression] "irqdomain: Don't set type when mapping an IRQ" breaks nexus7 gpio buttons |
| Message-ID | <s4TGW-1oz-19@gated-at.bofh.it> |
On 08/08/16 22:48, Linus Walleij wrote:
> On Sat, Aug 6, 2016 at 1:45 AM, John Stultz <john.stultz@linaro.org> wrote:
>
>> @@ -614,7 +615,11 @@ unsigned int irq_create_fwspec_mapping(struct
>> irq_fwspec *fwspec)
>> * it now and return the interrupt number.
>> */
>> if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
>> - irq_set_irq_type(virq, type);
>> + irq_data = irq_get_irq_data(virq);
>> + if (!irq_data)
>> + return 0;
>> +
>> + irqd_set_trigger_type(irq_data, type);
>> return virq;
>> }
>>
>> If I revert just that, it works again.
>
> This makes my platform work too.
> Tested-by: Linus Walleij <linus.walleij@linaro.org>
Hmmm. I'm now booting your kernel on the APQ8060, and reverting this
hunk doesn't fix it for me. I'm confused...
The interesting part is this:
109: 100000 0 msmgpio 88 Level (null)
which shows that the cascade interrupt has been disabled after 100000
unhandled interrupts. Somehow, this screams "misconfiguration"...
Thanks,
M.
--
Jazz is not dead. It just smells funny...
[toc] | [next] | [standalone]
| From | Jon Hunter <jonathanh@nvidia.com> |
|---|---|
| Date | 2016-08-11 11:50 +0200 |
| Message-ID | <s4UMG-24i-21@gated-at.bofh.it> |
| In reply to | #1460259 |
On 11/08/16 09:37, Marc Zyngier wrote:
> On 08/08/16 22:48, Linus Walleij wrote:
>> On Sat, Aug 6, 2016 at 1:45 AM, John Stultz <john.stultz@linaro.org> wrote:
>>
>>> @@ -614,7 +615,11 @@ unsigned int irq_create_fwspec_mapping(struct
>>> irq_fwspec *fwspec)
>>> * it now and return the interrupt number.
>>> */
>>> if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
>>> - irq_set_irq_type(virq, type);
>>> + irq_data = irq_get_irq_data(virq);
>>> + if (!irq_data)
>>> + return 0;
>>> +
>>> + irqd_set_trigger_type(irq_data, type);
>>> return virq;
>>> }
>>>
>>> If I revert just that, it works again.
>>
>> This makes my platform work too.
>> Tested-by: Linus Walleij <linus.walleij@linaro.org>
>
> Hmmm. I'm now booting your kernel on the APQ8060, and reverting this
> hunk doesn't fix it for me. I'm confused...
>
> The interesting part is this:
> 109: 100000 0 msmgpio 88 Level (null)
88 is the pm8058 parent interrupt and so I am surprised you would even
see this in /proc/interrupts as it should be a chained interrupt, right?
Are you seeing this with all the ethernet updates for the APQ8060 in
Linus' branch? I am curious what you see with stock v4.8-rc1 and if
interrupts work ok with the change I had proposed. Hard to tell if there
is more than one issue here.
Cheers
Jon
--
nvpublic
[toc] | [prev] | [next] | [standalone]
| From | Marc Zyngier <marc.zyngier@arm.com> |
|---|---|
| Date | 2016-08-11 14:00 +0200 |
| Message-ID | <s4WOt-3kP-11@gated-at.bofh.it> |
| In reply to | #1460342 |
On 11/08/16 12:45, Marc Zyngier wrote:
> On 11/08/16 10:47, Jon Hunter wrote:
>>
>> On 11/08/16 09:37, Marc Zyngier wrote:
>>> On 08/08/16 22:48, Linus Walleij wrote:
>>>> On Sat, Aug 6, 2016 at 1:45 AM, John Stultz <john.stultz@linaro.org> wrote:
>>>>
>>>>> @@ -614,7 +615,11 @@ unsigned int irq_create_fwspec_mapping(struct
>>>>> irq_fwspec *fwspec)
>>>>> * it now and return the interrupt number.
>>>>> */
>>>>> if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
>>>>> - irq_set_irq_type(virq, type);
>>>>> + irq_data = irq_get_irq_data(virq);
>>>>> + if (!irq_data)
>>>>> + return 0;
>>>>> +
>>>>> + irqd_set_trigger_type(irq_data, type);
>>>>> return virq;
>>>>> }
>>>>>
>>>>> If I revert just that, it works again.
>>>>
>>>> This makes my platform work too.
>>>> Tested-by: Linus Walleij <linus.walleij@linaro.org>
>>>
>>> Hmmm. I'm now booting your kernel on the APQ8060, and reverting this
>>> hunk doesn't fix it for me. I'm confused...
>>>
>>> The interesting part is this:
>>> 109: 100000 0 msmgpio 88 Level (null)
>>
>> 88 is the pm8058 parent interrupt and so I am surprised you would even
>> see this in /proc/interrupts as it should be a chained interrupt, right?
>
> That's because it repeatedly fires without a proper handler, and only
> appears then.
>
>> Are you seeing this with all the ethernet updates for the APQ8060 in
>> Linus' branch? I am curious what you see with stock v4.8-rc1 and if
>> interrupts work ok with the change I had proposed. Hard to tell if there
>> is more than one issue here.
>
> (mostly) stock v4.8-rc1 exhibits the same issue, and your fix doesn't
> help this particular issue. Reverting your fix *and* applying the above
> revert makes it work again. Which is just papering over the issue, as it
> only does something when the interrupt is seen for a second time.
Also: this behaviour only affects the second cascade (from the pm8058 to
the tlmm). I can perfectly configure the first one (from the tlmm to the
GIC) with your fix.
M.
--
Jazz is not dead. It just smells funny...
[toc] | [prev] | [next] | [standalone]
| From | Marc Zyngier <marc.zyngier@arm.com> |
|---|---|
| Date | 2016-08-11 14:00 +0200 |
| Message-ID | <s4WOt-3kP-13@gated-at.bofh.it> |
| In reply to | #1460342 |
On 11/08/16 10:47, Jon Hunter wrote:
>
> On 11/08/16 09:37, Marc Zyngier wrote:
>> On 08/08/16 22:48, Linus Walleij wrote:
>>> On Sat, Aug 6, 2016 at 1:45 AM, John Stultz <john.stultz@linaro.org> wrote:
>>>
>>>> @@ -614,7 +615,11 @@ unsigned int irq_create_fwspec_mapping(struct
>>>> irq_fwspec *fwspec)
>>>> * it now and return the interrupt number.
>>>> */
>>>> if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
>>>> - irq_set_irq_type(virq, type);
>>>> + irq_data = irq_get_irq_data(virq);
>>>> + if (!irq_data)
>>>> + return 0;
>>>> +
>>>> + irqd_set_trigger_type(irq_data, type);
>>>> return virq;
>>>> }
>>>>
>>>> If I revert just that, it works again.
>>>
>>> This makes my platform work too.
>>> Tested-by: Linus Walleij <linus.walleij@linaro.org>
>>
>> Hmmm. I'm now booting your kernel on the APQ8060, and reverting this
>> hunk doesn't fix it for me. I'm confused...
>>
>> The interesting part is this:
>> 109: 100000 0 msmgpio 88 Level (null)
>
> 88 is the pm8058 parent interrupt and so I am surprised you would even
> see this in /proc/interrupts as it should be a chained interrupt, right?
That's because it repeatedly fires without a proper handler, and only
appears then.
> Are you seeing this with all the ethernet updates for the APQ8060 in
> Linus' branch? I am curious what you see with stock v4.8-rc1 and if
> interrupts work ok with the change I had proposed. Hard to tell if there
> is more than one issue here.
(mostly) stock v4.8-rc1 exhibits the same issue, and your fix doesn't
help this particular issue. Reverting your fix *and* applying the above
revert makes it work again. Which is just papering over the issue, as it
only does something when the interrupt is seen for a second time.
I must be missing something obvious...
M.
--
Jazz is not dead. It just smells funny...
[toc] | [prev] | [next] | [standalone]
| From | Marc Zyngier <marc.zyngier@arm.com> |
|---|---|
| Date | 2016-08-11 14:50 +0200 |
| Message-ID | <s4XAS-3T5-7@gated-at.bofh.it> |
| In reply to | #1460342 |
On 11/08/16 10:47, Jon Hunter wrote:
>
> On 11/08/16 09:37, Marc Zyngier wrote:
>> On 08/08/16 22:48, Linus Walleij wrote:
>>> On Sat, Aug 6, 2016 at 1:45 AM, John Stultz <john.stultz@linaro.org> wrote:
>>>
>>>> @@ -614,7 +615,11 @@ unsigned int irq_create_fwspec_mapping(struct
>>>> irq_fwspec *fwspec)
>>>> * it now and return the interrupt number.
>>>> */
>>>> if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
>>>> - irq_set_irq_type(virq, type);
>>>> + irq_data = irq_get_irq_data(virq);
>>>> + if (!irq_data)
>>>> + return 0;
>>>> +
>>>> + irqd_set_trigger_type(irq_data, type);
>>>> return virq;
>>>> }
>>>>
>>>> If I revert just that, it works again.
>>>
>>> This makes my platform work too.
>>> Tested-by: Linus Walleij <linus.walleij@linaro.org>
>>
>> Hmmm. I'm now booting your kernel on the APQ8060, and reverting this
>> hunk doesn't fix it for me. I'm confused...
>>
>> The interesting part is this:
>> 109: 100000 0 msmgpio 88 Level (null)
>
> 88 is the pm8058 parent interrupt and so I am surprised you would even
> see this in /proc/interrupts as it should be a chained interrupt, right?
>
> Are you seeing this with all the ethernet updates for the APQ8060 in
> Linus' branch? I am curious what you see with stock v4.8-rc1 and if
> interrupts work ok with the change I had proposed. Hard to tell if there
> is more than one issue here.
Nailed the sucker:
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index b4c1bc7..9d7284a 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -820,6 +820,18 @@ __irq_do_set_handler(struct irq_desc *desc, irq_flow_handler_t handle,
desc->name = name;
if (handle != handle_bad_irq && is_chained) {
+ int ret;
+
+ ret = __irq_set_trigger(desc,
+ irqd_get_trigger_type(&desc->irq_data));
+ WARN_ON(ret);
+ /*
+ * This is beyond ugly: .set_type may have overridden
+ * the flow, not not knowing that we're dealing with a
+ * chained handler. Reset it here because we know
+ * better.
+ */
+ desc->handle_irq = handle;
irq_settings_set_noprobe(desc);
irq_settings_set_norequest(desc);
irq_settings_set_nothread(desc);
Linus, Jon: Can you please confirm this fixes your respective issues?
Thanks,
M.
--
Jazz is not dead. It just smells funny...
[toc] | [prev] | [next] | [standalone]
| From | Jon Hunter <jonathanh@nvidia.com> |
|---|---|
| Date | 2016-08-11 15:30 +0200 |
| Message-ID | <s4Ydz-4ms-1@gated-at.bofh.it> |
| In reply to | #1460478 |
On 11/08/16 13:46, Marc Zyngier wrote:
> On 11/08/16 10:47, Jon Hunter wrote:
>>
>> On 11/08/16 09:37, Marc Zyngier wrote:
>>> On 08/08/16 22:48, Linus Walleij wrote:
>>>> On Sat, Aug 6, 2016 at 1:45 AM, John Stultz <john.stultz@linaro.org> wrote:
>>>>
>>>>> @@ -614,7 +615,11 @@ unsigned int irq_create_fwspec_mapping(struct
>>>>> irq_fwspec *fwspec)
>>>>> * it now and return the interrupt number.
>>>>> */
>>>>> if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
>>>>> - irq_set_irq_type(virq, type);
>>>>> + irq_data = irq_get_irq_data(virq);
>>>>> + if (!irq_data)
>>>>> + return 0;
>>>>> +
>>>>> + irqd_set_trigger_type(irq_data, type);
>>>>> return virq;
>>>>> }
>>>>>
>>>>> If I revert just that, it works again.
>>>>
>>>> This makes my platform work too.
>>>> Tested-by: Linus Walleij <linus.walleij@linaro.org>
>>>
>>> Hmmm. I'm now booting your kernel on the APQ8060, and reverting this
>>> hunk doesn't fix it for me. I'm confused...
>>>
>>> The interesting part is this:
>>> 109: 100000 0 msmgpio 88 Level (null)
>>
>> 88 is the pm8058 parent interrupt and so I am surprised you would even
>> see this in /proc/interrupts as it should be a chained interrupt, right?
>>
>> Are you seeing this with all the ethernet updates for the APQ8060 in
>> Linus' branch? I am curious what you see with stock v4.8-rc1 and if
>> interrupts work ok with the change I had proposed. Hard to tell if there
>> is more than one issue here.
>
> Nailed the sucker:
Great!
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index b4c1bc7..9d7284a 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -820,6 +820,18 @@ __irq_do_set_handler(struct irq_desc *desc, irq_flow_handler_t handle,
> desc->name = name;
>
> if (handle != handle_bad_irq && is_chained) {
> + int ret;
> +
> + ret = __irq_set_trigger(desc,
> + irqd_get_trigger_type(&desc->irq_data));
> + WARN_ON(ret);
You could wrap the entire call in the WARN_ON(). I was not sure if there
was a better way to handle that.
> + /*
> + * This is beyond ugly: .set_type may have overridden
> + * the flow, not not knowing that we're dealing with a
> + * chained handler. Reset it here because we know
> + * better.
> + */
> + desc->handle_irq = handle;
Yes I see the call to irq_set_handler in the pinctrl-msm.c set_type.
Good catch!
Apart from the above ...
Acked-by: Jon Hunter <jonathanh@nvidia.com>
Cheers
Jon
--
nvpublic
[toc] | [prev] | [next] | [standalone]
| From | Marc Zyngier <marc.zyngier@arm.com> |
|---|---|
| Date | 2016-08-11 15:40 +0200 |
| Message-ID | <s4Ynf-4pW-7@gated-at.bofh.it> |
| In reply to | #1460513 |
On 11/08/16 14:29, Jon Hunter wrote:
>
> On 11/08/16 13:46, Marc Zyngier wrote:
>> On 11/08/16 10:47, Jon Hunter wrote:
>>>
>>> On 11/08/16 09:37, Marc Zyngier wrote:
>>>> On 08/08/16 22:48, Linus Walleij wrote:
>>>>> On Sat, Aug 6, 2016 at 1:45 AM, John Stultz <john.stultz@linaro.org> wrote:
>>>>>
>>>>>> @@ -614,7 +615,11 @@ unsigned int irq_create_fwspec_mapping(struct
>>>>>> irq_fwspec *fwspec)
>>>>>> * it now and return the interrupt number.
>>>>>> */
>>>>>> if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
>>>>>> - irq_set_irq_type(virq, type);
>>>>>> + irq_data = irq_get_irq_data(virq);
>>>>>> + if (!irq_data)
>>>>>> + return 0;
>>>>>> +
>>>>>> + irqd_set_trigger_type(irq_data, type);
>>>>>> return virq;
>>>>>> }
>>>>>>
>>>>>> If I revert just that, it works again.
>>>>>
>>>>> This makes my platform work too.
>>>>> Tested-by: Linus Walleij <linus.walleij@linaro.org>
>>>>
>>>> Hmmm. I'm now booting your kernel on the APQ8060, and reverting this
>>>> hunk doesn't fix it for me. I'm confused...
>>>>
>>>> The interesting part is this:
>>>> 109: 100000 0 msmgpio 88 Level (null)
>>>
>>> 88 is the pm8058 parent interrupt and so I am surprised you would even
>>> see this in /proc/interrupts as it should be a chained interrupt, right?
>>>
>>> Are you seeing this with all the ethernet updates for the APQ8060 in
>>> Linus' branch? I am curious what you see with stock v4.8-rc1 and if
>>> interrupts work ok with the change I had proposed. Hard to tell if there
>>> is more than one issue here.
>>
>> Nailed the sucker:
>
> Great!
>
>> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
>> index b4c1bc7..9d7284a 100644
>> --- a/kernel/irq/chip.c
>> +++ b/kernel/irq/chip.c
>> @@ -820,6 +820,18 @@ __irq_do_set_handler(struct irq_desc *desc, irq_flow_handler_t handle,
>> desc->name = name;
>>
>> if (handle != handle_bad_irq && is_chained) {
>> + int ret;
>> +
>> + ret = __irq_set_trigger(desc,
>> + irqd_get_trigger_type(&desc->irq_data));
>> + WARN_ON(ret);
>
> You could wrap the entire call in the WARN_ON(). I was not sure if there
> was a better way to handle that.
Actually, I've decided to drop it. We already have a message in
__irq_set_trigger(), and if we really want to scream, that's the one we
should consider upgrading to a WARN_ON().
>
>> + /*
>> + * This is beyond ugly: .set_type may have overridden
>> + * the flow, not not knowing that we're dealing with a
>> + * chained handler. Reset it here because we know
>> + * better.
>> + */
>> + desc->handle_irq = handle;
>
> Yes I see the call to irq_set_handler in the pinctrl-msm.c set_type.
> Good catch!
I can't believe it took me that much time to realize that. Guess I need
an extended weekend! ;-)
>
> Apart from the above ...
>
> Acked-by: Jon Hunter <jonathanh@nvidia.com>
Thanks a lot Jon,
M.
--
Jazz is not dead. It just smells funny...
[toc] | [prev] | [next] | [standalone]
| From | John Stultz <john.stultz@linaro.org> |
|---|---|
| Date | 2016-08-11 17:40 +0200 |
| Message-ID | <s50fo-5Dk-11@gated-at.bofh.it> |
| In reply to | #1460478 |
On Thu, Aug 11, 2016 at 5:46 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 11/08/16 10:47, Jon Hunter wrote:
>>
>> On 11/08/16 09:37, Marc Zyngier wrote:
>>> On 08/08/16 22:48, Linus Walleij wrote:
>>>> On Sat, Aug 6, 2016 at 1:45 AM, John Stultz <john.stultz@linaro.org> wrote:
>>>>
>>>>> @@ -614,7 +615,11 @@ unsigned int irq_create_fwspec_mapping(struct
>>>>> irq_fwspec *fwspec)
>>>>> * it now and return the interrupt number.
>>>>> */
>>>>> if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
>>>>> - irq_set_irq_type(virq, type);
>>>>> + irq_data = irq_get_irq_data(virq);
>>>>> + if (!irq_data)
>>>>> + return 0;
>>>>> +
>>>>> + irqd_set_trigger_type(irq_data, type);
>>>>> return virq;
>>>>> }
>>>>>
>>>>> If I revert just that, it works again.
>>>>
>>>> This makes my platform work too.
>>>> Tested-by: Linus Walleij <linus.walleij@linaro.org>
>>>
>>> Hmmm. I'm now booting your kernel on the APQ8060, and reverting this
>>> hunk doesn't fix it for me. I'm confused...
>>>
>>> The interesting part is this:
>>> 109: 100000 0 msmgpio 88 Level (null)
>>
>> 88 is the pm8058 parent interrupt and so I am surprised you would even
>> see this in /proc/interrupts as it should be a chained interrupt, right?
>>
>> Are you seeing this with all the ethernet updates for the APQ8060 in
>> Linus' branch? I am curious what you see with stock v4.8-rc1 and if
>> interrupts work ok with the change I had proposed. Hard to tell if there
>> is more than one issue here.
>
> Nailed the sucker:
>
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index b4c1bc7..9d7284a 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -820,6 +820,18 @@ __irq_do_set_handler(struct irq_desc *desc, irq_flow_handler_t handle,
> desc->name = name;
>
> if (handle != handle_bad_irq && is_chained) {
> + int ret;
> +
> + ret = __irq_set_trigger(desc,
> + irqd_get_trigger_type(&desc->irq_data));
> + WARN_ON(ret);
> + /*
> + * This is beyond ugly: .set_type may have overridden
> + * the flow, not not knowing that we're dealing with a
> + * chained handler. Reset it here because we know
> + * better.
> + */
> + desc->handle_irq = handle;
> irq_settings_set_noprobe(desc);
> irq_settings_set_norequest(desc);
> irq_settings_set_nothread(desc);
>
> Linus, Jon: Can you please confirm this fixes your respective issues?
Yep. That works for me!
Tested-by: John Stultz <john.stultz@linaro.org>
Thanks so much for hunting this down!
-john
[toc] | [prev] | [next] | [standalone]
| From | Marc Zyngier <marc.zyngier@arm.com> |
|---|---|
| Date | 2016-08-11 18:00 +0200 |
| Message-ID | <s50yK-5KE-29@gated-at.bofh.it> |
| In reply to | #1460616 |
On 11/08/16 16:32, John Stultz wrote:
> On Thu, Aug 11, 2016 at 5:46 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>
>> Nailed the sucker:
>>
>> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
>> index b4c1bc7..9d7284a 100644
>> --- a/kernel/irq/chip.c
>> +++ b/kernel/irq/chip.c
>> @@ -820,6 +820,18 @@ __irq_do_set_handler(struct irq_desc *desc, irq_flow_handler_t handle,
>> desc->name = name;
>>
>> if (handle != handle_bad_irq && is_chained) {
>> + int ret;
>> +
>> + ret = __irq_set_trigger(desc,
>> + irqd_get_trigger_type(&desc->irq_data));
>> + WARN_ON(ret);
>> + /*
>> + * This is beyond ugly: .set_type may have overridden
>> + * the flow, not not knowing that we're dealing with a
>> + * chained handler. Reset it here because we know
>> + * better.
>> + */
>> + desc->handle_irq = handle;
>> irq_settings_set_noprobe(desc);
>> irq_settings_set_norequest(desc);
>> irq_settings_set_nothread(desc);
>>
>> Linus, Jon: Can you please confirm this fixes your respective issues?
>
> Yep. That works for me!
>
> Tested-by: John Stultz <john.stultz@linaro.org>
>
> Thanks so much for hunting this down!
Thanks for the report and the testing! I'll post the final patch
shortly. tglx being away for a couple of weeks, it may take some time
before this hits mainline though.
M.
--
Jazz is not dead. It just smells funny...
[toc] | [prev] | [next] | [standalone]
| From | Linus Walleij <linus.walleij@linaro.org> |
|---|---|
| Date | 2016-08-11 23:10 +0200 |
| Message-ID | <s55oJ-JH-1@gated-at.bofh.it> |
| In reply to | #1460478 |
On Thu, Aug 11, 2016 at 2:46 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> Nailed the sucker:
>
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index b4c1bc7..9d7284a 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -820,6 +820,18 @@ __irq_do_set_handler(struct irq_desc *desc, irq_flow_handler_t handle,
> desc->name = name;
>
> if (handle != handle_bad_irq && is_chained) {
> + int ret;
> +
> + ret = __irq_set_trigger(desc,
> + irqd_get_trigger_type(&desc->irq_data));
> + WARN_ON(ret);
> + /*
> + * This is beyond ugly: .set_type may have overridden
> + * the flow, not not knowing that we're dealing with a
> + * chained handler. Reset it here because we know
> + * better.
> + */
> + desc->handle_irq = handle;
> irq_settings_set_noprobe(desc);
> irq_settings_set_norequest(desc);
> irq_settings_set_nothread(desc);
>
> Linus, Jon: Can you please confirm this fixes your respective issues?
Yes, bullseye.
Tested-by: Linus Walleij <linus.walleij@linaro.org>
Thanks for your efforts!
Yours,
Linus Walleij
[toc] | [prev] | [next] | [standalone]
| From | Bjorn Andersson <bjorn.andersson@linaro.org> |
|---|---|
| Date | 2016-08-11 23:30 +0200 |
| Message-ID | <s55I6-S7-3@gated-at.bofh.it> |
| In reply to | #1460478 |
On Thu 11 Aug 05:46 PDT 2016, Marc Zyngier wrote:
> On 11/08/16 10:47, Jon Hunter wrote:
> >
> > On 11/08/16 09:37, Marc Zyngier wrote:
> >> On 08/08/16 22:48, Linus Walleij wrote:
> >>> On Sat, Aug 6, 2016 at 1:45 AM, John Stultz <john.stultz@linaro.org> wrote:
> >>>
> >>>> @@ -614,7 +615,11 @@ unsigned int irq_create_fwspec_mapping(struct
> >>>> irq_fwspec *fwspec)
> >>>> * it now and return the interrupt number.
> >>>> */
> >>>> if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
> >>>> - irq_set_irq_type(virq, type);
> >>>> + irq_data = irq_get_irq_data(virq);
> >>>> + if (!irq_data)
> >>>> + return 0;
> >>>> +
> >>>> + irqd_set_trigger_type(irq_data, type);
> >>>> return virq;
> >>>> }
> >>>>
> >>>> If I revert just that, it works again.
> >>>
> >>> This makes my platform work too.
> >>> Tested-by: Linus Walleij <linus.walleij@linaro.org>
> >>
> >> Hmmm. I'm now booting your kernel on the APQ8060, and reverting this
> >> hunk doesn't fix it for me. I'm confused...
> >>
> >> The interesting part is this:
> >> 109: 100000 0 msmgpio 88 Level (null)
> >
> > 88 is the pm8058 parent interrupt and so I am surprised you would even
> > see this in /proc/interrupts as it should be a chained interrupt, right?
> >
> > Are you seeing this with all the ethernet updates for the APQ8060 in
> > Linus' branch? I am curious what you see with stock v4.8-rc1 and if
> > interrupts work ok with the change I had proposed. Hard to tell if there
> > is more than one issue here.
>
> Nailed the sucker:
>
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index b4c1bc7..9d7284a 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -820,6 +820,18 @@ __irq_do_set_handler(struct irq_desc *desc, irq_flow_handler_t handle,
> desc->name = name;
>
> if (handle != handle_bad_irq && is_chained) {
> + int ret;
> +
> + ret = __irq_set_trigger(desc,
> + irqd_get_trigger_type(&desc->irq_data));
> + WARN_ON(ret);
> + /*
> + * This is beyond ugly: .set_type may have overridden
> + * the flow, not not knowing that we're dealing with a
> + * chained handler. Reset it here because we know
> + * better.
> + */
Thanks for this Marc!
But it makes me (author of pinctrl-msm) wonder, am I supposed to not
implement .set_type like this for handling the transition between edge
and level handlers?
Regards,
Bjorn
[toc] | [prev] | [next] | [standalone]
| From | Marc Zyngier <marc.zyngier@arm.com> |
|---|---|
| Date | 2016-08-12 12:30 +0200 |
| Message-ID | <s5hSV-p0-3@gated-at.bofh.it> |
| In reply to | #1460806 |
On Thu, 11 Aug 2016 14:23:53 -0700
Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
Hi Bjorn,
> On Thu 11 Aug 05:46 PDT 2016, Marc Zyngier wrote:
>
> > On 11/08/16 10:47, Jon Hunter wrote:
> > >
> > > On 11/08/16 09:37, Marc Zyngier wrote:
> > >> On 08/08/16 22:48, Linus Walleij wrote:
> > >>> On Sat, Aug 6, 2016 at 1:45 AM, John Stultz <john.stultz@linaro.org> wrote:
> > >>>
> > >>>> @@ -614,7 +615,11 @@ unsigned int irq_create_fwspec_mapping(struct
> > >>>> irq_fwspec *fwspec)
> > >>>> * it now and return the interrupt number.
> > >>>> */
> > >>>> if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
> > >>>> - irq_set_irq_type(virq, type);
> > >>>> + irq_data = irq_get_irq_data(virq);
> > >>>> + if (!irq_data)
> > >>>> + return 0;
> > >>>> +
> > >>>> + irqd_set_trigger_type(irq_data, type);
> > >>>> return virq;
> > >>>> }
> > >>>>
> > >>>> If I revert just that, it works again.
> > >>>
> > >>> This makes my platform work too.
> > >>> Tested-by: Linus Walleij <linus.walleij@linaro.org>
> > >>
> > >> Hmmm. I'm now booting your kernel on the APQ8060, and reverting this
> > >> hunk doesn't fix it for me. I'm confused...
> > >>
> > >> The interesting part is this:
> > >> 109: 100000 0 msmgpio 88 Level (null)
> > >
> > > 88 is the pm8058 parent interrupt and so I am surprised you would even
> > > see this in /proc/interrupts as it should be a chained interrupt, right?
> > >
> > > Are you seeing this with all the ethernet updates for the APQ8060 in
> > > Linus' branch? I am curious what you see with stock v4.8-rc1 and if
> > > interrupts work ok with the change I had proposed. Hard to tell if there
> > > is more than one issue here.
> >
> > Nailed the sucker:
> >
> > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> > index b4c1bc7..9d7284a 100644
> > --- a/kernel/irq/chip.c
> > +++ b/kernel/irq/chip.c
> > @@ -820,6 +820,18 @@ __irq_do_set_handler(struct irq_desc *desc, irq_flow_handler_t handle,
> > desc->name = name;
> >
> > if (handle != handle_bad_irq && is_chained) {
> > + int ret;
> > +
> > + ret = __irq_set_trigger(desc,
> > + irqd_get_trigger_type(&desc->irq_data));
> > + WARN_ON(ret);
> > + /*
> > + * This is beyond ugly: .set_type may have overridden
> > + * the flow, not not knowing that we're dealing with a
> > + * chained handler. Reset it here because we know
> > + * better.
> > + */
>
> Thanks for this Marc!
>
> But it makes me (author of pinctrl-msm) wonder, am I supposed to not
> implement .set_type like this for handling the transition between edge
> and level handlers?
You definitely need to implement .set_type and set the flow handler
the way you do it, and there is hardly anything an irqchip driver can do
to detect that case.
The main issue is that as far as the core code is concerned, the
chained handler is just another flow handler. It just happen to be
provided by another irqchip.
We used to call .set_type early, and a driver like yours would set the
flow corresponding to the trigger of the interrupt. Later, the
secondary irqchip would then call irq_set_chained_handler_and_data(),
which would override the flow with the custom one. Now, we call it much
later, after the custom flow handler has been assigned. Kaboom, we
end-up calling the chained_action handler through one of the normal
flows, instead of going into our special flow.
We *could* make irq_set_handler_locked() check for this condition
though (testing for the chained_action pointer), probably at the cost of
un-inlining it.
I'll try to put something together next week, and see what sticks.
Thanks,
M.
--
Jazz is not dead. It just smells funny.
[toc] | [prev] | [next] | [standalone]
| From | Linus Walleij <linus.walleij@linaro.org> |
|---|---|
| Date | 2016-08-11 14:10 +0200 |
| Message-ID | <s4WY9-3Dt-5@gated-at.bofh.it> |
| In reply to | #1460259 |
On Thu, Aug 11, 2016 at 10:37 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 08/08/16 22:48, Linus Walleij wrote:
>> On Sat, Aug 6, 2016 at 1:45 AM, John Stultz <john.stultz@linaro.org> wrote:
>>
>>> @@ -614,7 +615,11 @@ unsigned int irq_create_fwspec_mapping(struct
>>> irq_fwspec *fwspec)
>>> * it now and return the interrupt number.
>>> */
>>> if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
>>> - irq_set_irq_type(virq, type);
>>> + irq_data = irq_get_irq_data(virq);
>>> + if (!irq_data)
>>> + return 0;
>>> +
>>> + irqd_set_trigger_type(irq_data, type);
>>> return virq;
>>> }
>>>
>>> If I revert just that, it works again.
>>
>> This makes my platform work too.
>> Tested-by: Linus Walleij <linus.walleij@linaro.org>
>
> Hmmm. I'm now booting your kernel on the APQ8060, and reverting this
> hunk doesn't fix it for me. I'm confused...
Not quite following ... If you build the branch apq8060-dragonboard
which has these patches:
ARM: dts: MSM8660 remove flags from SPMI/MPP IRQs
Revert "irqdomain: Don't set type when mapping an IRQ
iio: pressure: bmp280: fix runtime suspend/resume crash
DO NOT MERGE: ARM: qcom: uglyfix
(I'm sorry about the two patches in the bottom needed to boot
this platform...)
You should be seeing e.g. the power button IRQs appear if you press
the "power" button close to the DC connector.
If you remove the revert, the interrupts are gone. For me, atleaset,
no reaction to the power button.
The issue is the same with any PMIC IRQ I try to use: sensors,
ethernet, SD card slot ... the SD card slot can also be tested right
off just like the power button. Just insert/eject an SD card in the
primary card slot.
Yours,
Linus Walleij
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web