From: Valentin Schneider <valentin.schneider@arm.com>
To: Marc Zyngier <maz@kernel.org>
Cc: Guenter Roeck <linux@roeck-us.net>,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH] irqchip/gic: Convert to handle_strict_flow_irq()
Date: Tue, 17 Aug 2021 01:30:43 +0100 [thread overview]
Message-ID: <87k0kk7w0c.mognet@arm.com> (raw)
In-Reply-To: <87eeav19mc.wl-maz@kernel.org>
On 15/08/21 07:54, Marc Zyngier wrote:
> This is going and-up in a wack-a-mole game. There is probably a bunch
> of these all over the place. I'd rather squash it at the root,
> i.e. with something like this (untested):
>
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index 099bc7e13d1b..601ad3fc47cd 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -410,7 +410,12 @@ void irq_percpu_disable(struct irq_desc *desc, unsigned int cpu)
>
> void ack_irq(struct irq_desc *desc)
> {
> - desc->irq_data.chip->irq_ack(&desc->irq_data);
> + struct irq_data *data = &desc->irq_data;
> +
> + while (!data->chip->irq_ack)
> + data = data->parent_data;
> +
> + data->chip->irq_ack(&desc->irq_data);
>
> if (desc->irq_data.chip->flags & IRQCHIP_AUTOMASKS_FLOW)
> irq_state_set_flow_masked(desc);
>
> We probably need something similar for irq_eoi().
>
> This however shows a more fundamental problem, I'm afraid. We set
> IRQCHIP_AUTOMASKS_FLOW in the GIC drivers (i.e. at the root), but test
> for it at the top of the hierarchy. As soon as we have more than a
> single layer of irqchip, this will do the wrong thing (or at least
> miss the masking optimisation).
>
Yup.
> This probably advocates for moving the flag into the descriptor. This
> really makes sense, as the flow is global to the whole stack, not just
> to the localised irqchip.
>
Are we guaranteed to have
.irq_ack \in {NULL, irq_chip_ack_parent}
for all intermediate (!root) irqchips? I don't see why that wouldn't be the
case, and with that in mind what you described makes sense to me.
> In order to restore -next into a working state, I'm temporarily
> dropping this series. Hopefully, we can sort this out before the merge
> window and reinstate it.
>
I'm away from any keyboard for most of this week, but I'll get to it by the
weekend.
> Thanks,
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
WARNING: multiple messages have this Message-ID (diff)
From: Valentin Schneider <valentin.schneider@arm.com>
To: Marc Zyngier <maz@kernel.org>
Cc: Guenter Roeck <linux@roeck-us.net>,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH] irqchip/gic: Convert to handle_strict_flow_irq()
Date: Tue, 17 Aug 2021 01:30:43 +0100 [thread overview]
Message-ID: <87k0kk7w0c.mognet@arm.com> (raw)
In-Reply-To: <87eeav19mc.wl-maz@kernel.org>
On 15/08/21 07:54, Marc Zyngier wrote:
> This is going and-up in a wack-a-mole game. There is probably a bunch
> of these all over the place. I'd rather squash it at the root,
> i.e. with something like this (untested):
>
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index 099bc7e13d1b..601ad3fc47cd 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -410,7 +410,12 @@ void irq_percpu_disable(struct irq_desc *desc, unsigned int cpu)
>
> void ack_irq(struct irq_desc *desc)
> {
> - desc->irq_data.chip->irq_ack(&desc->irq_data);
> + struct irq_data *data = &desc->irq_data;
> +
> + while (!data->chip->irq_ack)
> + data = data->parent_data;
> +
> + data->chip->irq_ack(&desc->irq_data);
>
> if (desc->irq_data.chip->flags & IRQCHIP_AUTOMASKS_FLOW)
> irq_state_set_flow_masked(desc);
>
> We probably need something similar for irq_eoi().
>
> This however shows a more fundamental problem, I'm afraid. We set
> IRQCHIP_AUTOMASKS_FLOW in the GIC drivers (i.e. at the root), but test
> for it at the top of the hierarchy. As soon as we have more than a
> single layer of irqchip, this will do the wrong thing (or at least
> miss the masking optimisation).
>
Yup.
> This probably advocates for moving the flag into the descriptor. This
> really makes sense, as the flow is global to the whole stack, not just
> to the localised irqchip.
>
Are we guaranteed to have
.irq_ack \in {NULL, irq_chip_ack_parent}
for all intermediate (!root) irqchips? I don't see why that wouldn't be the
case, and with that in mind what you described makes sense to me.
> In order to restore -next into a working state, I'm temporarily
> dropping this series. Hopefully, we can sort this out before the merge
> window and reinstate it.
>
I'm away from any keyboard for most of this week, but I'll get to it by the
weekend.
> Thanks,
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
next prev parent reply other threads:[~2021-08-17 0:32 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-14 19:47 [PATCH] irqchip/gic: Convert to handle_strict_flow_irq() Guenter Roeck
2021-08-14 22:26 ` Valentin Schneider
2021-08-14 22:26 ` Valentin Schneider
2021-08-14 22:31 ` Valentin Schneider
2021-08-14 22:31 ` Valentin Schneider
2021-08-14 23:41 ` Guenter Roeck
2021-08-14 23:41 ` Guenter Roeck
2021-08-14 23:15 ` Guenter Roeck
2021-08-14 23:15 ` Guenter Roeck
2021-08-14 23:36 ` Guenter Roeck
2021-08-14 23:36 ` Guenter Roeck
2021-08-15 7:01 ` Marc Zyngier
2021-08-15 7:01 ` Marc Zyngier
2021-08-15 6:54 ` Marc Zyngier
2021-08-15 6:54 ` Marc Zyngier
2021-08-17 0:30 ` Valentin Schneider [this message]
2021-08-17 0:30 ` Valentin Schneider
2021-08-18 16:58 ` Marc Zyngier
2021-08-18 16:58 ` Marc Zyngier
2021-08-22 22:16 ` Valentin Schneider
2021-08-22 22:16 ` Valentin Schneider
2021-08-23 9:33 ` Marc Zyngier
2021-08-23 9:33 ` Marc Zyngier
2021-08-23 10:38 ` Valentin Schneider
2021-08-23 10:38 ` Valentin Schneider
2021-08-23 12:17 ` Marc Zyngier
2021-08-23 12:17 ` Marc Zyngier
2021-08-30 16:54 ` Valentin Schneider
2021-08-30 16:54 ` Valentin Schneider
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87k0kk7w0c.mognet@arm.com \
--to=valentin.schneider@arm.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=maz@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.