Access to controller region is not permitted.
use JobR's register space to access page 0 registers.

Fixes: 6a83830f649a ("crypto: caam - warn if blob_gen key is insecure")
Signed-off-by: Gaurav Jain <gaurav.jain@nxp.com>
---
 drivers/crypto/caam/blob_gen.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/caam/blob_gen.c b/drivers/crypto/caam/blob_gen.c
index 87781c1534ee..079a22cc9f02 100644
--- a/drivers/crypto/caam/blob_gen.c
+++ b/drivers/crypto/caam/blob_gen.c
@@ -2,6 +2,7 @@
 /*
  * Copyright (C) 2015 Pengutronix, Steffen Trumtrar <kernel@pengutronix.de>
  * Copyright (C) 2021 Pengutronix, Ahmad Fatoum <kernel@pengutronix.de>
+ * Copyright 2024 NXP
  */
 
 #define pr_fmt(fmt) "caam blob_gen: " fmt
@@ -104,7 +105,7 @@ int caam_process_blob(struct caam_blob_priv *priv,
 	}
 
 	ctrlpriv = dev_get_drvdata(jrdev->parent);
-	moo = FIELD_GET(CSTA_MOO, rd_reg32(&ctrlpriv->ctrl->perfmon.status));
+	moo = FIELD_GET(CSTA_MOO, rd_reg32(&ctrlpriv->jr[0]->perfmon.status));
 	if (moo != CSTA_MOO_SECURE && moo != CSTA_MOO_TRUSTED)
 		dev_warn(jrdev,
 			 "using insecure test key, enable HAB to use unique device key!\n");
-- 
2.25.1

Hello Guarav,

Thanks for your patch.

On 11.11.24 13:10, Gaurav Jain wrote:
> Access to controller region is not permitted.

It's permitted on most of the older SoCs. Please mention on which SoCs
this is no longer true and which SoCs you tested your change on.

> use JobR's register space to access page 0 registers.
> 
> Fixes: 6a83830f649a ("crypto: caam - warn if blob_gen key is insecure")

Did the CAAM even support any of the SoCs, where this doesn't work anymore
back when the code was mainlined?

> Signed-off-by: Gaurav Jain <gaurav.jain@nxp.com>
> ---
>  drivers/crypto/caam/blob_gen.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/crypto/caam/blob_gen.c b/drivers/crypto/caam/blob_gen.c
> index 87781c1534ee..079a22cc9f02 100644
> --- a/drivers/crypto/caam/blob_gen.c
> +++ b/drivers/crypto/caam/blob_gen.c
> @@ -2,6 +2,7 @@
>  /*
>   * Copyright (C) 2015 Pengutronix, Steffen Trumtrar <kernel@pengutronix.de>
>   * Copyright (C) 2021 Pengutronix, Ahmad Fatoum <kernel@pengutronix.de>
> + * Copyright 2024 NXP
>   */
>  
>  #define pr_fmt(fmt) "caam blob_gen: " fmt
> @@ -104,7 +105,7 @@ int caam_process_blob(struct caam_blob_priv *priv,
>  	}
>  
>  	ctrlpriv = dev_get_drvdata(jrdev->parent);
> -	moo = FIELD_GET(CSTA_MOO, rd_reg32(&ctrlpriv->ctrl->perfmon.status));
> +	moo = FIELD_GET(CSTA_MOO, rd_reg32(&ctrlpriv->jr[0]->perfmon.status));

I believe your change is correct, but I would prefer that ctrlpriv gets a perfmon
member that is initialized in caam_probe to either &ctrlpriv->ctrl->perfmon.status
or &ctrlpriv->jr[0]->perfmon.status and then the code here would just use
&ctrlpriv->perfmon->status.

This would simplify code not only here, but also in caam_ctrl_rng_init.

Thanks,
Ahmad


>  	if (moo != CSTA_MOO_SECURE && moo != CSTA_MOO_TRUSTED)
>  		dev_warn(jrdev,
>  			 "using insecure test key, enable HAB to use unique device key!\n");


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

On 11/11/2024 2:22 PM, Ahmad Fatoum wrote:
> Hello Guarav,
> 
> Thanks for your patch.
> 
> On 11.11.24 13:10, Gaurav Jain wrote:
[...]
>>  #define pr_fmt(fmt) "caam blob_gen: " fmt
>> @@ -104,7 +105,7 @@ int caam_process_blob(struct caam_blob_priv *priv,
>>  	}
>>  
>>  	ctrlpriv = dev_get_drvdata(jrdev->parent);
>> -	moo = FIELD_GET(CSTA_MOO, rd_reg32(&ctrlpriv->ctrl->perfmon.status));
>> +	moo = FIELD_GET(CSTA_MOO, rd_reg32(&ctrlpriv->jr[0]->perfmon.status));
> 
> I believe your change is correct, but I would prefer that ctrlpriv gets a perfmon
> member that is initialized in caam_probe to either &ctrlpriv->ctrl->perfmon.status
> or &ctrlpriv->jr[0]->perfmon.status and then the code here would just use
> &ctrlpriv->perfmon->status.
> 
> This would simplify code not only here, but also in caam_ctrl_rng_init.
> 
Agree.
But maybe this refactoring deserves a separate patch, given that is should also
cover the case when the Job Ring device corresponding to "ctrlpriv->jr[0]"
is unbounded from the driver.

Thanks,
Horia