Re: [PATCH] genirq/msi: Make sure PCI MSIs are activated early

[Bjorn Helgaas]

On Wed, Jul 13, 2016 at 05:18:33PM +0100, Marc Zyngier wrote:
> Bharat Kumar Gogada reported issues with the generic MSI code,
> where the end-point ended up with garbage in its MSI configuration
> (both for the vector and the message).
> 
> It turns out that the two MSI paths in the kernel are doing slightly
> different things:
> 
> generic MSI: disable MSI -> allocate MSI -> enable MSI -> setup EP
> PCI MSI: disable MSI -> allocate MSI -> setup EP -> enable MSI
> 
> and it turns out that end-points are allowed to latch the content
> of the MSI configuration registers as soon as MSIs are enabled.
> In Bharat's case, the end-point ends up using whatever was there
> already, which is not what you want.
> 
> In order to make things converge, we introduce a new MSI domain
> flag (MSI_FLAG_ACTIVATE_EARLY) that is unconditionally set for
> PCI/MSI. When set, this flag forces the programming of the end-point
> as soon as the MSIs are allocated.
> 
> A consequence of this is that we have an extra activate in
> irq_startup, but that should be without much consequence.
> 
> Reported-by: Bharat Kumar Gogada <bharat.kumar.gogada@xxxxxxxxxx>
> Tested-by: Bharat Kumar Gogada <bharat.kumar.gogada@xxxxxxxxxx>
> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>

Acked-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>

Thomas, let me know if you'd like me to take this.  It looks like the
real smarts here are in kernel/irq, so I assume you'll take it unless
I hear otherwise.

> ---
>  drivers/pci/msi.c   | 2 ++
>  include/linux/msi.h | 2 ++
>  kernel/irq/msi.c    | 7 +++++++
>  3 files changed, 11 insertions(+)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index a080f44..565e2a4 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -1277,6 +1277,8 @@ struct irq_domain *pci_msi_create_irq_domain(struct fwnode_handle *fwnode,
>  	if (info->flags & MSI_FLAG_USE_DEF_CHIP_OPS)
>  		pci_msi_domain_update_chip_ops(info);
>  
> +	info->flags |= MSI_FLAG_ACTIVATE_EARLY;
> +
>  	domain = msi_create_irq_domain(fwnode, info, parent);
>  	if (!domain)
>  		return NULL;
> diff --git a/include/linux/msi.h b/include/linux/msi.h
> index 8b425c6..513b7c7 100644
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -270,6 +270,8 @@ enum {
>  	MSI_FLAG_MULTI_PCI_MSI		= (1 << 3),
>  	/* Support PCI MSIX interrupts */
>  	MSI_FLAG_PCI_MSIX		= (1 << 4),
> +	/* Needs early activate, required for PCI */
> +	MSI_FLAG_ACTIVATE_EARLY		= (1 << 5),
>  };
>  
>  int msi_domain_set_affinity(struct irq_data *data, const struct cpumask *mask,
> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
> index 38e89ce..4ed2cca 100644
> --- a/kernel/irq/msi.c
> +++ b/kernel/irq/msi.c
> @@ -361,6 +361,13 @@ int msi_domain_alloc_irqs(struct irq_domain *domain, struct device *dev,
>  		else
>  			dev_dbg(dev, "irq [%d-%d] for MSI\n",
>  				virq, virq + desc->nvec_used - 1);
> +
> +		if (info->flags & MSI_FLAG_ACTIVATE_EARLY) {
> +			struct irq_data *irq_data;
> +
> +			irq_data = irq_domain_get_irq_data(domain, desc->irq);
> +			irq_domain_activate_irq(irq_data);
> +		}
>  	}
>  
>  	return 0;
> -- 
> 2.1.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html