* [PATCH v6] PCI: dwc: Strengthen the MSI address allocation logic
@ 2024-02-21 15:38 Ajay Agarwal
2024-02-21 17:32 ` Serge Semin
` (4 more replies)
0 siblings, 5 replies; 7+ messages in thread
From: Ajay Agarwal @ 2024-02-21 15:38 UTC (permalink / raw
To: Jingoo Han, Gustavo Pimentel, Manivannan Sadhasivam,
Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring,
Bjorn Helgaas, Manu Gautam, Sajid Dalvi, William McVicker,
Serge Semin, Robin Murphy
Cc: linux-pci, Ajay Agarwal
There can be platforms that do not use/have 32-bit DMA addresses.
The current implementation of 32-bit IOVA allocation can fail for
such platforms, eventually leading to the probe failure.
Try to allocate a 32-bit msi_data. If this allocation fails,
attempt a 64-bit address allocation. Please note that if the
64-bit MSI address is allocated, then the EPs supporting 32-bit
MSI address only will not work.
Signed-off-by: Ajay Agarwal <ajayagarwal@google.com>
---
Changelog since v5:
- Initialize temp variable 'msi_vaddr' to NULL
- Remove redundant print and check
Changelog since v4:
- Remove the 'DW_PCIE_CAP_MSI_DATA_SET' flag
- Refactor the comments and msi_data allocation logic
Changelog since v3:
- Add a new controller cap flag 'DW_PCIE_CAP_MSI_DATA_SET'
- Refactor the comments and print statements
Changelog since v2:
- If the vendor driver has setup the msi_data, use the same
Changelog since v1:
- Use reserved memory, if it exists, to setup the MSI data
- Fallback to 64-bit IOVA allocation if 32-bit allocation fails
.../pci/controller/dwc/pcie-designware-host.c | 21 ++++++++++++-------
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index d5fc31f8345f..d15a5c2d5b48 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -328,7 +328,7 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
struct device *dev = pci->dev;
struct platform_device *pdev = to_platform_device(dev);
- u64 *msi_vaddr;
+ u64 *msi_vaddr = NULL;
int ret;
u32 ctrl, num_ctrls;
@@ -379,15 +379,20 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
* memory.
*/
ret = dma_set_coherent_mask(dev, 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");
+ if (!ret)
+ msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
+ GFP_KERNEL);
- msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
- GFP_KERNEL);
if (!msi_vaddr) {
- dev_err(dev, "Failed to alloc and map MSI data\n");
- dw_pcie_free_msi(pp);
- return -ENOMEM;
+ dev_warn(dev, "Failed to allocate 32-bit MSI address\n");
+ dma_set_coherent_mask(dev, DMA_BIT_MASK(64));
+ msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
+ GFP_KERNEL);
+ if (!msi_vaddr) {
+ dev_err(dev, "Failed to allocate MSI address\n");
+ dw_pcie_free_msi(pp);
+ return -ENOMEM;
+ }
}
return 0;
--
2.44.0.rc0.258.g7320e95886-goog
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v6] PCI: dwc: Strengthen the MSI address allocation logic
2024-02-21 15:38 [PATCH v6] PCI: dwc: Strengthen the MSI address allocation logic Ajay Agarwal
@ 2024-02-21 17:32 ` Serge Semin
2024-02-21 23:13 ` William McVicker
` (3 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Serge Semin @ 2024-02-21 17:32 UTC (permalink / raw
To: Ajay Agarwal
Cc: Jingoo Han, Gustavo Pimentel, Manivannan Sadhasivam,
Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring,
Bjorn Helgaas, Manu Gautam, Sajid Dalvi, William McVicker,
Robin Murphy, linux-pci
On Wed, Feb 21, 2024 at 09:08:40PM +0530, Ajay Agarwal wrote:
> There can be platforms that do not use/have 32-bit DMA addresses.
> The current implementation of 32-bit IOVA allocation can fail for
> such platforms, eventually leading to the probe failure.
>
> Try to allocate a 32-bit msi_data. If this allocation fails,
> attempt a 64-bit address allocation. Please note that if the
> 64-bit MSI address is allocated, then the EPs supporting 32-bit
> MSI address only will not work.
LGTM now. Thanks!
Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
-Serge(y)
>
> Signed-off-by: Ajay Agarwal <ajayagarwal@google.com>
> ---
> Changelog since v5:
> - Initialize temp variable 'msi_vaddr' to NULL
> - Remove redundant print and check
>
> Changelog since v4:
> - Remove the 'DW_PCIE_CAP_MSI_DATA_SET' flag
> - Refactor the comments and msi_data allocation logic
>
> Changelog since v3:
> - Add a new controller cap flag 'DW_PCIE_CAP_MSI_DATA_SET'
> - Refactor the comments and print statements
>
> Changelog since v2:
> - If the vendor driver has setup the msi_data, use the same
>
> Changelog since v1:
> - Use reserved memory, if it exists, to setup the MSI data
> - Fallback to 64-bit IOVA allocation if 32-bit allocation fails
>
> .../pci/controller/dwc/pcie-designware-host.c | 21 ++++++++++++-------
> 1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index d5fc31f8345f..d15a5c2d5b48 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -328,7 +328,7 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
> struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> struct device *dev = pci->dev;
> struct platform_device *pdev = to_platform_device(dev);
> - u64 *msi_vaddr;
> + u64 *msi_vaddr = NULL;
> int ret;
> u32 ctrl, num_ctrls;
>
> @@ -379,15 +379,20 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
> * memory.
> */
> ret = dma_set_coherent_mask(dev, 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");
> + if (!ret)
> + msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
> + GFP_KERNEL);
>
> - msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
> - GFP_KERNEL);
> if (!msi_vaddr) {
> - dev_err(dev, "Failed to alloc and map MSI data\n");
> - dw_pcie_free_msi(pp);
> - return -ENOMEM;
> + dev_warn(dev, "Failed to allocate 32-bit MSI address\n");
> + dma_set_coherent_mask(dev, DMA_BIT_MASK(64));
> + msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
> + GFP_KERNEL);
> + if (!msi_vaddr) {
> + dev_err(dev, "Failed to allocate MSI address\n");
> + dw_pcie_free_msi(pp);
> + return -ENOMEM;
> + }
> }
>
> return 0;
> --
> 2.44.0.rc0.258.g7320e95886-goog
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v6] PCI: dwc: Strengthen the MSI address allocation logic
2024-02-21 15:38 [PATCH v6] PCI: dwc: Strengthen the MSI address allocation logic Ajay Agarwal
2024-02-21 17:32 ` Serge Semin
@ 2024-02-21 23:13 ` William McVicker
2024-02-27 14:45 ` Manivannan Sadhasivam
` (2 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: William McVicker @ 2024-02-21 23:13 UTC (permalink / raw
To: Ajay Agarwal
Cc: Jingoo Han, Gustavo Pimentel, Manivannan Sadhasivam,
Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring,
Bjorn Helgaas, Manu Gautam, Sajid Dalvi, Serge Semin,
Robin Murphy, linux-pci, kernel-team
On 02/21/2024, Ajay Agarwal wrote:
> There can be platforms that do not use/have 32-bit DMA addresses.
> The current implementation of 32-bit IOVA allocation can fail for
> such platforms, eventually leading to the probe failure.
>
> Try to allocate a 32-bit msi_data. If this allocation fails,
> attempt a 64-bit address allocation. Please note that if the
> 64-bit MSI address is allocated, then the EPs supporting 32-bit
> MSI address only will not work.
>
> Signed-off-by: Ajay Agarwal <ajayagarwal@google.com>
> ---
> Changelog since v5:
> - Initialize temp variable 'msi_vaddr' to NULL
> - Remove redundant print and check
>
> Changelog since v4:
> - Remove the 'DW_PCIE_CAP_MSI_DATA_SET' flag
> - Refactor the comments and msi_data allocation logic
>
> Changelog since v3:
> - Add a new controller cap flag 'DW_PCIE_CAP_MSI_DATA_SET'
> - Refactor the comments and print statements
>
> Changelog since v2:
> - If the vendor driver has setup the msi_data, use the same
>
> Changelog since v1:
> - Use reserved memory, if it exists, to setup the MSI data
> - Fallback to 64-bit IOVA allocation if 32-bit allocation fails
>
> .../pci/controller/dwc/pcie-designware-host.c | 21 ++++++++++++-------
> 1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index d5fc31f8345f..d15a5c2d5b48 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -328,7 +328,7 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
> struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> struct device *dev = pci->dev;
> struct platform_device *pdev = to_platform_device(dev);
> - u64 *msi_vaddr;
> + u64 *msi_vaddr = NULL;
> int ret;
> u32 ctrl, num_ctrls;
>
> @@ -379,15 +379,20 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
> * memory.
> */
> ret = dma_set_coherent_mask(dev, 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");
> + if (!ret)
> + msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
> + GFP_KERNEL);
>
> - msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
> - GFP_KERNEL);
> if (!msi_vaddr) {
> - dev_err(dev, "Failed to alloc and map MSI data\n");
> - dw_pcie_free_msi(pp);
> - return -ENOMEM;
> + dev_warn(dev, "Failed to allocate 32-bit MSI address\n");
> + dma_set_coherent_mask(dev, DMA_BIT_MASK(64));
> + msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
> + GFP_KERNEL);
> + if (!msi_vaddr) {
> + dev_err(dev, "Failed to allocate MSI address\n");
> + dw_pcie_free_msi(pp);
> + return -ENOMEM;
> + }
> }
>
> return 0;
> --
> 2.44.0.rc0.258.g7320e95886-goog
>
Thanks for working through all the kinks, Ajay! The patch looks good to me.
I tested it on my Pixel 8 with ZONE_DMA32 disabled. I wasn't able to reproduce
the case where there was no 32-bit addresses available on boot, but I did
artificially test it by commenting out the first call to dmam_alloc_coherent()
to exercise the fallback case where msi_vaddr is NULL and the 64-bit coherent
mask is set. In both cases, I verified the PCIe device probes successfully with
this change and wifi works.
Feel free to include,
Reviewed-by: Will McVicker <willmcvicker@google.com>
Tested-by: Will McVicker <willmcvicker@google.com>
Thanks,
Will
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v6] PCI: dwc: Strengthen the MSI address allocation logic
2024-02-21 15:38 [PATCH v6] PCI: dwc: Strengthen the MSI address allocation logic Ajay Agarwal
2024-02-21 17:32 ` Serge Semin
2024-02-21 23:13 ` William McVicker
@ 2024-02-27 14:45 ` Manivannan Sadhasivam
2024-03-10 18:15 ` Krzysztof Wilczyński
2024-03-11 16:58 ` Bjorn Helgaas
4 siblings, 0 replies; 7+ messages in thread
From: Manivannan Sadhasivam @ 2024-02-27 14:45 UTC (permalink / raw
To: Ajay Agarwal
Cc: Jingoo Han, Gustavo Pimentel, Manivannan Sadhasivam,
Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring,
Bjorn Helgaas, Manu Gautam, Sajid Dalvi, William McVicker,
Serge Semin, Robin Murphy, linux-pci
On Wed, Feb 21, 2024 at 09:08:40PM +0530, Ajay Agarwal wrote:
> There can be platforms that do not use/have 32-bit DMA addresses.
> The current implementation of 32-bit IOVA allocation can fail for
> such platforms, eventually leading to the probe failure.
>
> Try to allocate a 32-bit msi_data. If this allocation fails,
> attempt a 64-bit address allocation. Please note that if the
> 64-bit MSI address is allocated, then the EPs supporting 32-bit
> MSI address only will not work.
>
> Signed-off-by: Ajay Agarwal <ajayagarwal@google.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
- Mani
> ---
> Changelog since v5:
> - Initialize temp variable 'msi_vaddr' to NULL
> - Remove redundant print and check
>
> Changelog since v4:
> - Remove the 'DW_PCIE_CAP_MSI_DATA_SET' flag
> - Refactor the comments and msi_data allocation logic
>
> Changelog since v3:
> - Add a new controller cap flag 'DW_PCIE_CAP_MSI_DATA_SET'
> - Refactor the comments and print statements
>
> Changelog since v2:
> - If the vendor driver has setup the msi_data, use the same
>
> Changelog since v1:
> - Use reserved memory, if it exists, to setup the MSI data
> - Fallback to 64-bit IOVA allocation if 32-bit allocation fails
>
> .../pci/controller/dwc/pcie-designware-host.c | 21 ++++++++++++-------
> 1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index d5fc31f8345f..d15a5c2d5b48 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -328,7 +328,7 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
> struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> struct device *dev = pci->dev;
> struct platform_device *pdev = to_platform_device(dev);
> - u64 *msi_vaddr;
> + u64 *msi_vaddr = NULL;
> int ret;
> u32 ctrl, num_ctrls;
>
> @@ -379,15 +379,20 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
> * memory.
> */
> ret = dma_set_coherent_mask(dev, 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");
> + if (!ret)
> + msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
> + GFP_KERNEL);
>
> - msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
> - GFP_KERNEL);
> if (!msi_vaddr) {
> - dev_err(dev, "Failed to alloc and map MSI data\n");
> - dw_pcie_free_msi(pp);
> - return -ENOMEM;
> + dev_warn(dev, "Failed to allocate 32-bit MSI address\n");
> + dma_set_coherent_mask(dev, DMA_BIT_MASK(64));
> + msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
> + GFP_KERNEL);
> + if (!msi_vaddr) {
> + dev_err(dev, "Failed to allocate MSI address\n");
> + dw_pcie_free_msi(pp);
> + return -ENOMEM;
> + }
> }
>
> return 0;
> --
> 2.44.0.rc0.258.g7320e95886-goog
>
>
--
மணிவண்ணன் சதாசிவம்
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v6] PCI: dwc: Strengthen the MSI address allocation logic
2024-02-21 15:38 [PATCH v6] PCI: dwc: Strengthen the MSI address allocation logic Ajay Agarwal
` (2 preceding siblings ...)
2024-02-27 14:45 ` Manivannan Sadhasivam
@ 2024-03-10 18:15 ` Krzysztof Wilczyński
2024-03-11 16:58 ` Bjorn Helgaas
4 siblings, 0 replies; 7+ messages in thread
From: Krzysztof Wilczyński @ 2024-03-10 18:15 UTC (permalink / raw
To: Ajay Agarwal
Cc: Jingoo Han, Gustavo Pimentel, Manivannan Sadhasivam,
Lorenzo Pieralisi, Rob Herring, Bjorn Helgaas, Manu Gautam,
Sajid Dalvi, William McVicker, Serge Semin, Robin Murphy,
linux-pci
Hello,
> There can be platforms that do not use/have 32-bit DMA addresses.
> The current implementation of 32-bit IOVA allocation can fail for
> such platforms, eventually leading to the probe failure.
>
> Try to allocate a 32-bit msi_data. If this allocation fails,
> attempt a 64-bit address allocation. Please note that if the
> 64-bit MSI address is allocated, then the EPs supporting 32-bit
> MSI address only will not work.
Applied to controller/dwc, thank you!
[1/1] PCI: dwc: Strengthen the MSI address allocation logic
https://git.kernel.org/pci/pci/c/f3a296405b6e
Krzysztof
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v6] PCI: dwc: Strengthen the MSI address allocation logic
2024-02-21 15:38 [PATCH v6] PCI: dwc: Strengthen the MSI address allocation logic Ajay Agarwal
` (3 preceding siblings ...)
2024-03-10 18:15 ` Krzysztof Wilczyński
@ 2024-03-11 16:58 ` Bjorn Helgaas
2024-03-12 13:52 ` Robin Murphy
4 siblings, 1 reply; 7+ messages in thread
From: Bjorn Helgaas @ 2024-03-11 16:58 UTC (permalink / raw
To: Ajay Agarwal
Cc: Jingoo Han, Gustavo Pimentel, Manivannan Sadhasivam,
Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring,
Bjorn Helgaas, Manu Gautam, Sajid Dalvi, William McVicker,
Serge Semin, Robin Murphy, linux-pci
On Wed, Feb 21, 2024 at 09:08:40PM +0530, Ajay Agarwal wrote:
> There can be platforms that do not use/have 32-bit DMA addresses.
> The current implementation of 32-bit IOVA allocation can fail for
> such platforms, eventually leading to the probe failure.
>
> Try to allocate a 32-bit msi_data. If this allocation fails,
> attempt a 64-bit address allocation. Please note that if the
> 64-bit MSI address is allocated, then the EPs supporting 32-bit
> MSI address only will not work.
What happens when we fail to allocate a 32-bit address, we allocate a
64-bit address, we have an endpoint that only supports 32-bit
addresses, and the driver tries to enable MSI? Does it fall back to
INTx? Fail the MSI enable? Emit a warning?
> Signed-off-by: Ajay Agarwal <ajayagarwal@google.com>
> ---
> Changelog since v5:
> - Initialize temp variable 'msi_vaddr' to NULL
> - Remove redundant print and check
>
> Changelog since v4:
> - Remove the 'DW_PCIE_CAP_MSI_DATA_SET' flag
> - Refactor the comments and msi_data allocation logic
>
> Changelog since v3:
> - Add a new controller cap flag 'DW_PCIE_CAP_MSI_DATA_SET'
> - Refactor the comments and print statements
>
> Changelog since v2:
> - If the vendor driver has setup the msi_data, use the same
>
> Changelog since v1:
> - Use reserved memory, if it exists, to setup the MSI data
> - Fallback to 64-bit IOVA allocation if 32-bit allocation fails
>
> .../pci/controller/dwc/pcie-designware-host.c | 21 ++++++++++++-------
> 1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index d5fc31f8345f..d15a5c2d5b48 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -328,7 +328,7 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
> struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> struct device *dev = pci->dev;
> struct platform_device *pdev = to_platform_device(dev);
> - u64 *msi_vaddr;
> + u64 *msi_vaddr = NULL;
> int ret;
> u32 ctrl, num_ctrls;
>
> @@ -379,15 +379,20 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
> * memory.
> */
> ret = dma_set_coherent_mask(dev, 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");
> + if (!ret)
> + msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
> + GFP_KERNEL);
>
> - msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
> - GFP_KERNEL);
> if (!msi_vaddr) {
> - dev_err(dev, "Failed to alloc and map MSI data\n");
> - dw_pcie_free_msi(pp);
> - return -ENOMEM;
> + dev_warn(dev, "Failed to allocate 32-bit MSI address\n");
> + dma_set_coherent_mask(dev, DMA_BIT_MASK(64));
> + msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
> + GFP_KERNEL);
> + if (!msi_vaddr) {
> + dev_err(dev, "Failed to allocate MSI address\n");
> + dw_pcie_free_msi(pp);
> + return -ENOMEM;
> + }
> }
>
> return 0;
> --
> 2.44.0.rc0.258.g7320e95886-goog
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v6] PCI: dwc: Strengthen the MSI address allocation logic
2024-03-11 16:58 ` Bjorn Helgaas
@ 2024-03-12 13:52 ` Robin Murphy
0 siblings, 0 replies; 7+ messages in thread
From: Robin Murphy @ 2024-03-12 13:52 UTC (permalink / raw
To: Bjorn Helgaas, Ajay Agarwal
Cc: Jingoo Han, Gustavo Pimentel, Manivannan Sadhasivam,
Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring,
Bjorn Helgaas, Manu Gautam, Sajid Dalvi, William McVicker,
Serge Semin, linux-pci
On 11/03/2024 4:58 pm, Bjorn Helgaas wrote:
> On Wed, Feb 21, 2024 at 09:08:40PM +0530, Ajay Agarwal wrote:
>> There can be platforms that do not use/have 32-bit DMA addresses.
>> The current implementation of 32-bit IOVA allocation can fail for
>> such platforms, eventually leading to the probe failure.
>>
>> Try to allocate a 32-bit msi_data. If this allocation fails,
>> attempt a 64-bit address allocation. Please note that if the
>> 64-bit MSI address is allocated, then the EPs supporting 32-bit
>> MSI address only will not work.
>
> What happens when we fail to allocate a 32-bit address, we allocate a
> 64-bit address, we have an endpoint that only supports 32-bit
> addresses, and the driver tries to enable MSI? Does it fall back to
> INTx? Fail the MSI enable? Emit a warning?
In general __pci_write_msi_msg() will write the address, which will then
be truncated by the device, and when it subsequently tries to signal the
MSI it will go off into the weeds and not raise an interrupt. We'll have
already warned about failing to get a 32-bit MSI address when initially
probing the controller - although apparently now no longer explicitly
calling out the implication for 32-bit devices :( - which seems to be
about as much as we can reasonably do.
I suppose pci_write_msg_msi() could additionally scream if it sees
address_hi being nonzero when the given capability is only 32-bit, but
it still can't fail. Within the current design, I'm not sure we can
reasonably tell that a device is incompatible with the MSI domain until
irq_chip_compose_msi_msg(), by which point it's already way too late...
Thanks,
Robin.
>
>> Signed-off-by: Ajay Agarwal <ajayagarwal@google.com>
>> ---
>> Changelog since v5:
>> - Initialize temp variable 'msi_vaddr' to NULL
>> - Remove redundant print and check
>>
>> Changelog since v4:
>> - Remove the 'DW_PCIE_CAP_MSI_DATA_SET' flag
>> - Refactor the comments and msi_data allocation logic
>>
>> Changelog since v3:
>> - Add a new controller cap flag 'DW_PCIE_CAP_MSI_DATA_SET'
>> - Refactor the comments and print statements
>>
>> Changelog since v2:
>> - If the vendor driver has setup the msi_data, use the same
>>
>> Changelog since v1:
>> - Use reserved memory, if it exists, to setup the MSI data
>> - Fallback to 64-bit IOVA allocation if 32-bit allocation fails
>>
>> .../pci/controller/dwc/pcie-designware-host.c | 21 ++++++++++++-------
>> 1 file changed, 13 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
>> index d5fc31f8345f..d15a5c2d5b48 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>> @@ -328,7 +328,7 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
>> struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> struct device *dev = pci->dev;
>> struct platform_device *pdev = to_platform_device(dev);
>> - u64 *msi_vaddr;
>> + u64 *msi_vaddr = NULL;
>> int ret;
>> u32 ctrl, num_ctrls;
>>
>> @@ -379,15 +379,20 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
>> * memory.
>> */
>> ret = dma_set_coherent_mask(dev, 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");
>> + if (!ret)
>> + msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
>> + GFP_KERNEL);
>>
>> - msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
>> - GFP_KERNEL);
>> if (!msi_vaddr) {
>> - dev_err(dev, "Failed to alloc and map MSI data\n");
>> - dw_pcie_free_msi(pp);
>> - return -ENOMEM;
>> + dev_warn(dev, "Failed to allocate 32-bit MSI address\n");
>> + dma_set_coherent_mask(dev, DMA_BIT_MASK(64));
>> + msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
>> + GFP_KERNEL);
>> + if (!msi_vaddr) {
>> + dev_err(dev, "Failed to allocate MSI address\n");
>> + dw_pcie_free_msi(pp);
>> + return -ENOMEM;
>> + }
>> }
>>
>> return 0;
>> --
>> 2.44.0.rc0.258.g7320e95886-goog
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-03-12 13:52 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-21 15:38 [PATCH v6] PCI: dwc: Strengthen the MSI address allocation logic Ajay Agarwal
2024-02-21 17:32 ` Serge Semin
2024-02-21 23:13 ` William McVicker
2024-02-27 14:45 ` Manivannan Sadhasivam
2024-03-10 18:15 ` Krzysztof Wilczyński
2024-03-11 16:58 ` Bjorn Helgaas
2024-03-12 13:52 ` Robin Murphy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).