[PATCH] PCI: dwc: Modify the check about MSI DMA mask 32-bit

[PATCH] PCI: dwc: Modify the check about MSI DMA mask 32-bit

[이왕석]

If dma_mask is more than 32 bits this will trigger an error occurs when
dma_map_single_attrs() is performed.

dma_map_single_attrs() -> dma_map_page_attrs()->
error return in dma_direct_map_page().

On ARTPEC-8, this fails with:
artpec8-pcie 17200000.pcie: DMA addr 0x0000000106b052c8+2 overflow
(mask ffffffff, bus limit 27fffffff)

There is no sequence that re-sets dev->dma_mask to more than 32 bits
before call dma_map_single_attrs().
The dev->dma_mask is first set just prior to the dw_pcie_host_init() call.
Therefore, the check logic was modified to be performed only when
the dev-dma_mask is not set larger than 32 bits.

Always setting dma_mask to 32 bits is not always correct,
for example the ARTPEC-8 is an arm64 platform, and can access
more than 32 bits

Signed-off-by: Wangseok Lee <wangseok.lee@samsung.com>
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 2fa86f3..7e25958 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -388,9 +388,11 @@ int dw_pcie_host_init(struct pcie_port *pp)
 							    dw_chained_msi_isr,
 							    pp);
 
-			ret = dma_set_mask(pci->dev, DMA_BIT_MASK(32));
-			if (ret)
-				dev_warn(pci->dev, "Failed to set DMA mask to 32-bit. Devices with only 32-bit MSI support may not work properly\n");
+			if (!(*dev->dma_mask &  ~(GENMASK(31, 0)))) {
+				ret = dma_set_mask(pci->dev, DMA_BIT_MASK(32));
+					if (ret)
+						dev_warn(pci->dev, "Failed to set DMA mask to 32-bit. Devices with only 32-bit MSI support may not work properly\n");
+			}
 
 			pp->msi_data = dma_map_single_attrs(pci->dev, &pp->msi_msg,
 						      sizeof(pp->msi_msg),
-- 
2.9.5

Re: [PATCH] PCI: dwc: Modify the check about MSI DMA mask 32-bit

[Alexander Lobakin]

From: 이왕석 <wangseok.lee@samsung.com>
Date: Mon, 28 Mar 2022 11:30:09 +0900

> If dma_mask is more than 32 bits this will trigger an error occurs when
> dma_map_single_attrs() is performed.
> 
> dma_map_single_attrs() -> dma_map_page_attrs()->
> error return in dma_direct_map_page().
> 
> On ARTPEC-8, this fails with:
> artpec8-pcie 17200000.pcie: DMA addr 0x0000000106b052c8+2 overflow
> (mask ffffffff, bus limit 27fffffff)

Isn't it a bug in the platform DMA code? dma_set_mask(32)
explicitly says that the system *must not* give DMA addresses wider
than 32 bits. If the system can't satisfy this requirement, then it
should return failure on dma_set_mask(32) -- this way you will only
get the corresponding warning, but there'll be no overflows (as the
mask will not be changed).
The idea of this call is to try to avoid getting 33+ bit mappings
so that PCI controllers which support only 32-bit masks could still
work correctly on the 64-bit systems. If the call fails, then this
message gets printed that you've been warned and it's your
responsibility to make sure that the controller won't get truncated
addresses. Having the call succeeded and then 33+ bit DMA addresses
is wrong.

Please correct me if I'm wrong.

