[v5,1/2] PCI: dwc: Drop dependency on ZONE_DMA32

Commit Message

William McVicker Aug. 25, 2022, 6:50 p.m. UTC

Re-work the msi_msg DMA allocation logic to use dmam_alloc_coherent() which
uses the coherent DMA mask to try to return an allocation within the DMA
mask limits. With that, we now can drop the msi_page parameter in struct
dw_pcie_rp. This allows kernel configurations that disable ZONE_DMA32 to
continue supporting a 32-bit DMA mask. Without this patch, the PCIe host
device will fail to probe when ZONE_DMA32 is disabled.

Fixes: 35797e672ff0 ("PCI: dwc: Fix MSI msi_msg DMA mapping")
Reported-by: Isaac J. Manjarres <[e-mail]>
Signed-off-by: Will McVicker <[e-mail]>
Acked-by: Jingoo Han <[e-mail]>
Reviewed-by: Rob Herring <[e-mail]>
---
 .../pci/controller/dwc/pcie-designware-host.c | 28 +++++--------------
 drivers/pci/controller/dwc/pcie-designware.h  |  1 -
 2 files changed, 7 insertions(+), 22 deletions(-)

Comments

Serge Semin Sept. 28, 2022, 11:41 a.m. UTC | #1

On Thu, Aug 25, 2022 at 06:50:24PM +0000, Will McVicker wrote:
> Re-work the msi_msg DMA allocation logic to use dmam_alloc_coherent() which
> uses the coherent DMA mask to try to return an allocation within the DMA
> mask limits. With that, we now can drop the msi_page parameter in struct
> dw_pcie_rp. This allows kernel configurations that disable ZONE_DMA32 to
> continue supporting a 32-bit DMA mask. Without this patch, the PCIe host
> device will fail to probe when ZONE_DMA32 is disabled.

As Rob already said here
https://lore.kernel.org/all/CAL_JsqJh=d-B51b6yPBRq0tOwbChN=[e-mail]/
and I mentioned in this thread
https://lore.kernel.org/linux-pci/20220912000211.ct6asuhhmnatje5e@mobilestation/
DW PCIe MSI doesn't cause any DMA due to the way the iMSI-RX engine is
designed. So reserving any real system memory is a waste of one in
this case. Reserving DMA-coherent even more inappropriate since it
can be expensive on some platforms (see note in Part Ia of
Documentation/core-api/dma-api.rst). For instance on MIPS32 with
non-corehent common DMA.

> 
> Fixes: 35797e672ff0 ("PCI: dwc: Fix MSI msi_msg DMA mapping")
> Reported-by: Isaac J. Manjarres <[e-mail]>
> Signed-off-by: Will McVicker <[e-mail]>
> Acked-by: Jingoo Han <[e-mail]>
> Reviewed-by: Rob Herring <[e-mail]>
> ---
>  .../pci/controller/dwc/pcie-designware-host.c | 28 +++++--------------
>  drivers/pci/controller/dwc/pcie-designware.h  |  1 -
>  2 files changed, 7 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 7746f94a715f..39f3b37d4033 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -267,15 +267,6 @@ static void dw_pcie_free_msi(struct dw_pcie_rp *pp)
>  
>  	irq_domain_remove(pp->msi_domain);
>  	irq_domain_remove(pp->irq_domain);
> -
> -	if (pp->msi_data) {
> -		struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> -		struct device *dev = pci->dev;
> -
> -		dma_unmap_page(dev, pp->msi_data, PAGE_SIZE, DMA_FROM_DEVICE);
> -		if (pp->msi_page)
> -			__free_page(pp->msi_page);
> -	}
>  }
>  
>  static void dw_pcie_msi_init(struct dw_pcie_rp *pp)
> @@ -336,6 +327,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;
>  	int ret;
>  	u32 ctrl, num_ctrls;
>  
> @@ -375,22 +367,16 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
>  						    dw_chained_msi_isr, pp);
>  	}
>  

> -	ret = dma_set_mask(dev, DMA_BIT_MASK(32));
> +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));

This has been redundant in the first place since none of the DW PCIe
low-level drivers update the mask, and it's of 32-bits wide by default
anyway:
https://elixir.bootlin.com/linux/latest/source/drivers/of/platform.c#L167

>  	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");
>  
> -	pp->msi_page = alloc_page(GFP_DMA32);
> -	pp->msi_data = dma_map_page(dev, pp->msi_page, 0,
> -				    PAGE_SIZE, DMA_FROM_DEVICE);
> -	ret = dma_mapping_error(dev, pp->msi_data);
> -	if (ret) {
> -		dev_err(pci->dev, "Failed to map MSI data\n");
> -		__free_page(pp->msi_page);
> -		pp->msi_page = NULL;
> -		pp->msi_data = 0;
> +	msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
> +					GFP_KERNEL);

Changing the whole device DMA-mask due to something that doesn't
perform seems inappropriate. I'd suggest to preserve the ZONE_DMA32
here until there is something like suggested by @Robin
https://lore.kernel.org/linux-pci/[e-mail]/
in the last paragraph is implemented. Especially seeing there still
common drivers in kernel which still rely on that zone.

-Sergey

> +	if (!msi_vaddr) {
> +		dev_err(dev, "Failed to alloc and map MSI data\n");
>  		dw_pcie_free_msi(pp);
> -
> -		return ret;
> +		return -ENOMEM;
>  	}
>  
>  	return 0;
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 09b887093a84..a871ae7eb59e 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -243,7 +243,6 @@ struct dw_pcie_rp {
>  	struct irq_domain	*irq_domain;
>  	struct irq_domain	*msi_domain;
>  	dma_addr_t		msi_data;
> -	struct page		*msi_page;
>  	struct irq_chip		*msi_irq_chip;
>  	u32			num_vectors;
>  	u32			irq_mask[MAX_MSI_CTRLS];
> -- 
> 2.37.2.672.g94769d06f0-goog
> 
>

Robin Murphy Sept. 29, 2022, 6:25 p.m. UTC | #2

On 2022-09-28 12:41, Serge Semin wrote:
> On Thu, Aug 25, 2022 at 06:50:24PM +0000, Will McVicker wrote:
>> Re-work the msi_msg DMA allocation logic to use dmam_alloc_coherent() which
>> uses the coherent DMA mask to try to return an allocation within the DMA
>> mask limits. With that, we now can drop the msi_page parameter in struct
>> dw_pcie_rp. This allows kernel configurations that disable ZONE_DMA32 to
>> continue supporting a 32-bit DMA mask. Without this patch, the PCIe host
>> device will fail to probe when ZONE_DMA32 is disabled.
> 
> As Rob already said here
> https://lore.kernel.org/all/CAL_JsqJh=d-B51b6yPBRq0tOwbChN=[e-mail]/
> and I mentioned in this thread
> https://lore.kernel.org/linux-pci/20220912000211.ct6asuhhmnatje5e@mobilestation/
> DW PCIe MSI doesn't cause any DMA due to the way the iMSI-RX engine is
> designed. So reserving any real system memory is a waste of one in
> this case. Reserving DMA-coherent even more inappropriate since it
> can be expensive on some platforms (see note in Part Ia of
> Documentation/core-api/dma-api.rst). For instance on MIPS32 with
> non-corehent common DMA.

This has been discussed before - in general it is difficult to pick an 
arbitrary MSI address that is *guaranteed* not to overlap any valid DMA 
address that somebody may try to use later. However there is a very easy 
way to guarantee that the DMA API won't give anyone a particular DMA 
address, which is to get an address directly from the DMA API and keep 
it. Yes, that can technically be done with a streaming mapping *if* you 
already have some memory allocated in a suitable physical location, but 
coherent allocations are even more foolproof, simpler to clean up 
(particularly with devres), and unlikely to be an issue on relevant 
platforms (do any MIPS32 systems use this driver?)

>> Fixes: 35797e672ff0 ("PCI: dwc: Fix MSI msi_msg DMA mapping")
>> Reported-by: Isaac J. Manjarres <[e-mail]>
>> Signed-off-by: Will McVicker <[e-mail]>
>> Acked-by: Jingoo Han <[e-mail]>
>> Reviewed-by: Rob Herring <[e-mail]>
>> ---
>>   .../pci/controller/dwc/pcie-designware-host.c | 28 +++++--------------
>>   drivers/pci/controller/dwc/pcie-designware.h  |  1 -
>>   2 files changed, 7 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
>> index 7746f94a715f..39f3b37d4033 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>> @@ -267,15 +267,6 @@ static void dw_pcie_free_msi(struct dw_pcie_rp *pp)
>>   
>>   	irq_domain_remove(pp->msi_domain);
>>   	irq_domain_remove(pp->irq_domain);
>> -
>> -	if (pp->msi_data) {
>> -		struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> -		struct device *dev = pci->dev;
>> -
>> -		dma_unmap_page(dev, pp->msi_data, PAGE_SIZE, DMA_FROM_DEVICE);
>> -		if (pp->msi_page)
>> -			__free_page(pp->msi_page);
>> -	}
>>   }
>>   
>>   static void dw_pcie_msi_init(struct dw_pcie_rp *pp)
>> @@ -336,6 +327,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;
>>   	int ret;
>>   	u32 ctrl, num_ctrls;
>>   
>> @@ -375,22 +367,16 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
>>   						    dw_chained_msi_isr, pp);
>>   	}
>>   
> 
>> -	ret = dma_set_mask(dev, DMA_BIT_MASK(32));
>> +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
> 
> This has been redundant in the first place since none of the DW PCIe
> low-level drivers update the mask, and it's of 32-bits wide by default
> anyway:
> https://elixir.bootlin.com/linux/latest/source/drivers/of/platform.c#L167

No, in general drivers should always explicitly set their mask(s) and 
check the return value to make sure DMA is possible at all before trying 
any other DMA API calls. There's no guarantee that the default mask is 
usable (e.g. some systems don't have any 32-bit addressable RAM), or 
that it's even always 32 bits (due to crufty reasons of something 
of_dma_configure() tried to do a long time ago).

Thanks,
Robin.

>>   	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");
>>   
>> -	pp->msi_page = alloc_page(GFP_DMA32);
>> -	pp->msi_data = dma_map_page(dev, pp->msi_page, 0,
>> -				    PAGE_SIZE, DMA_FROM_DEVICE);
>> -	ret = dma_mapping_error(dev, pp->msi_data);
>> -	if (ret) {
>> -		dev_err(pci->dev, "Failed to map MSI data\n");
>> -		__free_page(pp->msi_page);
>> -		pp->msi_page = NULL;
>> -		pp->msi_data = 0;
>> +	msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
>> +					GFP_KERNEL);
> 
> Changing the whole device DMA-mask due to something that doesn't
> perform seems inappropriate. I'd suggest to preserve the ZONE_DMA32
> here until there is something like suggested by @Robin
> https://lore.kernel.org/linux-pci/[e-mail]/
> in the last paragraph is implemented. Especially seeing there still
> common drivers in kernel which still rely on that zone.
> 
> -Sergey
> 
>> +	if (!msi_vaddr) {
>> +		dev_err(dev, "Failed to alloc and map MSI data\n");
>>   		dw_pcie_free_msi(pp);
>> -
>> -		return ret;
>> +		return -ENOMEM;
>>   	}
>>   
>>   	return 0;
>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
>> index 09b887093a84..a871ae7eb59e 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware.h
>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
>> @@ -243,7 +243,6 @@ struct dw_pcie_rp {
>>   	struct irq_domain	*irq_domain;
>>   	struct irq_domain	*msi_domain;
>>   	dma_addr_t		msi_data;
>> -	struct page		*msi_page;
>>   	struct irq_chip		*msi_irq_chip;
>>   	u32			num_vectors;
>>   	u32			irq_mask[MAX_MSI_CTRLS];
>> -- 
>> 2.37.2.672.g94769d06f0-goog
>>
>>

Serge Semin Sept. 29, 2022, 7:32 p.m. UTC | #3

On Thu, Sep 29, 2022 at 07:25:03PM +0100, Robin Murphy wrote:
> On 2022-09-28 12:41, Serge Semin wrote:
> > On Thu, Aug 25, 2022 at 06:50:24PM +0000, Will McVicker wrote:
> > > Re-work the msi_msg DMA allocation logic to use dmam_alloc_coherent() which
> > > uses the coherent DMA mask to try to return an allocation within the DMA
> > > mask limits. With that, we now can drop the msi_page parameter in struct
> > > dw_pcie_rp. This allows kernel configurations that disable ZONE_DMA32 to
> > > continue supporting a 32-bit DMA mask. Without this patch, the PCIe host
> > > device will fail to probe when ZONE_DMA32 is disabled.
> > 
> > As Rob already said here
> > https://lore.kernel.org/all/CAL_JsqJh=d-B51b6yPBRq0tOwbChN=[e-mail]/
> > and I mentioned in this thread
> > https://lore.kernel.org/linux-pci/20220912000211.ct6asuhhmnatje5e@mobilestation/
> > DW PCIe MSI doesn't cause any DMA due to the way the iMSI-RX engine is
> > designed. So reserving any real system memory is a waste of one in
> > this case. Reserving DMA-coherent even more inappropriate since it
> > can be expensive on some platforms (see note in Part Ia of
> > Documentation/core-api/dma-api.rst). For instance on MIPS32 with
> > non-corehent common DMA.
> 

> This has been discussed before - in general it is difficult to pick an
> arbitrary MSI address that is *guaranteed* not to overlap any valid DMA
> address that somebody may try to use later. However there is a very easy way
> to guarantee that the DMA API won't give anyone a particular DMA address,
> which is to get an address directly from the DMA API and keep it. Yes, that
> can technically be done with a streaming mapping *if* you already have some
> memory allocated in a suitable physical location, but coherent allocations
> are even more foolproof, simpler to clean up (particularly with devres), and
> unlikely to be an issue on relevant platforms (do any MIPS32 systems use
> this driver?)

My patchset adds the DW PCIe RP controller support on MIPS32 arch:
https://lore.kernel.org/linux-pci/[e-mail]/

> 
> > > Fixes: 35797e672ff0 ("PCI: dwc: Fix MSI msi_msg DMA mapping")
> > > Reported-by: Isaac J. Manjarres <[e-mail]>
> > > Signed-off-by: Will McVicker <[e-mail]>
> > > Acked-by: Jingoo Han <[e-mail]>
> > > Reviewed-by: Rob Herring <[e-mail]>
> > > ---
> > >   .../pci/controller/dwc/pcie-designware-host.c | 28 +++++--------------
> > >   drivers/pci/controller/dwc/pcie-designware.h  |  1 -
> > >   2 files changed, 7 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > index 7746f94a715f..39f3b37d4033 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > @@ -267,15 +267,6 @@ static void dw_pcie_free_msi(struct dw_pcie_rp *pp)
> > >   	irq_domain_remove(pp->msi_domain);
> > >   	irq_domain_remove(pp->irq_domain);
> > > -
> > > -	if (pp->msi_data) {
> > > -		struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > -		struct device *dev = pci->dev;
> > > -
> > > -		dma_unmap_page(dev, pp->msi_data, PAGE_SIZE, DMA_FROM_DEVICE);
> > > -		if (pp->msi_page)
> > > -			__free_page(pp->msi_page);
> > > -	}
> > >   }
> > >   static void dw_pcie_msi_init(struct dw_pcie_rp *pp)
> > > @@ -336,6 +327,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;
> > >   	int ret;
> > >   	u32 ctrl, num_ctrls;
> > > @@ -375,22 +367,16 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
> > >   						    dw_chained_msi_isr, pp);
> > >   	}
> > 
> > > -	ret = dma_set_mask(dev, DMA_BIT_MASK(32));
> > > +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
> > 
> > This has been redundant in the first place since none of the DW PCIe
> > low-level drivers update the mask, and it's of 32-bits wide by default
> > anyway:
> > https://elixir.bootlin.com/linux/latest/source/drivers/of/platform.c#L167
> 

