Hi Thomas,
Any comment on that one?
Thanks,
M.
On 17/01/17 16:00, Marc Zyngier wrote:
> Since commit f3b0946d629c ("genirq/msi: Make sure PCI MSIs are
> activated early"), we can end-up activating a PCI/MSI twice (once
> at allocation time, and once at startup time).
>
> This is normally of no consequences, except that there is some
> HW out there that may misbehave if activate is used more than once
> (the GICv3 ITS, for example, uses the activate callback
> to issue the MAPVI command, and the architecture spec says that
> "If there is an existing mapping for the EventID-DeviceID
> combination, behavior is UNPREDICTABLE").
>
> While this could be worked around in each individual driver, it may
> make more sense to tackle the issue at the core level. In order to
> avoid getting in that situation, let's have a per-interrupt flag
> to remember if we have already activated that interrupt or not.
>
> Fixes: f3b0946d629c ("genirq/msi: Make sure PCI MSIs are activated early")
> Reported-by: Andre Przywara <[email protected]>
> Tested-by: Andre Przywara <[email protected]>
> Signed-off-by: Marc Zyngier <[email protected]>
> ---
> include/linux/irq.h | 17 +++++++++++++++++
> kernel/irq/irqdomain.c | 44 ++++++++++++++++++++++++++++++--------------
> 2 files changed, 47 insertions(+), 14 deletions(-)
>
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index e798755..39e3254 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -184,6 +184,7 @@ struct irq_data {
> *
> * IRQD_TRIGGER_MASK - Mask for the trigger type bits
> * IRQD_SETAFFINITY_PENDING - Affinity setting is pending
> + * IRQD_ACTIVATED - Interrupt has already been activated
> * IRQD_NO_BALANCING - Balancing disabled for this IRQ
> * IRQD_PER_CPU - Interrupt is per cpu
> * IRQD_AFFINITY_SET - Interrupt affinity was set
> @@ -202,6 +203,7 @@ struct irq_data {
> enum {
> IRQD_TRIGGER_MASK = 0xf,
> IRQD_SETAFFINITY_PENDING = (1 << 8),
> + IRQD_ACTIVATED = (1 << 9),
> IRQD_NO_BALANCING = (1 << 10),
> IRQD_PER_CPU = (1 << 11),
> IRQD_AFFINITY_SET = (1 << 12),
> @@ -312,6 +314,21 @@ static inline bool irqd_affinity_is_managed(struct
> irq_data *d)
> return __irqd_to_state(d) & IRQD_AFFINITY_MANAGED;
> }
>
> +static inline bool irqd_is_activated(struct irq_data *d)
> +{
> + return __irqd_to_state(d) & IRQD_ACTIVATED;
> +}
> +
> +static inline void irqd_set_activated(struct irq_data *d)
> +{
> + __irqd_to_state(d) |= IRQD_ACTIVATED;
> +}
> +
> +static inline void irqd_clr_activated(struct irq_data *d)
> +{
> + __irqd_to_state(d) &= ~IRQD_ACTIVATED;
> +}
> +
> #undef __irqd_to_state
>
> static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 8c0a0ae..b59e676 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -1346,6 +1346,30 @@ void irq_domain_free_irqs_parent(struct irq_domain
> *domain,
> }
> EXPORT_SYMBOL_GPL(irq_domain_free_irqs_parent);
>
> +static void __irq_domain_activate_irq(struct irq_data *irq_data)
> +{
> + if (irq_data && irq_data->domain) {
> + struct irq_domain *domain = irq_data->domain;
> +
> + if (irq_data->parent_data)
> + __irq_domain_activate_irq(irq_data->parent_data);
> + if (domain->ops->activate)
> + domain->ops->activate(domain, irq_data);
> + }
> +}
> +
> +static void __irq_domain_deactivate_irq(struct irq_data *irq_data)
> +{
> + if (irq_data && irq_data->domain) {
> + struct irq_domain *domain = irq_data->domain;
> +
> + if (domain->ops->deactivate)
> + domain->ops->deactivate(domain, irq_data);
> + if (irq_data->parent_data)
> + __irq_domain_deactivate_irq(irq_data->parent_data);
> + }
> +}
> +
> /**
> * irq_domain_activate_irq - Call domain_ops->activate recursively to
> activate
> * interrupt
> @@ -1356,13 +1380,9 @@ EXPORT_SYMBOL_GPL(irq_domain_free_irqs_parent);
> */
> void irq_domain_activate_irq(struct irq_data *irq_data)
> {
> - if (irq_data && irq_data->domain) {
> - struct irq_domain *domain = irq_data->domain;
> -
> - if (irq_data->parent_data)
> - irq_domain_activate_irq(irq_data->parent_data);
> - if (domain->ops->activate)
> - domain->ops->activate(domain, irq_data);
> + if (!irqd_is_activated(irq_data)) {
> + __irq_domain_activate_irq(irq_data);
> + irqd_set_activated(irq_data);
> }
> }
>
> @@ -1376,13 +1396,9 @@ void irq_domain_activate_irq(struct irq_data *irq_data)
> */
> void irq_domain_deactivate_irq(struct irq_data *irq_data)
> {
> - if (irq_data && irq_data->domain) {
> - struct irq_domain *domain = irq_data->domain;
> -
> - if (domain->ops->deactivate)
> - domain->ops->deactivate(domain, irq_data);
> - if (irq_data->parent_data)
> - irq_domain_deactivate_irq(irq_data->parent_data);
> + if (irqd_is_activated(irq_data)) {
> + __irq_domain_deactivate_irq(irq_data);
> + irqd_clr_activated(irq_data);
> }
> }
>
>
--
Jazz is not dead. It just smells funny...