> 
> There is no sequence that re-sets dev->dma_mask to more than 32 bits
> before call dma_map_single_attrs().
> The dev->dma_mask is first set just prior to the dw_pcie_host_init() call.
> Therefore, the check logic was modified to be performed only when
> the dev-dma_mask is not set larger than 32 bits.
> 
> Always setting dma_mask to 32 bits is not always correct,
> for example the ARTPEC-8 is an arm64 platform, and can access
> more than 32 bits
> 
> Signed-off-by: Wangseok Lee <wangseok.lee@samsung.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 2fa86f3..7e25958 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -388,9 +388,11 @@ int dw_pcie_host_init(struct pcie_port *pp)
>  							    dw_chained_msi_isr,
>  							    pp);
>  
> -			ret = dma_set_mask(pci->dev, DMA_BIT_MASK(32));
> -			if (ret)
> -				dev_warn(pci->dev, "Failed to set DMA mask to 32-bit. Devices with only 32-bit MSI support may not work properly\n");
> +			if (!(*dev->dma_mask &  ~(GENMASK(31, 0)))) {
> +				ret = dma_set_mask(pci->dev, DMA_BIT_MASK(32));
> +					if (ret)
> +						dev_warn(pci->dev, "Failed to set DMA mask to 32-bit. Devices with only 32-bit MSI support may not work properly\n");
> +			}
>  
>  			pp->msi_data = dma_map_single_attrs(pci->dev, &pp->msi_msg,
>  						      sizeof(pp->msi_msg),
> -- 
> 2.9.5

Thanks,
Al

RE: Re: [PATCH] PCI: dwc: Modify the check about MSI DMA mask 32-bit

[이왕석]

> --------- Original Message ---------
> Sender : Alexander Lobakin <alexandr.lobakin@intel.com>
> Date : 2022-03-28 23:34 (GMT+9)
> Title : Re: [PATCH] PCI: dwc: Modify the check about MSI DMA mask 32-bit
> 
> From: 이왕석 <wangseok.lee@samsung.com>
> Date: Mon, 28 Mar 2022 11:30:09 +0900
> 
>> If dma_mask is more than 32 bits this will trigger an error occurs when
>> dma_map_single_attrs() is performed.
>> 
>> dma_map_single_attrs() -> dma_map_page_attrs()->
>> error return in dma_direct_map_page().
>> 
>> On ARTPEC-8, this fails with:
>> artpec8-pcie 17200000.pcie: DMA addr 0x0000000106b052c8+2 overflow
>> (mask ffffffff, bus limit 27fffffff)
> 
> Isn't it a bug in the platform DMA code? dma_set_mask(32)
> explicitly says that the system *must not* give DMA addresses wider
> than 32 bits. If the system can't satisfy this requirement, then it
> should return failure on dma_set_mask(32) -- this way you will only
> get the corresponding warning, but there'll be no overflows (as the
> mask will not be changed).
> The idea of this call is to try to avoid getting 33+ bit mappings
> so that PCI controllers which support only 32-bit masks could still
> work correctly on the 64-bit systems. If the call fails, then this
> message gets printed that you've been warned and it's your
> responsibility to make sure that the controller won't get truncated
> addresses. Having the call succeeded and then 33+ bit DMA addresses
> is wrong.
> 
> Please correct me if I'm wrong.
> 

Hello, Alexander Lobakin
Thanks for your review.

You are right.
My concern is that case of trying to use 33+bit dma mappings on 
64bit system.
It is about the call sequence of the functions related to dma 
setting, not the operation of the dma_set_mask() function.
If dma_set_mask(33+) is performed before dw_pcie_host_init()
for using 33+bit dma mapping, following error occurs 
in dma_map_single_attrs()
ex) DMA addr 0x0000000106b052c8+2 overflow 
   (mask ffffffff, bus limit 27fffffff)
dma_set_mask(33+) -> dw_pcie_host_init(): dma_set_mask(32) ->
dma_map_single_attrs() -> 
error return in dma_direct_map_page(): 
because dma addr is 33+ but masking value is 32

So if the user has already set dma_mask to 33+ in order to use 33+,
i suggested to modify dma_set_mask(32) not to be called.

Please let me know your opinion.

Thank you.

