V3:
- Added Cc: clk driver maintainers - Fabio Estevam
- Added Cc: i.MX arch maintainers - Fabio Estevam
- Removed bouncing email address for Herbert Xu
V2-resend:
- Patch 0005 lost in the ether - resending
V2:
- Endian detection is ok with TrustZone enabled Horia.
Endian detection logic tested with TrustZone enabled. The register that
this relies on though isn't affected by the lock-down in the first page.
Assuming set of affected registers is actually just the 'deco' registers
though there is no formal statement of that, that I am aware of.
- Moving of TrustZone work-around into u-boot
This set actually doesn't need to deal with TrustZone at all now but, for
the sake of consistency keeping thread title
https://patchwork.ozlabs.org/patch/866460/
https://patchwork.ozlabs.org/patch/866462/
https://patchwork.ozlabs.org/patch/865890/
- Reworded endless loop fix to read a bit better
- Fixes to DTS additions - Rui
- Fixes to number of clocks declared - Rui
V1:
This patch-set enables CAAM on the i.MX7s and fixes a number of issues
identified with the CAAM driver and hardware when TrustZone mode is
enabled.
The first block of patches are simple bug-fixes, followed by a second block
of patches which are simple enabling patches for the i.MX7Solo - note we
aren't enabling for the i.MX7Dual since we don't have hardware to test that
out but it should be a 1:1 mapping for others to enable when appropriate.
The final block in this series implements a fix for using the CAAM when
OPTEE/TrustZone is enabled. The various details are logged in these
threads.
Link: https://github.com/OP-TEE/optee_os/issues/1408
Link: https://tinyurl.com/yam5gv9a
Link: https://patchwork.ozlabs.org/cover/865042
In simple terms, when TrustZone is active the first page of the CAAM
becomes inaccessible to Linux as it has a special 'TZ bit' associated with
it that software cannot toggle or even view AFAIK.
The patches here then
1. Detect when TrustZone is active
2. Detect if u-boot (or OPTEE) has already initialized the RNG
and loads the CAAM driver in a different way - skipping over the RNG
initialization that Linux now no-longer has permissions to carry out.
Should #1 be true but #2 not be true, driver loading stops (and Rui's patch
for the NULL pointer dereference fixes a cash on this path). If #2 is true
but #1 is not then it's a NOP as Linux has full permission to rewrite the
deco registers in the first page of CAAM registers.
Finally then if #1 and #2 are true, the fixes here allow the CAAM to come
up and for the RNG to be useable again.
Bryan O'Donoghue (1):
crypto: caam: Fix endless loop when RNG is already initialized
Rui Miguel Silva (4):
crypto: caam: Fix null dereference at error path
crypto: caam: do not use mem and emi_slow clock for imx7x
clk: imx7d: add CAAM clock
ARM: dts: imx7s: add CAAM device node
arch/arm/boot/dts/imx7s.dtsi | 31 +++++++++++++++++++++++
drivers/clk/imx/clk-imx7d.c | 1 +
drivers/crypto/caam/ctrl.c | 45 ++++++++++++++++++++-------------
include/dt-bindings/clock/imx7d-clock.h | 3 ++-
4 files changed, 61 insertions(+), 19 deletions(-)
--
2.7.4
From: Rui Miguel Silva <[email protected]>
Add CAAM device node to the i.MX7s device tree.
Signed-off-by: Rui Miguel Silva <[email protected]>
Cc: Shawn Guo <[email protected]>
Cc: Sascha Hauer <[email protected]>
Cc: [email protected]
Cc: "Horia Geantă" <[email protected]>
Cc: Aymen Sghaier <[email protected]>
Cc: Fabio Estevam <[email protected]>
Cc: Peng Fan <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Lukas Auer <[email protected]>
Signed-off-by: Bryan O'Donoghue <[email protected]>
---
arch/arm/boot/dts/imx7s.dtsi | 31 +++++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)
diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
index 82ad26e..e38c159 100644
--- a/arch/arm/boot/dts/imx7s.dtsi
+++ b/arch/arm/boot/dts/imx7s.dtsi
@@ -805,6 +805,37 @@
status = "disabled";
};
+ crypto: caam@30900000 {
+ compatible = "fsl,sec-v4.0";
+ fsl,sec-era = <4>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ reg = <0x30900000 0x40000>;
+ ranges = <0 0x30900000 0x40000>;
+ interrupts = <GIC_SPI 91 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clks IMX7D_CAAM_CLK>,
+ <&clks IMX7D_AHB_CHANNEL_ROOT_CLK>;
+ clock-names = "ipg", "aclk";
+
+ sec_jr0: jr0@1000 {
+ compatible = "fsl,sec-v4.0-job-ring";
+ reg = <0x1000 0x1000>;
+ interrupts = <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>;
+ };
+
+ sec_jr1: jr1@2000 {
+ compatible = "fsl,sec-v4.0-job-ring";
+ reg = <0x2000 0x1000>;
+ interrupts = <GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>;
+ };
+
+ sec_jr2: jr1@3000 {
+ compatible = "fsl,sec-v4.0-job-ring";
+ reg = <0x3000 0x1000>;
+ interrupts = <GIC_SPI 114 IRQ_TYPE_LEVEL_HIGH>;
+ };
+ };
+
flexcan1: can@30a00000 {
compatible = "fsl,imx7d-flexcan", "fsl,imx6q-flexcan";
reg = <0x30a00000 0x10000>;
--
2.7.4
From: Rui Miguel Silva <[email protected]>
Add CAAM clock so that we could use the Cryptographic Acceleration and
Assurance Module (CAAM) hardware block.
Signed-off-by: Rui Miguel Silva <[email protected]>
Cc: Michael Turquette <[email protected]>
Cc: Stephen Boyd <[email protected]>
Cc: [email protected]
Cc: "Horia Geantă" <[email protected]>
Cc: Aymen Sghaier <[email protected]>
Cc: Fabio Estevam <[email protected]>
Cc: Peng Fan <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Lukas Auer <[email protected]>
Signed-off-by: Bryan O'Donoghue <[email protected]>
---
drivers/clk/imx/clk-imx7d.c | 1 +
include/dt-bindings/clock/imx7d-clock.h | 3 ++-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/imx/clk-imx7d.c b/drivers/clk/imx/clk-imx7d.c
index 80dc211..52ab096 100644
--- a/drivers/clk/imx/clk-imx7d.c
+++ b/drivers/clk/imx/clk-imx7d.c
@@ -795,6 +795,7 @@ static void __init imx7d_clocks_init(struct device_node *ccm_node)
clks[IMX7D_DRAM_PHYM_ALT_ROOT_CLK] = imx_clk_gate4("dram_phym_alt_root_clk", "dram_phym_alt_post_div", base + 0x4130, 0);
clks[IMX7D_DRAM_ALT_ROOT_CLK] = imx_clk_gate4("dram_alt_root_clk", "dram_alt_post_div", base + 0x4130, 0);
clks[IMX7D_OCOTP_CLK] = imx_clk_gate4("ocotp_clk", "ipg_root_clk", base + 0x4230, 0);
+ clks[IMX7D_CAAM_CLK] = imx_clk_gate4("caam_clk", "ipg_root_clk", base + 0x4240, 0);
clks[IMX7D_USB_HSIC_ROOT_CLK] = imx_clk_gate4("usb_hsic_root_clk", "usb_hsic_post_div", base + 0x4420, 0);
clks[IMX7D_SDMA_CORE_CLK] = imx_clk_gate4("sdma_root_clk", "ahb_root_clk", base + 0x4480, 0);
clks[IMX7D_PCIE_CTRL_ROOT_CLK] = imx_clk_gate4("pcie_ctrl_root_clk", "pcie_ctrl_post_div", base + 0x4600, 0);
diff --git a/include/dt-bindings/clock/imx7d-clock.h b/include/dt-bindings/clock/imx7d-clock.h
index e2f99ae..2bc5618 100644
--- a/include/dt-bindings/clock/imx7d-clock.h
+++ b/include/dt-bindings/clock/imx7d-clock.h
@@ -452,5 +452,6 @@
#define IMX7D_OCOTP_CLK 439
#define IMX7D_NAND_RAWNAND_CLK 440
#define IMX7D_NAND_USDHC_BUS_RAWNAND_CLK 441
-#define IMX7D_CLK_END 442
+#define IMX7D_CAAM_CLK 442
+#define IMX7D_CLK_END 443
#endif /* __DT_BINDINGS_CLOCK_IMX7D_H */
--
2.7.4
commit 1005bccd7a4a ("crypto: caam - enable instantiation of all RNG4 state
handles") introduces a control when incrementing ent_delay which contains
the following comment above it:
/*
* If either SH were instantiated by somebody else
* (e.g. u-boot) then it is assumed that the entropy
* parameters are properly set and thus the function
* setting these (kick_trng(...)) is skipped.
* Also, if a handle was instantiated, do not change
* the TRNG parameters.
*/
This is a problem observed when sec_init() has been run in u-boot and
and TrustZone is enabled. We can fix this by instantiating all rng state
handles in u-boot but, on the Kernel side we should ensure that this
non-terminating path is dealt with.
Fixes: 1005bccd7a4a ("crypto: caam - enable instantiation of all RNG4 state
handles")
Reported-by: Ryan Harkin <[email protected]>
Cc: "Horia Geantă" <[email protected]>
Cc: Aymen Sghaier <[email protected]>
Cc: Fabio Estevam <[email protected]>
Cc: Peng Fan <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Lukas Auer <[email protected]>
Cc: <[email protected]> # 4.12+
Signed-off-by: Bryan O'Donoghue <[email protected]>
---
drivers/crypto/caam/ctrl.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index 98986d3..0a1e96b 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c
@@ -704,7 +704,10 @@ static int caam_probe(struct platform_device *pdev)
ent_delay);
kick_trng(pdev, ent_delay);
ent_delay += 400;
+ } else if (ctrlpriv->rng4_sh_init && inst_handles) {
+ ent_delay += 400;
}
+
/*
* if instantiate_rng(...) fails, the loop will rerun
* and the kick_trng(...) function will modfiy the
--
2.7.4
From: Rui Miguel Silva <[email protected]>
caam_remove already removes the debugfs entry, so we need to remove the one
immediately before calling caam_remove.
This fix a NULL dereference at error paths is caam_probe fail.
[bod: changed name prefix to "crypto: caam: Fix .."]
[bod: added Fixes tag]
Fixes: 67c2315def06 ("crypto: caam - add Queue Interface (QI) backend
support")
Tested-by: Ryan Harkin <[email protected]>
Signed-off-by: Rui Miguel Silva <[email protected]>
Cc: "Horia Geantă" <[email protected]>
Cc: Aymen Sghaier <[email protected]>
Cc: Fabio Estevam <[email protected]>
Cc: Peng Fan <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Lukas Auer <[email protected]>
Cc: <[email protected]> # 4.12+
Reviewed-by: Horia Geantă <[email protected]>
Signed-off-by: Bryan O'Donoghue <[email protected]>
---
drivers/crypto/caam/ctrl.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index 027e121..98986d3 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c
@@ -809,9 +809,6 @@ static int caam_probe(struct platform_device *pdev)
return 0;
caam_remove:
-#ifdef CONFIG_DEBUG_FS
- debugfs_remove_recursive(ctrlpriv->dfs_root);
-#endif
caam_remove(pdev);
return ret;
--
2.7.4
From: Rui Miguel Silva <[email protected]>
I.MX7x only use two clocks for the CAAM module, so make sure we do not try to
use the mem and the emi_slow clock when running in that imx7d and imx7s machine
type.
[bod: fixed minor trailing whitespace issue]
Signed-off-by: Rui Miguel Silva <[email protected]>
Cc: "Horia Geantă" <[email protected]>
Cc: Aymen Sghaier <[email protected]>
Cc: Fabio Estevam <[email protected]>
Cc: Peng Fan <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Lukas Auer <[email protected]>
Signed-off-by: Bryan O'Donoghue <[email protected]>
---
drivers/crypto/caam/ctrl.c | 39 ++++++++++++++++++++++++---------------
1 file changed, 24 insertions(+), 15 deletions(-)
diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index 0a1e96b..1f2dd6a 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c
@@ -333,7 +333,8 @@ static int caam_remove(struct platform_device *pdev)
/* shut clocks off before finalizing shutdown */
clk_disable_unprepare(ctrlpriv->caam_ipg);
- clk_disable_unprepare(ctrlpriv->caam_mem);
+ if (ctrlpriv->caam_mem)
+ clk_disable_unprepare(ctrlpriv->caam_mem);
clk_disable_unprepare(ctrlpriv->caam_aclk);
if (ctrlpriv->caam_emi_slow)
clk_disable_unprepare(ctrlpriv->caam_emi_slow);
@@ -462,14 +463,17 @@ static int caam_probe(struct platform_device *pdev)
}
ctrlpriv->caam_ipg = clk;
- clk = caam_drv_identify_clk(&pdev->dev, "mem");
- if (IS_ERR(clk)) {
- ret = PTR_ERR(clk);
- dev_err(&pdev->dev,
- "can't identify CAAM mem clk: %d\n", ret);
- return ret;
+ if (!of_machine_is_compatible("fsl,imx7d") &&
+ !of_machine_is_compatible("fsl,imx7s")) {
+ clk = caam_drv_identify_clk(&pdev->dev, "mem");
+ if (IS_ERR(clk)) {
+ ret = PTR_ERR(clk);
+ dev_err(&pdev->dev,
+ "can't identify CAAM mem clk: %d\n", ret);
+ return ret;
+ }
+ ctrlpriv->caam_mem = clk;
}
- ctrlpriv->caam_mem = clk;
clk = caam_drv_identify_clk(&pdev->dev, "aclk");
if (IS_ERR(clk)) {
@@ -480,7 +484,9 @@ static int caam_probe(struct platform_device *pdev)
}
ctrlpriv->caam_aclk = clk;
- if (!of_machine_is_compatible("fsl,imx6ul")) {
+ if (!of_machine_is_compatible("fsl,imx6ul") &&
+ !of_machine_is_compatible("fsl,imx7d") &&
+ !of_machine_is_compatible("fsl,imx7s")) {
clk = caam_drv_identify_clk(&pdev->dev, "emi_slow");
if (IS_ERR(clk)) {
ret = PTR_ERR(clk);
@@ -497,11 +503,13 @@ static int caam_probe(struct platform_device *pdev)
return ret;
}
- ret = clk_prepare_enable(ctrlpriv->caam_mem);
- if (ret < 0) {
- dev_err(&pdev->dev, "can't enable CAAM secure mem clock: %d\n",
- ret);
- goto disable_caam_ipg;
+ if (ctrlpriv->caam_mem) {
+ ret = clk_prepare_enable(ctrlpriv->caam_mem);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "can't enable CAAM secure mem clock: %d\n",
+ ret);
+ goto disable_caam_ipg;
+ }
}
ret = clk_prepare_enable(ctrlpriv->caam_aclk);
@@ -823,7 +831,8 @@ static int caam_probe(struct platform_device *pdev)
disable_caam_aclk:
clk_disable_unprepare(ctrlpriv->caam_aclk);
disable_caam_mem:
- clk_disable_unprepare(ctrlpriv->caam_mem);
+ if (ctrlpriv->caam_mem)
+ clk_disable_unprepare(ctrlpriv->caam_mem);
disable_caam_ipg:
clk_disable_unprepare(ctrlpriv->caam_ipg);
return ret;
--
2.7.4
On 1/31/2018 4:00 AM, Bryan O'Donoghue wrote:
> From: Rui Miguel Silva <[email protected]>
>
> Add CAAM device node to the i.MX7s device tree.
>
> Signed-off-by: Rui Miguel Silva <[email protected]>
> Cc: Shawn Guo <[email protected]>
> Cc: Sascha Hauer <[email protected]>
> Cc: [email protected]
> Cc: "Horia Geant?" <[email protected]>
> Cc: Aymen Sghaier <[email protected]>
> Cc: Fabio Estevam <[email protected]>
> Cc: Peng Fan <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: Lukas Auer <[email protected]>
> Signed-off-by: Bryan O'Donoghue <[email protected]>
> ---
> arch/arm/boot/dts/imx7s.dtsi | 31 +++++++++++++++++++++++++++++++
> 1 file changed, 31 insertions(+)
>
> diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
> index 82ad26e..e38c159 100644
> --- a/arch/arm/boot/dts/imx7s.dtsi
> +++ b/arch/arm/boot/dts/imx7s.dtsi
> @@ -805,6 +805,37 @@
> status = "disabled";
> };
>
> + crypto: caam@30900000 {
> + compatible = "fsl,sec-v4.0";
> + fsl,sec-era = <4>;
CCBVID[CAAM_ERA] = 8.
Either remove this (optional) property and let the bootloader add it
dynamically, or provide a correct value for it.
Thanks,
Horia
On 1/31/2018 4:00 AM, Bryan O'Donoghue wrote:
> commit 1005bccd7a4a ("crypto: caam - enable instantiation of all RNG4 state
> handles") introduces a control when incrementing ent_delay which contains
> the following comment above it:
>
> /*
> * If either SH were instantiated by somebody else
> * (e.g. u-boot) then it is assumed that the entropy
> * parameters are properly set and thus the function
> * setting these (kick_trng(...)) is skipped.
> * Also, if a handle was instantiated, do not change
> * the TRNG parameters.
> */
>
> This is a problem observed when sec_init() has been run in u-boot and
> and TrustZone is enabled. We can fix this by instantiating all rng state
> handles in u-boot but, on the Kernel side we should ensure that this
> non-terminating path is dealt with.
>
> Fixes: 1005bccd7a4a ("crypto: caam - enable instantiation of all RNG4 state
> handles")
>
> Reported-by: Ryan Harkin <[email protected]>
> Cc: "Horia Geant?" <[email protected]>
> Cc: Aymen Sghaier <[email protected]>
> Cc: Fabio Estevam <[email protected]>
> Cc: Peng Fan <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: Lukas Auer <[email protected]>
> Cc: <[email protected]> # 4.12+
> Signed-off-by: Bryan O'Donoghue <[email protected]>
> ---
> drivers/crypto/caam/ctrl.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
> index 98986d3..0a1e96b 100644
> --- a/drivers/crypto/caam/ctrl.c
> +++ b/drivers/crypto/caam/ctrl.c
> @@ -704,7 +704,10 @@ static int caam_probe(struct platform_device *pdev)
> ent_delay);
> kick_trng(pdev, ent_delay);
> ent_delay += 400;
> + } else if (ctrlpriv->rng4_sh_init && inst_handles) {
> + ent_delay += 400;
> }
If both RNG state handles are initialized before kernel runs, then
instantiate_rng() should be a no-op and return 0, which is enough to exit the
loop: while ((ret == -EAGAIN) && (ent_delay < RTSDCTL_ENT_DLY_MAX))
If the loop cannot exit based on value of "ret" != -EAGAIN, then it means
caam_probe() will eventually fail due to ret == -EAGAIN:
if (ret) {
dev_err(dev, "failed to instantiate RNG");
goto caam_remove;
}
Please provide more details, so that the root cause is found and fixed.
For e.g. what is the value of RDSTA (RNG DRNG Status register @0x6C0):
-before & after u-boot initializes RNG
-as seen by kernel during caam_probe()
Also provide related error messages displayed during boot.
Thanks,
Horia
On Wed, Jan 31, 2018 at 12:00 AM, Bryan O'Donoghue
<[email protected]> wrote:
> From: Rui Miguel Silva <[email protected]>
>
> Add CAAM clock so that we could use the Cryptographic Acceleration and
> Assurance Module (CAAM) hardware block.
>
> Signed-off-by: Rui Miguel Silva <[email protected]>
> Cc: Michael Turquette <[email protected]>
> Cc: Stephen Boyd <[email protected]>
> Cc: [email protected]
> Cc: "Horia Geantă" <[email protected]>
> Cc: Aymen Sghaier <[email protected]>
> Cc: Fabio Estevam <[email protected]>
> Cc: Peng Fan <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: Lukas Auer <[email protected]>
> Signed-off-by: Bryan O'Donoghue <[email protected]>
Reviewed-by: Fabio Estevam <[email protected]>
On 01/02/18 12:16, Horia Geant? wrote:
> If the loop cannot exit based on value of "ret" != -EAGAIN, then it means
> caam_probe() will eventually fail due to ret == -EAGAIN:
> if (ret) {
> dev_err(dev, "failed to instantiate RNG");
> goto caam_remove;
> }
For me it's an endless loop applying the first two
https://patchwork.ozlabs.org/patch/866460/
https://patchwork.ozlabs.org/patch/866462/
but not this one
https://patchwork.ozlabs.org/patch/865890/
> Please provide more details, so that the root cause is found and fixed.
np
---
bod
On Fri, 2018-02-02 at 11:20 +0000, Bryan O'Donoghue wrote:
> On 01/02/18 12:16, Horia Geantă wrote:
> > If the loop cannot exit based on value of "ret" != -EAGAIN, then it
> > means
> > caam_probe() will eventually fail due to ret == -EAGAIN:
> > if (ret) {
> > dev_err(dev, "failed to instantiate RNG");
> > goto caam_remove;
> > }
>
> For me it's an endless loop applying the first two
>
> https://patchwork.ozlabs.org/patch/866460/
> https://patchwork.ozlabs.org/patch/866462/
>
> but not this one
>
> https://patchwork.ozlabs.org/patch/865890/
>
> > Please provide more details, so that the root cause is found and
> > fixed.
>
> np
>
> ---
> bod
I think the problem lies in the instantiate_rng() function. If the
driver is unable to acquire DEC0 it'll return -ENODEV. This should
terminate the while loop in the probe function. However, the return
value is never checked and is instead overwritten with -EAGAIN, causing
the endless loop.
This problem only occurs if u-boot instantiates only one of the state
handles (ent_delay doesn't get incremented) and the kernel runs in non-
secure mode (DEC0 can't get acquired). Instantiating all state handles
in u-boot therefore fixes this problem. In addition, the return value
in instantiate_rng() should be handled correctly by including
if (ret)
break;
right after "ret = run_descriptor_deco0(ctrldev, desc, &status);".
Thanks,
Lukas
On 2/2/2018 2:54 PM, Auer, Lukas wrote:
> On Fri, 2018-02-02 at 11:20 +0000, Bryan O'Donoghue wrote:
>> On 01/02/18 12:16, Horia Geant? wrote:
>>> If the loop cannot exit based on value of "ret" != -EAGAIN, then it
>>> means
>>> caam_probe() will eventually fail due to ret == -EAGAIN:
>>> if (ret) {
>>> dev_err(dev, "failed to instantiate RNG");
>>> goto caam_remove;
>>> }
>>
>> For me it's an endless loop applying the first two
>>
>> https://patchwork.ozlabs.org/patch/866460/
>> https://patchwork.ozlabs.org/patch/866462/
>>
>> but not this one
>>
>> https://patchwork.ozlabs.org/patch/865890/
>>
[snip]
>
> I think the problem lies in the instantiate_rng() function. If the
> driver is unable to acquire DEC0 it'll return -ENODEV. This should
> terminate the while loop in the probe function. However, the return
> value is never checked and is instead overwritten with -EAGAIN, causing
> the endless loop.
>
> This problem only occurs if u-boot instantiates only one of the state
> handles (ent_delay doesn't get incremented) and the kernel runs in non-
> secure mode (DEC0 can't get acquired). Instantiating all state handles
> in u-boot therefore fixes this problem. In addition, the return value
> in instantiate_rng() should be handled correctly by including
>
> if (ret)
> break;
>
> right after "ret = run_descriptor_deco0(ctrldev, desc, &status);".
>
Indeed, the error path is incorrect and should be fixed as you mentioned.
I will send a patch replacing this one.
Note that this fixes only the error path, meaning caam_probe() won't go into an
endless loop and instead will return -ENODEV, due to being unable to acquire
control of DECO0.
There are still a few hurdles to cross for CAAM to work in a TZ environment.
For e.g. could you please check / confirm whether DECO0MIDR (DECO0 MID registers
@0xA0, @0xA4) are set such that Linux kernel is allowed to r/w DECO0-related
registers?
Thanks,
Horia
In case DECO0 cannot be acquired - i.e. run_descriptor_deco0() fails
with -ENODEV, caam_probe() enters an endless loop:
run_descriptor_deco0
ret -ENODEV
-> instantiate_rng
-ENODEV, overwritten by -EAGAIN
ret -EAGAIN
-> caam_probe
-EAGAIN results in endless loop
It turns out the error path in instantiate_rng() is incorrect,
the checks are done in the wrong order.
Cc: <[email protected]> # 3.13+
Fixes: 1005bccd7a4a6 ("crypto: caam - enable instantiation of all RNG4 state handles")
Reported-by: Bryan O'Donoghue <[email protected]>
Suggested-by: Auer Lukas <[email protected]>
Signed-off-by: Horia Geantă <[email protected]>
---
drivers/crypto/caam/ctrl.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index 75d280cb2dc0..e843cf410373 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c
@@ -228,12 +228,16 @@ static int instantiate_rng(struct device *ctrldev, int state_handle_mask,
* without any error (HW optimizations for later
* CAAM eras), then try again.
*/
+ if (ret)
+ break;
+
rdsta_val = rd_reg32(&ctrl->r4tst[0].rdsta) & RDSTA_IFMASK;
if ((status && status != JRSTA_SSRC_JUMP_HALT_CC) ||
- !(rdsta_val & (1 << sh_idx)))
+ !(rdsta_val & (1 << sh_idx))) {
ret = -EAGAIN;
- if (ret)
break;
+ }
+
dev_info(ctrldev, "Instantiated RNG4 SH%d\n", sh_idx);
/* Clear the contents before recreating the descriptor */
memset(desc, 0x00, CAAM_CMD_SZ * 7);
--
2.12.0.264.gd6db3f216544
On Mon, 2018-02-05 at 08:45 +0000, Horia Geantă wrote:
> On 2/2/2018 2:54 PM, Auer, Lukas wrote:
> > On Fri, 2018-02-02 at 11:20 +0000, Bryan O'Donoghue wrote:
> > > On 01/02/18 12:16, Horia Geantă wrote:
> > > > If the loop cannot exit based on value of "ret" != -EAGAIN,
> > > > then it
> > > > means
> > > > caam_probe() will eventually fail due to ret == -EAGAIN:
> > > > if (ret) {
> > > > dev_err(dev, "failed to instantiate RNG");
> > > > goto caam_remove;
> > > > }
> > >
> > > For me it's an endless loop applying the first two
> > >
> > > https://patchwork.ozlabs.org/patch/866460/
> > > https://patchwork.ozlabs.org/patch/866462/
> > >
> > > but not this one
> > >
> > > https://patchwork.ozlabs.org/patch/865890/
> > >
>
> [snip]
> >
> > I think the problem lies in the instantiate_rng() function. If the
> > driver is unable to acquire DEC0 it'll return -ENODEV. This should
> > terminate the while loop in the probe function. However, the return
> > value is never checked and is instead overwritten with -EAGAIN,
> > causing
> > the endless loop.
> >
> > This problem only occurs if u-boot instantiates only one of the
> > state
> > handles (ent_delay doesn't get incremented) and the kernel runs in
> > non-
> > secure mode (DEC0 can't get acquired). Instantiating all state
> > handles
> > in u-boot therefore fixes this problem. In addition, the return
> > value
> > in instantiate_rng() should be handled correctly by including
> >
> > if (ret)
> > break;
> >
> > right after "ret = run_descriptor_deco0(ctrldev, desc, &status);".
> >
>
> Indeed, the error path is incorrect and should be fixed as you
> mentioned.
> I will send a patch replacing this one.
> Note that this fixes only the error path, meaning caam_probe() won't
> go into an
> endless loop and instead will return -ENODEV, due to being unable to
> acquire
> control of DECO0.
>
> There are still a few hurdles to cross for CAAM to work in a TZ
> environment.
>
> For e.g. could you please check / confirm whether DECO0MIDR (DECO0
> MID registers
> @0xA0, @0xA4) are set such that Linux kernel is allowed to r/w DECO0-
> related
> registers?
>
> Thanks,
> Horia
On my board DECO0 MID ms is set to 0x8001, which I believe (going by
the structure of the other MID registers, since some of the bits are
only marked as reserved) is a MID of 1 (A7 cores) in secure mode.
Changing this to 0x9 for a MID of 1 in non-secure mode still fails the
DEC0 acquisition step in the probe call.
So unfortunately I am not sure what / if other steps are required to
use the CAAM in non-secure mode. Running a quick test with openssl
speed (using CAAM with cryptodev), it at least seems to be working.
Thanks,
Lukas