Re: [PATCH v2] PCI/MSI: Improve MSI alias detection

[Bjorn Helgaas]

On Tue, Aug 01, 2017 at 06:59:08PM +0100, Robin Murphy wrote:
> Currently, we handle all DMA aliases equally when calculating MSI
> requester IDs for the generic infrastructure. This turns out to be the
> wrong thing to do in the face of pure DMA quirks like those of Marvell
> SATA cards, where in the usual case the last thing seen in the alias
> walk is the DMA phantom function: we end up configuring the MSI
> doorbell to expect that alias, then find we have no interrupts since
> the MSI writes still come from the 'real' RID, thus get filtered out
> and ignored.
> 
> Improve the alias walk to only account for the topological aliases that
> matter, based on the logic from the Intel IRQ remapping code.
> 
> Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx>

Applied with Marc's ack to pci/msi for v4.14, thanks!

I used the subject line:

  PCI/MSI: Assume MSIs use real Requester ID, not an alias

> ---
> 
> v2: Properly document the rationale.
> 
>  drivers/pci/msi.c | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 253d92409bb3..2f0dd02d78b7 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -1458,13 +1458,30 @@ struct irq_domain *pci_msi_create_irq_domain(struct fwnode_handle *fwnode,
>  }
>  EXPORT_SYMBOL_GPL(pci_msi_create_irq_domain);
>  
> +/*
> + * Users of the generic MSI infrastructure expect a device to have a single ID,
> + * so with DMA aliases we have to pick the least-worst compromise. Devices with
> + * DMA phantom functions tend to still emit MSIs from the real function number,
> + * so we ignore those and only consider topological aliases where either the
> + * alias device or RID appears on a different bus number. We also make the
> + * reasonable assumption that bridges are walked in an upstream direction (so
> + * the last one seen wins), and the much braver assumption that the most likely
> + * case is that of PCI->PCIe so we should always use the alias RID. This echoes
> + * the logic from intel_irq_remapping's set_msi_sid(), which presumably works
> + * well enough in practice; in the face of the horrible PCIe<->PCI-X conditions
> + * for taking ownership all we can really do is close our eyes and hope...
> + */
>  static int get_msi_id_cb(struct pci_dev *pdev, u16 alias, void *data)
>  {
>  	u32 *pa = data;
> +	u8 bus = PCI_BUS_NUM(*pa);
> +
> +	if (pdev->bus->number != bus || PCI_BUS_NUM(alias) != bus)
> +		*pa = alias;
>  
> -	*pa = alias;
>  	return 0;
>  }
> +
>  /**
>   * pci_msi_domain_get_msi_rid - Get the MSI requester id (RID)
>   * @domain:	The interrupt domain
> @@ -1478,7 +1495,7 @@ static int get_msi_id_cb(struct pci_dev *pdev, u16 alias, void *data)
>  u32 pci_msi_domain_get_msi_rid(struct irq_domain *domain, struct pci_dev *pdev)
>  {
>  	struct device_node *of_node;
> -	u32 rid = 0;
> +	u32 rid = PCI_DEVID(pdev->bus->number, pdev->devfn);
>  
>  	pci_for_each_dma_alias(pdev, get_msi_id_cb, &rid);
>  
> @@ -1494,14 +1511,14 @@ u32 pci_msi_domain_get_msi_rid(struct irq_domain *domain, struct pci_dev *pdev)
>   * @pdev:	The PCI device
>   *
>   * Use the firmware data to find a device-specific MSI domain
> - * (i.e. not one that is ste as a default).
> + * (i.e. not one that is set as a default).
>   *
> - * Returns: The coresponding MSI domain or NULL if none has been found.
> + * Returns: The corresponding MSI domain or NULL if none has been found.
>   */
>  struct irq_domain *pci_msi_get_device_domain(struct pci_dev *pdev)
>  {
>  	struct irq_domain *dom;
> -	u32 rid = 0;
> +	u32 rid = PCI_DEVID(pdev->bus->number, pdev->devfn);
>  
>  	pci_for_each_dma_alias(pdev, get_msi_id_cb, &rid);
>  	dom = of_msi_map_get_device_domain(&pdev->dev, rid);
> -- 
> 2.12.2.dirty
>