>> 
>> There is no sequence that re-sets dev->dma_mask to more than 32 bits
>> before call dma_map_single_attrs().
>> The dev->dma_mask is first set just prior to the dw_pcie_host_init() call.
>> Therefore, the check logic was modified to be performed only when
>> the dev-dma_mask is not set larger than 32 bits.
>> 
>> Always setting dma_mask to 32 bits is not always correct,
>> for example the ARTPEC-8 is an arm64 platform, and can access
>> more than 32 bits
>> 
>> Signed-off-by: Wangseok Lee <wangseok.lee@samsung.com>
>> ---
>>  drivers/pci/controller/dwc/pcie-designware-host.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
>> index 2fa86f3..7e25958 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>> @@ -388,9 +388,11 @@ int dw_pcie_host_init(struct pcie_port *pp)
>>                                                              dw_chained_msi_isr,
>>                                                              pp);
>>  
>> -                        ret = dma_set_mask(pci->dev, DMA_BIT_MASK(32));
>> -                        if (ret)
>> -                                dev_warn(pci->dev, "Failed to set DMA mask to 32-bit. Devices with only 32-bit MSI support may not work properly>\n");
>> +                        if (!(*dev->dma_mask &  ~(GENMASK(31, 0)))) {
>> +                                ret = dma_set_mask(pci->dev, DMA_BIT_MASK(32));
>> +                                        if (ret)
>> +                                                dev_warn(pci->dev, "Failed to set DMA mask to 32-bit. Devices with only 32-bit MSI support may not work properly\n");
>> +                        }
>>  
>>                          pp->msi_data = dma_map_single_attrs(pci->dev, &pp->msi_msg,
>>                                                        sizeof(pp->msi_msg),
>> -- 
>> 2.9.5
> 
> Thanks,
> Al

Re: [PATCH] PCI: dwc: Modify the check about MSI DMA mask 32-bit

[Alexander Lobakin]

From: 이왕석 <wangseok.lee@samsung.com>
Date: Wed, 30 Mar 2022 12:52:03 +0900

> > --------- Original Message ---------
> > Sender : Alexander Lobakin <alexandr.lobakin@intel.com>
> > Date : 2022-03-28 23:34 (GMT+9)
> > Title : Re: [PATCH] PCI: dwc: Modify the check about MSI DMA mask 32-bit
> >
> > From: 이왕석 <wangseok.lee@samsung.com>
> > Date: Mon, 28 Mar 2022 11:30:09 +0900
> >
> >>  If dma_mask is more than 32 bits this will trigger an error occurs when
> >>  dma_map_single_attrs() is performed.
> >>  
> >>  dma_map_single_attrs() -> dma_map_page_attrs()->
> >>  error return in dma_direct_map_page().
> >>  
> >>  On ARTPEC-8, this fails with:
> >>  artpec8-pcie 17200000.pcie: DMA addr 0x0000000106b052c8+2 overflow
> >>  (mask ffffffff, bus limit 27fffffff)
> >
> > Isn't it a bug in the platform DMA code? dma_set_mask(32)
> > explicitly says that the system *must not* give DMA addresses wider
> > than 32 bits. If the system can't satisfy this requirement, then it
> > should return failure on dma_set_mask(32) -- this way you will only
> > get the corresponding warning, but there'll be no overflows (as the
> > mask will not be changed).
> > The idea of this call is to try to avoid getting 33+ bit mappings
> > so that PCI controllers which support only 32-bit masks could still
> > work correctly on the 64-bit systems. If the call fails, then this
> > message gets printed that you've been warned and it's your
> > responsibility to make sure that the controller won't get truncated
> > addresses. Having the call succeeded and then 33+ bit DMA addresses
> > is wrong.
> >
> > Please correct me if I'm wrong.
> >
> 
> Hello, Alexander Lobakin
> Thanks for your review.
> 
> You are right.
> My concern is that case of trying to use 33+bit dma mappings on 
> 64bit system.
> It is about the call sequence of the functions related to dma 
> setting, not the operation of the dma_set_mask() function.
> If dma_set_mask(33+) is performed before dw_pcie_host_init()
> for using 33+bit dma mapping, following error occurs 
> in dma_map_single_attrs()
> ex) DMA addr 0x0000000106b052c8+2 overflow 
>    (mask ffffffff, bus limit 27fffffff)
> dma_set_mask(33+) -> dw_pcie_host_init(): dma_set_mask(32) ->
> dma_map_single_attrs() -> 
> error return in dma_direct_map_page(): 
> because dma addr is 33+ but masking value is 32
> 
> So if the user has already set dma_mask to 33+ in order to use 33+,
> i suggested to modify dma_set_mask(32) not to be called.
> 
> Please let me know your opinion.

I'm not super familiar with the DMA internals, so adding Chris here,
maybe he'd like to comment, but anyway, the lower/arch layer must
not give the DMA addresses wider than the number of bits passed to
dma_set_mask() if that call returned 0.

> 
> Thank you.

--- 8< ---

> >>  -- 
> >>  2.9.5
> >
> > Thanks,
> > Al

Thanks,
Al

Re: [PATCH] PCI: dwc: Modify the check about MSI DMA mask 32-bit

[Christoph Hellwig]

On Wed, Mar 30, 2022 at 11:35:26AM +0200, Alexander Lobakin wrote:
> I'm not super familiar with the DMA internals, so adding Chris here,
> maybe he'd like to comment, but anyway, the lower/arch layer must
> not give the DMA addresses wider than the number of bits passed to
> dma_set_mask() if that call returned 0.

So, the basic assumption in the kernel is that 32-bit DMA is always
supported, and dma_set_maks for that should not fail.  If the
system (or root port, internal interconnect) supports less than that
we'll bounce buffer.  But how and why would you hand out addresses
larger than that?  It really is not valid, but I can't even see how
it could happen.

Re: [PATCH] PCI: dwc: Modify the check about MSI DMA mask 32-bit

[Wangseok Lee]

Hi,

Could you please review this patch in the context of the following patch?

https://patchwork.ozlabs.org/project/linux-pci/patch/20220328023009epcms2p309a5dfc2ff29d0a9945f65799963193c@epcms2p3/

