| Message ID | 20200923142607.10c89bd2@xhacker.debian |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: dwc: Move allocate and map page for msi out of dw_pcie_msi_init() | expand |
On Wed, Sep 23, 2020 at 12:27 AM Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote: > > Currently, dw_pcie_msi_init() allocates and maps page for msi, then > program the PCIE_MSI_ADDR_LO and PCIE_MSI_ADDR_HI. The Root Complex > may lose power during suspend-to-RAM, so when we resume, we want to > redo the latter but not the former. If designware based driver (for > example, pcie-tegra194.c) calls dw_pcie_msi_init() in resume path, the > previous msi page will be leaked. It's worse than this. I think there's also error paths too leaking the page. Also, there's never a dma_unmap_page call which should happen before freeing. > Move the allocate and map msi page from dw_pcie_msi_init() to > dw_pcie_host_init() to fix this problem. > > Fixes: 56e15a238d92 ("PCI: tegra: Add Tegra194 PCIe support") > Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com> > --- > drivers/pci/controller/dwc/pci-dra7xx.c | 18 ++++++++++++- > .../pci/controller/dwc/pcie-designware-host.c | 27 +++++++++---------- > 2 files changed, 30 insertions(+), 15 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c > index dc387724cf08..4301cf844a4c 100644 > --- a/drivers/pci/controller/dwc/pci-dra7xx.c > +++ b/drivers/pci/controller/dwc/pci-dra7xx.c > @@ -490,7 +490,9 @@ static struct irq_chip dra7xx_pci_msi_bottom_irq_chip = { > static int dra7xx_pcie_msi_host_init(struct pcie_port *pp) > { > struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > + struct device *dev = pci->dev; > u32 ctrl, num_ctrls; > + int ret; > > pp->msi_irq_chip = &dra7xx_pci_msi_bottom_irq_chip; > > @@ -506,7 +508,21 @@ static int dra7xx_pcie_msi_host_init(struct pcie_port *pp) > ~0); > } > > - return dw_pcie_allocate_domains(pp); > + ret = dw_pcie_allocate_domains(pp); > + if (ret) > + return ret; > + > + pp->msi_page = alloc_page(GFP_KERNEL); > + pp->msi_data = dma_map_page(dev, pp->msi_page, 0, PAGE_SIZE, > + DMA_FROM_DEVICE); > + ret = dma_mapping_error(dev, pp->msi_data); > + if (ret) { > + dev_err(dev, "Failed to map MSI data\n"); > + __free_page(pp->msi_page); > + pp->msi_page = NULL; > + dw_pcie_free_msi(pp); > + } I don't like having 2 copies of the same thing. Also, doesn't keystone need this too? The other thing is .msi_host_init() is abused by having an empty function to disable MSI support. We should have a flag instead to enable/disable MSI support and then we can key off of that in the common code. > + return ret; > } > > static const struct dw_pcie_host_ops dra7xx_pcie_host_ops = { > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > index 9dafecba347f..c23ba64f64fe 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > @@ -294,20 +294,7 @@ void dw_pcie_free_msi(struct pcie_port *pp) > > void dw_pcie_msi_init(struct pcie_port *pp) Might be good to rename this function with exactly what it does. There's too many 'init' and 'setup' functions... > { > - struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > - struct device *dev = pci->dev; > - u64 msi_target; > - > - pp->msi_page = alloc_page(GFP_KERNEL); > - pp->msi_data = dma_map_page(dev, pp->msi_page, 0, PAGE_SIZE, > - DMA_FROM_DEVICE); > - if (dma_mapping_error(dev, pp->msi_data)) { > - dev_err(dev, "Failed to map MSI data\n"); > - __free_page(pp->msi_page); > - pp->msi_page = NULL; > - return; > - } > - msi_target = (u64)pp->msi_data; > + u64 msi_target = (u64)pp->msi_data; > > /* Program the msi_data */ > dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_LO, 4, > @@ -440,6 +427,18 @@ int dw_pcie_host_init(struct pcie_port *pp) > irq_set_chained_handler_and_data(pp->msi_irq, > dw_chained_msi_isr, > pp); > + > + pp->msi_page = alloc_page(GFP_KERNEL); > + pp->msi_data = dma_map_page(pci->dev, pp->msi_page, > + 0, PAGE_SIZE, > + DMA_FROM_DEVICE); > + ret = dma_mapping_error(pci->dev, pp->msi_data); > + if (ret) { > + dev_err(pci->dev, "Failed to map MSI data\n"); > + __free_page(pp->msi_page); > + pp->msi_page = NULL; > + goto err_free_msi; > + } > } else { > ret = pp->ops->msi_host_init(pp); > if (ret < 0) > -- > 2.28.0 >
Hi Rob, On Wed, 23 Sep 2020 10:41:45 -0600 Rob Herring wrote: > > On Wed, Sep 23, 2020 at 12:27 AM Jisheng Zhang > <Jisheng.Zhang@synaptics.com> wrote: > > > > Currently, dw_pcie_msi_init() allocates and maps page for msi, then > > program the PCIE_MSI_ADDR_LO and PCIE_MSI_ADDR_HI. The Root Complex > > may lose power during suspend-to-RAM, so when we resume, we want to > > redo the latter but not the former. If designware based driver (for > > example, pcie-tegra194.c) calls dw_pcie_msi_init() in resume path, the > > previous msi page will be leaked. > > It's worse than this. I think there's also error paths too leaking the I think you mean the leaking in pcie-tegra194.c's error path. Synaptics SoC pcie driver(not mainlined) needs to call dw_pcie_msi_init() in resume path, pcie-tegra194.c shares the same problem, so I mentioned it in the commit msg, but the patch isn't targeting to fix all the leaking issues in pcie-tegra194.c. This patch at least fix one of the issue. > page. Also, there's never a dma_unmap_page call which should happen > before freeing. Thanks for pointing it out. I will add it in v2 > > > Move the allocate and map msi page from dw_pcie_msi_init() to > > dw_pcie_host_init() to fix this problem. > > > > Fixes: 56e15a238d92 ("PCI: tegra: Add Tegra194 PCIe support") > > Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com> > > --- > > drivers/pci/controller/dwc/pci-dra7xx.c | 18 ++++++++++++- > > .../pci/controller/dwc/pcie-designware-host.c | 27 +++++++++---------- > > 2 files changed, 30 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c > > index dc387724cf08..4301cf844a4c 100644 > > --- a/drivers/pci/controller/dwc/pci-dra7xx.c > > +++ b/drivers/pci/controller/dwc/pci-dra7xx.c > > @@ -490,7 +490,9 @@ static struct irq_chip dra7xx_pci_msi_bottom_irq_chip = { > > static int dra7xx_pcie_msi_host_init(struct pcie_port *pp) > > { > > struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > > + struct device *dev = pci->dev; > > u32 ctrl, num_ctrls; > > + int ret; > > > > pp->msi_irq_chip = &dra7xx_pci_msi_bottom_irq_chip; > > > > @@ -506,7 +508,21 @@ static int dra7xx_pcie_msi_host_init(struct pcie_port *pp) > > ~0); > > } > > > > - return dw_pcie_allocate_domains(pp); > > + ret = dw_pcie_allocate_domains(pp); > > + if (ret) > > + return ret; > > + > > + pp->msi_page = alloc_page(GFP_KERNEL); > > + pp->msi_data = dma_map_page(dev, pp->msi_page, 0, PAGE_SIZE, > > + DMA_FROM_DEVICE); > > + ret = dma_mapping_error(dev, pp->msi_data); > > + if (ret) { > > + dev_err(dev, "Failed to map MSI data\n"); > > + __free_page(pp->msi_page); > > + pp->msi_page = NULL; > > + dw_pcie_free_msi(pp); > > + } > > I don't like having 2 copies of the same thing. Also, doesn't keystone > need this too? what about introduce dw_pcie_msi_alloc() to do this? IIUC, keystone doesn't need this. > > The other thing is .msi_host_init() is abused by having an empty > function to disable MSI support. We should have a flag instead to > enable/disable MSI support and then we can key off of that in the > common code. FWICT, the .msi_host_init() is to init soc's own msi support rather than init the DWC's integrated MSI module. So the usage is correct. > > > + return ret; > > } > > > > static const struct dw_pcie_host_ops dra7xx_pcie_host_ops = { > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > > index 9dafecba347f..c23ba64f64fe 100644 > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > > @@ -294,20 +294,7 @@ void dw_pcie_free_msi(struct pcie_port *pp) > > > > void dw_pcie_msi_init(struct pcie_port *pp) > > Might be good to rename this function with exactly what it does. > There's too many 'init' and 'setup' functions... If we move the msi page allocation out of dw_pcie_msi_init(), then it only initializes the integrated MSI. Thanks > > > { > > - struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > > - struct device *dev = pci->dev; > > - u64 msi_target; > > - > > - pp->msi_page = alloc_page(GFP_KERNEL); > > - pp->msi_data = dma_map_page(dev, pp->msi_page, 0, PAGE_SIZE, > > - DMA_FROM_DEVICE); > > - if (dma_mapping_error(dev, pp->msi_data)) { > > - dev_err(dev, "Failed to map MSI data\n"); > > - __free_page(pp->msi_page); > > - pp->msi_page = NULL; > > - return; > > - } > > - msi_target = (u64)pp->msi_data; > > + u64 msi_target = (u64)pp->msi_data; > > > > /* Program the msi_data */ > > dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_LO, 4, > > @@ -440,6 +427,18 @@ int dw_pcie_host_init(struct pcie_port *pp) > > irq_set_chained_handler_and_data(pp->msi_irq, > > dw_chained_msi_isr, > > pp); > > + > > + pp->msi_page = alloc_page(GFP_KERNEL); > > + pp->msi_data = dma_map_page(pci->dev, pp->msi_page, > > + 0, PAGE_SIZE, > > + DMA_FROM_DEVICE); > > + ret = dma_mapping_error(pci->dev, pp->msi_data); > > + if (ret) { > > + dev_err(pci->dev, "Failed to map MSI data\n"); > > + __free_page(pp->msi_page); > > + pp->msi_page = NULL; > > + goto err_free_msi; > > + } > > } else { > > ret = pp->ops->msi_host_init(pp); > > if (ret < 0) > > -- > > 2.28.0 > >
On Wed, 23 Sep 2020 at 08:28, Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote: > > Currently, dw_pcie_msi_init() allocates and maps page for msi, then > program the PCIE_MSI_ADDR_LO and PCIE_MSI_ADDR_HI. The Root Complex > may lose power during suspend-to-RAM, so when we resume, we want to > redo the latter but not the former. If designware based driver (for > example, pcie-tegra194.c) calls dw_pcie_msi_init() in resume path, the > previous msi page will be leaked. > > Move the allocate and map msi page from dw_pcie_msi_init() to > dw_pcie_host_init() to fix this problem. > > Fixes: 56e15a238d92 ("PCI: tegra: Add Tegra194 PCIe support") > Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com> Why do you allocate a page for this in the first place? Isn't PCIE_MSI_ADDR_HI:PCIE_MSI_ADDR_LO simply a magic DMA address that never gets forwarded across to the CPU side of the host bridge, and triggers a SPI instead, which gets handled by reading PCIE_MSI_INTR0_STATUS ? Couldn't you just map the zero page instead? > --- > drivers/pci/controller/dwc/pci-dra7xx.c | 18 ++++++++++++- > .../pci/controller/dwc/pcie-designware-host.c | 27 +++++++++---------- > 2 files changed, 30 insertions(+), 15 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c > index dc387724cf08..4301cf844a4c 100644 > --- a/drivers/pci/controller/dwc/pci-dra7xx.c > +++ b/drivers/pci/controller/dwc/pci-dra7xx.c > @@ -490,7 +490,9 @@ static struct irq_chip dra7xx_pci_msi_bottom_irq_chip = { > static int dra7xx_pcie_msi_host_init(struct pcie_port *pp) > { > struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > + struct device *dev = pci->dev; > u32 ctrl, num_ctrls; > + int ret; > > pp->msi_irq_chip = &dra7xx_pci_msi_bottom_irq_chip; > > @@ -506,7 +508,21 @@ static int dra7xx_pcie_msi_host_init(struct pcie_port *pp) > ~0); > } > > - return dw_pcie_allocate_domains(pp); > + ret = dw_pcie_allocate_domains(pp); > + if (ret) > + return ret; > + > + pp->msi_page = alloc_page(GFP_KERNEL); > + pp->msi_data = dma_map_page(dev, pp->msi_page, 0, PAGE_SIZE, > + DMA_FROM_DEVICE); > + ret = dma_mapping_error(dev, pp->msi_data); > + if (ret) { > + dev_err(dev, "Failed to map MSI data\n"); > + __free_page(pp->msi_page); > + pp->msi_page = NULL; > + dw_pcie_free_msi(pp); > + } > + return ret; > } > > static const struct dw_pcie_host_ops dra7xx_pcie_host_ops = { > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > index 9dafecba347f..c23ba64f64fe 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > @@ -294,20 +294,7 @@ void dw_pcie_free_msi(struct pcie_port *pp) > > void dw_pcie_msi_init(struct pcie_port *pp) > { > - struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > - struct device *dev = pci->dev; > - u64 msi_target; > - > - pp->msi_page = alloc_page(GFP_KERNEL); > - pp->msi_data = dma_map_page(dev, pp->msi_page, 0, PAGE_SIZE, > - DMA_FROM_DEVICE); > - if (dma_mapping_error(dev, pp->msi_data)) { > - dev_err(dev, "Failed to map MSI data\n"); > - __free_page(pp->msi_page); > - pp->msi_page = NULL; > - return; > - } > - msi_target = (u64)pp->msi_data; > + u64 msi_target = (u64)pp->msi_data; > > /* Program the msi_data */ > dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_LO, 4, > @@ -440,6 +427,18 @@ int dw_pcie_host_init(struct pcie_port *pp) > irq_set_chained_handler_and_data(pp->msi_irq, > dw_chained_msi_isr, > pp); > + > + pp->msi_page = alloc_page(GFP_KERNEL); > + pp->msi_data = dma_map_page(pci->dev, pp->msi_page, > + 0, PAGE_SIZE, > + DMA_FROM_DEVICE); > + ret = dma_mapping_error(pci->dev, pp->msi_data); > + if (ret) { > + dev_err(pci->dev, "Failed to map MSI data\n"); > + __free_page(pp->msi_page); > + pp->msi_page = NULL; > + goto err_free_msi; > + } > } else { > ret = pp->ops->msi_host_init(pp); > if (ret < 0) > -- > 2.28.0 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Sep 24, 2020 at 5:00 AM Ard Biesheuvel <ardb@kernel.org> wrote: > > On Wed, 23 Sep 2020 at 08:28, Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote: > > > > Currently, dw_pcie_msi_init() allocates and maps page for msi, then > > program the PCIE_MSI_ADDR_LO and PCIE_MSI_ADDR_HI. The Root Complex > > may lose power during suspend-to-RAM, so when we resume, we want to > > redo the latter but not the former. If designware based driver (for > > example, pcie-tegra194.c) calls dw_pcie_msi_init() in resume path, the > > previous msi page will be leaked. > > > > Move the allocate and map msi page from dw_pcie_msi_init() to > > dw_pcie_host_init() to fix this problem. > > > > Fixes: 56e15a238d92 ("PCI: tegra: Add Tegra194 PCIe support") > > Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com> > > Why do you allocate a page for this in the first place? Isn't > PCIE_MSI_ADDR_HI:PCIE_MSI_ADDR_LO simply a magic DMA address that > never gets forwarded across to the CPU side of the host bridge, and > triggers a SPI instead, which gets handled by reading > PCIE_MSI_INTR0_STATUS ? My question too after digging into this some more. I've asked the question on the thread that further complicated all this changing from virt_to_phys() to dma_map_page()[1]. > Couldn't you just map the zero page instead? Why a page even? You could use PCIE_MSI_ADDR_LO address itself even. Or just an address in the driver data which is what some other drivers do. Rob [1] https://lore.kernel.org/linux-pci/20200923231846.GA1499246@bogus/
On Thu, 24 Sep 2020 at 15:28, Rob Herring <robh@kernel.org> wrote: > > On Thu, Sep 24, 2020 at 5:00 AM Ard Biesheuvel <ardb@kernel.org> wrote: > > > > On Wed, 23 Sep 2020 at 08:28, Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote: > > > > > > Currently, dw_pcie_msi_init() allocates and maps page for msi, then > > > program the PCIE_MSI_ADDR_LO and PCIE_MSI_ADDR_HI. The Root Complex > > > may lose power during suspend-to-RAM, so when we resume, we want to > > > redo the latter but not the former. If designware based driver (for > > > example, pcie-tegra194.c) calls dw_pcie_msi_init() in resume path, the > > > previous msi page will be leaked. > > > > > > Move the allocate and map msi page from dw_pcie_msi_init() to > > > dw_pcie_host_init() to fix this problem. > > > > > > Fixes: 56e15a238d92 ("PCI: tegra: Add Tegra194 PCIe support") > > > Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com> > > > > Why do you allocate a page for this in the first place? Isn't > > PCIE_MSI_ADDR_HI:PCIE_MSI_ADDR_LO simply a magic DMA address that > > never gets forwarded across to the CPU side of the host bridge, and > > triggers a SPI instead, which gets handled by reading > > PCIE_MSI_INTR0_STATUS ? > > My question too after digging into this some more. I've asked the > question on the thread that further complicated all this changing from > virt_to_phys() to dma_map_page()[1]. > > > Couldn't you just map the zero page instead? > > Why a page even? You could use PCIE_MSI_ADDR_LO address itself even. > Or just an address in the driver data which is what some other drivers > do. > PCIE_MSI_ADDR_LO itself could collide with an actual DRAM address if any translation is applied on inbound transactions. Using an actual DRAM address avoids that.
On Thu, 24 Sep 2020 at 15:45, Ard Biesheuvel <ardb@kernel.org> wrote: > > On Thu, 24 Sep 2020 at 15:28, Rob Herring <robh@kernel.org> wrote: > > > > On Thu, Sep 24, 2020 at 5:00 AM Ard Biesheuvel <ardb@kernel.org> wrote: > > > > > > On Wed, 23 Sep 2020 at 08:28, Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote: > > > > > > > > Currently, dw_pcie_msi_init() allocates and maps page for msi, then > > > > program the PCIE_MSI_ADDR_LO and PCIE_MSI_ADDR_HI. The Root Complex > > > > may lose power during suspend-to-RAM, so when we resume, we want to > > > > redo the latter but not the former. If designware based driver (for > > > > example, pcie-tegra194.c) calls dw_pcie_msi_init() in resume path, the > > > > previous msi page will be leaked. > > > > > > > > Move the allocate and map msi page from dw_pcie_msi_init() to > > > > dw_pcie_host_init() to fix this problem. > > > > > > > > Fixes: 56e15a238d92 ("PCI: tegra: Add Tegra194 PCIe support") > > > > Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com> > > > > > > Why do you allocate a page for this in the first place? Isn't > > > PCIE_MSI_ADDR_HI:PCIE_MSI_ADDR_LO simply a magic DMA address that > > > never gets forwarded across to the CPU side of the host bridge, and > > > triggers a SPI instead, which gets handled by reading > > > PCIE_MSI_INTR0_STATUS ? > > > > My question too after digging into this some more. I've asked the > > question on the thread that further complicated all this changing from > > virt_to_phys() to dma_map_page()[1]. > > > > > Couldn't you just map the zero page instead? > > > > Why a page even? You could use PCIE_MSI_ADDR_LO address itself even. > > Or just an address in the driver data which is what some other drivers > > do. > > > > PCIE_MSI_ADDR_LO itself could collide with an actual DRAM address if > any translation is applied on inbound transactions. Using an actual > DRAM address avoids that. ... although the MSI doorbell register on the GIC, for instance, needs to be DMA addressable as well, of course.
On Thu, Sep 24, 2020 at 8:33 AM Ard Biesheuvel <ardb@kernel.org> wrote: > > On Thu, 24 Sep 2020 at 15:45, Ard Biesheuvel <ardb@kernel.org> wrote: > > > > On Thu, 24 Sep 2020 at 15:28, Rob Herring <robh@kernel.org> wrote: > > > > > > On Thu, Sep 24, 2020 at 5:00 AM Ard Biesheuvel <ardb@kernel.org> wrote: > > > > > > > > On Wed, 23 Sep 2020 at 08:28, Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote: > > > > > > > > > > Currently, dw_pcie_msi_init() allocates and maps page for msi, then > > > > > program the PCIE_MSI_ADDR_LO and PCIE_MSI_ADDR_HI. The Root Complex > > > > > may lose power during suspend-to-RAM, so when we resume, we want to > > > > > redo the latter but not the former. If designware based driver (for > > > > > example, pcie-tegra194.c) calls dw_pcie_msi_init() in resume path, the > > > > > previous msi page will be leaked. > > > > > > > > > > Move the allocate and map msi page from dw_pcie_msi_init() to > > > > > dw_pcie_host_init() to fix this problem. > > > > > > > > > > Fixes: 56e15a238d92 ("PCI: tegra: Add Tegra194 PCIe support") > > > > > Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com> > > > > > > > > Why do you allocate a page for this in the first place? Isn't > > > > PCIE_MSI_ADDR_HI:PCIE_MSI_ADDR_LO simply a magic DMA address that > > > > never gets forwarded across to the CPU side of the host bridge, and > > > > triggers a SPI instead, which gets handled by reading > > > > PCIE_MSI_INTR0_STATUS ? > > > > > > My question too after digging into this some more. I've asked the > > > question on the thread that further complicated all this changing from > > > virt_to_phys() to dma_map_page()[1]. > > > > > > > Couldn't you just map the zero page instead? > > > > > > Why a page even? You could use PCIE_MSI_ADDR_LO address itself even. > > > Or just an address in the driver data which is what some other drivers > > > do. > > > > > > > PCIE_MSI_ADDR_LO itself could collide with an actual DRAM address if > > any translation is applied on inbound transactions. Using an actual > > DRAM address avoids that. Good point, and the inbound windows could be less than all DRAM if there's any restrictions. However, the DWC doesn't do any inbound setup (at least for platforms with iATU), so I guess the default is all traffic is forwarded. > ... although the MSI doorbell register on the GIC, for instance, needs > to be DMA addressable as well, of course. There's at least one DWC driver that has its own doorbell register too. I suppose there's a way for the host driver to retrieve the GIC address if configuration is needed. Doesn't seem to be needed on DWC given the above. Rob
On Thu, 24 Sep 2020 07:28:25 -0600 Rob Herring <robh@kernel.org> wrote: > CAUTION: Email originated externally, do not click links or open attachments unless you recognize the sender and know the content is safe. > > > On Thu, Sep 24, 2020 at 5:00 AM Ard Biesheuvel <ardb@kernel.org> wrote: > > > > On Wed, 23 Sep 2020 at 08:28, Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote: > > > > > > Currently, dw_pcie_msi_init() allocates and maps page for msi, then > > > program the PCIE_MSI_ADDR_LO and PCIE_MSI_ADDR_HI. The Root Complex > > > may lose power during suspend-to-RAM, so when we resume, we want to > > > redo the latter but not the former. If designware based driver (for > > > example, pcie-tegra194.c) calls dw_pcie_msi_init() in resume path, the > > > previous msi page will be leaked. > > > > > > Move the allocate and map msi page from dw_pcie_msi_init() to > > > dw_pcie_host_init() to fix this problem. > > > > > > Fixes: 56e15a238d92 ("PCI: tegra: Add Tegra194 PCIe support") > > > Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com> > > > > Why do you allocate a page for this in the first place? Isn't > > PCIE_MSI_ADDR_HI:PCIE_MSI_ADDR_LO simply a magic DMA address that > > never gets forwarded across to the CPU side of the host bridge, and > > triggers a SPI instead, which gets handled by reading > > PCIE_MSI_INTR0_STATUS ? > > My question too after digging into this some more. I've asked the > question on the thread that further complicated all this changing from > virt_to_phys() to dma_map_page()[1]. > > > Couldn't you just map the zero page instead? > > Why a page even? You could use PCIE_MSI_ADDR_LO address itself even. > Or just an address in the driver data which is what some other drivers > do. > Thank Ard and Rob. I have sent out V3 (https://lkml.org/lkml/2020/9/25/272) which uses an address in the driver data for MSI address. Hi Niklas, can you please try v3 to see whether it solves your problem as well? Thanks
diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c index dc387724cf08..4301cf844a4c 100644 --- a/drivers/pci/controller/dwc/pci-dra7xx.c +++ b/drivers/pci/controller/dwc/pci-dra7xx.c @@ -490,7 +490,9 @@ static struct irq_chip dra7xx_pci_msi_bottom_irq_chip = { static int dra7xx_pcie_msi_host_init(struct pcie_port *pp) { struct dw_pcie *pci = to_dw_pcie_from_pp(pp); + struct device *dev = pci->dev; u32 ctrl, num_ctrls; + int ret; pp->msi_irq_chip = &dra7xx_pci_msi_bottom_irq_chip; @@ -506,7 +508,21 @@ static int dra7xx_pcie_msi_host_init(struct pcie_port *pp) ~0); } - return dw_pcie_allocate_domains(pp); + ret = dw_pcie_allocate_domains(pp); + if (ret) + return ret; + + pp->msi_page = alloc_page(GFP_KERNEL); + pp->msi_data = dma_map_page(dev, pp->msi_page, 0, PAGE_SIZE, + DMA_FROM_DEVICE); + ret = dma_mapping_error(dev, pp->msi_data); + if (ret) { + dev_err(dev, "Failed to map MSI data\n"); + __free_page(pp->msi_page); + pp->msi_page = NULL; + dw_pcie_free_msi(pp); + } + return ret; } static const struct dw_pcie_host_ops dra7xx_pcie_host_ops = { diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c index 9dafecba347f..c23ba64f64fe 100644 --- a/drivers/pci/controller/dwc/pcie-designware-host.c +++ b/drivers/pci/controller/dwc/pcie-designware-host.c @@ -294,20 +294,7 @@ void dw_pcie_free_msi(struct pcie_port *pp) void dw_pcie_msi_init(struct pcie_port *pp) { - struct dw_pcie *pci = to_dw_pcie_from_pp(pp); - struct device *dev = pci->dev; - u64 msi_target; - - pp->msi_page = alloc_page(GFP_KERNEL); - pp->msi_data = dma_map_page(dev, pp->msi_page, 0, PAGE_SIZE, - DMA_FROM_DEVICE); - if (dma_mapping_error(dev, pp->msi_data)) { - dev_err(dev, "Failed to map MSI data\n"); - __free_page(pp->msi_page); - pp->msi_page = NULL; - return; - } - msi_target = (u64)pp->msi_data; + u64 msi_target = (u64)pp->msi_data; /* Program the msi_data */ dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_LO, 4, @@ -440,6 +427,18 @@ int dw_pcie_host_init(struct pcie_port *pp) irq_set_chained_handler_and_data(pp->msi_irq, dw_chained_msi_isr, pp); + + pp->msi_page = alloc_page(GFP_KERNEL); + pp->msi_data = dma_map_page(pci->dev, pp->msi_page, + 0, PAGE_SIZE, + DMA_FROM_DEVICE); + ret = dma_mapping_error(pci->dev, pp->msi_data); + if (ret) { + dev_err(pci->dev, "Failed to map MSI data\n"); + __free_page(pp->msi_page); + pp->msi_page = NULL; + goto err_free_msi; + } } else { ret = pp->ops->msi_host_init(pp); if (ret < 0)
Currently, dw_pcie_msi_init() allocates and maps page for msi, then program the PCIE_MSI_ADDR_LO and PCIE_MSI_ADDR_HI. The Root Complex may lose power during suspend-to-RAM, so when we resume, we want to redo the latter but not the former. If designware based driver (for example, pcie-tegra194.c) calls dw_pcie_msi_init() in resume path, the previous msi page will be leaked. Move the allocate and map msi page from dw_pcie_msi_init() to dw_pcie_host_init() to fix this problem. Fixes: 56e15a238d92 ("PCI: tegra: Add Tegra194 PCIe support") Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com> --- drivers/pci/controller/dwc/pci-dra7xx.c | 18 ++++++++++++- .../pci/controller/dwc/pcie-designware-host.c | 27 +++++++++---------- 2 files changed, 30 insertions(+), 15 deletions(-)