> No, in general drivers should always explicitly set their mask(s) and check
> the return value to make sure DMA is possible at all before trying any other
> DMA API calls. There's no guarantee that the default mask is usable (e.g.
> some systems don't have any 32-bit addressable RAM), or that it's even
> always 32 bits (due to crufty reasons of something of_dma_configure() tried
> to do a long time ago).

Suppose you are right and DMA-mask should be always set before any
mapping. What do you suggest to do in this case? (1) The code above
overrides the real DMA-mask which could be set by the platform
drivers, which in its turn are normally aware of the device DMA
capabilities. But in this case due to override afterwards any buffers
above 4GB mapping will cause using the bounce buffers. (2) It's set
here for something which isn't actual DMA. So to speak on one side is
this patchset which overrides the mask for something which isn't DMA,
and there are another patchsets:
https://lore.kernel.org/linux-pci/[e-mail]/
and
https://lore.kernel.org/linux-pci/[e-mail]/
which add the real DMA support to DW PCIe driver and for which setting
the real DMA-mask is crucial. What do you suggest? Setting the mask
twice: before allocating MSI-buffer and afterwards for the sake of
eDMA buffers mapping? Moving DMA-mask setting from the generic DW PCIe
code to the platform drivers?

-Sergey

> 
> Thanks,
> Robin.
> 
> > >   	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");
> > > -	pp->msi_page = alloc_page(GFP_DMA32);
> > > -	pp->msi_data = dma_map_page(dev, pp->msi_page, 0,
> > > -				    PAGE_SIZE, DMA_FROM_DEVICE);
> > > -	ret = dma_mapping_error(dev, pp->msi_data);
> > > -	if (ret) {
> > > -		dev_err(pci->dev, "Failed to map MSI data\n");
> > > -		__free_page(pp->msi_page);
> > > -		pp->msi_page = NULL;
> > > -		pp->msi_data = 0;
> > > +	msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
> > > +					GFP_KERNEL);
> > 
> > Changing the whole device DMA-mask due to something that doesn't
> > perform seems inappropriate. I'd suggest to preserve the ZONE_DMA32
> > here until there is something like suggested by @Robin
> > https://lore.kernel.org/linux-pci/[e-mail]/
> > in the last paragraph is implemented. Especially seeing there still
> > common drivers in kernel which still rely on that zone.
> > 
> > -Sergey
> > 
> > > +	if (!msi_vaddr) {
> > > +		dev_err(dev, "Failed to alloc and map MSI data\n");
> > >   		dw_pcie_free_msi(pp);
> > > -
> > > -		return ret;
> > > +		return -ENOMEM;
> > >   	}
> > >   	return 0;
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > > index 09b887093a84..a871ae7eb59e 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > > @@ -243,7 +243,6 @@ struct dw_pcie_rp {
> > >   	struct irq_domain	*irq_domain;
> > >   	struct irq_domain	*msi_domain;
> > >   	dma_addr_t		msi_data;
> > > -	struct page		*msi_page;
> > >   	struct irq_chip		*msi_irq_chip;
> > >   	u32			num_vectors;
> > >   	u32			irq_mask[MAX_MSI_CTRLS];
> > > -- 
> > > 2.37.2.672.g94769d06f0-goog
> > > 
> > >

Robin Murphy Sept. 30, 2022, 11:01 a.m. UTC | #4

On 2022-09-29 20:32, Serge Semin wrote:
> On Thu, Sep 29, 2022 at 07:25:03PM +0100, Robin Murphy wrote:
>> On 2022-09-28 12:41, Serge Semin wrote:
>>> On Thu, Aug 25, 2022 at 06:50:24PM +0000, Will McVicker wrote:
>>>> Re-work the msi_msg DMA allocation logic to use dmam_alloc_coherent() which
>>>> uses the coherent DMA mask to try to return an allocation within the DMA
>>>> mask limits. With that, we now can drop the msi_page parameter in struct
>>>> dw_pcie_rp. This allows kernel configurations that disable ZONE_DMA32 to
>>>> continue supporting a 32-bit DMA mask. Without this patch, the PCIe host
>>>> device will fail to probe when ZONE_DMA32 is disabled.
>>>
>>> As Rob already said here
>>> https://lore.kernel.org/all/CAL_JsqJh=d-B51b6yPBRq0tOwbChN=[e-mail]/
>>> and I mentioned in this thread
>>> https://lore.kernel.org/linux-pci/20220912000211.ct6asuhhmnatje5e@mobilestation/
>>> DW PCIe MSI doesn't cause any DMA due to the way the iMSI-RX engine is
>>> designed. So reserving any real system memory is a waste of one in
>>> this case. Reserving DMA-coherent even more inappropriate since it
>>> can be expensive on some platforms (see note in Part Ia of
>>> Documentation/core-api/dma-api.rst). For instance on MIPS32 with
>>> non-corehent common DMA.
>>
> 
>> This has been discussed before - in general it is difficult to pick an
>> arbitrary MSI address that is *guaranteed* not to overlap any valid DMA
>> address that somebody may try to use later. However there is a very easy way
>> to guarantee that the DMA API won't give anyone a particular DMA address,
>> which is to get an address directly from the DMA API and keep it. Yes, that
>> can technically be done with a streaming mapping *if* you already have some
>> memory allocated in a suitable physical location, but coherent allocations
>> are even more foolproof, simpler to clean up (particularly with devres), and
>> unlikely to be an issue on relevant platforms (do any MIPS32 systems use
>> this driver?)
> 
> My patchset adds the DW PCIe RP controller support on MIPS32 arch:
> https://lore.kernel.org/linux-pci/[e-mail]/
> 
>>
>>>> Fixes: 35797e672ff0 ("PCI: dwc: Fix MSI msi_msg DMA mapping")
>>>> Reported-by: Isaac J. Manjarres <[e-mail]>
>>>> Signed-off-by: Will McVicker <[e-mail]>
>>>> Acked-by: Jingoo Han <[e-mail]>
>>>> Reviewed-by: Rob Herring <[e-mail]>
>>>> ---
>>>>    .../pci/controller/dwc/pcie-designware-host.c | 28 +++++--------------
>>>>    drivers/pci/controller/dwc/pcie-designware.h  |  1 -
>>>>    2 files changed, 7 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
>>>> index 7746f94a715f..39f3b37d4033 100644
>>>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>>>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>>>> @@ -267,15 +267,6 @@ static void dw_pcie_free_msi(struct dw_pcie_rp *pp)
>>>>    	irq_domain_remove(pp->msi_domain);
>>>>    	irq_domain_remove(pp->irq_domain);
>>>> -
>>>> -	if (pp->msi_data) {
>>>> -		struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>>> -		struct device *dev = pci->dev;
>>>> -
>>>> -		dma_unmap_page(dev, pp->msi_data, PAGE_SIZE, DMA_FROM_DEVICE);
>>>> -		if (pp->msi_page)
>>>> -			__free_page(pp->msi_page);
>>>> -	}
>>>>    }
>>>>    static void dw_pcie_msi_init(struct dw_pcie_rp *pp)
>>>> @@ -336,6 +327,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;
>>>>    	int ret;
>>>>    	u32 ctrl, num_ctrls;
>>>> @@ -375,22 +367,16 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
>>>>    						    dw_chained_msi_isr, pp);
>>>>    	}
>>>
>>>> -	ret = dma_set_mask(dev, DMA_BIT_MASK(32));
>>>> +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
>>>
>>> This has been redundant in the first place since none of the DW PCIe
>>> low-level drivers update the mask, and it's of 32-bits wide by default
>>> anyway:
>>> https://elixir.bootlin.com/linux/latest/source/drivers/of/platform.c#L167
>>
> 
>> No, in general drivers should always explicitly set their mask(s) and check
>> the return value to make sure DMA is possible at all before trying any other
>> DMA API calls. There's no guarantee that the default mask is usable (e.g.
>> some systems don't have any 32-bit addressable RAM), or that it's even
>> always 32 bits (due to crufty reasons of something of_dma_configure() tried
>> to do a long time ago).
> 
> Suppose you are right and DMA-mask should be always set before any
> mapping. What do you suggest to do in this case? (1) The code above
> overrides the real DMA-mask which could be set by the platform
> drivers, which in its turn are normally aware of the device DMA
> capabilities.

I am right. Appropriate DMA API usage as defined by the DMA API 
maintainers is not a matter of supposition. I literally just explained 
right there why drivers can't blindly assume the default mask is usable 
on modern systems (yes, it was different 20 years ago when system 
topologies were simpler).

However, having now gone and looked at the whole driver rather than 
unclear fragments of patch context, the code here *is* technically 
wrong. I've been mistakenly thinking all along that this was operating 
on the PCI device because I know that's what it *should* be doing, and 
seeing misleading things like "dev = pci->dev" falsely affirmed that 
assumption that it would be correct because it's been around for ages. 
AFAIU the correct PCI device won't actually exist until we've got far 
enough through pci_host_probe(), so I'm not sure how to easily solve this :/

Of course *this* patch doesn't change any of that either, so it's no 
worse than the existing code and I don't see that dropping it helps you 
at all; the current driver is already trampling your 64-bit mask back to 
32 bits and reserving the doorbell address in the wrong DMA address 
space (modulo the other dma-ranges bug which also took far too long to 
figure out). At this point I'd rather keep it since getting rid of the 
__GFP_DMA32 abuse is objectively good. If losing one page of coherent 
memory is a measurably significant problem for T1 once the other issues 
are worked out and that series lands, then you're welcome to propose a 
change on top (but I would prefer that all the drivers using this trick 
are changed consistently).

Thanks,
Robin.

