| Message ID | 20200203101850.22570-1-matthias.schiffer@ew.tq-group.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | [RFC] crypto: caam: re-init JR on resume | expand |
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,
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
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,
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(-)