Thnak you.
> --------- Original Message ---------
> Sender : Wangseok Lee <wangseok.lee@samsung.com>Foundry Design Service팀(Foundry)/삼성전자
> Date : 2022-03-31 14:34 (GMT+9)
> Title : Re: [PATCH] PCI: dwc: Modify the check about MSI DMA mask 32-bit
> 
>> --------- Original Message ---------
>> Sender : Christoph Hellwig <hch@infradead.org>
>> Date : 2022-03-31 00:45 (GMT+9)
>> Title : Re: [PATCH] PCI: dwc: Modify the check about MSI DMA mask 32-bit
>> 
>> On Wed, Mar 30, 2022 at 11:35:26AM +0200, Alexander Lobakin wrote:
>>> I'm not super familiar with the DMA internals, so adding Chris here,
>>> maybe he'd like to comment, but anyway, the lower/arch layer must
>>> not give the DMA addresses wider than the number of bits passed to
>>> dma_set_mask() if that call returned 0.
>> 
>> So, the basic assumption in the kernel is that 32-bit DMA is always
>> supported, and dma_set_maks for that should not fail.  If the
>> system (or root port, internal interconnect) supports less than that
>> we'll bounce buffer.  But how and why would you hand out addresses
>> larger than that?  It really is not valid, but I can't even see how
>> it could happen.
> 
> Hello,
> thank you for your review.
> 
> Yes, the dma address should not be used in excess of the mask value
> set through dma_set_mask().
> 
> If I want to use 33+bit dma address on 64bit system, I have to call
> dma_set_mask(33+) again after below code in dw_pcie_host_init() is
> performed.
> This is because dw_pcie_host_init(32) is always called in 
> dw_pcie_host_init() without any conditions.
> Is this right?
> 
> Also, if I assign 33+bit dma address before dw_pcie_host_init() to 
> use 33+bit dma address on 64bit system, dma_set_mask(32) is called 
> and dma_mask is changed to 32 but dma address is maintained 33+,
> so error occurs.
> it is not a issue with the dma_set_mask() function, but the 
> called condition must need to be modified.
> 
> Thank you.

Re: [PATCH] PCI: dwc: Modify the check about MSI DMA mask 32-bit

[Christoph Hellwig]

On Fri, Apr 08, 2022 at 11:34:01AM +0900, Wangseok Lee wrote:
> Hi,
> 
> Could you please review this patch in the context of the following patch?
> 
> https://patchwork.ozlabs.org/project/linux-pci/patch/20220328023009epcms2p309a5dfc2ff29d0a9945f65799963193c@epcms2p

Isn't that the same (broken) patch?

Re: [PATCH] PCI: dwc: Modify the check about MSI DMA mask 32-bit

[이왕석]

> --------- Original Message ---------
> Sender : Christoph Hellwig <hch@infradead.org>
> Date : 2022-03-31 00:45 (GMT+9)
> Title : Re: [PATCH] PCI: dwc: Modify the check about MSI DMA mask 32-bit
> 
> On Wed, Mar 30, 2022 at 11:35:26AM +0200, Alexander Lobakin wrote:
>> I'm not super familiar with the DMA internals, so adding Chris here,
>> maybe he'd like to comment, but anyway, the lower/arch layer must
>> not give the DMA addresses wider than the number of bits passed to
>> dma_set_mask() if that call returned 0.
> 
> So, the basic assumption in the kernel is that 32-bit DMA is always
> supported, and dma_set_maks for that should not fail.  If the
> system (or root port, internal interconnect) supports less than that
> we'll bounce buffer.  But how and why would you hand out addresses
> larger than that?  It really is not valid, but I can't even see how
> it could happen.

Hello,
thank you for your review.

Yes, the dma address should not be used in excess of the mask value
set through dma_set_mask().

If I want to use 33+bit dma address on 64bit system, I have to call
dma_set_mask(33+) again after below code in dw_pcie_host_init() is
performed.
This is because dw_pcie_host_init(32) is always called in 
dw_pcie_host_init() without any conditions.
Is this right?

Also, if I assign 33+bit dma address before dw_pcie_host_init() to 
use 33+bit dma address on 64bit system, dma_set_mask(32) is called 
and dma_mask is changed to 32 but dma address is maintained 33+,
so error occurs.
it is not a issue with the dma_set_mask() function, but the 
called condition must need to be modified.

Thank you.

Re: [PATCH] PCI: dwc: Modify the check about MSI DMA mask 32-bit

[Wangseok Lee]

> --------- Original Message ---------
> Sender : Christoph Hellwig <hch@infradead.org>
> Date : 2022-04-08 14:06 (GMT+9)
> Title : Re: [PATCH] PCI: dwc: Modify the check about MSI DMA mask 32-bit
> 
> On Fri, Apr 08, 2022 at 11:34:01AM +0900, Wangseok Lee wrote:
>> Hi,
>> 
>> Could you please review this patch in the context of the following patch?
>> 
>> https://protect2.fireeye.com/v1/url?k=dff16c49-806a5556-dff0e706-000babdfecba-c817c3fb701d2897&q=1&e=5862d6bb-abdb-4e80-b515-8bc024accd0c&u=https%3A%2F%2Fpatchwork.ozlabs.org%2Fproject%2Flinux-pci%2Fpatch%2F20220328023009epcms2p309a5dfc2ff29d0a9945f65799963193c%40epcms2p
> 
> Isn't that the same (broken) patch?