> But in this case due to override afterwards any buffers
> above 4GB mapping will cause using the bounce buffers. (2) It's set
> here for something which isn't actual DMA. So to speak on one side is
> this patchset which overrides the mask for something which isn't DMA,
> and there are another patchsets:
> https://lore.kernel.org/linux-pci/[e-mail]/
> and
> https://lore.kernel.org/linux-pci/[e-mail]/
> which add the real DMA support to DW PCIe driver and for which setting
> the real DMA-mask is crucial. What do you suggest? Setting the mask
> twice: before allocating MSI-buffer and afterwards for the sake of
> eDMA buffers mapping? Moving DMA-mask setting from the generic DW PCIe
> code to the platform drivers?
> 
> -Sergey
> 
>>
>> Thanks,
>> Robin.
>>
>>>>    	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");
>>>> -	pp->msi_page = alloc_page(GFP_DMA32);
>>>> -	pp->msi_data = dma_map_page(dev, pp->msi_page, 0,
>>>> -				    PAGE_SIZE, DMA_FROM_DEVICE);
>>>> -	ret = dma_mapping_error(dev, pp->msi_data);
>>>> -	if (ret) {
>>>> -		dev_err(pci->dev, "Failed to map MSI data\n");
>>>> -		__free_page(pp->msi_page);
>>>> -		pp->msi_page = NULL;
>>>> -		pp->msi_data = 0;
>>>> +	msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
>>>> +					GFP_KERNEL);
>>>
>>> Changing the whole device DMA-mask due to something that doesn't
>>> perform seems inappropriate. I'd suggest to preserve the ZONE_DMA32
>>> here until there is something like suggested by @Robin
>>> https://lore.kernel.org/linux-pci/[e-mail]/
>>> in the last paragraph is implemented. Especially seeing there still
>>> common drivers in kernel which still rely on that zone.
>>>
>>> -Sergey
>>>
>>>> +	if (!msi_vaddr) {
>>>> +		dev_err(dev, "Failed to alloc and map MSI data\n");
>>>>    		dw_pcie_free_msi(pp);
>>>> -
>>>> -		return ret;
>>>> +		return -ENOMEM;
>>>>    	}
>>>>    	return 0;
>>>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
>>>> index 09b887093a84..a871ae7eb59e 100644
>>>> --- a/drivers/pci/controller/dwc/pcie-designware.h
>>>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
>>>> @@ -243,7 +243,6 @@ struct dw_pcie_rp {
>>>>    	struct irq_domain	*irq_domain;
>>>>    	struct irq_domain	*msi_domain;
>>>>    	dma_addr_t		msi_data;
>>>> -	struct page		*msi_page;
>>>>    	struct irq_chip		*msi_irq_chip;
>>>>    	u32			num_vectors;
>>>>    	u32			irq_mask[MAX_MSI_CTRLS];
>>>> -- 
>>>> 2.37.2.672.g94769d06f0-goog
>>>>
>>>>

Serge Semin Sept. 30, 2022, 12:57 p.m. UTC | #5

On Fri, Sep 30, 2022 at 12:01:58PM +0100, Robin Murphy wrote:
> On 2022-09-29 20:32, Serge Semin wrote:
> > On Thu, Sep 29, 2022 at 07:25:03PM +0100, Robin Murphy wrote:
> > > On 2022-09-28 12:41, Serge Semin wrote:
> > > > On Thu, Aug 25, 2022 at 06:50:24PM +0000, Will McVicker wrote:
> > > > > Re-work the msi_msg DMA allocation logic to use dmam_alloc_coherent() which
> > > > > uses the coherent DMA mask to try to return an allocation within the DMA
> > > > > mask limits. With that, we now can drop the msi_page parameter in struct
> > > > > dw_pcie_rp. This allows kernel configurations that disable ZONE_DMA32 to
> > > > > continue supporting a 32-bit DMA mask. Without this patch, the PCIe host
> > > > > device will fail to probe when ZONE_DMA32 is disabled.
> > > > 
> > > > As Rob already said here
> > > > https://lore.kernel.org/all/CAL_JsqJh=d-B51b6yPBRq0tOwbChN=[e-mail]/
> > > > and I mentioned in this thread
> > > > https://lore.kernel.org/linux-pci/20220912000211.ct6asuhhmnatje5e@mobilestation/
> > > > DW PCIe MSI doesn't cause any DMA due to the way the iMSI-RX engine is
> > > > designed. So reserving any real system memory is a waste of one in
> > > > this case. Reserving DMA-coherent even more inappropriate since it
> > > > can be expensive on some platforms (see note in Part Ia of
> > > > Documentation/core-api/dma-api.rst). For instance on MIPS32 with
> > > > non-corehent common DMA.
> > > 
> > 
> > > This has been discussed before - in general it is difficult to pick an
> > > arbitrary MSI address that is *guaranteed* not to overlap any valid DMA
> > > address that somebody may try to use later. However there is a very easy way
> > > to guarantee that the DMA API won't give anyone a particular DMA address,
> > > which is to get an address directly from the DMA API and keep it. Yes, that
> > > can technically be done with a streaming mapping *if* you already have some
> > > memory allocated in a suitable physical location, but coherent allocations
> > > are even more foolproof, simpler to clean up (particularly with devres), and
> > > unlikely to be an issue on relevant platforms (do any MIPS32 systems use
> > > this driver?)
> > 
> > My patchset adds the DW PCIe RP controller support on MIPS32 arch:
> > https://lore.kernel.org/linux-pci/[e-mail]/
> > 
> > > 
> > > > > Fixes: 35797e672ff0 ("PCI: dwc: Fix MSI msi_msg DMA mapping")
> > > > > Reported-by: Isaac J. Manjarres <[e-mail]>
> > > > > Signed-off-by: Will McVicker <[e-mail]>
> > > > > Acked-by: Jingoo Han <[e-mail]>
> > > > > Reviewed-by: Rob Herring <[e-mail]>
> > > > > ---
> > > > >    .../pci/controller/dwc/pcie-designware-host.c | 28 +++++--------------
> > > > >    drivers/pci/controller/dwc/pcie-designware.h  |  1 -
> > > > >    2 files changed, 7 insertions(+), 22 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > index 7746f94a715f..39f3b37d4033 100644
> > > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > @@ -267,15 +267,6 @@ static void dw_pcie_free_msi(struct dw_pcie_rp *pp)
> > > > >    	irq_domain_remove(pp->msi_domain);
> > > > >    	irq_domain_remove(pp->irq_domain);
> > > > > -
> > > > > -	if (pp->msi_data) {
> > > > > -		struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > > > -		struct device *dev = pci->dev;
> > > > > -
> > > > > -		dma_unmap_page(dev, pp->msi_data, PAGE_SIZE, DMA_FROM_DEVICE);
> > > > > -		if (pp->msi_page)
> > > > > -			__free_page(pp->msi_page);
> > > > > -	}
> > > > >    }
> > > > >    static void dw_pcie_msi_init(struct dw_pcie_rp *pp)
> > > > > @@ -336,6 +327,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;
> > > > >    	int ret;
> > > > >    	u32 ctrl, num_ctrls;
> > > > > @@ -375,22 +367,16 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
> > > > >    						    dw_chained_msi_isr, pp);
> > > > >    	}
> > > > 
> > > > > -	ret = dma_set_mask(dev, DMA_BIT_MASK(32));
> > > > > +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
> > > > 
> > > > This has been redundant in the first place since none of the DW PCIe
> > > > low-level drivers update the mask, and it's of 32-bits wide by default
> > > > anyway:
> > > > https://elixir.bootlin.com/linux/latest/source/drivers/of/platform.c#L167
> > > 
> > 
> > > No, in general drivers should always explicitly set their mask(s) and check
> > > the return value to make sure DMA is possible at all before trying any other
> > > DMA API calls. There's no guarantee that the default mask is usable (e.g.
> > > some systems don't have any 32-bit addressable RAM), or that it's even
> > > always 32 bits (due to crufty reasons of something of_dma_configure() tried
> > > to do a long time ago).
> > 
> > Suppose you are right and DMA-mask should be always set before any
> > mapping. What do you suggest to do in this case? (1) The code above
> > overrides the real DMA-mask which could be set by the platform
> > drivers, which in its turn are normally aware of the device DMA
> > capabilities.
> 
> I am right. Appropriate DMA API usage as defined by the DMA API maintainers
> is not a matter of supposition. I literally just explained right there why
> drivers can't blindly assume the default mask is usable on modern systems
> (yes, it was different 20 years ago when system topologies were simpler).
> 

> However, having now gone and looked at the whole driver rather than unclear
> fragments of patch context, the code here *is* technically wrong. I've been
> mistakenly thinking all along that this was operating on the PCI device
> because I know that's what it *should* be doing, and seeing misleading
> things like "dev = pci->dev" falsely affirmed that assumption that it would
> be correct because it's been around for ages.
> AFAIU the correct PCI device
> won't actually exist until we've got far enough through pci_host_probe(), so
> I'm not sure how to easily solve this :/

Right. The code affected by the subject patch has nothing to do with
the real PCI devices. The DMA-mask is set to the DW PCIe Host controller
platform device in order to force a page being allocated within 32-bit
address space. That's it.

Here is a log of the related changes:

0. Initially a GFP_KERNEL-based page was allocated for the MSI buffer.
It may cause having the DMA/PCIe-address above 4GB, which wouldn't work
for the PCIe peripherals with only 32-bit MSI capability. Though
nobody bothered back then.

1. 07940c369a6b ("PCI: dwc: Fix MSI page leakage in suspend/resume")
After this commit nothing really has changed, but instead of
allocating the MSI-message pseudo-buffer turned to be embedded into
the private data. It could be allocated at any base address with no
actual limitation (because private data is kmalloc'ed).

2. 660c486590aa ("PCI: dwc: Set 32-bit DMA mask for MSI target address allocation")
Someone found out that some devices failed to deliver MSI to the
address above 4GB of PCIe address space and fixed the problem by
force-setting the DMA-mask to being 32-bit before mapping the MSI
buffer. It indeed fixed the problem, but the actual buffer still left
being allocated from any address space. Instead, the mapping procedure
just bounced the buffer to 4GB space. So basically the solution was
very clumsy since turns a bounce buffer being reserved forever.

3. 35797e672ff0 PCI: dwc: Fix MSI msi_msg DMA mapping
@William basically got things back to (0) but instead of GFP_KERNEL
the page was allocated from GFP_DMA32. At this stage he should have
dropped the DMA-mask setting too since the buffer was already
guaranteed to be within 4GB space, but he didn't.

So now we have what we have. The DMA-mask is pointlessly changed for
something not really implying any DMA. That's why I insisted on
dropping it at the very least. Another reason I thought was also
appropriate was the default DMA-mask being set to 32bits anyway.
But you said we shouldn't rely on the default DMA-mask setting. So
ok, it doesn't count then. But it doesn't change the uselessness of the
DMA-mask change in the current driver. 

> AFAIU the correct PCI device
> won't actually exist until we've got far enough through pci_host_probe(), so
> I'm not sure how to easily solve this :/

Walk over all PCIe devices detected on the PCIe-bus. Check their
MSI-capability flags. If any of them have no 64-bit MSI flag set, then
make sure the MSI-base address is allocated within 4GB memory region.
It isn't that easy to implement though...

> 
> Of course *this* patch doesn't change any of that either, so it's no worse
> than the existing code and I don't see that dropping it helps you at all;
> the current driver is already trampling your 64-bit mask back to 32 bits 

Yes, and by this pathset @William intend to fix the DMA-mask-override
behaviour by using the dma_alloc_coherent() method. So any
platform-specific DMA-mask setting will be overwritten, and the
DMA-mask setting won't be able to be moved/dropped due to the
dma_alloc_coherent() method usage.

I have added a DW eDMA-engine support to the DW PCIe driver (you've
already seen my patches) and the engine initialization is supposed to
be performed after any basic initializations like CSRs mapping, data
allocations, MSI, etc. Since DMA is performed by the controller itself
it's required to have a correct DMA-mask set to the DW PCIe platform
device otherwise any consequent mapping will be bounce buffered to the
lowest 4GB even though the corresponding platform can support more
than 4GB of memory (even our MIPS32 can) with DW eDMA easily reaching
that memory. What would help me in this case if the MSI-buffer
allocation procedure wouldn't change the device DMA-mask.  As an
alternative to completely dropping the DMA-mask setting, the DMA-mask
setup process could be moved to the low-level platform device drivers.
It would be even more justified since the platform-specific device
capabilities (like DW PCIe AXI-interface address-bus width) are
unknown in the generic code.

As another alternative I could override the DMA-mask within the DW
eDMA probe procedure. But that would make things more complicated than
relying on the low-level platform drivers doing that.

> and
> reserving the doorbell address in the wrong DMA address space (modulo the
> other dma-ranges bug which also took far too long to figure out).

Actually current driver (without William patch) reserve the doorbell
address in the correct DMA address space (if we don't take the
dma-ranges settings into account). It works as expected in case if the
PCIe<->CPU space has one-on-one mapping (which is true in the most of
the cases). The only thing which is wrong is the pointless DMA-mask
update. I could have easily dropped it in my patchset. But after the
@William patchset is applied I won't be able to do that due to using
the dma_alloc_coherent() here.

> At this
> point I'd rather keep it since getting rid of the __GFP_DMA32 abuse is
> objectively good. If losing one page of coherent memory is a measurably
> significant problem for T1 once the other issues are worked out and that
> series lands, then you're welcome to propose a change on top (but I would
> prefer that all the drivers using this trick are changed consistently).

Regarding DMA-coherent allocation. I am not happy with losing a whole
page of the dma-coherent memory, but we can live with that. What give
additional difficulty for our eDMA-patches is the DMA-mask override.
If you still insist on preserving the @William patchset as it is,
where do you suggest for me to update the DMA-mask if the low-level
driver won't be suitable for that anymore?

-Sergey

