[PATCH] crypto: caam - do not reset pointer size if caam_ptr_size is 64 bits

[PATCH] crypto: caam - do not reset pointer size if caam_ptr_size is 64 bits

[Iuliana Prodan]

In commit 'a1cf573ee95 ("crypto: caam - select DMA address
size at runtime")' CAAM pointer size (caam_ptr_size) is changed
from sizeof(dma_addr_t) to runtime value computed from MCFGR register.
At some point, the bit for Pointer Size should be reset to 0,
but only for the case when the CAAM pointer size if 32 bits.
Therefore, use caam_ptr_size instead of sizeof(dma_addr_t).

Fixes: a1cf573ee95 ("crypto: caam - select DMA address size at runtime")
Cc: <stable@vger.kernel.org>
Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com>
---
 drivers/crypto/caam/ctrl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index d7c3c38..786eef6 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c
@@ -674,7 +674,7 @@ static int caam_probe(struct platform_device *pdev)
 		clrsetbits_32(&ctrl->mcr, MCFGR_AWCACHE_MASK | MCFGR_LONG_PTR,
 			      MCFGR_AWCACHE_CACH | MCFGR_AWCACHE_BUFF |
 			      MCFGR_WDENABLE | MCFGR_LARGE_BURST |
-			      (sizeof(dma_addr_t) == sizeof(u64) ?
+			      (caam_ptr_sz == sizeof(u64) ?
 			       MCFGR_LONG_PTR : 0));
 
 	handle_imx6_err005766(&ctrl->mcr);
-- 
2.1.0

Re: [PATCH] crypto: caam - do not reset pointer size if caam_ptr_size is 64 bits

[Horia Geanta]

On 11/25/2019 12:33 AM, Iuliana Prodan wrote:
> In commit 'a1cf573ee95 ("crypto: caam - select DMA address
> size at runtime")' CAAM pointer size (caam_ptr_size) is changed
> from sizeof(dma_addr_t) to runtime value computed from MCFGR register.
> At some point, the bit for Pointer Size should be reset to 0,
> but only for the case when the CAAM pointer size if 32 bits.
> Therefore, use caam_ptr_size instead of sizeof(dma_addr_t).
> 
The logic is circular, see below.

> Fixes: a1cf573ee95 ("crypto: caam - select DMA address size at runtime")
Please Cc the author(s) when adding Fixes tag.

> Cc: <stable@vger.kernel.org>
> Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com>
> ---
>  drivers/crypto/caam/ctrl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
> index d7c3c38..786eef6 100644
> --- a/drivers/crypto/caam/ctrl.c
> +++ b/drivers/crypto/caam/ctrl.c
> @@ -674,7 +674,7 @@ static int caam_probe(struct platform_device *pdev)
>  		clrsetbits_32(&ctrl->mcr, MCFGR_AWCACHE_MASK | MCFGR_LONG_PTR,
>  			      MCFGR_AWCACHE_CACH | MCFGR_AWCACHE_BUFF |
>  			      MCFGR_WDENABLE | MCFGR_LARGE_BURST |
> -			      (sizeof(dma_addr_t) == sizeof(u64) ?
> +			      (caam_ptr_sz == sizeof(u64) ?
>  			       MCFGR_LONG_PTR : 0));
>  
Before this in caam_probe() there's:
	if (comp_params & CTPR_MS_PS && rd_reg32(&ctrl->mcr) & MCFGR_LONG_PTR)
		caam_ptr_sz = sizeof(u64);
	else
		caam_ptr_sz = sizeof(u32);

thus caam_ptr_sz is determined based on MCFGR:
MCFGR[PS] ==> caam_ptr_sz
so it no longer makes sense to reconfigure MCFGR based on caam_ptr_sz:
caam_ptr_sz ==> MCFGR[PS]

The short-term fix would be to no longer touch MCFGR[PS], which is in line with
commit a1cf573ee95 ("crypto: caam - select DMA address size at runtime")

Note however there's a small issue with the logic of kernel relying on
MCFGR[PS] being previously configured (by U-boot etc.), see
commit 39eaf759466f ("crypto: caam - fix pointer size for AArch64 boot loader, AArch32 kernel")

In the long run, IMO the proper thing for the driver to do is to rely,
whenever possible, on DT ("dma-ranges" property) / dma_mask provided by
the device driver infrastructure, and not on MCFGR[PS]:
[ DT "dma-ranges" ==> ] dma_mask ==> MCFGR[PS], caam_ptr_sz 
but that would probably involve too many changes for getting into -stable.

Horia

Re: [PATCH] crypto: caam - do not reset pointer size if caam_ptr_size is 64 bits

[Iuliana Prodan]

On 11/25/2019 9:49 AM, Horia Geanta wrote:
> On 11/25/2019 12:33 AM, Iuliana Prodan wrote:
>> In commit 'a1cf573ee95 ("crypto: caam - select DMA address
>> size at runtime")' CAAM pointer size (caam_ptr_size) is changed
>> from sizeof(dma_addr_t) to runtime value computed from MCFGR register.
>> At some point, the bit for Pointer Size should be reset to 0,
>> but only for the case when the CAAM pointer size if 32 bits.
>> Therefore, use caam_ptr_size instead of sizeof(dma_addr_t).
>>
> The logic is circular, see below.
> 
>> Fixes: a1cf573ee95 ("crypto: caam - select DMA address size at runtime")
> Please Cc the author(s) when adding Fixes tag.
> 
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com>
>> ---
>>   drivers/crypto/caam/ctrl.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
>> index d7c3c38..786eef6 100644
>> --- a/drivers/crypto/caam/ctrl.c
>> +++ b/drivers/crypto/caam/ctrl.c
>> @@ -674,7 +674,7 @@ static int caam_probe(struct platform_device *pdev)
>>   		clrsetbits_32(&ctrl->mcr, MCFGR_AWCACHE_MASK | MCFGR_LONG_PTR,
>>   			      MCFGR_AWCACHE_CACH | MCFGR_AWCACHE_BUFF |
>>   			      MCFGR_WDENABLE | MCFGR_LARGE_BURST |
>> -			      (sizeof(dma_addr_t) == sizeof(u64) ?
>> +			      (caam_ptr_sz == sizeof(u64) ?
>>   			       MCFGR_LONG_PTR : 0));
>>   
> Before this in caam_probe() there's:
> 	if (comp_params & CTPR_MS_PS && rd_reg32(&ctrl->mcr) & MCFGR_LONG_PTR)
> 		caam_ptr_sz = sizeof(u64);
> 	else
> 		caam_ptr_sz = sizeof(u32);
> 
> thus caam_ptr_sz is determined based on MCFGR:
> MCFGR[PS] ==> caam_ptr_sz
> so it no longer makes sense to reconfigure MCFGR based on caam_ptr_sz:
> caam_ptr_sz ==> MCFGR[PS]
> 
> The short-term fix would be to no longer touch MCFGR[PS], which is in line with
> commit a1cf573ee95 ("crypto: caam - select DMA address size at runtime")
> 
> Note however there's a small issue with the logic of kernel relying on
> MCFGR[PS] being previously configured (by U-boot etc.), see
> commit 39eaf759466f ("crypto: caam - fix pointer size for AArch64 boot loader, AArch32 kernel")
> 

So, what's your suggestion? We shouldn't reset the MCFGR[PS] at all? We 
should use the value set by u-boot?

Thanks,
Iulia

> In the long run, IMO the proper thing for the driver to do is to rely,
> whenever possible, on DT ("dma-ranges" property) / dma_mask provided by
> the device driver infrastructure, and not on MCFGR[PS]:
> [ DT "dma-ranges" ==> ] dma_mask ==> MCFGR[PS], caam_ptr_sz
> but that would probably involve too many changes for getting into -stable.
> 
> Horia
> 

Re: [PATCH] crypto: caam - do not reset pointer size if caam_ptr_size is 64 bits

[Horia Geanta]

On 11/25/2019 2:02 PM, Iuliana Prodan wrote:
> So, what's your suggestion? We shouldn't reset the MCFGR[PS] at all? We 
> should use the value set by u-boot?
> Yes.

Horia