| Message ID | 20240221064846.3798047-1-ajayagarwal@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v5] PCI: dwc: Strengthen the MSI address allocation logic | expand |
On Wed, Feb 21, 2024 at 12:18:46PM +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> > --- > 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, 14 insertions(+), 7 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > index d5fc31f8345f..9c905e5c4904 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > @@ -379,15 +379,22 @@ 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 will be left uninitialized if the mask setting fails. Robin had it initialized in v2 discussion: https://lore.kernel.org/linux-pci/7cd42851-37cc-49d6-b9ad-74a256a73904@arm.com/ > > - 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 configure 32-bit MSI address. Devices with only 32-bit MSI support may not work properly\n"); Can we short it up already? I suggested something like "Failed to allocate 32-bit MSI target address" in v2. It means the problem with 32-bit MSIs which can be perceived as the possible reason of the respective peripheral devices not working correctly. > + ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(64)); > + if (!ret) Redundant check. It was discussed some time ago. See the comment from Robin: https://lore.kernel.org/linux-pci/335d730d-529e-7ce0-8bac-008a4daa6e3c@arm.com/ -Serge(y) > + msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data, > + GFP_KERNEL); > + > + if (!msi_vaddr) { > + dev_err(dev, "Failed to configure MSI address\n"); > + dw_pcie_free_msi(pp); > + return -ENOMEM; > + } > } > > return 0; > -- > 2.44.0.rc0.258.g7320e95886-goog >
On Wed, Feb 21, 2024 at 03:52:43PM +0300, Serge Semin wrote: > On Wed, Feb 21, 2024 at 12:18:46PM +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> > > --- > > 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, 14 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > > index d5fc31f8345f..9c905e5c4904 100644 > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > > @@ -379,15 +379,22 @@ 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 will be left uninitialized if the mask setting fails. Robin > had it initialized in v2 discussion: > https://lore.kernel.org/linux-pci/7cd42851-37cc-49d6-b9ad-74a256a73904@arm.com/ > ACK. Will initialize to NULL in the next version. > > > > > - 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 configure 32-bit MSI address. Devices with only 32-bit MSI support may not work properly\n"); > > Can we short it up already? I suggested something like "Failed to > allocate 32-bit MSI target address" in v2. It means the problem with > 32-bit MSIs which can be perceived as the possible reason of the > respective peripheral devices not working correctly. > ACK. Will shorten the message in the next version. > > + ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(64)); > > + if (!ret) > > Redundant check. It was discussed some time ago. See the comment from > Robin: > https://lore.kernel.org/linux-pci/335d730d-529e-7ce0-8bac-008a4daa6e3c@arm.com/ > ACK. Will remove the check in the next version. > -Serge(y) > > > + msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data, > > + GFP_KERNEL); > > + > > + if (!msi_vaddr) { > > + dev_err(dev, "Failed to configure 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..9c905e5c4904 100644 --- a/drivers/pci/controller/dwc/pcie-designware-host.c +++ b/drivers/pci/controller/dwc/pcie-designware-host.c @@ -379,15 +379,22 @@ 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 configure 32-bit MSI address. Devices with only 32-bit MSI support may not work properly\n"); + ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(64)); + if (!ret) + msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data, + GFP_KERNEL); + + if (!msi_vaddr) { + dev_err(dev, "Failed to configure 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 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, 14 insertions(+), 7 deletions(-)