> 
> Thanks,
> Robin.
> 
> > But in this case due to override afterwards any buffers
> > above 4GB mapping will cause using the bounce buffers. (2) It's set
> > here for something which isn't actual DMA. So to speak on one side is
> > this patchset which overrides the mask for something which isn't DMA,
> > and there are another patchsets:
> > https://lore.kernel.org/linux-pci/[e-mail]/
> > and
> > https://lore.kernel.org/linux-pci/[e-mail]/
> > which add the real DMA support to DW PCIe driver and for which setting
> > the real DMA-mask is crucial. What do you suggest? Setting the mask
> > twice: before allocating MSI-buffer and afterwards for the sake of
> > eDMA buffers mapping? Moving DMA-mask setting from the generic DW PCIe
> > code to the platform drivers?
> > 
> > -Sergey
> > 
> > > 
> > > Thanks,
> > > Robin.
> > > 
> > > > >    	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");
> > > > > -	pp->msi_page = alloc_page(GFP_DMA32);
> > > > > -	pp->msi_data = dma_map_page(dev, pp->msi_page, 0,
> > > > > -				    PAGE_SIZE, DMA_FROM_DEVICE);
> > > > > -	ret = dma_mapping_error(dev, pp->msi_data);
> > > > > -	if (ret) {
> > > > > -		dev_err(pci->dev, "Failed to map MSI data\n");
> > > > > -		__free_page(pp->msi_page);
> > > > > -		pp->msi_page = NULL;
> > > > > -		pp->msi_data = 0;
> > > > > +	msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
> > > > > +					GFP_KERNEL);
> > > > 
> > > > Changing the whole device DMA-mask due to something that doesn't
> > > > perform seems inappropriate. I'd suggest to preserve the ZONE_DMA32
> > > > here until there is something like suggested by @Robin
> > > > https://lore.kernel.org/linux-pci/[e-mail]/
> > > > in the last paragraph is implemented. Especially seeing there still
> > > > common drivers in kernel which still rely on that zone.
> > > > 
> > > > -Sergey
> > > > 
> > > > > +	if (!msi_vaddr) {
> > > > > +		dev_err(dev, "Failed to alloc and map MSI data\n");
> > > > >    		dw_pcie_free_msi(pp);
> > > > > -
> > > > > -		return ret;
> > > > > +		return -ENOMEM;
> > > > >    	}
> > > > >    	return 0;
> > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > > > > index 09b887093a84..a871ae7eb59e 100644
> > > > > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > > > > @@ -243,7 +243,6 @@ struct dw_pcie_rp {
> > > > >    	struct irq_domain	*irq_domain;
> > > > >    	struct irq_domain	*msi_domain;
> > > > >    	dma_addr_t		msi_data;
> > > > > -	struct page		*msi_page;
> > > > >    	struct irq_chip		*msi_irq_chip;
> > > > >    	u32			num_vectors;
> > > > >    	u32			irq_mask[MAX_MSI_CTRLS];
> > > > > -- 
> > > > > 2.37.2.672.g94769d06f0-goog
> > > > > 
> > > > >

Robin Murphy Sept. 30, 2022, 3:39 p.m. UTC | #6

On 2022-09-30 13:57, Serge Semin wrote:
> On Fri, Sep 30, 2022 at 12:01:58PM +0100, Robin Murphy wrote:
>> On 2022-09-29 20:32, Serge Semin wrote:
>>> On Thu, Sep 29, 2022 at 07:25:03PM +0100, Robin Murphy wrote:
>>>> On 2022-09-28 12:41, Serge Semin wrote:
>>>>> On Thu, Aug 25, 2022 at 06:50:24PM +0000, Will McVicker wrote:
>>>>>> Re-work the msi_msg DMA allocation logic to use dmam_alloc_coherent() which
>>>>>> uses the coherent DMA mask to try to return an allocation within the DMA
>>>>>> mask limits. With that, we now can drop the msi_page parameter in struct
>>>>>> dw_pcie_rp. This allows kernel configurations that disable ZONE_DMA32 to
>>>>>> continue supporting a 32-bit DMA mask. Without this patch, the PCIe host
>>>>>> device will fail to probe when ZONE_DMA32 is disabled.
>>>>>
>>>>> As Rob already said here
>>>>> https://lore.kernel.org/all/CAL_JsqJh=d-B51b6yPBRq0tOwbChN=[e-mail]/
>>>>> and I mentioned in this thread
>>>>> https://lore.kernel.org/linux-pci/20220912000211.ct6asuhhmnatje5e@mobilestation/
>>>>> DW PCIe MSI doesn't cause any DMA due to the way the iMSI-RX engine is
>>>>> designed. So reserving any real system memory is a waste of one in
>>>>> this case. Reserving DMA-coherent even more inappropriate since it
>>>>> can be expensive on some platforms (see note in Part Ia of
>>>>> Documentation/core-api/dma-api.rst). For instance on MIPS32 with
>>>>> non-corehent common DMA.
>>>>
>>>
>>>> This has been discussed before - in general it is difficult to pick an
>>>> arbitrary MSI address that is *guaranteed* not to overlap any valid DMA
>>>> address that somebody may try to use later. However there is a very easy way
>>>> to guarantee that the DMA API won't give anyone a particular DMA address,
>>>> which is to get an address directly from the DMA API and keep it. Yes, that
>>>> can technically be done with a streaming mapping *if* you already have some
>>>> memory allocated in a suitable physical location, but coherent allocations
>>>> are even more foolproof, simpler to clean up (particularly with devres), and
>>>> unlikely to be an issue on relevant platforms (do any MIPS32 systems use
>>>> this driver?)
>>>
>>> My patchset adds the DW PCIe RP controller support on MIPS32 arch:
>>> https://lore.kernel.org/linux-pci/[e-mail]/
>>>
>>>>
>>>>>> Fixes: 35797e672ff0 ("PCI: dwc: Fix MSI msi_msg DMA mapping")
>>>>>> Reported-by: Isaac J. Manjarres <[e-mail]>
>>>>>> Signed-off-by: Will McVicker <[e-mail]>
>>>>>> Acked-by: Jingoo Han <[e-mail]>
>>>>>> Reviewed-by: Rob Herring <[e-mail]>
>>>>>> ---
>>>>>>     .../pci/controller/dwc/pcie-designware-host.c | 28 +++++--------------
>>>>>>     drivers/pci/controller/dwc/pcie-designware.h  |  1 -
>>>>>>     2 files changed, 7 insertions(+), 22 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
>>>>>> index 7746f94a715f..39f3b37d4033 100644
>>>>>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>>>>>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>>>>>> @@ -267,15 +267,6 @@ static void dw_pcie_free_msi(struct dw_pcie_rp *pp)
>>>>>>     	irq_domain_remove(pp->msi_domain);
>>>>>>     	irq_domain_remove(pp->irq_domain);
>>>>>> -
>>>>>> -	if (pp->msi_data) {
>>>>>> -		struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>>>>> -		struct device *dev = pci->dev;
>>>>>> -
>>>>>> -		dma_unmap_page(dev, pp->msi_data, PAGE_SIZE, DMA_FROM_DEVICE);
>>>>>> -		if (pp->msi_page)
>>>>>> -			__free_page(pp->msi_page);
>>>>>> -	}
>>>>>>     }
>>>>>>     static void dw_pcie_msi_init(struct dw_pcie_rp *pp)
>>>>>> @@ -336,6 +327,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;
>>>>>>     	int ret;
>>>>>>     	u32 ctrl, num_ctrls;
>>>>>> @@ -375,22 +367,16 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
>>>>>>     						    dw_chained_msi_isr, pp);
>>>>>>     	}
>>>>>
>>>>>> -	ret = dma_set_mask(dev, DMA_BIT_MASK(32));
>>>>>> +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
>>>>>
>>>>> This has been redundant in the first place since none of the DW PCIe
>>>>> low-level drivers update the mask, and it's of 32-bits wide by default
>>>>> anyway:
>>>>> https://elixir.bootlin.com/linux/latest/source/drivers/of/platform.c#L167
>>>>
>>>
>>>> No, in general drivers should always explicitly set their mask(s) and check
>>>> the return value to make sure DMA is possible at all before trying any other
>>>> DMA API calls. There's no guarantee that the default mask is usable (e.g.
>>>> some systems don't have any 32-bit addressable RAM), or that it's even
>>>> always 32 bits (due to crufty reasons of something of_dma_configure() tried
>>>> to do a long time ago).
>>>
>>> Suppose you are right and DMA-mask should be always set before any
>>> mapping. What do you suggest to do in this case? (1) The code above
>>> overrides the real DMA-mask which could be set by the platform
>>> drivers, which in its turn are normally aware of the device DMA
>>> capabilities.
>>
>> I am right. Appropriate DMA API usage as defined by the DMA API maintainers
>> is not a matter of supposition. I literally just explained right there why
>> drivers can't blindly assume the default mask is usable on modern systems
>> (yes, it was different 20 years ago when system topologies were simpler).
>>
> 
>> However, having now gone and looked at the whole driver rather than unclear
>> fragments of patch context, the code here *is* technically wrong. I've been
>> mistakenly thinking all along that this was operating on the PCI device
>> because I know that's what it *should* be doing, and seeing misleading
>> things like "dev = pci->dev" falsely affirmed that assumption that it would
>> be correct because it's been around for ages.
>> AFAIU the correct PCI device
>> won't actually exist until we've got far enough through pci_host_probe(), so
>> I'm not sure how to easily solve this :/
> 
> Right. The code affected by the subject patch has nothing to do with
> the real PCI devices. The DMA-mask is set to the DW PCIe Host controller
> platform device in order to force a page being allocated within 32-bit
> address space. That's it.
> 
> Here is a log of the related changes:
> 
> 0. Initially a GFP_KERNEL-based page was allocated for the MSI buffer.
> It may cause having the DMA/PCIe-address above 4GB, which wouldn't work
> for the PCIe peripherals with only 32-bit MSI capability. Though
> nobody bothered back then.
> 
> 1. 07940c369a6b ("PCI: dwc: Fix MSI page leakage in suspend/resume")
> After this commit nothing really has changed, but instead of
> allocating the MSI-message pseudo-buffer turned to be embedded into
> the private data. It could be allocated at any base address with no
> actual limitation (because private data is kmalloc'ed).
> 
> 2. 660c486590aa ("PCI: dwc: Set 32-bit DMA mask for MSI target address allocation")
> Someone found out that some devices failed to deliver MSI to the
> address above 4GB of PCIe address space and fixed the problem by
> force-setting the DMA-mask to being 32-bit before mapping the MSI
> buffer. It indeed fixed the problem, but the actual buffer still left
> being allocated from any address space. Instead, the mapping procedure
> just bounced the buffer to 4GB space. So basically the solution was
> very clumsy since turns a bounce buffer being reserved forever.
> 
> 3. 35797e672ff0 PCI: dwc: Fix MSI msi_msg DMA mapping
> @William basically got things back to (0) but instead of GFP_KERNEL
> the page was allocated from GFP_DMA32. At this stage he should have
> dropped the DMA-mask setting too since the buffer was already
> guaranteed to be within 4GB space, but he didn't.

I never saw that change, but frankly the justification in the commit 
message is wrong. I know that there are Android systems with memory 
above 32 bits that run with SWIOTLB disabled because they think they 
know what they're doing, which are almost certainly the same ones that 
also want to disable ZONE_DMA32 for similar reasons. That patch is 
really just another hack around an unexpected configuration, but without 
saying so.

> So now we have what we have. The DMA-mask is pointlessly changed for
> something not really implying any DMA. That's why I insisted on
> dropping it at the very least. Another reason I thought was also
> appropriate was the default DMA-mask being set to 32bits anyway.
> But you said we shouldn't rely on the default DMA-mask setting. So
> ok, it doesn't count then. But it doesn't change the uselessness of the
> DMA-mask change in the current driver.

As I keep saying, it *is* relevant to DMA. The MSI doorbell may not be 
accessing memory, but it is still a thing that occupies DMA address 
space like a mapping of memory does, and DMA masks are how we control 
how DMA address space is allocated. Unless and until we have an API for 
arbitrarily reserving DMA address space within a given range, the 
approach used here and in other drivers is the next best thing, however 
much you don't like it.

>> AFAIU the correct PCI device
>> won't actually exist until we've got far enough through pci_host_probe(), so
>> I'm not sure how to easily solve this :/
> 
> Walk over all PCIe devices detected on the PCIe-bus. Check their
> MSI-capability flags. If any of them have no 64-bit MSI flag set, then
> make sure the MSI-base address is allocated within 4GB memory region.
> It isn't that easy to implement though...

It has nothing to do with capabilities (but also: consider hotplug). We 
simply need the host bridge PCI device to pass to the DMA API to ensure 
that the mapping/allocation is relative to PCI Mem space rather than 
system physical address space, because the two don't have to be 
identical. The challenge is how to reliably pick up that device and set 
up the doorbell *before* any other PCI devices also discovered by 
pci_host_probe() have a chance to start binding drivers and trying to 
request MSIs.

>> Of course *this* patch doesn't change any of that either, so it's no worse
>> than the existing code and I don't see that dropping it helps you at all;
>> the current driver is already trampling your 64-bit mask back to 32 bits
> 
> Yes, and by this pathset @William intend to fix the DMA-mask-override
> behaviour by using the dma_alloc_coherent() method.

No, that is effectively unchanged. Whether it's a streaming mapping with 
dma_mask or a coherent allocation with coherent_dma_mask, masks are 
getting set way, it's the fact that it's on the wrong device that's the 
real problem.

If you expose the eDMA as a generic dmaengine device then there's every 
chance some consumer would make a streaming mapping with it, so even 
though the current code happens to not override the coherent mask it's 
still biting you in the streaming mask.

> So any
> platform-specific DMA-mask setting will be overwritten, and the
> DMA-mask setting won't be able to be moved/dropped due to the
> dma_alloc_coherent() method usage.
> 
> I have added a DW eDMA-engine support to the DW PCIe driver (you've
> already seen my patches) and the engine initialization is supposed to
> be performed after any basic initializations like CSRs mapping, data
> allocations, MSI, etc. Since DMA is performed by the controller itself
> it's required to have a correct DMA-mask set to the DW PCIe platform
> device otherwise any consequent mapping will be bounce buffered to the
> lowest 4GB even though the corresponding platform can support more
> than 4GB of memory (even our MIPS32 can) with DW eDMA easily reaching
> that memory. What would help me in this case if the MSI-buffer
> allocation procedure wouldn't change the device DMA-mask.  As an
> alternative to completely dropping the DMA-mask setting, the DMA-mask
> setup process could be moved to the low-level platform device drivers.
> It would be even more justified since the platform-specific device
> capabilities (like DW PCIe AXI-interface address-bus width) are
> unknown in the generic code.
> 
> As another alternative I could override the DMA-mask within the DW
> eDMA probe procedure. But that would make things more complicated than
> relying on the low-level platform drivers doing that.
> 
>> and
>> reserving the doorbell address in the wrong DMA address space (modulo the
>> other dma-ranges bug which also took far too long to figure out).
> 
> Actually current driver (without William patch) reserve the doorbell
> address in the correct DMA address space (if we don't take the
> dma-ranges settings into account).

No it does not. With or without this patch it is still passing the 
*platform device* to the DMA API, which means the mapping is relative to 
the platform address space, not PCI Mem space on the other side of the 
iATU. The fact that the iATU's dma-ranges translation is erroneously 
applied to the platform device at the moment is, as I have said, a bug.

> It works as expected in case if the
> PCIe<->CPU space has one-on-one mapping (which is true in the most of
> the cases). The only thing which is wrong is the pointless DMA-mask
> update. I could have easily dropped it in my patchset. But after the
> @William patchset is applied I won't be able to do that due to using
> the dma_alloc_coherent() here.

Once again, it is not pointless. There is no guarantee that __GFP_DMA32 
does anything, since ZONE_DMA32 may not exist (per this patch), and the 
zones may not be as expected anyway (look at what arm64 currently does 
if all RAM is above 32 bits, but save those complaints for another thread).

>> At this
>> point I'd rather keep it since getting rid of the __GFP_DMA32 abuse is
>> objectively good. If losing one page of coherent memory is a measurably
>> significant problem for T1 once the other issues are worked out and that
>> series lands, then you're welcome to propose a change on top (but I would
>> prefer that all the drivers using this trick are changed consistently).
> 
> Regarding DMA-coherent allocation. I am not happy with losing a whole
> page of the dma-coherent memory, but we can live with that. What give
> additional difficulty for our eDMA-patches is the DMA-mask override.
> If you still insist on preserving the @William patchset as it is,
> where do you suggest for me to update the DMA-mask if the low-level
> driver won't be suitable for that anymore?