yes, The same patch that was reviewing.
I would like to continue reviewing the pcie-designware-host.c patch below.
https://lore.kernel.org/all/20220328023009epcms2p309a5dfc2ff29d0a9945f65799963193c@epcms2p3/

Re: [PATCH] PCI: dwc: Modify the check about MSI DMA mask 32-bit

[Christoph Hellwig]

On Fri, Apr 08, 2022 at 02:32:46PM +0900, Wangseok Lee wrote:
> I would like to continue reviewing the pcie-designware-host.c patch below.
> https://lore.kernel.org/all/20220328023009epcms2p309a5dfc2ff29d0a9945f65799963193c@epcms2p3/

The only new comment that I have is that the existing code also looks
completely broken.  Why did the DMA_ATTR_SKIP_CPU_SYNC magically appear
in commit 07940c369a6b ("PCI: dwc: Fix MSI page leakage in suspend/resume")
and what is is tƦying to do except for completely breaking non-coherent
plaforms?

Re: [PATCH] PCI: dwc: Modify the check about MSI DMA mask 32-bit

[Lorenzo Pieralisi]

On Fri, Apr 08, 2022 at 02:32:46PM +0900, Wangseok Lee wrote:
> > --------- Original Message ---------
> > Sender : Christoph Hellwig <hch@infradead.org>
> > Date : 2022-04-08 14:06 (GMT+9)
> > Title : Re: [PATCH] PCI: dwc: Modify the check about MSI DMA mask 32-bit
> > 
> > On Fri, Apr 08, 2022 at 11:34:01AM +0900, Wangseok Lee wrote:
> >> Hi,
> >> 
> >> Could you please review this patch in the context of the following patch?
> >> 
> >> https://protect2.fireeye.com/v1/url?k=dff16c49-806a5556-dff0e706-000babdfecba-c817c3fb701d2897&q=1&e=5862d6bb-abdb-4e80-b515-8bc024accd0c&u=https%3A%2F%2Fpatchwork.ozlabs.org%2Fproject%2Flinux-pci%2Fpatch%2F20220328023009epcms2p309a5dfc2ff29d0a9945f65799963193c%40epcms2p
> > 
> > Isn't that the same (broken) patch?
> 
> yes, The same patch that was reviewing.
> I would like to continue reviewing the pcie-designware-host.c patch below.
> https://lore.kernel.org/all/20220328023009epcms2p309a5dfc2ff29d0a9945f65799963193c@epcms2p3/

Would you please instead provide call stack (full) details of the
problem you are trying to fix ? You received feedback already on the
information you provided - to understand where the problem is I would
ask you please the full call stack leading to the failure (inclusive of
kernel version, platform, firmware and whether you are using a vanilla
kernel or out of tree patches on top - in which case we can't really
help), it is impossible to comment further otherwise.

Thanks,
Lorenzo

Re: [PATCH] PCI: dwc: Modify the check about MSI DMA mask 32-bit

[Wangseok Lee]

> On Fri, Apr 08, 2022 at 02:32:46PM +0900, Wangseok Lee wrote:
> > --------- Original Message ---------
> > Sender : Christoph Hellwig <hch@infradead.org>
> > Date : 2022-04-08 14:06 (GMT+9)
> > Title : Re: [PATCH] PCI: dwc: Modify the check about MSI DMA mask 32-bit
> > 
> > On Fri, Apr 08, 2022 at 11:34:01AM +0900, Wangseok Lee wrote:
> >> Hi,
> >> 
> >> Could you please review this patch in the context of the following patch?
> >> 
> >> https://protect2.fireeye.com/v1/url?k=dff16c49-806a5556-dff0e706-000babdfecba-c817c3fb701d2897&q=1&e=5862d6bb-abdb-4e80-b515-8bc024accd0c&u=https%3A%2F%2Fpatchwork.ozlabs.org%2Fproject%2Flinux-pci%2Fpatch%2F20220328023009epcms2p309a5dfc2ff29d0a9945f65799963193c%40epcms2p
> > 
> > Isn't that the same (broken) patch?
> 
> yes, The same patch that was reviewing.
> I would like to continue reviewing the pcie-designware-host.c patch below.
> https://lore.kernel.org/all/20220328023009epcms2p309a5dfc2ff29d0a9945f65799963193c@epcms2p3/
> 
> Would you please instead provide call stack (full) details of the
> problem you are trying to fix ? You received feedback already on the
> information you provided - to understand where the problem is I would
> ask you please the full call stack leading to the failure (inclusive of
> kernel version, platform, firmware and whether you are using a vanilla
> kernel or out of tree patches on top - in which case we can't really
> help), it is impossible to comment further otherwise.
> 
> Thanks,
> Lorenzo

