| Message ID | 20230808105527.1707039-1-meenakshi.aggarwal@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | crypto: caam: fix unchecked return value error | expand |
Reviewed-by: Gaurav Jain <gaurav.jain@nxp.com> > -----Original Message----- > From: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> > Sent: Tuesday, August 8, 2023 4:25 PM > To: Horia Geanta <horia.geanta@nxp.com>; Varun Sethi <V.Sethi@nxp.com>; > Pankaj Gupta <pankaj.gupta@nxp.com>; Gaurav Jain <gaurav.jain@nxp.com>; > herbert@gondor.apana.org.au; davem@davemloft.net; linux- > crypto@vger.kernel.org; linux-kernel@vger.kernel.org > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> > Subject: [PATCH] crypto: caam/jr - fix shared IRQ line handling > > From: Horia Geantă <horia.geanta@nxp.com> > > There are cases when the interrupt status register (JRINTR) is non-zero, even > though: > 1. An interrupt was generated, but it was masked OR 2. There was no interrupt > generated at all for the corresponding job ring. > > 1. The case when interrupt is masked (JRCFGR_LS[IMSK]=1b'1) while other > events have happened and are being accounted for, e.g. > -JRINTR[HALT]=2b'10 - input job ring underwent a flush of all on-going jobs and > processing of still-existing jobs (sitting in the ring) has been halted > -JRINTR[HALT]=2b'01 - input job ring is currently undergoing a flush > -JRINTR[ENTER_FAIL]=1b'1 - SecMon / SNVS transitioned to FAIL MODE It > doesn't matter whether these events would assert the interrupt signal or not, > interrupt is anyhow masked. > > 2. The case when interrupt is not masked (JRCFGR_LS[IMSK]=1b'0), however the > events accounted for in JRINTR do not generate interrupts, e.g.: > -JRINTR[HALT]=2b'01 > -JRINTR[ENTER_FAIL]=1b'1 and JRCFGR_MS[FAIL_MODE]=1b'0 > > Currently in these cases, when the JR interrupt handler is invoked (as a > consequence of JR sharing the interrupt line with other devices - e.g. > the two JRs on i.MX7ULP) it continues execution instead of returning IRQ_NONE. > This could lead to situations like interrupt handler clearing JRINTR (and thus also > the JRINTR[HALT] field) while corresponding job ring is suspended and then that > job ring failing on resume path, due to expecting > JRINTR[HALT]=b'10 and reading instead JRINTR[HALT]=b'00. > > Fix this by checking status of JRINTR[JRI] in the JR interrupt handler. > If JRINTR[JRI]=1b'0, there was no interrupt generated for this JR and handler > must return IRQ_NONE. > > Signed-off-by: Horia Geantă <horia.geanta@nxp.com> > Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> > --- > drivers/crypto/caam/jr.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c index > 5507d5d34a4c..07ec2f27cc68 100644 > --- a/drivers/crypto/caam/jr.c > +++ b/drivers/crypto/caam/jr.c > @@ -232,7 +232,7 @@ static irqreturn_t caam_jr_interrupt(int irq, void *st_dev) > * tasklet if jobs done. > */ > irqstate = rd_reg32(&jrp->rregs->jrintstatus); > - if (!irqstate) > + if (!(irqstate & JRINT_JR_INT)) > return IRQ_NONE; > > /* > -- > 2.25.1
Reviewed-by: Gaurav Jain <gaurav.jain@nxp.com> > -----Original Message----- > From: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> > Sent: Tuesday, August 8, 2023 4:25 PM > To: Horia Geanta <horia.geanta@nxp.com>; Varun Sethi <V.Sethi@nxp.com>; > Pankaj Gupta <pankaj.gupta@nxp.com>; Gaurav Jain <gaurav.jain@nxp.com>; > herbert@gondor.apana.org.au; davem@davemloft.net; linux- > crypto@vger.kernel.org; linux-kernel@vger.kernel.org > Cc: Iuliana Prodan <iuliana.prodan@nxp.com>; Meenakshi Aggarwal > <meenakshi.aggarwal@nxp.com> > Subject: [PATCH] crypto: caam - increase the domain of write memory barrier to > full system > > From: Iuliana Prodan <iuliana.prodan@nxp.com> > > In caam_jr_enqueue, under heavy DDR load, smp_wmb() or dma_wmb() fail to > make the input ring be updated before the CAAM starts reading it. So, CAAM will > process, again, an old descriptor address and will put it in the output ring. This > will make caam_jr_dequeue() to fail, since this old descriptor is not in the > software ring. > To fix this, use wmb() which works on the full system instead of inner/outer > shareable domains. > > Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com> > Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> > --- > drivers/crypto/caam/jr.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c index > 767fbf052536..5507d5d34a4c 100644 > --- a/drivers/crypto/caam/jr.c > +++ b/drivers/crypto/caam/jr.c > @@ -464,8 +464,16 @@ int caam_jr_enqueue(struct device *dev, u32 *desc, > * Guarantee that the descriptor's DMA address has been written to > * the next slot in the ring before the write index is updated, since > * other cores may update this index independently. > + * > + * Under heavy DDR load, smp_wmb() or dma_wmb() fail to make the > input > + * ring be updated before the CAAM starts reading it. So, CAAM will > + * process, again, an old descriptor address and will put it in the > + * output ring. This will make caam_jr_dequeue() to fail, since this > + * old descriptor is not in the software ring. > + * To fix this, use wmb() which works on the full system instead of > + * inner/outer shareable domains. > */ > - smp_wmb(); > + wmb(); > > jrp->head = (head + 1) & (JOBR_DEPTH - 1); > > -- > 2.25.1
Reviewed-by: Gaurav Jain <gaurav.jain@nxp.com> > -----Original Message----- > From: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> > Sent: Tuesday, August 8, 2023 4:25 PM > To: Horia Geanta <horia.geanta@nxp.com>; Varun Sethi <V.Sethi@nxp.com>; > Pankaj Gupta <pankaj.gupta@nxp.com>; Gaurav Jain <gaurav.jain@nxp.com>; > herbert@gondor.apana.org.au; davem@davemloft.net; linux- > crypto@vger.kernel.org; linux-kernel@vger.kernel.org > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> > Subject: [PATCH] crypto: caam: fix unchecked return value error > > From: Gaurav Jain <gaurav.jain@nxp.com> > > error: > Unchecked return value (CHECKED_RETURN) > check_return: Calling sg_miter_next without checking return value > > fix: > added check if(!sg_miter_next) > > Fixes: 8a2a0dd35f2e ("crypto: caam - strip input zeros from RSA input buffer") > Signed-off-by: Gaurav Jain <gaurav.jain@nxp.com> > Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> > --- > drivers/crypto/caam/caampkc.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/crypto/caam/caampkc.c b/drivers/crypto/caam/caampkc.c > index 72afc249d42f..7e08af751e4e 100644 > --- a/drivers/crypto/caam/caampkc.c > +++ b/drivers/crypto/caam/caampkc.c > @@ -225,7 +225,9 @@ static int caam_rsa_count_leading_zeros(struct > scatterlist *sgl, > if (len && *buff) > break; > > - sg_miter_next(&miter); > + if (!sg_miter_next(&miter)) > + break; > + > buff = miter.addr; > len = miter.length; > > -- > 2.25.1
On Tue, Aug 08, 2023 at 12:55:26PM +0200, meenakshi.aggarwal@nxp.com wrote: > From: Iuliana Prodan <iuliana.prodan@nxp.com> > > In caam_jr_enqueue, under heavy DDR load, smp_wmb() or dma_wmb() > fail to make the input ring be updated before the CAAM starts > reading it. So, CAAM will process, again, an old descriptor address > and will put it in the output ring. This will make caam_jr_dequeue() > to fail, since this old descriptor is not in the software ring. > To fix this, use wmb() which works on the full system instead of > inner/outer shareable domains. > > Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com> > Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> > --- > drivers/crypto/caam/jr.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) Indeed, smp_wmb is always wrong for barriers separating DMA writes. I wonder if these should be changed to: $ git grep smp_wmb drivers/crypto/ drivers/crypto/caam/jr.c: smp_wmb(); drivers/crypto/cavium/cpt/cptvf_reqmanager.c: smp_wmb(); drivers/crypto/hisilicon/qm.c: smp_wmb(); drivers/crypto/marvell/octeontx/otx_cptvf_reqmgr.c: smp_wmb(); drivers/crypto/marvell/octeontx2/otx2_cptpf_mbox.c: smp_wmb(); drivers/crypto/marvell/octeontx2/otx2_cptpf_mbox.c: smp_wmb(); drivers/crypto/marvell/octeontx2/otx2_cptpf_mbox.c: smp_wmb(); drivers/crypto/marvell/octeontx2/otx2_cptpf_mbox.c: smp_wmb(); drivers/crypto/talitos.c: smp_wmb(); drivers/crypto/talitos.c: smp_wmb(); $ Cheers,
On Tue, Aug 08, 2023 at 12:55:25PM +0200, meenakshi.aggarwal@nxp.com wrote: > From: Gaurav Jain <gaurav.jain@nxp.com> > > error: > Unchecked return value (CHECKED_RETURN) > check_return: Calling sg_miter_next without checking return value > > fix: > added check if(!sg_miter_next) > > Fixes: 8a2a0dd35f2e ("crypto: caam - strip input zeros from RSA input buffer") > Signed-off-by: Gaurav Jain <gaurav.jain@nxp.com> > Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> > --- > drivers/crypto/caam/caampkc.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) Patch applied. Thanks.
On Tue, Aug 08, 2023 at 12:55:26PM +0200, meenakshi.aggarwal@nxp.com wrote: > From: Iuliana Prodan <iuliana.prodan@nxp.com> > > In caam_jr_enqueue, under heavy DDR load, smp_wmb() or dma_wmb() > fail to make the input ring be updated before the CAAM starts > reading it. So, CAAM will process, again, an old descriptor address > and will put it in the output ring. This will make caam_jr_dequeue() > to fail, since this old descriptor is not in the software ring. > To fix this, use wmb() which works on the full system instead of > inner/outer shareable domains. > > Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com> > Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> > --- > drivers/crypto/caam/jr.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) Patch applied. Thanks.
On Tue, Aug 08, 2023 at 12:55:27PM +0200, meenakshi.aggarwal@nxp.com wrote: > From: Horia Geantă <horia.geanta@nxp.com> > > There are cases when the interrupt status register (JRINTR) is non-zero, > even though: > 1. An interrupt was generated, but it was masked OR > 2. There was no interrupt generated at all > for the corresponding job ring. > > 1. The case when interrupt is masked (JRCFGR_LS[IMSK]=1b'1) > while other events have happened and are being accounted for, e.g. > -JRINTR[HALT]=2b'10 - input job ring underwent a flush of all on-going > jobs and processing of still-existing jobs (sitting in the ring) has been > halted > -JRINTR[HALT]=2b'01 - input job ring is currently undergoing a flush > -JRINTR[ENTER_FAIL]=1b'1 - SecMon / SNVS transitioned to FAIL MODE > It doesn't matter whether these events would assert the interrupt signal > or not, interrupt is anyhow masked. > > 2. The case when interrupt is not masked (JRCFGR_LS[IMSK]=1b'0), however > the events accounted for in JRINTR do not generate interrupts, e.g.: > -JRINTR[HALT]=2b'01 > -JRINTR[ENTER_FAIL]=1b'1 and JRCFGR_MS[FAIL_MODE]=1b'0 > > Currently in these cases, when the JR interrupt handler is invoked (as a > consequence of JR sharing the interrupt line with other devices - e.g. > the two JRs on i.MX7ULP) it continues execution instead of returning > IRQ_NONE. > This could lead to situations like interrupt handler clearing JRINTR (and > thus also the JRINTR[HALT] field) while corresponding job ring is > suspended and then that job ring failing on resume path, due to expecting > JRINTR[HALT]=b'10 and reading instead JRINTR[HALT]=b'00. > > Fix this by checking status of JRINTR[JRI] in the JR interrupt handler. > If JRINTR[JRI]=1b'0, there was no interrupt generated for this JR and > handler must return IRQ_NONE. > > Signed-off-by: Horia Geantă <horia.geanta@nxp.com> > Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> > --- > drivers/crypto/caam/jr.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Patch applied. Thanks.
diff --git a/drivers/crypto/caam/caampkc.c b/drivers/crypto/caam/caampkc.c index 72afc249d42f..7e08af751e4e 100644 --- a/drivers/crypto/caam/caampkc.c +++ b/drivers/crypto/caam/caampkc.c @@ -225,7 +225,9 @@ static int caam_rsa_count_leading_zeros(struct scatterlist *sgl, if (len && *buff) break; - sg_miter_next(&miter); + if (!sg_miter_next(&miter)) + break; + buff = miter.addr; len = miter.length;