diff mbox series

[v3,2/2] PCI: dwc: add support for 64-bit MSI target address

Message ID 20220810231445.2242126-3-willmcvicker@google.com (mailing list archive)
State Superseded
Headers show
Series PCI: dwc: Add support for 64-bit MSI target addresses | expand

Commit Message

William McVicker Aug. 10, 2022, 11:14 p.m. UTC 
Since not all devices require a 32-bit MSI address, add support to the
PCIe host driver to allow setting the DMA mask to 64-bits. This allows
kernels to disable ZONE_DMA32 and bounce buffering (swiotlb) without
risking not being able to get a 32-bit address during DMA allocation.
Basically, in the slim chance that there are no 32-bit allocations
available, the current PCIe host driver will fail to allocate the
msi_msg page due to a DMA address overflow (seen in [1]). With this
patch, the PCIe driver can advertise 64-bit support via it's MSI
capabilities to hint to the PCIe host driver to set the DMA mask to
64-bits.

[1] https://lore.kernel.org/all/Yo0soniFborDl7+C@google.com/

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Will McVicker <willmcvicker@google.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 14 ++++++++++++--
 drivers/pci/controller/dwc/pcie-designware.c      |  9 +++++++++
 drivers/pci/controller/dwc/pcie-designware.h      |  1 +
 3 files changed, 22 insertions(+), 2 deletions(-)

Comments

Bjorn Helgaas Aug. 11, 2022, 4:07 p.m. UTC | #1 
No need to rush the versions (I see v1 on 8/9, v2 and v3 on 8/10).

But if/when you update this, capitalize the subject lines
("PCI: dwc: Add support ...") to match previous history.

On Wed, Aug 10, 2022 at 11:14:44PM +0000, Will McVicker wrote:
> Since not all devices require a 32-bit MSI address, add support to the
> PCIe host driver to allow setting the DMA mask to 64-bits. This allows
> kernels to disable ZONE_DMA32 and bounce buffering (swiotlb) without
> risking not being able to get a 32-bit address during DMA allocation.
> Basically, in the slim chance that there are no 32-bit allocations
> available, the current PCIe host driver will fail to allocate the
> msi_msg page due to a DMA address overflow (seen in [1]).

> With this patch, the PCIe driver can advertise 64-bit support via
> it's MSI capabilities to hint to the PCIe host driver to set the DMA
> mask to 64-bits.

s/via it's/via its/

I'm not quite sure what this sentence is saying.  I think it's
actually the *device* (not the PCIe driver) that advertises 64-bit
support in its MSI capability.

> +u16 dw_pcie_msi_capabilities(struct dw_pcie *pci)
> +{
> +	u8 offset;
> +
> +	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
> +	return dw_pcie_readw_dbi(pci, offset + PCI_MSI_FLAGS);
> +}
> +EXPORT_SYMBOL_GPL(dw_pcie_msi_capabilities);

Why does this need to be exported?  CONFIG_PCIE_DW and
CONFIG_PCIE_DW_HOST are both bool, so I don't see any callers from
modules.

I see there are some other functions in pcie-designware.c that are
exported, but I'm a little dubious about those, too.  There are
several DWC drivers that are tristate (PCI_DRA7XX, PCI_EXYNOS, etc),
but they select PCIE_DW_HOST and PCIE_DW, which are bool.  I guess
this means the DWC core code gets built-in while the dra7xx, exynos,
etc code is a module.

If we want these to be modules, it would make more sense to me to have
the module include both the DWC core code and the specific driver.
I.e., the DWC core code would not be statically included at all, and
the dra7xx module would contain DWC core and dra7xx, the exynos module
would contain DWC core and exynos, etc.

Maybe Kconfig isn't expressive enough for that?  I don't know Kconfig
well enough.

Bjorn
William McVicker Aug. 11, 2022, 4:31 p.m. UTC | #2 
On 08/11/2022, Bjorn Helgaas wrote:
> No need to rush the versions (I see v1 on 8/9, v2 and v3 on 8/10).
> 
> But if/when you update this, capitalize the subject lines
> ("PCI: dwc: Add support ...") to match previous history.

Sorry for the quick updates. I'll be sure to update the subject on the next
iteration.

> 
> On Wed, Aug 10, 2022 at 11:14:44PM +0000, Will McVicker wrote:
> > Since not all devices require a 32-bit MSI address, add support to the
> > PCIe host driver to allow setting the DMA mask to 64-bits. This allows
> > kernels to disable ZONE_DMA32 and bounce buffering (swiotlb) without
> > risking not being able to get a 32-bit address during DMA allocation.
> > Basically, in the slim chance that there are no 32-bit allocations
> > available, the current PCIe host driver will fail to allocate the
> > msi_msg page due to a DMA address overflow (seen in [1]).
> 
> > With this patch, the PCIe driver can advertise 64-bit support via
> > it's MSI capabilities to hint to the PCIe host driver to set the DMA
> > mask to 64-bits.
> 
> s/via it's/via its/

Thanks.