Take artpec-8 SoC using 64bit system as an example.
artpec-8 is currently upstreaming. 
However, I think the same phenomenon will occur 
in platform that uses other 64bit systems.

driver_init() ->
-> platform_dma_configure() / platform.c
  |-> of_dma_configure() 
     |-> of_dma_configure_id()
        :Here, set dma of 33+ address.
         dma_set_mask(0xf`ffff`ffff), bus_dma_limit(0xf`ffff`ffff)
-> artpec8_pcie_probe() / artpec-8 pcie driver code
  |-> dw_pcie_host_init() / pcie-designware-host.c
     |-> dma_set_mask(32)
         : Here, set the dma mask of 32 addresses.
     |-> dma_map_single_attrs() 
        |-> dma_map_page_attrs()
           |-> dma_direct_map_page()
              : Error return occurs here.
                dma address has 33+ address and 
                dma bus limit is 33+. 
                However, this is because the mask value 
                has 32 addresses.


Therefore, the dma_set_mask(32)(in dw_pcie_host_init())
was modified to be performed only when
the dev-dma_mask is not set larger than 32 bits.

Thank you.

Re: [PATCH] PCI: dwc: Modify the check about MSI DMA mask 32-bit

[Christoph Hellwig]

On Mon, Apr 11, 2022 at 03:59:05PM +0900, Wangseok Lee wrote:
> driver_init() ->
> -> platform_dma_configure() / platform.c
>   |-> of_dma_configure() 
>      |-> of_dma_configure_id()
>         :Here, set dma of 33+ address.
>          dma_set_mask(0xf`ffff`ffff), bus_dma_limit(0xf`ffff`ffff)

Where do we set a large than 32-bit dma mask here?  I can't find the
code, and if there is we need to fix it.  In Linux devices to come
up with 32-bit DMA masks for historical reasons (they really should
with a zero dma mask, but it is probably to lte to fix it).

> -> artpec8_pcie_probe() / artpec-8 pcie driver code
>   |-> dw_pcie_host_init() / pcie-designware-host.c
>      |-> dma_set_mask(32)
>          : Here, set the dma mask of 32 addresses.
>      |-> dma_map_single_attrs() 
>         |-> dma_map_page_attrs()
>            |-> dma_direct_map_page()
>               : Error return occurs here.
>                 dma address has 33+ address and 
>                 dma bus limit is 33+. 
>                 However, this is because the mask value 
>                 has 32 addresses.

If the dma_mask is set to 32-bits, we should never generate a
large dma address, but bounce if it would othewise generate a
larger address.

That being said I think this code would be much better off using
dma_alloc_coherent anyway.

> Therefore, the dma_set_mask(32)(in dw_pcie_host_init())
> was modified to be performed only when
> the dev-dma_mask is not set larger than 32 bits.

So what sets dev->dma_mask to a larger than 32-bit value here?
We need to find and fix that.

Re: [PATCH] PCI: dwc: Modify the check about MSI DMA mask 32-bit

[Wangseok Lee]

> On Mon, Apr 11, 2022 at 03:59:05PM +0900, Wangseok Lee wrote:
>> driver_init() ->
>> -> platform_dma_configure() / platform.c
>>   |-> of_dma_configure() 
>>      |-> of_dma_configure_id()
>>         :Here, set dma of 33+ address.
>>          dma_set_mask(0xf`ffff`ffff), bus_dma_limit(0xf`ffff`ffff)
> 
> Where do we set a large than 32-bit dma mask here?  I can't find the
> code, and if there is we need to fix it.  In Linux devices to come
> up with 32-bit DMA masks for historical reasons (they really should
> with a zero dma mask, but it is probably to lte to fix it).
> 
>> -> artpec8_pcie_probe() / artpec-8 pcie driver code
>>   |-> dw_pcie_host_init() / pcie-designware-host.c
>>      |-> dma_set_mask(32)
>>          : Here, set the dma mask of 32 addresses.
>>      |-> dma_map_single_attrs() 
>>         |-> dma_map_page_attrs()
>>            |-> dma_direct_map_page()
>>               : Error return occurs here.
>>                 dma address has 33+ address and 
>>                 dma bus limit is 33+. 
>>                 However, this is because the mask value 
>>                 has 32 addresses.
> 
> If the dma_mask is set to 32-bits, we should never generate a
> large dma address, but bounce if it would othewise generate a
> larger address.
> 
> That being said I think this code would be much better off using
> dma_alloc_coherent anyway.
> 
>> Therefore, the dma_set_mask(32)(in dw_pcie_host_init())
>> was modified to be performed only when
>> the dev-dma_mask is not set larger than 32 bits.
> 
> So what sets dev->dma_mask to a larger than 32-bit value here?
> We need to find and fix that.

