Messages in this thread |  | | From | John Ernberg <> | | Subject | Re: [PATCH 1/4] crypto: caam - Prevent crash on suspend with iMX8QM / iMX8ULP | | Date | Fri, 23 May 2025 14:19:38 +0000 |
| |
Hi Frank,
On 5/23/25 3:53 PM, Frank Li wrote: > On Fri, May 23, 2025 at 01:18:32PM +0000, John Ernberg wrote: >> Since the CAAM on these SoCs is managed by another ARM core, called the >> SECO (Security Controller) on iMX8QM and Secure Enclave on iMX8ULP, which >> also reserves access to register page 0 suspend operations cannot touch >> this page. >> >> Introduce a variable to track this situation. Since this is synonymous >> with the optee case in suspend/resume the optee check is replaced with >> this new check. >> >> Fixes the following splat at suspend: >> >> Internal error: synchronous external abort: 0000000096000010 [#1] SMP >> Hardware name: Freescale i.MX8QXP ACU6C (DT) >> pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) >> pc : readl+0x0/0x18 >> lr : rd_reg32+0x18/0x3c >> sp : ffffffc08192ba20 >> x29: ffffffc08192ba20 x28: ffffff8025190000 x27: 0000000000000000 >> x26: ffffffc0808ae808 x25: ffffffc080922338 x24: ffffff8020e89090 >> x23: 0000000000000000 x22: ffffffc080922000 x21: ffffff8020e89010 >> x20: ffffffc080387ef8 x19: ffffff8020e89010 x18: 000000005d8000d5 >> x17: 0000000030f35963 x16: 000000008f785f3f x15: 000000003b8ef57c >> x14: 00000000c418aef8 x13: 00000000f5fea526 x12: 0000000000000001 >> x11: 0000000000000002 x10: 0000000000000001 x9 : 0000000000000000 >> x8 : ffffff8025190870 x7 : ffffff8021726880 x6 : 0000000000000002 >> x5 : ffffff80217268f0 x4 : ffffff8021726880 x3 : ffffffc081200000 >> x2 : 0000000000000001 x1 : ffffff8020e89010 x0 : ffffffc081200004 >> Call trace: >> readl+0x0/0x18 >> caam_ctrl_suspend+0x30/0xdc >> dpm_run_callback.constprop.0+0x24/0x5c >> device_suspend+0x170/0x2e8 >> dpm_suspend+0xa0/0x104 >> dpm_suspend_start+0x48/0x50 >> suspend_devices_and_enter+0x7c/0x45c >> pm_suspend+0x148/0x160 >> state_store+0xb4/0xf8 >> kobj_attr_store+0x14/0x24 >> sysfs_kf_write+0x38/0x48 >> kernfs_fop_write_iter+0xb4/0x178 >> vfs_write+0x118/0x178 >> ksys_write+0x6c/0xd0 >> __arm64_sys_write+0x14/0x1c >> invoke_syscall.constprop.0+0x64/0xb0 >> do_el0_svc+0x90/0xb0 >> el0_svc+0x18/0x44 >> el0t_64_sync_handler+0x88/0x124 >> el0t_64_sync+0x150/0x154 >> Code: 88dffc21 88dffc21 5ac00800 d65f03c0 (b9400000) >> >> Fixes: d2835701d93c ("crypto: caam - i.MX8ULP donot have CAAM page0 access") >> Fixes: 61bb8db6f682 ("crypto: caam - Add support for i.MX8QM") >> Cc: stable@kernel.org # v6.10+ >> Signed-off-by: John Ernberg <john.ernberg@actia.se> >> >> --- >> >> I noticed this when enabling the iMX8QXP support (next patch), hence the >> iMX8QXP backtrace, but the iMX8QM CAAM integration works exactly the same >> and according to the NXP tree [1] the iMX8ULP suffers the same issue. >> >> [1]: https://github.com/nxp-imx/linux-imx/commit/653712ffe52dd59f407af1b781ce318f3d9e17bb >> --- >> drivers/crypto/caam/ctrl.c | 5 +++-- >> drivers/crypto/caam/intern.h | 1 + >> 2 files changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c >> index 38ff931059b4..766c447c9cfb 100644 >> --- a/drivers/crypto/caam/ctrl.c >> +++ b/drivers/crypto/caam/ctrl.c >> @@ -831,7 +831,7 @@ static int caam_ctrl_suspend(struct device *dev) >> { >> const struct caam_drv_private *ctrlpriv = dev_get_drvdata(dev); >> >> - if (ctrlpriv->caam_off_during_pm && !ctrlpriv->optee_en) >> + if (ctrlpriv->caam_off_during_pm && !ctrlpriv->no_page0) >> caam_state_save(dev); >> >> return 0; >> @@ -842,7 +842,7 @@ static int caam_ctrl_resume(struct device *dev) >> struct caam_drv_private *ctrlpriv = dev_get_drvdata(dev); >> int ret = 0; >> >> - if (ctrlpriv->caam_off_during_pm && !ctrlpriv->optee_en) { >> + if (ctrlpriv->caam_off_during_pm && !ctrlpriv->no_page0) { >> caam_state_restore(dev); >> >> /* HW and rng will be reset so deinstantiation can be removed */ >> @@ -908,6 +908,7 @@ static int caam_probe(struct platform_device *pdev) >> >> imx_soc_data = imx_soc_match->data; >> reg_access = reg_access && imx_soc_data->page0_access; >> + ctrlpriv->no_page0 = !reg_access; > > If you want to use no_page0 to control if call caam_state_save(), you'd > better set ctrlpriv->no_page0 also after ctrlpriv->optee_en = !!np; > > Frank
I'm not sure I understand, I cannot see a code path where no_page0 will be (un)set incorrectly.
optee disables the page0 access, so reg_access is already the inverse of optee_en. reg_access == false when optee_en == true.
Thus, if optee is loaded on a SoC that normally has page0_access the `reg_access = reg_access && imx_soc_data->page0_access;` statement on the line above setting no_page0 already takes care of it, so: reg_access = false && true -> false.
Similarly if both reg_access == false and page0_access == false, reg_access will still be false.
Thanks! // John Ernberg |  |