From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752579AbdFSVwT (ORCPT ); Mon, 19 Jun 2017 17:52:19 -0400 Received: from mail.kernel.org ([198.145.29.99]:36372 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752419AbdFSVwR (ORCPT ); Mon, 19 Jun 2017 17:52:17 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 0E3C2239D0 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=helgaas@kernel.org Date: Mon, 19 Jun 2017 16:52:14 -0500 From: Bjorn Helgaas To: Robin Murphy Cc: bhelgaas@google.com, marc.zyngier@arm.com, linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, David Daney , Alex Williamson Subject: Re: [PATCH] PCI/MSI: Improve MSI alias detection Message-ID: <20170619215214.GB554@bhelgaas-glaptop.roam.corp.google.com> References: <20170531175230.16529-2-robin.murphy@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170531175230.16529-2-robin.murphy@arm.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.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 > --- > 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 >