| Message ID | 20220416135412.4109213-1-festevam@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | [v2] crypto: caam - fix i.MX6SX entropy delay value | expand |
Hi Horia and Varun, On Sat, Apr 16, 2022 at 10:54 AM Fabio Estevam <festevam@gmail.com> wrote: > > From: Fabio Estevam <festevam@denx.de> > > Since commit 358ba762d9f1 ("crypto: caam - enable prediction resistance > in HRWNG") the following CAAM errors can be seen on i.MX6SX: > > caam_jr 2101000.jr: 20003c5b: CCB: desc idx 60: RNG: Hardware error > hwrng: no data available > caam_jr 2101000.jr: 20003c5b: CCB: desc idx 60: RNG: Hardware error > ... > > This error is due to an incorrect entropy delay for i.MX6SX. > > Fix it by increasing the minimum entropy delay for i.MX6SX > as done in U-Boot: > https://patchwork.ozlabs.org/project/uboot/patch/20220415111049.2565744-1-gaurav.jain@nxp.com/ > > Signed-off-by: Fabio Estevam <festevam@denx.de> > --- > Change since v1: > - Align the fix with U-Boot. Actually, after thinking more about it, I realize that this issue is not i.MX6SX specific as I have seen reports of the same failures on i.MX6D as well. Would it make sense to fix it like this instead? --- a/drivers/crypto/caam/regs.h +++ b/drivers/crypto/caam/regs.h @@ -516,7 +516,7 @@ struct rng4tst { }; #define RTSDCTL_ENT_DLY_SHIFT 16 #define RTSDCTL_ENT_DLY_MASK (0xffff << RTSDCTL_ENT_DLY_SHIFT) -#define RTSDCTL_ENT_DLY_MIN 3200 +#define RTSDCTL_ENT_DLY_MIN 12000 #define RTSDCTL_ENT_DLY_MAX 12800 u32 rtsdctl; /* seed control register */ union { Any drawbacks in using this generic approach? Please advise.
Hi Fabio, Vabhav is working on a fix for the Linux driver. He would be introducing a new property in the CAAM device tree node, which would be used for specifying the entropy delay value. This would make the solution generic. This property is optional. Also, please note that the entropy delay value has been derived for i.MX6SX after a silicon characterization study. Regards Varun > -----Original Message----- > From: Fabio Estevam <festevam@gmail.com> > Sent: Saturday, April 16, 2022 7:24 PM > To: herbert@gondor.apana.org.au > Cc: Horia Geanta <horia.geanta@nxp.com>; Gaurav Jain > <gaurav.jain@nxp.com>; Varun Sethi <V.Sethi@nxp.com>; linux- > crypto@vger.kernel.org; Fabio Estevam <festevam@denx.de> > Subject: [EXT] [PATCH v2] crypto: caam - fix i.MX6SX entropy delay value > > Caution: EXT Email > > From: Fabio Estevam <festevam@denx.de> > > Since commit 358ba762d9f1 ("crypto: caam - enable prediction resistance in > HRWNG") the following CAAM errors can be seen on i.MX6SX: > > caam_jr 2101000.jr: 20003c5b: CCB: desc idx 60: RNG: Hardware error > hwrng: no data available > caam_jr 2101000.jr: 20003c5b: CCB: desc idx 60: RNG: Hardware error ... > > This error is due to an incorrect entropy delay for i.MX6SX. > > Fix it by increasing the minimum entropy delay for i.MX6SX as done in U-Boot: > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwo > rk.ozlabs.org%2Fproject%2Fuboot%2Fpatch%2F20220415111049.2565744-1- > gaurav.jain%40nxp.com%2F&data=04%7C01%7CV.Sethi%40nxp.com%7C7 > 9659c64ea334807ca2c08da1fb0b1b3%7C686ea1d3bc2b4c6fa92cd99c5c301635 > %7C0%7C0%7C637857141005203180%7CUnknown%7CTWFpbGZsb3d8eyJWIjoi > MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000 > &sdata=%2BpR3ecPQNZ001zRGWe%2FWo%2FayfltO6zYK1p%2BwQ57c6J > U%3D&reserved=0 > > Signed-off-by: Fabio Estevam <festevam@denx.de> > --- > Change since v1: > - Align the fix with U-Boot. > > drivers/crypto/caam/ctrl.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c index > ca0361b2dbb0..c515c20442d5 100644 > --- a/drivers/crypto/caam/ctrl.c > +++ b/drivers/crypto/caam/ctrl.c > @@ -648,6 +648,8 @@ static int caam_probe(struct platform_device *pdev) > return ret; > } > > + if (of_machine_is_compatible("fsl,imx6sx")) > + ent_delay = 12000; > > /* Get configuration properties from device tree */ > /* First, get register page */ > @@ -871,6 +873,15 @@ static int caam_probe(struct platform_device *pdev) > */ > ret = instantiate_rng(dev, inst_handles, > gen_sk); > + /* > + * Entropy delay is calculated via self-test method. > + * Self-test is run across different voltages and > + * temperatures. > + * If worst case value for ent_dly is identified, > + * the loop can be skipped for that platform. > + */ > + if (of_machine_is_compatible("fsl,imx6sx")) > + break; > if (ret == -EAGAIN) > /* > * if here, the loop will rerun, > -- > 2.25.1
Hi Fabio, Just responded to the patch posted by you. For i.MX6D I will check with the hardware IP team. As mentioned in the earlier thread we will be passing the entropy delay value via device tree property. Regards Varun > -----Original Message----- > From: Fabio Estevam <festevam@gmail.com> > Sent: Saturday, April 16, 2022 8:04 PM > To: Herbert Xu <herbert@gondor.apana.org.au> > Cc: Horia Geanta <horia.geanta@nxp.com>; Gaurav Jain > <gaurav.jain@nxp.com>; Varun Sethi <V.Sethi@nxp.com>; open list:HARDWARE > RANDOM NUMBER GENERATOR CORE <linux-crypto@vger.kernel.org>; Fabio > Estevam <festevam@denx.de> > Subject: [EXT] Re: [PATCH v2] crypto: caam - fix i.MX6SX entropy delay value > > Caution: EXT Email > > Hi Horia and Varun, > > On Sat, Apr 16, 2022 at 10:54 AM Fabio Estevam <festevam@gmail.com> wrote: > > > > From: Fabio Estevam <festevam@denx.de> > > > > Since commit 358ba762d9f1 ("crypto: caam - enable prediction > > resistance in HRWNG") the following CAAM errors can be seen on i.MX6SX: > > > > caam_jr 2101000.jr: 20003c5b: CCB: desc idx 60: RNG: Hardware error > > hwrng: no data available > > caam_jr 2101000.jr: 20003c5b: CCB: desc idx 60: RNG: Hardware error > > ... > > > > This error is due to an incorrect entropy delay for i.MX6SX. > > > > Fix it by increasing the minimum entropy delay for i.MX6SX as done in > > U-Boot: > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc > > > hwork.ozlabs.org%2Fproject%2Fuboot%2Fpatch%2F20220415111049.2565744- > 1- > > > gaurav.jain%40nxp.com%2F&data=04%7C01%7CV.Sethi%40nxp.com%7C9 > 6c829 > > > a14c394eb5e20508da1fb62811%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0 > %7C1% > > > 7C637857164474600314%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwM > DAiLCJQI > > > joiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=jKZQyH0q > Z50 > > rZhRV6%2FtOgkacpQZ9pZnwo6dLEkMUARs%3D&reserved=0 > > > > Signed-off-by: Fabio Estevam <festevam@denx.de> > > --- > > Change since v1: > > - Align the fix with U-Boot. > > Actually, after thinking more about it, I realize that this issue is not i.MX6SX > specific as I have seen reports of the same failures on i.MX6D as well. > > Would it make sense to fix it like this instead? > > --- a/drivers/crypto/caam/regs.h > +++ b/drivers/crypto/caam/regs.h > @@ -516,7 +516,7 @@ struct rng4tst { > }; > #define RTSDCTL_ENT_DLY_SHIFT 16 > #define RTSDCTL_ENT_DLY_MASK (0xffff << RTSDCTL_ENT_DLY_SHIFT) - > #define RTSDCTL_ENT_DLY_MIN 3200 > +#define RTSDCTL_ENT_DLY_MIN 12000 > #define RTSDCTL_ENT_DLY_MAX 12800 > u32 rtsdctl; /* seed control register */ > union { > > Any drawbacks in using this generic approach? > > Please advise.
Hi Varun, On Sat, Apr 16, 2022 at 3:00 PM Varun Sethi <V.Sethi@nxp.com> wrote: > > Hi Fabio, > Vabhav is working on a fix for the Linux driver. He would be introducing a new property in the CAAM device tree node, which would be used for specifying the entropy delay value. This would make the solution generic. This property is optional. Unfortunately, a devicetree property solution via optional property would not work. Such a solution would not be backported to stable kernels and people running old devicetree with new kernels would still face the problem. This problem is seen since kernel 5.10, so we need a kernel-only fix. Thanks
On 4/16/2022 10:11 PM, Fabio Estevam wrote: > Hi Varun, > > On Sat, Apr 16, 2022 at 3:00 PM Varun Sethi <V.Sethi@nxp.com> wrote: >> >> Hi Fabio, >> Vabhav is working on a fix for the Linux driver. He would be introducing a new property in the CAAM device tree node, which would be used for specifying the entropy delay value. This would make the solution generic. This property is optional. > > Unfortunately, a devicetree property solution via optional property > would not work. > > Such a solution would not be backported to stable kernels and people running old > devicetree with new kernels would still face the problem. > Indeed. > This problem is seen since kernel 5.10, so we need a kernel-only fix. > I'd prefer to have this patch (or a kernel-only fix in general) applied only on the stable kernels, and use the device tree approach on current kernel. However, I'm not sure this is acceptable from stable kernels rules' perspective. The alternative is to merge this patch (btw, please add a Fixes tag) and then the DT-based solution would also include its removal. Horia
Hi Horia, On Mon, Apr 18, 2022 at 9:10 AM Horia Geantă <horia.geanta@nxp.com> wrote: > The alternative is to merge this patch (btw, please add a Fixes tag) > and then the DT-based solution would also include its removal. Sounds good. I have sent a v3 with a Fixes tag.
> -----Original Message----- > From: Fabio Estevam <festevam@gmail.com> > Sent: Sunday, April 17, 2022 12:41 AM > To: Varun Sethi <V.Sethi@nxp.com> > Cc: herbert@gondor.apana.org.au; Vabhav Sharma > <vabhav.sharma@nxp.com>; Horia Geanta <horia.geanta@nxp.com>; > Gaurav Jain <gaurav.jain@nxp.com>; linux-crypto@vger.kernel.org; Fabio > Estevam <festevam@denx.de> > Subject: Re: [EXT] [PATCH v2] crypto: caam - fix i.MX6SX entropy delay value > > Caution: EXT Email > > Hi Varun, > > On Sat, Apr 16, 2022 at 3:00 PM Varun Sethi <V.Sethi@nxp.com> wrote: > > > > Hi Fabio, > > Vabhav is working on a fix for the Linux driver. He would be introducing a > new property in the CAAM device tree node, which would be used for > specifying the entropy delay value. This would make the solution generic. > This property is optional. > > Unfortunately, a devicetree property solution via optional property would > not work. > > Such a solution would not be backported to stable kernels and people > running old devicetree with new kernels would still face the problem. Please elaborate and specify the version for kernel, devicetree > > This problem is seen since kernel 5.10, so we need a kernel-only fix. Kernel 5.10 support devicetree, Do you mean customer using kernel without device tree? > > Thanks
On 4/16/2022 5:34 PM, Fabio Estevam wrote: > Hi Horia and Varun, > > On Sat, Apr 16, 2022 at 10:54 AM Fabio Estevam <festevam@gmail.com> wrote: >> >> From: Fabio Estevam <festevam@denx.de> >> >> Since commit 358ba762d9f1 ("crypto: caam - enable prediction resistance >> in HRWNG") the following CAAM errors can be seen on i.MX6SX: >> >> caam_jr 2101000.jr: 20003c5b: CCB: desc idx 60: RNG: Hardware error >> hwrng: no data available >> caam_jr 2101000.jr: 20003c5b: CCB: desc idx 60: RNG: Hardware error >> ... >> >> This error is due to an incorrect entropy delay for i.MX6SX. >> >> Fix it by increasing the minimum entropy delay for i.MX6SX >> as done in U-Boot: >> https://patchwork.ozlabs.org/project/uboot/patch/20220415111049.2565744-1-gaurav.jain@nxp.com/ >> >> Signed-off-by: Fabio Estevam <festevam@denx.de> >> --- >> Change since v1: >> - Align the fix with U-Boot. > > Actually, after thinking more about it, I realize that this issue is > not i.MX6SX specific as > I have seen reports of the same failures on i.MX6D as well. > Someone will need to check whether this solves the issue on i.MX6D, the root cause might be different. Besides this, as Varun said, we should check with the HW team, such that the value used for entropy delay is optimal. IOW we need TRNG characterization for i.MX6D. > Would it make sense to fix it like this instead? > > --- a/drivers/crypto/caam/regs.h > +++ b/drivers/crypto/caam/regs.h > @@ -516,7 +516,7 @@ struct rng4tst { > }; > #define RTSDCTL_ENT_DLY_SHIFT 16 > #define RTSDCTL_ENT_DLY_MASK (0xffff << RTSDCTL_ENT_DLY_SHIFT) > -#define RTSDCTL_ENT_DLY_MIN 3200 > +#define RTSDCTL_ENT_DLY_MIN 12000 > #define RTSDCTL_ENT_DLY_MAX 12800 > u32 rtsdctl; /* seed control register */ > union { > > Any drawbacks in using this generic approach? > One drawback is performance, not sure if there are others. Horia
On Mon, Apr 18, 2022 at 11:43 AM Horia Geantă <horia.geanta@nxp.com> wrote: > Someone will need to check whether this solves the issue on i.MX6D, > the root cause might be different. > > Besides this, as Varun said, we should check with the HW team, > such that the value used for entropy delay is optimal. > IOW we need TRNG characterization for i.MX6D. Let's focus on the i.MX6SX for now then. Please check v3 of the patch that I sent today.
Hi Vabhav, > -----Original Message----- > From: Vabhav Sharma <vabhav.sharma@nxp.com> > Sent: Monday, April 18, 2022 7:11 PM > To: Fabio Estevam <festevam@gmail.com>; Varun Sethi <V.Sethi@nxp.com> > Cc: herbert@gondor.apana.org.au; Horia Geanta <horia.geanta@nxp.com>; > Gaurav Jain <gaurav.jain@nxp.com>; linux-crypto@vger.kernel.org; Fabio > Estevam <festevam@denx.de> > Subject: RE: [EXT] [PATCH v2] crypto: caam - fix i.MX6SX entropy delay value > > > > > -----Original Message----- > > From: Fabio Estevam <festevam@gmail.com> > > Sent: Sunday, April 17, 2022 12:41 AM > > To: Varun Sethi <V.Sethi@nxp.com> > > Cc: herbert@gondor.apana.org.au; Vabhav Sharma > > <vabhav.sharma@nxp.com>; Horia Geanta <horia.geanta@nxp.com>; Gaurav > > Jain <gaurav.jain@nxp.com>; linux-crypto@vger.kernel.org; Fabio > > Estevam <festevam@denx.de> > > Subject: Re: [EXT] [PATCH v2] crypto: caam - fix i.MX6SX entropy delay > > value > > > > Caution: EXT Email > > > > Hi Varun, > > > > On Sat, Apr 16, 2022 at 3:00 PM Varun Sethi <V.Sethi@nxp.com> wrote: > > > > > > Hi Fabio, > > > Vabhav is working on a fix for the Linux driver. He would be > > > introducing a > > new property in the CAAM device tree node, which would be used for > > specifying the entropy delay value. This would make the solution generic. > > This property is optional. > > > > Unfortunately, a devicetree property solution via optional property > > would not work. > > > > Such a solution would not be backported to stable kernels and people > > running old devicetree with new kernels would still face the problem. > Please elaborate and specify the version for kernel, devicetree > > > > This problem is seen since kernel 5.10, so we need a kernel-only fix. > Kernel 5.10 support devicetree, Do you mean customer using kernel without > device tree? Fabio's concern is about using new kernel image with an old device tree. Fabio, We feel that it would be better to have a provision to provide entropy delay parameter via device tree. This offers more flexibility. For i.M6SX we can follow the approach proposed by you but at the same time we can still have the device tree provision. Regards Varun
On Mon, Apr 18, 2022 at 12:36 PM Varun Sethi <V.Sethi@nxp.com> wrote: > Fabio, > We feel that it would be better to have a provision to provide entropy delay parameter via device tree. This offers more flexibility. > For i.M6SX we can follow the approach proposed by you but at the same time we can still have the device tree provision. Yes, agreed.
diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c index ca0361b2dbb0..c515c20442d5 100644 --- a/drivers/crypto/caam/ctrl.c +++ b/drivers/crypto/caam/ctrl.c @@ -648,6 +648,8 @@ static int caam_probe(struct platform_device *pdev) return ret; } + if (of_machine_is_compatible("fsl,imx6sx")) + ent_delay = 12000; /* Get configuration properties from device tree */ /* First, get register page */ @@ -871,6 +873,15 @@ static int caam_probe(struct platform_device *pdev) */ ret = instantiate_rng(dev, inst_handles, gen_sk); + /* + * Entropy delay is calculated via self-test method. + * Self-test is run across different voltages and + * temperatures. + * If worst case value for ent_dly is identified, + * the loop can be skipped for that platform. + */ + if (of_machine_is_compatible("fsl,imx6sx")) + break; if (ret == -EAGAIN) /* * if here, the loop will rerun,