| Message ID | 20240204112425.125627-1-ajayagarwal@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] PCI: dwc: Strengthen the MSI address allocation logic | expand |
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@google.com> > --- > 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 >
On Mon, Feb 05, 2024 at 12:52:45AM +0300, Serge Semin wrote: > 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@google.com> > > --- > > 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. > ACK. > > + * 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. > ACK. > > 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." > ACK. > > + * 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. > ACK. > > */ > > > + 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. > That is a fair point. What do you suggest we do? Shall we define another op `set_msi_data` (like init/msi_init/start_link) and if it is defined by the vendor, then call it? Then vendor has to set the pp->msi_data there? Let me know. > > + 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; As you pointed out already, this looks pretty clumsy. I think we should stick to the more descriptive and readable code that I suggested. > } > } > > 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 > >
On Tue, Feb 06, 2024 at 10:12:44PM +0530, Ajay Agarwal wrote: > On Mon, Feb 05, 2024 at 12:52:45AM +0300, Serge Semin wrote: > > 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@google.com> > > > --- > > > 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. > > > ACK. > > > > + * 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. > > > ACK. > > > > 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." > > > ACK. > > > > + * 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. > > > ACK. > > > > */ > > > > > + 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. > > > That is a fair point. What do you suggest we do? Shall we define another > op `set_msi_data` (like init/msi_init/start_link) and if it is defined > by the vendor, then call it? Then vendor has to set the pp->msi_data > there? Let me know. You can define a new capability flag here drivers/pci/controller/dwc/pcie-designware.h (see DW_PCIE_CAP_* macros) , set it in the glue driver by means of the dw_pcie_cap_set() macro function and instead of checking msi_data value test the flag for being set by dw_pcie_cap_is(). > > > > + 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; > > As you pointed out already, this looks pretty clumsy. I think we should > stick to the more descriptive and readable code that I suggested. I do not know which solution is better really. Both have pros and cons. Let's wait for Bjorn, Mani or Robin opinion about this. -Serge(y) > > > } > > } > > > > 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 > > >
On Tue, Feb 06, 2024 at 07:53:19PM +0300, Serge Semin wrote: > On Tue, Feb 06, 2024 at 10:12:44PM +0530, Ajay Agarwal wrote: > > On Mon, Feb 05, 2024 at 12:52:45AM +0300, Serge Semin wrote: > > > 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@google.com> > > > > --- > > > > 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. > > > > > ACK. > > > > > > + * 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. > > > > > ACK. > > > > > > 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." > > > > > ACK. > > > > > > + * 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. > > > > > ACK. > > > > > > */ > > > > > > > + 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. > > > > > > That is a fair point. What do you suggest we do? Shall we define another > > op `set_msi_data` (like init/msi_init/start_link) and if it is defined > > by the vendor, then call it? Then vendor has to set the pp->msi_data > > there? Let me know. > > You can define a new capability flag here > drivers/pci/controller/dwc/pcie-designware.h (see DW_PCIE_CAP_* macros) > , set it in the glue driver by means of the dw_pcie_cap_set() macro > function and instead of checking msi_data value test the flag for > being set by dw_pcie_cap_is(). > Sure, good suggestion. ACK. > > > > > > + 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; > > > > As you pointed out already, this looks pretty clumsy. I think we should > > stick to the more descriptive and readable code that I suggested. > > I do not know which solution is better really. Both have pros and > cons. Let's wait for Bjorn, Mani or Robin opinion about this. > > -Serge(y) > Bjorn/Mani/Robin, Can you please provide your comment? > > > > > } > > > } > > > > > > 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 > > > >
On 13/02/2024 2:52 am, Ajay Agarwal wrote: > On Tue, Feb 06, 2024 at 07:53:19PM +0300, Serge Semin wrote: >> On Tue, Feb 06, 2024 at 10:12:44PM +0530, Ajay Agarwal wrote: >>> On Mon, Feb 05, 2024 at 12:52:45AM +0300, Serge Semin wrote: >>>> 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@google.com> >>>>> --- >>>>> 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. >>>> >>> ACK. >>> >>>>> + * 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. >>>> >>> ACK. >>> >>>>> 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." >>>> >>> ACK. >>> >>>>> + * 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. >>>> >>> ACK. >>> >>>>> */ >>>> >>>>> + 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. >>>> >> >>> That is a fair point. What do you suggest we do? Shall we define another >>> op `set_msi_data` (like init/msi_init/start_link) and if it is defined >>> by the vendor, then call it? Then vendor has to set the pp->msi_data >>> there? Let me know. >> >> You can define a new capability flag here >> drivers/pci/controller/dwc/pcie-designware.h (see DW_PCIE_CAP_* macros) >> , set it in the glue driver by means of the dw_pcie_cap_set() macro >> function and instead of checking msi_data value test the flag for >> being set by dw_pcie_cap_is(). >> > Sure, good suggestion. ACK. > >>> >>>>> + 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; >>> >>> As you pointed out already, this looks pretty clumsy. I think we should >>> stick to the more descriptive and readable code that I suggested. >> >> I do not know which solution is better really. Both have pros and >> cons. Let's wait for Bjorn, Mani or Robin opinion about this. >> >> -Serge(y) >> > Bjorn/Mani/Robin, > Can you please provide your comment? FWIW I had a go at sketching out what I think I'd do, on top of the v3 patch. Apparently I'm not in a too-clever-for-my-own-good mood today, since what came out seems to have ended up pretty much just simplifying the pre-existing code. I'll leave the choice up to you. Thanks, Robin. ----->8----- diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c index 512eb2d6591f..7b68c65e5d11 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; @@ -387,18 +387,16 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp) 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"); - - msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data, - GFP_KERNEL); + if (!ret) + msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data, + GFP_KERNEL); if (!msi_vaddr) { - dev_warn(dev, "Failed to alloc 32-bit MSI data. Attempting 64-bit now\n"); + dev_warn(dev, "Failed to configure 32-bit MSI address. Devices with only 32-bit MSI support may not work properly\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"); + dev_err(dev, "Failed to configure MSI address\n"); dw_pcie_free_msi(pp); return -ENOMEM; }
On Tue, Feb 13, 2024 at 03:32:14PM +0000, Robin Murphy wrote: > On 13/02/2024 2:52 am, Ajay Agarwal wrote: > > On Tue, Feb 06, 2024 at 07:53:19PM +0300, Serge Semin wrote: > > > On Tue, Feb 06, 2024 at 10:12:44PM +0530, Ajay Agarwal wrote: > > > > On Mon, Feb 05, 2024 at 12:52:45AM +0300, Serge Semin wrote: > > > > > 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@google.com> > > > > > > --- > > > > > > 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. > > > > > > > > > ACK. > > > > > > > > > > + * 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. > > > > > > > > > ACK. > > > > > > > > > > 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." > > > > > > > > > ACK. > > > > > > > > > > + * 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. > > > > > > > > > ACK. > > > > > > > > > > */ > > > > > > > > > > > + 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. > > > > > > > > > > > > That is a fair point. What do you suggest we do? Shall we define another > > > > op `set_msi_data` (like init/msi_init/start_link) and if it is defined > > > > by the vendor, then call it? Then vendor has to set the pp->msi_data > > > > there? Let me know. > > > > > > You can define a new capability flag here > > > drivers/pci/controller/dwc/pcie-designware.h (see DW_PCIE_CAP_* macros) > > > , set it in the glue driver by means of the dw_pcie_cap_set() macro > > > function and instead of checking msi_data value test the flag for > > > being set by dw_pcie_cap_is(). > > > > > Sure, good suggestion. ACK. > > > > > > > > > > > > + 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; > > > > > > > > As you pointed out already, this looks pretty clumsy. I think we should > > > > stick to the more descriptive and readable code that I suggested. > > > > > > I do not know which solution is better really. Both have pros and > > > cons. Let's wait for Bjorn, Mani or Robin opinion about this. > > > > > > -Serge(y) > > > > > Bjorn/Mani/Robin, > > Can you please provide your comment? > > FWIW I had a go at sketching out what I think I'd do, on top of the v3 > patch. Apparently I'm not in a too-clever-for-my-own-good mood today, > since what came out seems to have ended up pretty much just simplifying > the pre-existing code. I'll leave the choice up to you. > > Thanks, > Robin. > > ----->8----- > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > index 512eb2d6591f..7b68c65e5d11 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; > @@ -387,18 +387,16 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp) > 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"); > - > - msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data, > - GFP_KERNEL); > + if (!ret) > + msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data, > + GFP_KERNEL); > if (!msi_vaddr) { > - dev_warn(dev, "Failed to alloc 32-bit MSI data. Attempting 64-bit now\n"); > + dev_warn(dev, "Failed to configure 32-bit MSI address. Devices with only 32-bit MSI support may not work properly\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"); > + dev_err(dev, "Failed to configure MSI address\n"); > dw_pcie_free_msi(pp); > return -ENOMEM; > } Thanks Robin for your suggestion. I will keep my patch but pick up the dev_err prints from your suggestion.
On Tue, Feb 13, 2024 at 08:22:38AM +0530, Ajay Agarwal wrote: > On Tue, Feb 06, 2024 at 07:53:19PM +0300, Serge Semin wrote: > > On Tue, Feb 06, 2024 at 10:12:44PM +0530, Ajay Agarwal wrote: > > > On Mon, Feb 05, 2024 at 12:52:45AM +0300, Serge Semin wrote: > > > > 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@google.com> > > > > > --- > > > > > 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. > > > > > > > ACK. > > > > > > > > + * 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. > > > > > > > ACK. > > > > > > > > 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." > > > > > > > ACK. > > > > > > > > + * 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. > > > > > > > ACK. > > > > > > > > */ > > > > > > > > > + 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. > > > > > > > > > That is a fair point. What do you suggest we do? Shall we define another > > > op `set_msi_data` (like init/msi_init/start_link) and if it is defined > > > by the vendor, then call it? Then vendor has to set the pp->msi_data > > > there? Let me know. > > > > You can define a new capability flag here > > drivers/pci/controller/dwc/pcie-designware.h (see DW_PCIE_CAP_* macros) > > , set it in the glue driver by means of the dw_pcie_cap_set() macro > > function and instead of checking msi_data value test the flag for > > being set by dw_pcie_cap_is(). > > > Sure, good suggestion. ACK. > > > > > > > > > + 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; > > > > > > As you pointed out already, this looks pretty clumsy. I think we should > > > stick to the more descriptive and readable code that I suggested. > > > > I do not know which solution is better really. Both have pros and > > cons. Let's wait for Bjorn, Mani or Robin opinion about this. > > > > -Serge(y) > > > Bjorn/Mani/Robin, > Can you please provide your comment? > Sergey's suggestion masks out the error coming from "dma_set_coherent_mask()" by using the same error log. This is equivalent to printing the same error log for both API failures: ret = dma_set_coherent_mask() if (ret) dev_warn(dev, "Failed to allocate 32-bit MSI target address\n"); ret = dmam_alloc_coherent() if (ret) dev_warn(dev, "Failed to allocate 32-bit MSI target address\n"); Which doesn't look correct to me. So let's stick to what Ajay had initially. - Mani
On 2024-02-14 6:55 am, Manivannan Sadhasivam wrote: > On Tue, Feb 13, 2024 at 08:22:38AM +0530, Ajay Agarwal wrote: >> On Tue, Feb 06, 2024 at 07:53:19PM +0300, Serge Semin wrote: >>> On Tue, Feb 06, 2024 at 10:12:44PM +0530, Ajay Agarwal wrote: >>>> On Mon, Feb 05, 2024 at 12:52:45AM +0300, Serge Semin wrote: >>>>> 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@google.com> >>>>>> --- >>>>>> 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. >>>>> >>>> ACK. >>>> >>>>>> + * 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. >>>>> >>>> ACK. >>>> >>>>>> 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." >>>>> >>>> ACK. >>>> >>>>>> + * 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. >>>>> >>>> ACK. >>>> >>>>>> */ >>>>> >>>>>> + 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. >>>>> >>> >>>> That is a fair point. What do you suggest we do? Shall we define another >>>> op `set_msi_data` (like init/msi_init/start_link) and if it is defined >>>> by the vendor, then call it? Then vendor has to set the pp->msi_data >>>> there? Let me know. >>> >>> You can define a new capability flag here >>> drivers/pci/controller/dwc/pcie-designware.h (see DW_PCIE_CAP_* macros) >>> , set it in the glue driver by means of the dw_pcie_cap_set() macro >>> function and instead of checking msi_data value test the flag for >>> being set by dw_pcie_cap_is(). >>> >> Sure, good suggestion. ACK. >> >>>> >>>>>> + 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; >>>> >>>> As you pointed out already, this looks pretty clumsy. I think we should >>>> stick to the more descriptive and readable code that I suggested. >>> >>> I do not know which solution is better really. Both have pros and >>> cons. Let's wait for Bjorn, Mani or Robin opinion about this. >>> >>> -Serge(y) >>> >> Bjorn/Mani/Robin, >> Can you please provide your comment? >> > > Sergey's suggestion masks out the error coming from "dma_set_coherent_mask()" by > using the same error log. This is equivalent to printing the same error log for > both API failures: > > ret = dma_set_coherent_mask() > if (ret) > dev_warn(dev, "Failed to allocate 32-bit MSI target address\n"); > > ret = dmam_alloc_coherent() > if (ret) > dev_warn(dev, "Failed to allocate 32-bit MSI target address\n"); > > Which doesn't look correct to me. So let's stick to what Ajay had initially. My approach did the same, and it was intentional - the significant information for the user is that some devices may go wrong because we couldn't support 32-bit MSIs; the intimate details of exactly *why* we couldn't support 32-bit MSIs don't really matter, and are unlikely to be actionable. Furthermore, setting a 32-bit DMA mask should never fail on any reasonable platform where this driver would actually run, so there's no practical value in anticipating it - handling that particular return value is really just a matter of logical correctness. Thanks, Robin.
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. + * Check if the vendor driver has setup the MSI address already. If yes, + * pick up the same. 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. + * 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. */ + if (pp->msi_data) + 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; + } } return 0;
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@google.com> --- 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(-)