At the code of of_dma_configure_id() of driver/of/device.c..
In the 64bit system, if the dma start addr is used as 0x1'0000'0000
and the size is used as 0xf'0000'0000, "u64 end" is 0xf'ffff'ffff. 
And the dma_mask value is changed from 0xffff'ffff'ffff'ffff to
0xf'ffff'ffffff due to the code below.

181 line, driver/of/device.c
-------------------------------------------------
end = dma_start + size - 1;
mask = DMA_BIT_MASK(ilog2(end) + 1);
dev->coherent_dma_mask &= mask;
*dev->dma_mask &= mask;
-------------------------------------------------
Please let me know if I'm mistaken.

Thank you.

Re: [PATCH] PCI: dwc: Modify the check about MSI DMA mask 32-bit

[Christoph Hellwig]

On Mon, Apr 11, 2022 at 06:47:44PM +0900, Wangseok Lee wrote:
> >> Therefore, the dma_set_mask(32)(in dw_pcie_host_init())
> >> was modified to be performed only when
> >> the dev-dma_mask is not set larger than 32 bits.
> > 
> > So what sets dev->dma_mask to a larger than 32-bit value here?
> > We need to find and fix that.
> 
> At the code of of_dma_configure_id() of driver/of/device.c..
> In the 64bit system, if the dma start addr is used as 0x1'0000'0000
> and the size is used as 0xf'0000'0000, "u64 end" is 0xf'ffff'ffff. 
> And the dma_mask value is changed from 0xffff'ffff'ffff'ffff to
> 0xf'ffff'ffffff due to the code below.

That does look rather wrong to me, as any limitation should only
be in the bus mask.  Unless I'm missing something (and Robin should
know the code much better than I do) we should do something like
the patch below:

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 874f031442dc7..b197861fcde08 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -113,8 +113,7 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
 {
 	const struct iommu_ops *iommu;
 	const struct bus_dma_region *map = NULL;
-	u64 dma_start = 0;
-	u64 mask, end, size = 0;
+	u64 dma_start = 0, size = 0;
 	bool coherent;
 	int ret;
 
@@ -156,6 +155,9 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
 			kfree(map);
 			return -EINVAL;
 		}
+
+		dev->bus_dma_limit = dma_start + size - 1;
+		dev->dma_range_map = map;
 	}
 
 	/*
@@ -169,25 +171,6 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
 		dev->dma_mask = &dev->coherent_dma_mask;
 	}
 
-	if (!size && dev->coherent_dma_mask)
-		size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1);
-	else if (!size)
-		size = 1ULL << 32;
-
-	/*
-	 * Limit coherent and dma mask based on size and default mask
-	 * set by the driver.
-	 */
-	end = dma_start + size - 1;
-	mask = DMA_BIT_MASK(ilog2(end) + 1);
-	dev->coherent_dma_mask &= mask;
-	*dev->dma_mask &= mask;
-	/* ...but only set bus limit and range map if we found valid dma-ranges earlier */
-	if (!ret) {
-		dev->bus_dma_limit = end;
-		dev->dma_range_map = map;
-	}
-
 	coherent = of_dma_is_coherent(np);
 	dev_dbg(dev, "device is%sdma coherent\n",
 		coherent ? " " : " not ");

Re: [PATCH] PCI: dwc: Modify the check about MSI DMA mask 32-bit

[Robin Murphy]

On 2022-04-11 13:54, Christoph Hellwig wrote:
> On Mon, Apr 11, 2022 at 06:47:44PM +0900, Wangseok Lee wrote:
>>>> �Therefore,�the�dma_set_mask(32)(in�dw_pcie_host_init())
>>>> �was�modified�to�be�performed�only�when
>>>> �the�dev-dma_mask�is�not�set�larger�than�32�bits.
>>>
>>> So�what�sets�dev->dma_mask�to�a�larger�than�32-bit�value�here?
>>> We�need�to�find�and�fix�that.
>>
>> At the code of of_dma_configure_id() of driver/of/device.c..
>> In the 64bit system, if the dma start addr is used as 0x1'0000'0000
>> and the size is used as 0xf'0000'0000, "u64 end" is 0xf'ffff'ffff.
>> And the dma_mask value is changed from 0xffff'ffff'ffff'ffff to
>> 0xf'ffff'ffffff due to the code below.
> 
> That does look rather wrong to me, as any limitation should only
> be in the bus mask.  Unless I'm missing something (and Robin should
> know the code much better than I do) we should do something like
> the patch below:

