On Sun, Feb 04, 2024 at 04:54:25PM +0530, Ajay Agarwal wrote:
> There can be platforms that do not use/have 32-bit DMA addresses
> but want to enumerate endpoints which support only 32-bit MSI
> address. The current implementation of 32-bit IOVA allocation can
> fail for such platforms, eventually leading to the probe failure.
>
> If there vendor driver has already setup the MSI address using
> some mechanism, use the same. This method can be used by the
> platforms described above to support EPs they wish to.
>
> Else, if the memory region is not reserved, try to allocate a
> 32-bit IOVA. Additionally, if this allocation also fails, attempt
> a 64-bit allocation for probe to be successful. If the 64-bit MSI
> address is allocated, then the EPs supporting 32-bit MSI address
> will not work.
>
> Signed-off-by: Ajay Agarwal <ajayagarwal@xxxxxxxxxx>
> ---
> 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 | 26 ++++++++++++++-----
> 1 file changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index d5fc31f8345f..512eb2d6591f 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -374,10 +374,18 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
> * order not to miss MSI TLPs from those devices the MSI target
> * address has to be within the lowest 4GB.
> *
> - * Note until there is a better alternative found the reservation is
> - * done by allocating from the artificially limited DMA-coherent
> - * memory.
Why do you keep deleting this statement? The driver still uses the
DMA-coherent memory as a workaround. Your solution doesn't solve the
problem completely. This is another workaround. One more time: the
correct solution would be to allocate a 32-bit address or some range
within the 4GB PCIe bus memory with no _RAM_ or some other IO behind.
Your solution relies on the platform firmware/glue-driver doing that,
which isn't universally applicable. So please don't drop the comment.
> + * Check if the vendor driver has setup the MSI address already. If yes,
> + * pick up the same.
This is inferred from the code below. So drop it.
> This will be helpful for platforms that do not
> + * use/have 32-bit DMA addresses but want to use endpoints which support
> + * only 32-bit MSI address.
Please merge it into the first part of the comment as like: "Permit
the platforms to override the MSI target address if they have a free
PCIe-bus memory specifically reserved for that."
> + * Else, if the memory region is not reserved, try to allocate a 32-bit
> + * IOVA. Additionally, if this allocation also fails, attempt a 64-bit
> + * allocation. If the 64-bit MSI address is allocated, then the EPs
> + * supporting 32-bit MSI address will not work.
This is easily inferred from the code below. So drop it.
> */
> + if (pp->msi_data)
Note this is a physical address for which even zero value might be
valid. In this case it's the address of the PCIe bus space for which
AFAICS zero isn't reserved for something special.
> + return 0;
> +
> 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");
> @@ -385,9 +393,15 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
> 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 alloc 32-bit MSI data. Attempting 64-bit now\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 alloc and map MSI data\n");
> + dw_pcie_free_msi(pp);
> + return -ENOMEM;
> + }
On Tue, Jan 30, 2024 at 08:40:48PM +0000, Robin Murphy wrote:
> Yeah, something like that. Personally I'd still be tempted to try some
> mildly more involved logic to just have a single dev_warn(), but I think
> that's less important than just having something which clearly works.
I guess this can be done but in a bit clumsy way. Like this:
ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(32)) ||
!dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data, GFP_KERNEL);
if (ret) {
dev_warn(dev, "Failed to allocate 32-bit MSI target address\n");
dma_set_coherent_mask(dev, DMA_BIT_MASK(64));
ret = !dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data, GFP_KERNEL);
if (ret) {
dev_err(dev, "Failed to allocate MSI target address\n");
return -ENOMEM;
}
}
Not sure whether it's much better than what Ajay suggested but at
least it has a single warning string describing the error, and we can
drop the unused msi_vaddr variable.
-Serge(y)
> }
>
> return 0;
> --
> 2.43.0.594.gd9cf4e227d-goog
>
