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

[Bjorn Helgaas <helgaas@kernel.org>]

[+cc David, Alex]

On Wed, May 31, 2017 at 06:52:28PM +0100, Robin Murphy wrote:
> Currently, we consider all DMA aliases when calculating MSI requester
> IDs. This turns out to be the wrong thing to do in the face of pure DMA
> quirks like those of Marvell SATA cards, where we can end up configuring
> the MSI doorbell to expect the phantom function alias, such that it then
> goes on to happily ignore actual MSI writes coming from the card on the
> real RID.

I guess you're making the assumption that DMA might use phantom
function aliases (aliases on the same bus as the actual device), but
MSI writes will not?

> Improve the alias walk to only account for the topological aliases that
> matter, based on the logic from the Intel IRQ remapping code.

Can you include a specific function reference here?  It sounds like
it'd be useful to compare this with the Intel IRQ remapping code.

> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/pci/msi.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index ba44fdfda66b..7b34c434970e 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -1468,13 +1468,21 @@ struct irq_domain *pci_msi_create_irq_domain(struct fwnode_handle *fwnode,
>  }
>  EXPORT_SYMBOL_GPL(pci_msi_create_irq_domain);
>  
> +/*
> + * If the alias device/RID is on a different bus, it's a topological alias
> + * we should care about; otherwise, it's a DMA quirk and we don't.
> + */
>  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;

Aliases can arise both because of device defects, e.g., the phantom
function quirks, and because of bridges, e.g., all the stuff in
pci_for_each_dma_alias().

Why is it that the users of get_msi_id_cb() can get away with
collapsing any of these bridge aliases into the last one that
pci_for_each_dma_alias() finds?  I guess this is another question
about exactly why DMA and MSI are different here.

>  	return 0;
>  }
> +
>  /**
>   * pci_msi_domain_get_msi_rid - Get the MSI requester id (RID)
>   * @domain:	The interrupt domain
> @@ -1488,7 +1496,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);
>  
> @@ -1504,14 +1512,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
>