I'm not insisting anything, it's just that this patch is already 
reviewed and queued, is a small step towards being less wrong overall, 
and dropping it wouldn't actually solve any of your problems anyway, so 
I just feel that being obstructive because it falls short of perfection 
isn't helpful.

Robin.

William McVicker Sept. 30, 2022, 5:02 p.m. UTC | #7

On 09/30/2022, Robin Murphy wrote:
> On 2022-09-30 13:57, Serge Semin wrote:
> > On Fri, Sep 30, 2022 at 12:01:58PM +0100, Robin Murphy wrote:
> > > On 2022-09-29 20:32, Serge Semin wrote:
> > > > On Thu, Sep 29, 2022 at 07:25:03PM +0100, Robin Murphy wrote:
> > > > > On 2022-09-28 12:41, Serge Semin wrote:
> > > > > > On Thu, Aug 25, 2022 at 06:50:24PM +0000, Will McVicker wrote:
> > > > > > > Re-work the msi_msg DMA allocation logic to use dmam_alloc_coherent() which
> > > > > > > uses the coherent DMA mask to try to return an allocation within the DMA
> > > > > > > mask limits. With that, we now can drop the msi_page parameter in struct
> > > > > > > dw_pcie_rp. This allows kernel configurations that disable ZONE_DMA32 to
> > > > > > > continue supporting a 32-bit DMA mask. Without this patch, the PCIe host
> > > > > > > device will fail to probe when ZONE_DMA32 is disabled.
> > > > > > 
> > > > > > As Rob already said here
> > > > > > https://lore.kernel.org/all/CAL_JsqJh=d-B51b6yPBRq0tOwbChN=[e-mail]/
> > > > > > and I mentioned in this thread
> > > > > > https://lore.kernel.org/linux-pci/20220912000211.ct6asuhhmnatje5e@mobilestation/
> > > > > > DW PCIe MSI doesn't cause any DMA due to the way the iMSI-RX engine is
> > > > > > designed. So reserving any real system memory is a waste of one in
> > > > > > this case. Reserving DMA-coherent even more inappropriate since it
> > > > > > can be expensive on some platforms (see note in Part Ia of
> > > > > > Documentation/core-api/dma-api.rst). For instance on MIPS32 with
> > > > > > non-corehent common DMA.
> > > > > 
> > > > 
> > > > > This has been discussed before - in general it is difficult to pick an
> > > > > arbitrary MSI address that is *guaranteed* not to overlap any valid DMA
> > > > > address that somebody may try to use later. However there is a very easy way
> > > > > to guarantee that the DMA API won't give anyone a particular DMA address,
> > > > > which is to get an address directly from the DMA API and keep it. Yes, that
> > > > > can technically be done with a streaming mapping *if* you already have some
> > > > > memory allocated in a suitable physical location, but coherent allocations
> > > > > are even more foolproof, simpler to clean up (particularly with devres), and
> > > > > unlikely to be an issue on relevant platforms (do any MIPS32 systems use
> > > > > this driver?)
> > > > 
> > > > My patchset adds the DW PCIe RP controller support on MIPS32 arch:
> > > > https://lore.kernel.org/linux-pci/[e-mail]/
> > > > 
> > > > > 
> > > > > > > Fixes: 35797e672ff0 ("PCI: dwc: Fix MSI msi_msg DMA mapping")
> > > > > > > Reported-by: Isaac J. Manjarres <[e-mail]>
> > > > > > > Signed-off-by: Will McVicker <[e-mail]>
> > > > > > > Acked-by: Jingoo Han <[e-mail]>
> > > > > > > Reviewed-by: Rob Herring <[e-mail]>
> > > > > > > ---
> > > > > > >     .../pci/controller/dwc/pcie-designware-host.c | 28 +++++--------------
> > > > > > >     drivers/pci/controller/dwc/pcie-designware.h  |  1 -
> > > > > > >     2 files changed, 7 insertions(+), 22 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > > index 7746f94a715f..39f3b37d4033 100644
> > > > > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > > @@ -267,15 +267,6 @@ static void dw_pcie_free_msi(struct dw_pcie_rp *pp)
> > > > > > >     	irq_domain_remove(pp->msi_domain);
> > > > > > >     	irq_domain_remove(pp->irq_domain);
> > > > > > > -
> > > > > > > -	if (pp->msi_data) {
> > > > > > > -		struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > > > > > -		struct device *dev = pci->dev;
> > > > > > > -
> > > > > > > -		dma_unmap_page(dev, pp->msi_data, PAGE_SIZE, DMA_FROM_DEVICE);
> > > > > > > -		if (pp->msi_page)
> > > > > > > -			__free_page(pp->msi_page);
> > > > > > > -	}
> > > > > > >     }
> > > > > > >     static void dw_pcie_msi_init(struct dw_pcie_rp *pp)
> > > > > > > @@ -336,6 +327,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;
> > > > > > >     	int ret;
> > > > > > >     	u32 ctrl, num_ctrls;
> > > > > > > @@ -375,22 +367,16 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
> > > > > > >     						    dw_chained_msi_isr, pp);
> > > > > > >     	}
> > > > > > 
> > > > > > > -	ret = dma_set_mask(dev, DMA_BIT_MASK(32));
> > > > > > > +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
> > > > > > 
> > > > > > This has been redundant in the first place since none of the DW PCIe
> > > > > > low-level drivers update the mask, and it's of 32-bits wide by default
> > > > > > anyway:
> > > > > > https://elixir.bootlin.com/linux/latest/source/drivers/of/platform.c#L167
> > > > > 
> > > > 
> > > > > No, in general drivers should always explicitly set their mask(s) and check
> > > > > the return value to make sure DMA is possible at all before trying any other
> > > > > DMA API calls. There's no guarantee that the default mask is usable (e.g.
> > > > > some systems don't have any 32-bit addressable RAM), or that it's even
> > > > > always 32 bits (due to crufty reasons of something of_dma_configure() tried
> > > > > to do a long time ago).
> > > > 
> > > > Suppose you are right and DMA-mask should be always set before any
> > > > mapping. What do you suggest to do in this case? (1) The code above
> > > > overrides the real DMA-mask which could be set by the platform
> > > > drivers, which in its turn are normally aware of the device DMA
> > > > capabilities.
> > > 
> > > I am right. Appropriate DMA API usage as defined by the DMA API maintainers
> > > is not a matter of supposition. I literally just explained right there why
> > > drivers can't blindly assume the default mask is usable on modern systems
> > > (yes, it was different 20 years ago when system topologies were simpler).
> > > 
> > 
> > > However, having now gone and looked at the whole driver rather than unclear
> > > fragments of patch context, the code here *is* technically wrong. I've been
> > > mistakenly thinking all along that this was operating on the PCI device
> > > because I know that's what it *should* be doing, and seeing misleading
> > > things like "dev = pci->dev" falsely affirmed that assumption that it would
> > > be correct because it's been around for ages.
> > > AFAIU the correct PCI device
> > > won't actually exist until we've got far enough through pci_host_probe(), so
> > > I'm not sure how to easily solve this :/
> > 
> > Right. The code affected by the subject patch has nothing to do with
> > the real PCI devices. The DMA-mask is set to the DW PCIe Host controller
> > platform device in order to force a page being allocated within 32-bit
> > address space. That's it.
> > 
> > Here is a log of the related changes:
> > 
> > 0. Initially a GFP_KERNEL-based page was allocated for the MSI buffer.
> > It may cause having the DMA/PCIe-address above 4GB, which wouldn't work
> > for the PCIe peripherals with only 32-bit MSI capability. Though
> > nobody bothered back then.
> > 
> > 1. 07940c369a6b ("PCI: dwc: Fix MSI page leakage in suspend/resume")
> > After this commit nothing really has changed, but instead of
> > allocating the MSI-message pseudo-buffer turned to be embedded into
> > the private data. It could be allocated at any base address with no
> > actual limitation (because private data is kmalloc'ed).
> > 
> > 2. 660c486590aa ("PCI: dwc: Set 32-bit DMA mask for MSI target address allocation")
> > Someone found out that some devices failed to deliver MSI to the
> > address above 4GB of PCIe address space and fixed the problem by
> > force-setting the DMA-mask to being 32-bit before mapping the MSI
> > buffer. It indeed fixed the problem, but the actual buffer still left
> > being allocated from any address space. Instead, the mapping procedure
> > just bounced the buffer to 4GB space. So basically the solution was
> > very clumsy since turns a bounce buffer being reserved forever.
> > 
> > 3. 35797e672ff0 PCI: dwc: Fix MSI msi_msg DMA mapping
> > @William basically got things back to (0) but instead of GFP_KERNEL
> > the page was allocated from GFP_DMA32. At this stage he should have
> > dropped the DMA-mask setting too since the buffer was already
> > guaranteed to be within 4GB space, but he didn't.
> 
> I never saw that change, but frankly the justification in the commit message
> is wrong. I know that there are Android systems with memory above 32 bits
> that run with SWIOTLB disabled because they think they know what they're
> doing, which are almost certainly the same ones that also want to disable
> ZONE_DMA32 for similar reasons. That patch is really just another hack
> around an unexpected configuration, but without saying so.

Whether or not the Android OEM knows what they're doing is up to them :). We
shouldn't force enabling/disabling configs based on our own opinions about who
is an expert. Phones at least have the advantage of a controlled environment
which allows them to tune their drivers based on the hardware they ship and
don't need to worry about hotplugging unexpected hardware.

> 
> > So now we have what we have. The DMA-mask is pointlessly changed for
> > something not really implying any DMA. That's why I insisted on
> > dropping it at the very least. Another reason I thought was also
> > appropriate was the default DMA-mask being set to 32bits anyway.
> > But you said we shouldn't rely on the default DMA-mask setting. So
> > ok, it doesn't count then. But it doesn't change the uselessness of the
> > DMA-mask change in the current driver.
> 
> As I keep saying, it *is* relevant to DMA. The MSI doorbell may not be
> accessing memory, but it is still a thing that occupies DMA address space
> like a mapping of memory does, and DMA masks are how we control how DMA
> address space is allocated. Unless and until we have an API for arbitrarily
> reserving DMA address space within a given range, the approach used here and
> in other drivers is the next best thing, however much you don't like it.
> 
> > > AFAIU the correct PCI device
> > > won't actually exist until we've got far enough through pci_host_probe(), so
> > > I'm not sure how to easily solve this :/
> > 
> > Walk over all PCIe devices detected on the PCIe-bus. Check their
> > MSI-capability flags. If any of them have no 64-bit MSI flag set, then
> > make sure the MSI-base address is allocated within 4GB memory region.
> > It isn't that easy to implement though...
> 
> It has nothing to do with capabilities (but also: consider hotplug). We
> simply need the host bridge PCI device to pass to the DMA API to ensure that
> the mapping/allocation is relative to PCI Mem space rather than system
> physical address space, because the two don't have to be identical. The
> challenge is how to reliably pick up that device and set up the doorbell
> *before* any other PCI devices also discovered by pci_host_probe() have a
> chance to start binding drivers and trying to request MSIs.

Maybe I can provide some more insights on my patches that may help you guys
understand the idea behind the MSI capabilities flag. Basically, on a given
Android phone there are going to be multiple PCIe endpoints -- wifi and modem
are good examples. Some of these PCIe endpoints may only support a 32-bit MSI
capability structure, but others could support a 64-bit MSI capability
structure. So my intent was to have the PCIe RC driver detect the endpoint
device's MSI capabilites during the EP device probe and then set the
PCI_MSI_FLAGS_64BIT accordingly before calling dw_pcie_host_init(). Since the
PCIe RC drivers are the ones to call dw_pcie_host_init(), we can dynamically
change the DMA mask and allocate the doorbell target address based on the
PCI_MSI_FLAGS_64BIT.

This all really gets a lot more complicated with the introduction of Serge's
eDMA device. So I full well expect dw_pcie_msi_host_init() to be re-factored
for that.

> 
> > > Of course *this* patch doesn't change any of that either, so it's no worse
> > > than the existing code and I don't see that dropping it helps you at all;
> > > the current driver is already trampling your 64-bit mask back to 32 bits
> > 
> > Yes, and by this pathset @William intend to fix the DMA-mask-override
> > behaviour by using the dma_alloc_coherent() method.
> 
> No, that is effectively unchanged. Whether it's a streaming mapping with
> dma_mask or a coherent allocation with coherent_dma_mask, masks are getting
> set way, it's the fact that it's on the wrong device that's the real
> problem.
> 
> If you expose the eDMA as a generic dmaengine device then there's every
> chance some consumer would make a streaming mapping with it, so even though
> the current code happens to not override the coherent mask it's still biting
> you in the streaming mask.
> 
> > So any
> > platform-specific DMA-mask setting will be overwritten, and the
> > DMA-mask setting won't be able to be moved/dropped due to the
> > dma_alloc_coherent() method usage.
> > 
> > I have added a DW eDMA-engine support to the DW PCIe driver (you've
> > already seen my patches) and the engine initialization is supposed to
> > be performed after any basic initializations like CSRs mapping, data
> > allocations, MSI, etc. Since DMA is performed by the controller itself
> > it's required to have a correct DMA-mask set to the DW PCIe platform
> > device otherwise any consequent mapping will be bounce buffered to the
> > lowest 4GB even though the corresponding platform can support more
> > than 4GB of memory (even our MIPS32 can) with DW eDMA easily reaching
> > that memory. What would help me in this case if the MSI-buffer
> > allocation procedure wouldn't change the device DMA-mask.  As an
> > alternative to completely dropping the DMA-mask setting, the DMA-mask
> > setup process could be moved to the low-level platform device drivers.
> > It would be even more justified since the platform-specific device
> > capabilities (like DW PCIe AXI-interface address-bus width) are
> > unknown in the generic code.
> > 
> > As another alternative I could override the DMA-mask within the DW
> > eDMA probe procedure. But that would make things more complicated than
> > relying on the low-level platform drivers doing that.
> > 
> > > and
> > > reserving the doorbell address in the wrong DMA address space (modulo the
> > > other dma-ranges bug which also took far too long to figure out).
> > 
> > Actually current driver (without William patch) reserve the doorbell
> > address in the correct DMA address space (if we don't take the
> > dma-ranges settings into account).
> 
> No it does not. With or without this patch it is still passing the *platform
> device* to the DMA API, which means the mapping is relative to the platform
> address space, not PCI Mem space on the other side of the iATU. The fact
> that the iATU's dma-ranges translation is erroneously applied to the
> platform device at the moment is, as I have said, a bug.

Thanks for pointing this out. I agree this is a bug and I guess it hasn't
really been a problem because there isn't really any DMA'ing going on. With the
new eDMA device being introduced, this bug will likely need to be fixed.

> 
> > It works as expected in case if the
> > PCIe<->CPU space has one-on-one mapping (which is true in the most of
> > the cases). The only thing which is wrong is the pointless DMA-mask
> > update. I could have easily dropped it in my patchset. But after the
> > @William patchset is applied I won't be able to do that due to using
> > the dma_alloc_coherent() here.
> 
> Once again, it is not pointless. There is no guarantee that __GFP_DMA32 does
> anything, since ZONE_DMA32 may not exist (per this patch), and the zones may
> not be as expected anyway (look at what arm64 currently does if all RAM is
> above 32 bits, but save those complaints for another thread).
> 
> > > At this
> > > point I'd rather keep it since getting rid of the __GFP_DMA32 abuse is
> > > objectively good. If losing one page of coherent memory is a measurably
> > > significant problem for T1 once the other issues are worked out and that
> > > series lands, then you're welcome to propose a change on top (but I would
> > > prefer that all the drivers using this trick are changed consistently).
> > 
> > Regarding DMA-coherent allocation. I am not happy with losing a whole
> > page of the dma-coherent memory, but we can live with that. What give
> > additional difficulty for our eDMA-patches is the DMA-mask override.
> > If you still insist on preserving the @William patchset as it is,
> > where do you suggest for me to update the DMA-mask if the low-level
> > driver won't be suitable for that anymore?
> 
> I'm not insisting anything, it's just that this patch is already reviewed
> and queued, is a small step towards being less wrong overall, and dropping
> it wouldn't actually solve any of your problems anyway, so I just feel that
> being obstructive because it falls short of perfection isn't helpful.

