[RFC] crypto: caam: re-init JR on resume

[RFC] crypto: caam: re-init JR on resume

[Matthias Schiffer]

The JR loses its configuration during suspend-to-RAM (at least on
i.MX6UL). Re-initialize the hardware on resume.

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---

I've come across the issue that the CAAM would not work anymore after
deep sleep on i.MX6UL. It turned out that the CAAM loses its state
during suspend-to-RAM, so all registers read as zero and need to be
reinitialized.

This patch is my first attempt at fixing the issue. It seems to work
well enough, but I assume I'm missing some synchronization to prevent
that some CAAM operation is currently under way when the suspend
happens? I don't know the PM and crypto subsystems well enough to judge
if this is possible, and if it is, how to prevent it.

I've only compile-tested this version of the patch, as I had to port it
from our board kernel, which is based on the heavily-modified NXP branch.


 drivers/crypto/caam/intern.h |  3 ++
 drivers/crypto/caam/jr.c     | 62 +++++++++++++++++++++++++-----------
 2 files changed, 46 insertions(+), 19 deletions(-)

diff --git a/drivers/crypto/caam/intern.h b/drivers/crypto/caam/intern.h
index c7c10c90464b..5d2e9091d5c2 100644
--- a/drivers/crypto/caam/intern.h
+++ b/drivers/crypto/caam/intern.h
@@ -47,6 +47,9 @@ struct caam_drv_private_jr {
 	struct tasklet_struct irqtask;
 	int irq;			/* One per queue */
 
+	dma_addr_t inpbusaddr;
+	dma_addr_t outbusaddr;
+
 	/* Number of scatterlist crypt transforms active on the JobR */
 	atomic_t tfm_count ____cacheline_aligned;
 
diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c
index fc97cde27059..2dabf5fd7818 100644
--- a/drivers/crypto/caam/jr.c
+++ b/drivers/crypto/caam/jr.c
@@ -418,13 +418,31 @@ int caam_jr_enqueue(struct device *dev, u32 *desc,
 }
 EXPORT_SYMBOL(caam_jr_enqueue);
 
+static void caam_jr_setup_rings(struct caam_drv_private_jr *jrp)
+{
+	jrp->out_ring_read_index = 0;
+	jrp->head = 0;
+	jrp->tail = 0;
+
+	wr_reg64(&jrp->rregs->inpring_base, jrp->inpbusaddr);
+	wr_reg64(&jrp->rregs->outring_base, jrp->outbusaddr);
+	wr_reg32(&jrp->rregs->inpring_size, JOBR_DEPTH);
+	wr_reg32(&jrp->rregs->outring_size, JOBR_DEPTH);
+
+	jrp->inpring_avail = JOBR_DEPTH;
+
+	/* Select interrupt coalescing parameters */
+	clrsetbits_32(&jrp->rregs->rconfig_lo, 0, JOBR_INTC |
+		      (JOBR_INTC_COUNT_THLD << JRCFG_ICDCT_SHIFT) |
+		      (JOBR_INTC_TIME_THLD << JRCFG_ICTT_SHIFT));
+}
+
 /*
  * Init JobR independent of platform property detection
  */
 static int caam_jr_init(struct device *dev)
 {
 	struct caam_drv_private_jr *jrp;
-	dma_addr_t inpbusaddr, outbusaddr;
 	int i, error;
 
 	jrp = dev_get_drvdata(dev);
@@ -434,13 +452,13 @@ static int caam_jr_init(struct device *dev)
 		return error;
 
 	jrp->inpring = dmam_alloc_coherent(dev, SIZEOF_JR_INPENTRY *
-					   JOBR_DEPTH, &inpbusaddr,
+					   JOBR_DEPTH, &jrp->inpbusaddr,
 					   GFP_KERNEL);
 	if (!jrp->inpring)
 		return -ENOMEM;
 
 	jrp->outring = dmam_alloc_coherent(dev, SIZEOF_JR_OUTENTRY *
-					   JOBR_DEPTH, &outbusaddr,
+					   JOBR_DEPTH, &jrp->outbusaddr,
 					   GFP_KERNEL);
 	if (!jrp->outring)
 		return -ENOMEM;
@@ -453,24 +471,9 @@ static int caam_jr_init(struct device *dev)
 	for (i = 0; i < JOBR_DEPTH; i++)
 		jrp->entinfo[i].desc_addr_dma = !0;
 
-	/* Setup rings */
-	jrp->out_ring_read_index = 0;
-	jrp->head = 0;
-	jrp->tail = 0;
-
-	wr_reg64(&jrp->rregs->inpring_base, inpbusaddr);
-	wr_reg64(&jrp->rregs->outring_base, outbusaddr);
-	wr_reg32(&jrp->rregs->inpring_size, JOBR_DEPTH);
-	wr_reg32(&jrp->rregs->outring_size, JOBR_DEPTH);
-
-	jrp->inpring_avail = JOBR_DEPTH;
-
 	spin_lock_init(&jrp->inplock);
 
-	/* Select interrupt coalescing parameters */
-	clrsetbits_32(&jrp->rregs->rconfig_lo, 0, JOBR_INTC |
-		      (JOBR_INTC_COUNT_THLD << JRCFG_ICDCT_SHIFT) |
-		      (JOBR_INTC_TIME_THLD << JRCFG_ICTT_SHIFT));
+	caam_jr_setup_rings(jrp);
 
 	tasklet_init(&jrp->irqtask, caam_jr_dequeue, (unsigned long)dev);
 
@@ -486,6 +489,20 @@ static int caam_jr_init(struct device *dev)
 	return error;
 }
 
+static int caam_jr_reinit(struct device *dev)
+{
+	struct caam_drv_private_jr *jrp = dev_get_drvdata(dev);
+	int error;
+
+	error = caam_reset_hw_jr(dev);
+	if (error)
+		return error;
+
+	caam_jr_setup_rings(jrp);
+
+	return 0;
+}
+
 static void caam_jr_irq_dispose_mapping(void *data)
 {
 	irq_dispose_mapping((unsigned long)data);
@@ -578,10 +595,17 @@ static const struct of_device_id caam_jr_match[] = {
 };
 MODULE_DEVICE_TABLE(of, caam_jr_match);
 
+#ifdef CONFIG_PM
+static SIMPLE_DEV_PM_OPS(caam_jr_pm_ops, caam_reset_hw_jr, caam_jr_reinit);
+#endif
+
 static struct platform_driver caam_jr_driver = {
 	.driver = {
 		.name = "caam_jr",
 		.of_match_table = caam_jr_match,
+#ifdef CONFIG_PM
+		.pm = &caam_jr_pm_ops,
+#endif
 	},
 	.probe       = caam_jr_probe,
 	.remove      = caam_jr_remove,
-- 
2.17.1

Re: [RFC] crypto: caam: re-init JR on resume

[Matthias Schiffer]

On Mon, 2020-02-03 at 11:18 +0100, Matthias Schiffer wrote:
> The JR loses its configuration during suspend-to-RAM (at least on
> i.MX6UL). Re-initialize the hardware on resume.
> 
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
> 
> I've come across the issue that the CAAM would not work anymore after
> deep sleep on i.MX6UL. It turned out that the CAAM loses its state
> during suspend-to-RAM, so all registers read as zero and need to be
> reinitialized.
> 
> This patch is my first attempt at fixing the issue. It seems to work
> well enough, but I assume I'm missing some synchronization to prevent
> that some CAAM operation is currently under way when the suspend
> happens? I don't know the PM and crypto subsystems well enough to
> judge
> if this is possible, and if it is, how to prevent it.
> 
> I've only compile-tested this version of the patch, as I had to port
> it
> from our board kernel, which is based on the heavily-modified NXP
> branch.

It would be great to get some feedback on this patch. Is the hardware
support to lose its state? Does my fix look correct?

Kind regards,
Matthias



> 
> 
>  drivers/crypto/caam/intern.h |  3 ++
>  drivers/crypto/caam/jr.c     | 62 +++++++++++++++++++++++++---------
> --
>  2 files changed, 46 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/crypto/caam/intern.h
> b/drivers/crypto/caam/intern.h
> index c7c10c90464b..5d2e9091d5c2 100644
> --- a/drivers/crypto/caam/intern.h
> +++ b/drivers/crypto/caam/intern.h
> @@ -47,6 +47,9 @@ struct caam_drv_private_jr {
>  	struct tasklet_struct irqtask;
>  	int irq;			/* One per queue */
>  
> +	dma_addr_t inpbusaddr;
> +	dma_addr_t outbusaddr;
> +
>  	/* Number of scatterlist crypt transforms active on the JobR */
>  	atomic_t tfm_count ____cacheline_aligned;
>  
> diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c
> index fc97cde27059..2dabf5fd7818 100644
> --- a/drivers/crypto/caam/jr.c
> +++ b/drivers/crypto/caam/jr.c
> @@ -418,13 +418,31 @@ int caam_jr_enqueue(struct device *dev, u32
> *desc,
>  }
>  EXPORT_SYMBOL(caam_jr_enqueue);
>  
> +static void caam_jr_setup_rings(struct caam_drv_private_jr *jrp)
> +{
> +	jrp->out_ring_read_index = 0;
> +	jrp->head = 0;
> +	jrp->tail = 0;
> +
> +	wr_reg64(&jrp->rregs->inpring_base, jrp->inpbusaddr);
> +	wr_reg64(&jrp->rregs->outring_base, jrp->outbusaddr);
> +	wr_reg32(&jrp->rregs->inpring_size, JOBR_DEPTH);
> +	wr_reg32(&jrp->rregs->outring_size, JOBR_DEPTH);
> +
> +	jrp->inpring_avail = JOBR_DEPTH;
> +
> +	/* Select interrupt coalescing parameters */
> +	clrsetbits_32(&jrp->rregs->rconfig_lo, 0, JOBR_INTC |
> +		      (JOBR_INTC_COUNT_THLD << JRCFG_ICDCT_SHIFT) |
> +		      (JOBR_INTC_TIME_THLD << JRCFG_ICTT_SHIFT));
> +}
> +
>  /*
>   * Init JobR independent of platform property detection
>   */
>  static int caam_jr_init(struct device *dev)
>  {
>  	struct caam_drv_private_jr *jrp;
> -	dma_addr_t inpbusaddr, outbusaddr;
>  	int i, error;
>  
>  	jrp = dev_get_drvdata(dev);
> @@ -434,13 +452,13 @@ static int caam_jr_init(struct device *dev)
>  		return error;
>  
>  	jrp->inpring = dmam_alloc_coherent(dev, SIZEOF_JR_INPENTRY *
> -					   JOBR_DEPTH, &inpbusaddr,
> +					   JOBR_DEPTH, &jrp-
> >inpbusaddr,
>  					   GFP_KERNEL);
>  	if (!jrp->inpring)
>  		return -ENOMEM;
>  
>  	jrp->outring = dmam_alloc_coherent(dev, SIZEOF_JR_OUTENTRY *
> -					   JOBR_DEPTH, &outbusaddr,
> +					   JOBR_DEPTH, &jrp-
> >outbusaddr,
>  					   GFP_KERNEL);
>  	if (!jrp->outring)
>  		return -ENOMEM;
> @@ -453,24 +471,9 @@ static int caam_jr_init(struct device *dev)
>  	for (i = 0; i < JOBR_DEPTH; i++)
>  		jrp->entinfo[i].desc_addr_dma = !0;
>  
> -	/* Setup rings */
> -	jrp->out_ring_read_index = 0;
> -	jrp->head = 0;
> -	jrp->tail = 0;
> -
> -	wr_reg64(&jrp->rregs->inpring_base, inpbusaddr);
> -	wr_reg64(&jrp->rregs->outring_base, outbusaddr);
> -	wr_reg32(&jrp->rregs->inpring_size, JOBR_DEPTH);
> -	wr_reg32(&jrp->rregs->outring_size, JOBR_DEPTH);
> -
> -	jrp->inpring_avail = JOBR_DEPTH;
> -
>  	spin_lock_init(&jrp->inplock);
>  
> -	/* Select interrupt coalescing parameters */
> -	clrsetbits_32(&jrp->rregs->rconfig_lo, 0, JOBR_INTC |
> -		      (JOBR_INTC_COUNT_THLD << JRCFG_ICDCT_SHIFT) |
> -		      (JOBR_INTC_TIME_THLD << JRCFG_ICTT_SHIFT));
> +	caam_jr_setup_rings(jrp);
>  
>  	tasklet_init(&jrp->irqtask, caam_jr_dequeue, (unsigned
> long)dev);
>  
> @@ -486,6 +489,20 @@ static int caam_jr_init(struct device *dev)
>  	return error;
>  }
>  
> +static int caam_jr_reinit(struct device *dev)
> +{
> +	struct caam_drv_private_jr *jrp = dev_get_drvdata(dev);
> +	int error;
> +
> +	error = caam_reset_hw_jr(dev);
> +	if (error)
> +		return error;
> +
> +	caam_jr_setup_rings(jrp);
> +
> +	return 0;
> +}
> +
>  static void caam_jr_irq_dispose_mapping(void *data)
>  {
>  	irq_dispose_mapping((unsigned long)data);
> @@ -578,10 +595,17 @@ static const struct of_device_id
> caam_jr_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, caam_jr_match);
>  
> +#ifdef CONFIG_PM
> +static SIMPLE_DEV_PM_OPS(caam_jr_pm_ops, caam_reset_hw_jr,
> caam_jr_reinit);
> +#endif
> +
>  static struct platform_driver caam_jr_driver = {
>  	.driver = {
>  		.name = "caam_jr",
>  		.of_match_table = caam_jr_match,
> +#ifdef CONFIG_PM
> +		.pm = &caam_jr_pm_ops,
> +#endif
>  	},
>  	.probe       = caam_jr_probe,
>  	.remove      = caam_jr_remove,

Re: [RFC] crypto: caam: re-init JR on resume

[Horia Geanta]

On 2/21/2020 10:43 AM, Matthias Schiffer wrote:
> On Mon, 2020-02-03 at 11:18 +0100, Matthias Schiffer wrote:
>> The JR loses its configuration during suspend-to-RAM (at least on
>> i.MX6UL). Re-initialize the hardware on resume.
>>
>> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
>> ---
>>
>> I've come across the issue that the CAAM would not work anymore after
>> deep sleep on i.MX6UL. It turned out that the CAAM loses its state
>> during suspend-to-RAM, so all registers read as zero and need to be
>> reinitialized.
>>
>> This patch is my first attempt at fixing the issue. It seems to work
>> well enough, but I assume I'm missing some synchronization to prevent
>> that some CAAM operation is currently under way when the suspend
>> happens? I don't know the PM and crypto subsystems well enough to
>> judge
>> if this is possible, and if it is, how to prevent it.
>>
>> I've only compile-tested this version of the patch, as I had to port
>> it
>> from our board kernel, which is based on the heavily-modified NXP
>> branch.
> 
> It would be great to get some feedback on this patch. Is the hardware
> support to lose its state? Does my fix look correct?
> 
For most parts, yes, CAAM HW block loses state.

We are working at upstreaming PM support.

A non-exhaustive high-level list of things to be done, on top of your patch:
-caam controller driver (ctrl.c) also needs support for PM,
for e.g. RNG has to be reinitialized when resuming
-caam/jr driver: a few other registers have to be saved & restored
-caam/jr driver: flush/abort the jobs in the input ring when suspending
-implementations of algorithms using "split key" (a.k.a. "derived key"),
which is a black / encrypted key, have to convert the key to
a persistent blob since KEKs (JDKEK, TDKEK, TDSK registers) are lost
and in certain cases cannot be restored to initial values

Horia