| Message ID | 20240221153840.1789979-1-ajayagarwal@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | [v6] PCI: dwc: Strengthen the MSI address allocation logic | expand |
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 >
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
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 > >
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
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 >
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 >>
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;
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(-)