Thanks for the responses Robin! I agree that we don't have a complete solution
and need to fix this DMA address space bug, but dont think that's enough of
a reason to drop this patch series. At the very least I think patch 1/2 which
removes the ZONE_DMA32 dependency is a worthy patch to take for 6.1.

Thanks,
Will

> 
> Robin.

Lorenzo Pieralisi Oct. 3, 2022, 8:04 a.m. UTC | #8

On Fri, Sep 30, 2022 at 10:02:22AM -0700, William McVicker wrote:

[...]

> > > So now we have what we have. The DMA-mask is pointlessly changed for
> > > something not really implying any DMA. That's why I insisted on
> > > dropping it at the very least. Another reason I thought was also
> > > appropriate was the default DMA-mask being set to 32bits anyway.
> > > But you said we shouldn't rely on the default DMA-mask setting. So
> > > ok, it doesn't count then. But it doesn't change the uselessness of the
> > > DMA-mask change in the current driver.
> > 
> > As I keep saying, it *is* relevant to DMA. The MSI doorbell may not be
> > accessing memory, but it is still a thing that occupies DMA address space
> > like a mapping of memory does, and DMA masks are how we control how DMA
> > address space is allocated. Unless and until we have an API for arbitrarily
> > reserving DMA address space within a given range, the approach used here and
> > in other drivers is the next best thing, however much you don't like it.
> > 
> > > > AFAIU the correct PCI device
> > > > won't actually exist until we've got far enough through pci_host_probe(), so
> > > > I'm not sure how to easily solve this :/
> > > 
> > > Walk over all PCIe devices detected on the PCIe-bus. Check their
> > > MSI-capability flags. If any of them have no 64-bit MSI flag set, then
> > > make sure the MSI-base address is allocated within 4GB memory region.
> > > It isn't that easy to implement though...
> > 
> > It has nothing to do with capabilities (but also: consider hotplug). We
> > simply need the host bridge PCI device to pass to the DMA API to ensure that
> > the mapping/allocation is relative to PCI Mem space rather than system
> > physical address space, because the two don't have to be identical. The
> > challenge is how to reliably pick up that device and set up the doorbell
> > *before* any other PCI devices also discovered by pci_host_probe() have a
> > chance to start binding drivers and trying to request MSIs.
> 
> Maybe I can provide some more insights on my patches that may help you guys
> understand the idea behind the MSI capabilities flag. Basically, on a given
> Android phone there are going to be multiple PCIe endpoints -- wifi and modem
> are good examples. Some of these PCIe endpoints may only support a 32-bit MSI
> capability structure, but others could support a 64-bit MSI capability
> structure. So my intent was to have the PCIe RC driver detect the endpoint
> device's MSI capabilites during the EP device probe and then set the
> PCI_MSI_FLAGS_64BIT accordingly before calling dw_pcie_host_init(). Since the
> PCIe RC drivers are the ones to call dw_pcie_host_init(), we can dynamically
> change the DMA mask and allocate the doorbell target address based on the
> PCI_MSI_FLAGS_64BIT.

It seems to me we are all talking past each others to solve different
problems, so I am going to reset this discussion given that we are
in the merge window and I must finalise the PCI patch queue.

Patch (2/2) should be dropped IMO - I don't think the host bridge
platform device DMA mask should depend on the root port MSI cap
64/32 bit addressing - I don't think that's the right thing to do.

We should keep this discussion going for the next cycle, I will
drop patch (2/2) for the time being, sorry.

Thanks,
Lorenzo

> This all really gets a lot more complicated with the introduction of Serge's
> eDMA device. So I full well expect dw_pcie_msi_host_init() to be re-factored
> for that.
> 
> > 
> > > > Of course *this* patch doesn't change any of that either, so it's no worse
> > > > than the existing code and I don't see that dropping it helps you at all;
> > > > the current driver is already trampling your 64-bit mask back to 32 bits
> > > 
> > > Yes, and by this pathset @William intend to fix the DMA-mask-override
> > > behaviour by using the dma_alloc_coherent() method.
> > 
> > No, that is effectively unchanged. Whether it's a streaming mapping with
> > dma_mask or a coherent allocation with coherent_dma_mask, masks are getting
> > set way, it's the fact that it's on the wrong device that's the real
> > problem.
> > 
> > If you expose the eDMA as a generic dmaengine device then there's every
> > chance some consumer would make a streaming mapping with it, so even though
> > the current code happens to not override the coherent mask it's still biting
> > you in the streaming mask.
> > 
> > > So any
> > > platform-specific DMA-mask setting will be overwritten, and the
> > > DMA-mask setting won't be able to be moved/dropped due to the
> > > dma_alloc_coherent() method usage.
> > > 
> > > I have added a DW eDMA-engine support to the DW PCIe driver (you've
> > > already seen my patches) and the engine initialization is supposed to
> > > be performed after any basic initializations like CSRs mapping, data
> > > allocations, MSI, etc. Since DMA is performed by the controller itself
> > > it's required to have a correct DMA-mask set to the DW PCIe platform
> > > device otherwise any consequent mapping will be bounce buffered to the
> > > lowest 4GB even though the corresponding platform can support more
> > > than 4GB of memory (even our MIPS32 can) with DW eDMA easily reaching
> > > that memory. What would help me in this case if the MSI-buffer
> > > allocation procedure wouldn't change the device DMA-mask.  As an
> > > alternative to completely dropping the DMA-mask setting, the DMA-mask
> > > setup process could be moved to the low-level platform device drivers.
> > > It would be even more justified since the platform-specific device
> > > capabilities (like DW PCIe AXI-interface address-bus width) are
> > > unknown in the generic code.
> > > 
> > > As another alternative I could override the DMA-mask within the DW
> > > eDMA probe procedure. But that would make things more complicated than
> > > relying on the low-level platform drivers doing that.
> > > 
> > > > and
> > > > reserving the doorbell address in the wrong DMA address space (modulo the
> > > > other dma-ranges bug which also took far too long to figure out).
> > > 
> > > Actually current driver (without William patch) reserve the doorbell
> > > address in the correct DMA address space (if we don't take the
> > > dma-ranges settings into account).
> > 
> > No it does not. With or without this patch it is still passing the *platform
> > device* to the DMA API, which means the mapping is relative to the platform
> > address space, not PCI Mem space on the other side of the iATU. The fact
> > that the iATU's dma-ranges translation is erroneously applied to the
> > platform device at the moment is, as I have said, a bug.
> 
> Thanks for pointing this out. I agree this is a bug and I guess it hasn't
> really been a problem because there isn't really any DMA'ing going on. With the
> new eDMA device being introduced, this bug will likely need to be fixed.
> 
> > 
> > > It works as expected in case if the
> > > PCIe<->CPU space has one-on-one mapping (which is true in the most of
> > > the cases). The only thing which is wrong is the pointless DMA-mask
> > > update. I could have easily dropped it in my patchset. But after the
> > > @William patchset is applied I won't be able to do that due to using
> > > the dma_alloc_coherent() here.
> > 
> > Once again, it is not pointless. There is no guarantee that __GFP_DMA32 does
> > anything, since ZONE_DMA32 may not exist (per this patch), and the zones may
> > not be as expected anyway (look at what arm64 currently does if all RAM is
> > above 32 bits, but save those complaints for another thread).
> > 
> > > > At this
> > > > point I'd rather keep it since getting rid of the __GFP_DMA32 abuse is
> > > > objectively good. If losing one page of coherent memory is a measurably
> > > > significant problem for T1 once the other issues are worked out and that
> > > > series lands, then you're welcome to propose a change on top (but I would
> > > > prefer that all the drivers using this trick are changed consistently).
> > > 
> > > Regarding DMA-coherent allocation. I am not happy with losing a whole
> > > page of the dma-coherent memory, but we can live with that. What give
> > > additional difficulty for our eDMA-patches is the DMA-mask override.
> > > If you still insist on preserving the @William patchset as it is,
> > > where do you suggest for me to update the DMA-mask if the low-level
> > > driver won't be suitable for that anymore?
> > 
> > I'm not insisting anything, it's just that this patch is already reviewed
> > and queued, is a small step towards being less wrong overall, and dropping
> > it wouldn't actually solve any of your problems anyway, so I just feel that
> > being obstructive because it falls short of perfection isn't helpful.
> 
> Thanks for the responses Robin! I agree that we don't have a complete solution
> and need to fix this DMA address space bug, but dont think that's enough of
> a reason to drop this patch series. At the very least I think patch 1/2 which
> removes the ZONE_DMA32 dependency is a worthy patch to take for 6.1.
> 
> Thanks,
> Will
> 
> > 
> > Robin.

William McVicker Oct. 3, 2022, 4:40 p.m. UTC | #9

On 10/03/2022, Lorenzo Pieralisi wrote:
> On Fri, Sep 30, 2022 at 10:02:22AM -0700, William McVicker wrote:
> 
> [...]
> 
> > > > So now we have what we have. The DMA-mask is pointlessly changed for
> > > > something not really implying any DMA. That's why I insisted on
> > > > dropping it at the very least. Another reason I thought was also
> > > > appropriate was the default DMA-mask being set to 32bits anyway.
> > > > But you said we shouldn't rely on the default DMA-mask setting. So
> > > > ok, it doesn't count then. But it doesn't change the uselessness of the
> > > > DMA-mask change in the current driver.
> > > 
> > > As I keep saying, it *is* relevant to DMA. The MSI doorbell may not be
> > > accessing memory, but it is still a thing that occupies DMA address space
> > > like a mapping of memory does, and DMA masks are how we control how DMA
> > > address space is allocated. Unless and until we have an API for arbitrarily
> > > reserving DMA address space within a given range, the approach used here and
> > > in other drivers is the next best thing, however much you don't like it.
> > > 
> > > > > AFAIU the correct PCI device
> > > > > won't actually exist until we've got far enough through pci_host_probe(), so
> > > > > I'm not sure how to easily solve this :/
> > > > 
> > > > Walk over all PCIe devices detected on the PCIe-bus. Check their
> > > > MSI-capability flags. If any of them have no 64-bit MSI flag set, then
> > > > make sure the MSI-base address is allocated within 4GB memory region.
> > > > It isn't that easy to implement though...
> > > 
> > > It has nothing to do with capabilities (but also: consider hotplug). We
> > > simply need the host bridge PCI device to pass to the DMA API to ensure that
> > > the mapping/allocation is relative to PCI Mem space rather than system
> > > physical address space, because the two don't have to be identical. The
> > > challenge is how to reliably pick up that device and set up the doorbell
> > > *before* any other PCI devices also discovered by pci_host_probe() have a
> > > chance to start binding drivers and trying to request MSIs.
> > 
> > Maybe I can provide some more insights on my patches that may help you guys
> > understand the idea behind the MSI capabilities flag. Basically, on a given
> > Android phone there are going to be multiple PCIe endpoints -- wifi and modem
> > are good examples. Some of these PCIe endpoints may only support a 32-bit MSI
> > capability structure, but others could support a 64-bit MSI capability
> > structure. So my intent was to have the PCIe RC driver detect the endpoint
> > device's MSI capabilites during the EP device probe and then set the
> > PCI_MSI_FLAGS_64BIT accordingly before calling dw_pcie_host_init(). Since the
> > PCIe RC drivers are the ones to call dw_pcie_host_init(), we can dynamically
> > change the DMA mask and allocate the doorbell target address based on the
> > PCI_MSI_FLAGS_64BIT.
> 
> It seems to me we are all talking past each others to solve different
> problems, so I am going to reset this discussion given that we are
> in the merge window and I must finalise the PCI patch queue.
> 
> Patch (2/2) should be dropped IMO - I don't think the host bridge
> platform device DMA mask should depend on the root port MSI cap
> 64/32 bit addressing - I don't think that's the right thing to do.
> 
> We should keep this discussion going for the next cycle, I will
> drop patch (2/2) for the time being, sorry.