Yeah, there's some smelly history here... Originally, of_dma_configure() 
pre-set the masks as an attempt to impose any restriction represented by 
DT "dma-ranges" - the platform it was implemented in aid of happened to 
have a 31-bit DMA range, which may well have coloured some implicit 
assumptions. IIRC, when I first implemented the separate bus_dma_mask to 
properly solve the general constraint problem, I left the other 
mask-setting in place since even though it shouldn't have served any 
purpose any more, I figured it wasn't actively harmful, and by that 
point it had been around long enough that I was a little wary of opening 
a can of worms if anything *had* erroneously started relying on it.

I'm not against making the change now though - I'm about to get to the 
point of turning all the dma_configure stuff inside-out in the course of 
my IOMMU rework anyway, so I fully expect to be breaking things and 
picking up the pieces. Getting this in first so it's easily bisectable 
and leaves me less code to further break seems most sensible :)

If you're happy to write up the patch, please also do the equivalent for 
acpi_arch_dma_setup() too.

This is all orthogonal to why the original patch in this thread is 
wrong, though. If the pcie-designware driver could somehow guarantee 
that all endpoint functions present, or able to appear later, can handle 
MSI addresses of some width >32, then it could set its DMA mask for the 
fake DMA mapping accordingly, but that has nothing at all to do with how 
many address bits might happen to be wired up on the external AXI interface.

Robin.

> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index 874f031442dc7..b197861fcde08 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -113,8 +113,7 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
>   {
>   	const struct iommu_ops *iommu;
>   	const struct bus_dma_region *map = NULL;
> -	u64 dma_start = 0;
> -	u64 mask, end, size = 0;
> +	u64 dma_start = 0, size = 0;
>   	bool coherent;
>   	int ret;
>   
> @@ -156,6 +155,9 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
>   			kfree(map);
>   			return -EINVAL;
>   		}
> +
> +		dev->bus_dma_limit = dma_start + size - 1;
> +		dev->dma_range_map = map;
>   	}
>   
>   	/*
> @@ -169,25 +171,6 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
>   		dev->dma_mask = &dev->coherent_dma_mask;
>   	}
>   
> -	if (!size && dev->coherent_dma_mask)
> -		size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1);
> -	else if (!size)
> -		size = 1ULL << 32;
> -
> -	/*
> -	 * Limit coherent and dma mask based on size and default mask
> -	 * set by the driver.
> -	 */
> -	end = dma_start + size - 1;
> -	mask = DMA_BIT_MASK(ilog2(end) + 1);
> -	dev->coherent_dma_mask &= mask;
> -	*dev->dma_mask &= mask;
> -	/* ...but only set bus limit and range map if we found valid dma-ranges earlier */
> -	if (!ret) {
> -		dev->bus_dma_limit = end;
> -		dev->dma_range_map = map;
> -	}
> -
>   	coherent = of_dma_is_coherent(np);
>   	dev_dbg(dev, "device is%sdma coherent\n",
>   		coherent ? " " : " not ");

Re: [PATCH] PCI: dwc: Modify the check about MSI DMA mask 32-bit

[Wangseok Lee]

On 2022-04-11 13:54, Christoph Hellwig wrote:
> Yeah, there's some smelly history here... Originally, of_dma_configure() 
> pre-set the masks as an attempt to impose any restriction represented by 
> DT "dma-ranges" - the platform it was implemented in aid of happened to 
> have a 31-bit DMA range, which may well have coloured some implicit 
> assumptions. IIRC, when I first implemented the separate bus_dma_mask to 
> properly solve the general constraint problem, I left the other 
> mask-setting in place since even though it shouldn't have served any 
> purpose any more, I figured it wasn't actively harmful, and by that 
> point it had been around long enough that I was a little wary of opening 
> a can of worms if anything *had* erroneously started relying on it.
> 
> I'm not against making the change now though - I'm about to get to the 
> point of turning all the dma_configure stuff inside-out in the course of 
> my IOMMU rework anyway, so I fully expect to be breaking things and 
> picking up the pieces. Getting this in first so it's easily bisectable 
> and leaves me less code to further break seems most sensible :)
> 
> If you're happy to write up the patch, please also do the equivalent for 
> acpi_arch_dma_setup() too.
> 
> This is all orthogonal to why the original patch in this thread is 
> wrong, though. If the pcie-designware driver could somehow guarantee 
> that all endpoint functions present, or able to appear later, can handle 
> MSI addresses of some width >32, then it could set its DMA mask for the 
> fake DMA mapping accordingly, but that has nothing at all to do with how 
> many address bits might happen to be wired up on the external AXI interface.
> 
> Robin.

Hi,
Thank you for your review.
It is clear there is something we don't understand here,
I'll return when we have done some more work in this.