> 
> I'm not quite sure what this sentence is saying.  I think it's
> actually the *device* (not the PCIe driver) that advertises 64-bit
> support in its MSI capability.

You're right. The PCIe device is doing the advertising. I'll updae this
wording

> 
> > +u16 dw_pcie_msi_capabilities(struct dw_pcie *pci)
> > +{
> > +	u8 offset;
> > +
> > +	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
> > +	return dw_pcie_readw_dbi(pci, offset + PCI_MSI_FLAGS);
> > +}
> > +EXPORT_SYMBOL_GPL(dw_pcie_msi_capabilities);
> 
> Why does this need to be exported?  CONFIG_PCIE_DW and
> CONFIG_PCIE_DW_HOST are both bool, so I don't see any callers from
> modules.
> 
> I see there are some other functions in pcie-designware.c that are
> exported, but I'm a little dubious about those, too.  There are
> several DWC drivers that are tristate (PCI_DRA7XX, PCI_EXYNOS, etc),
> but they select PCIE_DW_HOST and PCIE_DW, which are bool.  I guess
> this means the DWC core code gets built-in while the dra7xx, exynos,
> etc code is a module.
> 
> If we want these to be modules, it would make more sense to me to have
> the module include both the DWC core code and the specific driver.
> I.e., the DWC core code would not be statically included at all, and
> the dra7xx module would contain DWC core and dra7xx, the exynos module
> would contain DWC core and exynos, etc.
> 
> Maybe Kconfig isn't expressive enough for that?  I don't know Kconfig
> well enough.

So you usually want to separate out the core driver from the device driver
and then add the core driver dependencies in the device driver Kconfig
definition. This allows you to statically include the core drivers
(commonly used code between multiple device drivers) into the kernel and
then build the device drivers as kernel modules. And just to note, it's
okay for PCIE_DW and PCIE_DW_HOST to be configured as a bool and selected
from a tristate config. If you take PCI_EXYNOS=m as an example, that will
result in:

  CONFIG_PCIE_DW=y
  CONFIG_PCIE_DW_HOST=y
  CONFIG_PCI_EXYNOS=m

With regards to exporting dw_pcie_msi_capabilities(), the idea is that this
function is a helper for the PCI DW device drivers. For those that want to
enable the 64-bit MSI flag, they can use this function to get the MSI
capabilities in order to update the MSI flags. I haven't added a use-case
to an existing driver yet.

> 
> Bjorn
> 
> -- 
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>
diff mbox series

Patch

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 39f3b37d4033..18cf96f911dc 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -330,6 +330,8 @@  static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
 	u64 *msi_vaddr;
 	int ret;
 	u32 ctrl, num_ctrls;
+	bool msi_64b = false;
+	u16 msi_capabilities;
 
 	for (ctrl = 0; ctrl < MAX_MSI_CTRLS; ctrl++)
 		pp->irq_mask[ctrl] = ~0;
@@ -367,9 +369,17 @@  static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
 						    dw_chained_msi_isr, pp);
 	}
 
-	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
+	msi_capabilities = dw_pcie_msi_capabilities(pci);
+	if (msi_capabilities & PCI_MSI_FLAGS_ENABLE)
+		msi_64b = msi_capabilities & PCI_MSI_FLAGS_64BIT ? true : false;
+
+	dev_dbg(dev, "Setting MSI DMA mask to %s-bit.\n",
+		msi_64b ? "64" : "32");
+	ret = dma_set_mask_and_coherent(dev, msi_64b ?
+					DMA_BIT_MASK(64) : DMA_BIT_MASK(32));
 	if (ret)
-		dev_warn(dev, "Failed to set DMA mask to 32-bit. Devices with only 32-bit MSI support may not work properly\n");
+		dev_warn(dev, "Failed to set DMA mask to %s-bit.\n",
+			 msi_64b ? "64" : "32");
 
 	msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
 					GFP_KERNEL);
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index c6725c519a47..8ed402307d7f 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -82,6 +82,15 @@  u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap)
 }
 EXPORT_SYMBOL_GPL(dw_pcie_find_capability);
 
+u16 dw_pcie_msi_capabilities(struct dw_pcie *pci)
+{
+	u8 offset;
+
+	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
+	return dw_pcie_readw_dbi(pci, offset + PCI_MSI_FLAGS);
+}
+EXPORT_SYMBOL_GPL(dw_pcie_msi_capabilities);
+
 static u16 dw_pcie_find_next_ext_capability(struct dw_pcie *pci, u16 start,
 					    u8 cap)
 {
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index a871ae7eb59e..45fcdfc8c035 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -332,6 +332,7 @@  void dw_pcie_version_detect(struct dw_pcie *pci);
 
 u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap);
 u16 dw_pcie_find_ext_capability(struct dw_pcie *pci, u8 cap);
+u16 dw_pcie_msi_capabilities(struct dw_pcie *pci);
 
 int dw_pcie_read(void __iomem *addr, int size, u32 *val);
 int dw_pcie_write(void __iomem *addr, int size, u32 val);