Thanks Lorenzo for taking patch 1/2. That solves my immediate issues. Let's
solve Serge's eDMA problem first. Then we can revisit the 64-bit MSI target
address issue.

Regards,
Will

> 
> Thanks,
> Lorenzo
> 
> > This all really gets a lot more complicated with the introduction of Serge's
> > eDMA device. So I full well expect dw_pcie_msi_host_init() to be re-factored
> > for that.
> > 
> > > 
> > > > > Of course *this* patch doesn't change any of that either, so it's no worse
> > > > > than the existing code and I don't see that dropping it helps you at all;
> > > > > the current driver is already trampling your 64-bit mask back to 32 bits
> > > > 
> > > > Yes, and by this pathset @William intend to fix the DMA-mask-override
> > > > behaviour by using the dma_alloc_coherent() method.
> > > 
> > > No, that is effectively unchanged. Whether it's a streaming mapping with
> > > dma_mask or a coherent allocation with coherent_dma_mask, masks are getting
> > > set way, it's the fact that it's on the wrong device that's the real
> > > problem.
> > > 
> > > If you expose the eDMA as a generic dmaengine device then there's every
> > > chance some consumer would make a streaming mapping with it, so even though
> > > the current code happens to not override the coherent mask it's still biting
> > > you in the streaming mask.
> > > 
> > > > So any
> > > > platform-specific DMA-mask setting will be overwritten, and the
> > > > DMA-mask setting won't be able to be moved/dropped due to the
> > > > dma_alloc_coherent() method usage.
> > > > 
> > > > I have added a DW eDMA-engine support to the DW PCIe driver (you've
> > > > already seen my patches) and the engine initialization is supposed to
> > > > be performed after any basic initializations like CSRs mapping, data
> > > > allocations, MSI, etc. Since DMA is performed by the controller itself
> > > > it's required to have a correct DMA-mask set to the DW PCIe platform
> > > > device otherwise any consequent mapping will be bounce buffered to the
> > > > lowest 4GB even though the corresponding platform can support more
> > > > than 4GB of memory (even our MIPS32 can) with DW eDMA easily reaching
> > > > that memory. What would help me in this case if the MSI-buffer
> > > > allocation procedure wouldn't change the device DMA-mask.  As an
> > > > alternative to completely dropping the DMA-mask setting, the DMA-mask
> > > > setup process could be moved to the low-level platform device drivers.
> > > > It would be even more justified since the platform-specific device
> > > > capabilities (like DW PCIe AXI-interface address-bus width) are
> > > > unknown in the generic code.
> > > > 
> > > > As another alternative I could override the DMA-mask within the DW
> > > > eDMA probe procedure. But that would make things more complicated than
> > > > relying on the low-level platform drivers doing that.
> > > > 
> > > > > and
> > > > > reserving the doorbell address in the wrong DMA address space (modulo the
> > > > > other dma-ranges bug which also took far too long to figure out).
> > > > 
> > > > Actually current driver (without William patch) reserve the doorbell
> > > > address in the correct DMA address space (if we don't take the
> > > > dma-ranges settings into account).
> > > 
> > > No it does not. With or without this patch it is still passing the *platform
> > > device* to the DMA API, which means the mapping is relative to the platform
> > > address space, not PCI Mem space on the other side of the iATU. The fact
> > > that the iATU's dma-ranges translation is erroneously applied to the
> > > platform device at the moment is, as I have said, a bug.
> > 
> > Thanks for pointing this out. I agree this is a bug and I guess it hasn't
> > really been a problem because there isn't really any DMA'ing going on. With the
> > new eDMA device being introduced, this bug will likely need to be fixed.
> > 
> > > 
> > > > It works as expected in case if the
> > > > PCIe<->CPU space has one-on-one mapping (which is true in the most of
> > > > the cases). The only thing which is wrong is the pointless DMA-mask
> > > > update. I could have easily dropped it in my patchset. But after the
> > > > @William patchset is applied I won't be able to do that due to using
> > > > the dma_alloc_coherent() here.
> > > 
> > > Once again, it is not pointless. There is no guarantee that __GFP_DMA32 does
> > > anything, since ZONE_DMA32 may not exist (per this patch), and the zones may
> > > not be as expected anyway (look at what arm64 currently does if all RAM is
> > > above 32 bits, but save those complaints for another thread).
> > > 
> > > > > At this
> > > > > point I'd rather keep it since getting rid of the __GFP_DMA32 abuse is
> > > > > objectively good. If losing one page of coherent memory is a measurably
> > > > > significant problem for T1 once the other issues are worked out and that
> > > > > series lands, then you're welcome to propose a change on top (but I would
> > > > > prefer that all the drivers using this trick are changed consistently).
> > > > 
> > > > Regarding DMA-coherent allocation. I am not happy with losing a whole
> > > > page of the dma-coherent memory, but we can live with that. What give
> > > > additional difficulty for our eDMA-patches is the DMA-mask override.
> > > > If you still insist on preserving the @William patchset as it is,
> > > > where do you suggest for me to update the DMA-mask if the low-level
> > > > driver won't be suitable for that anymore?
> > > 
> > > I'm not insisting anything, it's just that this patch is already reviewed
> > > and queued, is a small step towards being less wrong overall, and dropping
> > > it wouldn't actually solve any of your problems anyway, so I just feel that
> > > being obstructive because it falls short of perfection isn't helpful.
> > 
> > Thanks for the responses Robin! I agree that we don't have a complete solution
> > and need to fix this DMA address space bug, but dont think that's enough of
> > a reason to drop this patch series. At the very least I think patch 1/2 which
> > removes the ZONE_DMA32 dependency is a worthy patch to take for 6.1.
> > 
> > Thanks,
> > Will
> > 
> > > 
> > > Robin.

Serge Semin Oct. 7, 2022, 10:45 p.m. UTC | #10

On Fri, Sep 30, 2022 at 04:39:05PM +0100, Robin Murphy wrote:
> On 2022-09-30 13:57, Serge Semin wrote:
> > On Fri, Sep 30, 2022 at 12:01:58PM +0100, Robin Murphy wrote:
> > > On 2022-09-29 20:32, Serge Semin wrote:
> > > > On Thu, Sep 29, 2022 at 07:25:03PM +0100, Robin Murphy wrote:
> > > > > On 2022-09-28 12:41, Serge Semin wrote:
> > > > > > On Thu, Aug 25, 2022 at 06:50:24PM +0000, Will McVicker wrote:
> > > > > > > Re-work the msi_msg DMA allocation logic to use dmam_alloc_coherent() which
> > > > > > > uses the coherent DMA mask to try to return an allocation within the DMA
> > > > > > > mask limits. With that, we now can drop the msi_page parameter in struct
> > > > > > > dw_pcie_rp. This allows kernel configurations that disable ZONE_DMA32 to
> > > > > > > continue supporting a 32-bit DMA mask. Without this patch, the PCIe host
> > > > > > > device will fail to probe when ZONE_DMA32 is disabled.
> > > > > > 
> > > > > > As Rob already said here
> > > > > > https://lore.kernel.org/all/CAL_JsqJh=d-B51b6yPBRq0tOwbChN=[e-mail]/
> > > > > > and I mentioned in this thread
> > > > > > https://lore.kernel.org/linux-pci/20220912000211.ct6asuhhmnatje5e@mobilestation/
> > > > > > DW PCIe MSI doesn't cause any DMA due to the way the iMSI-RX engine is
> > > > > > designed. So reserving any real system memory is a waste of one in
> > > > > > this case. Reserving DMA-coherent even more inappropriate since it
> > > > > > can be expensive on some platforms (see note in Part Ia of
> > > > > > Documentation/core-api/dma-api.rst). For instance on MIPS32 with
> > > > > > non-corehent common DMA.
> > > > > 
> > > > 
> > > > > This has been discussed before - in general it is difficult to pick an
> > > > > arbitrary MSI address that is *guaranteed* not to overlap any valid DMA
> > > > > address that somebody may try to use later. However there is a very easy way
> > > > > to guarantee that the DMA API won't give anyone a particular DMA address,
> > > > > which is to get an address directly from the DMA API and keep it. Yes, that
> > > > > can technically be done with a streaming mapping *if* you already have some
> > > > > memory allocated in a suitable physical location, but coherent allocations
> > > > > are even more foolproof, simpler to clean up (particularly with devres), and
> > > > > unlikely to be an issue on relevant platforms (do any MIPS32 systems use
> > > > > this driver?)
> > > > 
> > > > My patchset adds the DW PCIe RP controller support on MIPS32 arch:
> > > > https://lore.kernel.org/linux-pci/[e-mail]/
> > > > 
> > > > > 
> > > > > > > Fixes: 35797e672ff0 ("PCI: dwc: Fix MSI msi_msg DMA mapping")
> > > > > > > Reported-by: Isaac J. Manjarres <[e-mail]>
> > > > > > > Signed-off-by: Will McVicker <[e-mail]>
> > > > > > > Acked-by: Jingoo Han <[e-mail]>
> > > > > > > Reviewed-by: Rob Herring <[e-mail]>
> > > > > > > ---
> > > > > > >     .../pci/controller/dwc/pcie-designware-host.c | 28 +++++--------------
> > > > > > >     drivers/pci/controller/dwc/pcie-designware.h  |  1 -
> > > > > > >     2 files changed, 7 insertions(+), 22 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > > index 7746f94a715f..39f3b37d4033 100644
> > > > > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > > @@ -267,15 +267,6 @@ static void dw_pcie_free_msi(struct dw_pcie_rp *pp)
> > > > > > >     	irq_domain_remove(pp->msi_domain);
> > > > > > >     	irq_domain_remove(pp->irq_domain);
> > > > > > > -
> > > > > > > -	if (pp->msi_data) {
> > > > > > > -		struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > > > > > -		struct device *dev = pci->dev;
> > > > > > > -
> > > > > > > -		dma_unmap_page(dev, pp->msi_data, PAGE_SIZE, DMA_FROM_DEVICE);
> > > > > > > -		if (pp->msi_page)
> > > > > > > -			__free_page(pp->msi_page);
> > > > > > > -	}
> > > > > > >     }
> > > > > > >     static void dw_pcie_msi_init(struct dw_pcie_rp *pp)
> > > > > > > @@ -336,6 +327,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;
> > > > > > >     	int ret;
> > > > > > >     	u32 ctrl, num_ctrls;
> > > > > > > @@ -375,22 +367,16 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
> > > > > > >     						    dw_chained_msi_isr, pp);
> > > > > > >     	}
> > > > > > 
> > > > > > > -	ret = dma_set_mask(dev, DMA_BIT_MASK(32));
> > > > > > > +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
> > > > > > 
> > > > > > This has been redundant in the first place since none of the DW PCIe
> > > > > > low-level drivers update the mask, and it's of 32-bits wide by default
> > > > > > anyway:
> > > > > > https://elixir.bootlin.com/linux/latest/source/drivers/of/platform.c#L167
> > > > > 
> > > > 
> > > > > No, in general drivers should always explicitly set their mask(s) and check
> > > > > the return value to make sure DMA is possible at all before trying any other
> > > > > DMA API calls. There's no guarantee that the default mask is usable (e.g.
> > > > > some systems don't have any 32-bit addressable RAM), or that it's even
> > > > > always 32 bits (due to crufty reasons of something of_dma_configure() tried
> > > > > to do a long time ago).
> > > > 
> > > > Suppose you are right and DMA-mask should be always set before any
> > > > mapping. What do you suggest to do in this case? (1) The code above
> > > > overrides the real DMA-mask which could be set by the platform
> > > > drivers, which in its turn are normally aware of the device DMA
> > > > capabilities.
> > > 
> > > I am right. Appropriate DMA API usage as defined by the DMA API maintainers
> > > is not a matter of supposition. I literally just explained right there why
> > > drivers can't blindly assume the default mask is usable on modern systems
> > > (yes, it was different 20 years ago when system topologies were simpler).
> > > 
> > 
> > > However, having now gone and looked at the whole driver rather than unclear
> > > fragments of patch context, the code here *is* technically wrong. I've been
> > > mistakenly thinking all along that this was operating on the PCI device
> > > because I know that's what it *should* be doing, and seeing misleading
> > > things like "dev = pci->dev" falsely affirmed that assumption that it would
> > > be correct because it's been around for ages.
> > > AFAIU the correct PCI device
> > > won't actually exist until we've got far enough through pci_host_probe(), so
> > > I'm not sure how to easily solve this :/
> > 
> > Right. The code affected by the subject patch has nothing to do with
> > the real PCI devices. The DMA-mask is set to the DW PCIe Host controller
> > platform device in order to force a page being allocated within 32-bit
> > address space. That's it.
> > 
> > Here is a log of the related changes:
> > 
> > 0. Initially a GFP_KERNEL-based page was allocated for the MSI buffer.
> > It may cause having the DMA/PCIe-address above 4GB, which wouldn't work
> > for the PCIe peripherals with only 32-bit MSI capability. Though
> > nobody bothered back then.
> > 
> > 1. 07940c369a6b ("PCI: dwc: Fix MSI page leakage in suspend/resume")
> > After this commit nothing really has changed, but instead of
> > allocating the MSI-message pseudo-buffer turned to be embedded into
> > the private data. It could be allocated at any base address with no
> > actual limitation (because private data is kmalloc'ed).
> > 
> > 2. 660c486590aa ("PCI: dwc: Set 32-bit DMA mask for MSI target address allocation")
> > Someone found out that some devices failed to deliver MSI to the
> > address above 4GB of PCIe address space and fixed the problem by
> > force-setting the DMA-mask to being 32-bit before mapping the MSI
> > buffer. It indeed fixed the problem, but the actual buffer still left
> > being allocated from any address space. Instead, the mapping procedure
> > just bounced the buffer to 4GB space. So basically the solution was
> > very clumsy since turns a bounce buffer being reserved forever.
> > 
> > 3. 35797e672ff0 PCI: dwc: Fix MSI msi_msg DMA mapping
> > @William basically got things back to (0) but instead of GFP_KERNEL
> > the page was allocated from GFP_DMA32. At this stage he should have
> > dropped the DMA-mask setting too since the buffer was already
> > guaranteed to be within 4GB space, but he didn't.
> 
> I never saw that change, but frankly the justification in the commit message
> is wrong. I know that there are Android systems with memory above 32 bits
> that run with SWIOTLB disabled because they think they know what they're
> doing, which are almost certainly the same ones that also want to disable
> ZONE_DMA32 for similar reasons. That patch is really just another hack
> around an unexpected configuration, but without saying so.
> 
> > So now we have what we have. The DMA-mask is pointlessly changed for
> > something not really implying any DMA. That's why I insisted on
> > dropping it at the very least. Another reason I thought was also
> > appropriate was the default DMA-mask being set to 32bits anyway.
> > But you said we shouldn't rely on the default DMA-mask setting. So
> > ok, it doesn't count then. But it doesn't change the uselessness of the
> > DMA-mask change in the current driver.
> 

> As I keep saying, it *is* relevant to DMA. The MSI doorbell may not be
> accessing memory, but it is still a thing that occupies DMA address space
> like a mapping of memory does, and DMA masks are how we control how DMA
> address space is allocated. Unless and until we have an API for arbitrarily
> reserving DMA address space within a given range, the approach used here and
> in other drivers is the next best thing, however much you don't like it.

It's not that I don't like the approach. Now after the @William's
patch is merged in I don't really see a well suitable place for
setting the real DMA-mask of the DW PCIe controller for the eDMA
operations. I didn't want to set the mask in the DW eDMA probe
function since it was common for both eDMA embedded into a remote PCIe
function and installed into the local Root Port/End-point device. In
the first case the DMA-mask is already set in the dw-edma-pcie.c
driver. In order to cover the later case I was going to use the
low-level platform drivers. But after not being able to get rid from
the DMA-mask override performed in the MSI-setup procedure I will have
no choice to hack the eDMA driver somehow (see the last comment for
details).

> 
> > > AFAIU the correct PCI device
> > > won't actually exist until we've got far enough through pci_host_probe(), so
> > > I'm not sure how to easily solve this :/
> > 
> > Walk over all PCIe devices detected on the PCIe-bus. Check their
> > MSI-capability flags. If any of them have no 64-bit MSI flag set, then
> > make sure the MSI-base address is allocated within 4GB memory region.
> > It isn't that easy to implement though...
> 

> It has nothing to do with capabilities (but also: consider hotplug).

Actually it does. Based on the MSI 64-bit MSI flags state we would need
to chose either the pure 32-bit or 64/32-bit mask. Of course hotplug
would be a problem. But it's not the point in this case.

> We
> simply need the host bridge PCI device to pass to the DMA API to ensure that
> the mapping/allocation is relative to PCI Mem space rather than system
> physical address space, because the two don't have to be identical.

Right, that would have solved the problem with setting the DMA-mask to
a correct device. But the PCIe-space-related "dma-ranges" would be
still left applied to the DW PCIe platform device. Moreover in general
the PCIe host interface can have its own "dma-ranges". So we'll need
to have two types of the "dma-ranges": for the PCIe-bus to initialize
the inbound iATU and for the DW PCIe-controller itself. Furthermore
the later translations are applicable for the former addresses too... 

> The
> challenge is how to reliably pick up that device and set up the doorbell
> *before* any other PCI devices also discovered by pci_host_probe() have a
> chance to start binding drivers and trying to request MSIs.

There is no way at the current pci_host_probe() method implementation
because it performs the host bridge device registration and the
PCIe-bus scanning. There either have to be a hook to be called after
the bridge is registered or the pci_host_probe() would have to be
split up into two sub-methods: bridge-registration and scan child
buses. I have doubts that either case would be happily welcomed by
@Bjorn.

> 
> > > Of course *this* patch doesn't change any of that either, so it's no worse
> > > than the existing code and I don't see that dropping it helps you at all;
> > > the current driver is already trampling your 64-bit mask back to 32 bits
> > 
> > Yes, and by this pathset @William intend to fix the DMA-mask-override
> > behaviour by using the dma_alloc_coherent() method.
> 

> No, that is effectively unchanged.

I meant the same. It's unchanged, but if before the @William' patch we
could have drop the DMA-mask change at the assumption that the CPU and
PCIe spaces are identical and relying on the DMA32 zone availability,
now we can't because of the dma_alloc_coherent() method utilization.

> Whether it's a streaming mapping with
> dma_mask or a coherent allocation with coherent_dma_mask, masks are getting
> set way, it's the fact that it's on the wrong device that's the real
> problem.
> 
> If you expose the eDMA as a generic dmaengine device then there's every
> chance some consumer would make a streaming mapping with it, so even though
> the current code happens to not override the coherent mask it's still biting
> you in the streaming mask.

Right. It is. That's why I was trying to come up with an approach for
@William to drop the DMA-mask override at all, i.e. either rely on the
default DMA-mask state or just move the mask setting to the low-level
DW PCIe platform drivers.

> 
> > So any
> > platform-specific DMA-mask setting will be overwritten, and the
> > DMA-mask setting won't be able to be moved/dropped due to the
> > dma_alloc_coherent() method usage.
> > 
> > I have added a DW eDMA-engine support to the DW PCIe driver (you've
> > already seen my patches) and the engine initialization is supposed to
> > be performed after any basic initializations like CSRs mapping, data
> > allocations, MSI, etc. Since DMA is performed by the controller itself
> > it's required to have a correct DMA-mask set to the DW PCIe platform
> > device otherwise any consequent mapping will be bounce buffered to the
> > lowest 4GB even though the corresponding platform can support more
> > than 4GB of memory (even our MIPS32 can) with DW eDMA easily reaching
> > that memory. What would help me in this case if the MSI-buffer
> > allocation procedure wouldn't change the device DMA-mask.  As an
> > alternative to completely dropping the DMA-mask setting, the DMA-mask
> > setup process could be moved to the low-level platform device drivers.
> > It would be even more justified since the platform-specific device
> > capabilities (like DW PCIe AXI-interface address-bus width) are
> > unknown in the generic code.
> > 
> > As another alternative I could override the DMA-mask within the DW
> > eDMA probe procedure. But that would make things more complicated than
> > relying on the low-level platform drivers doing that.
> > 
> > > and
> > > reserving the doorbell address in the wrong DMA address space (modulo the
> > > other dma-ranges bug which also took far too long to figure out).
> > 
> > Actually current driver (without William patch) reserve the doorbell
> > address in the correct DMA address space (if we don't take the
> > dma-ranges settings into account).
> 

> No it does not. With or without this patch it is still passing the *platform
> device* to the DMA API, which means the mapping is relative to the platform
> address space, not PCI Mem space on the other side of the iATU. The fact
> that the iATU's dma-ranges translation is erroneously applied to the
> platform device at the moment is, as I have said, a bug.

That's why I noted "if we don't take the dma-ranges settings into
account". Taking out a wrong device utilization and if the CPU and PCIe
spaces are identical then passing the *platform device* to the DMA API
will do what is expected by the current code.

> 
> > It works as expected in case if the
> > PCIe<->CPU space has one-on-one mapping (which is true in the most of
> > the cases). The only thing which is wrong is the pointless DMA-mask
> > update. I could have easily dropped it in my patchset. But after the
> > @William patchset is applied I won't be able to do that due to using
> > the dma_alloc_coherent() here.
> 

> Once again, it is not pointless. There is no guarantee that __GFP_DMA32 does
> anything, since ZONE_DMA32 may not exist (per this patch), and the zones may
> not be as expected anyway (look at what arm64 currently does if all RAM is
> above 32 bits, but save those complaints for another thread).

Ok. It is indeed not pointless taking the fact that the __GFP_DMA32 can
do nothing. (I would have renamed the __GFP_DMA32 flag to something like
__GFP_DMA32_NOT_GUARANTEED then)

> 
> > > At this
> > > point I'd rather keep it since getting rid of the __GFP_DMA32 abuse is
> > > objectively good. If losing one page of coherent memory is a measurably
> > > significant problem for T1 once the other issues are worked out and that
> > > series lands, then you're welcome to propose a change on top (but I would
> > > prefer that all the drivers using this trick are changed consistently).
> > 
> > Regarding DMA-coherent allocation. I am not happy with losing a whole
> > page of the dma-coherent memory, but we can live with that. What give
> > additional difficulty for our eDMA-patches is the DMA-mask override.
> > If you still insist on preserving the @William patchset as it is,
> > where do you suggest for me to update the DMA-mask if the low-level
> > driver won't be suitable for that anymore?
> 

> I'm not insisting anything, it's just that this patch is already reviewed
> and queued, is a small step towards being less wrong overall, and dropping
> it wouldn't actually solve any of your problems anyway, so I just feel that
> being obstructive because it falls short of perfection isn't helpful.

I am starting to get confused because the problem has turned to be
multi-dimensional. We started discussing the DMA-mask override, while
now we are talking about a wrong device usage and the dma-ranges bug.
Let me sum up so we could come to some conclusions on each problem. 

1. This patch is already merged in by @Lorenzo.
The streaming and coherent DMA-masks are still forced to be of 32-bits
in the DW PCIe platform device. The low-level DW PCIe platform drivers
can't be used to set a correct mask now since it will be overwritten
by the MSI-setup procedure.

* Temporary solution: I'll override the DMA-mask in the eDMA driver:
try 64-bit mask, if failed - fallback to 32-bit mask. In fact I'll set
the DMA-mask to the DMAengine backing device since it will be used for
the correct DMA-mappings anyway (see problem 3.)

2. The DMA-mask is set to a wrong device in the framework of the
MSI-setup procedure.
Indeed it is supposed to be set to the PCI bridge device. But we can't
do that due to the pci_host_probe() method implementation. The device
is not ready for the DMA-mask update and the memory allocation/mapping
at the stage of the MSI-setup procedure execution. Isn't it? (Note the
empty instance is available at that stage.)

3. PCIe-bus-related DMA-ranges are applied to the PCIe host interface.
This is a bug. It can't be easily fixed due to the PCIe-specific
semantic of the "dma-ranges" OF-property. @Robin promised to think of
a proper solution for this.

* Temporary solution: I'll set the dma_chan.dev->chan_dma_dev flag to
true, just initialize the DMA-parameters of the DMA-channel backing
device instance by using the {of,acpi}_dma_configure() methods and drop the
dma_range_map assigned to it. In addition I'll set the DMA-mask of the
DMA-channel backing device too to solve the problem 1 (see above).

Sounds like a doable plan. What do you think?

-Sergey

> 
> Robin.

Patch

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 7746f94a715f..39f3b37d4033 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -267,15 +267,6 @@  static void dw_pcie_free_msi(struct dw_pcie_rp *pp)
 
 	irq_domain_remove(pp->msi_domain);
 	irq_domain_remove(pp->irq_domain);
-
-	if (pp->msi_data) {
-		struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
-		struct device *dev = pci->dev;
-
-		dma_unmap_page(dev, pp->msi_data, PAGE_SIZE, DMA_FROM_DEVICE);
-		if (pp->msi_page)
-			__free_page(pp->msi_page);
-	}
 }
 
 static void dw_pcie_msi_init(struct dw_pcie_rp *pp)
@@ -336,6 +327,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;
 	int ret;
 	u32 ctrl, num_ctrls;
 
@@ -375,22 +367,16 @@  static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
 						    dw_chained_msi_isr, pp);
 	}
 
-	ret = dma_set_mask(dev, DMA_BIT_MASK(32));
+	ret = dma_set_mask_and_coherent(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");
 
-	pp->msi_page = alloc_page(GFP_DMA32);
-	pp->msi_data = dma_map_page(dev, pp->msi_page, 0,
-				    PAGE_SIZE, DMA_FROM_DEVICE);
-	ret = dma_mapping_error(dev, pp->msi_data);
-	if (ret) {
-		dev_err(pci->dev, "Failed to map MSI data\n");
-		__free_page(pp->msi_page);
-		pp->msi_page = NULL;
-		pp->msi_data = 0;
+	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 ret;
+		return -ENOMEM;
 	}
 
 	return 0;
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 09b887093a84..a871ae7eb59e 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -243,7 +243,6 @@  struct dw_pcie_rp {
 	struct irq_domain	*irq_domain;
 	struct irq_domain	*msi_domain;
 	dma_addr_t		msi_data;
-	struct page		*msi_page;
 	struct irq_chip		*msi_irq_chip;
 	u32			num_vectors;
 	u32			irq_mask[MAX_MSI_CTRLS];