Hi Frank,

On Fri, May 23, 2025 at 10:48:51AM -0400, Frank Li wrote:

On Fri, May 23, 2025 at 02:19:38PM +0000, John Ernberg wrote:

quoted

Hi Frank,

On 5/23/25 3:53 PM, Frank Li wrote:

quoted

On Fri, May 23, 2025 at 01:18:32PM +0000, John Ernberg wrote:

quoted

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 <redacted>

---

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.

Okay, I check original code. You are right. You'd better to add descripton
in commit message about no_page0 is true when optee_en is true.

Frank

Thanks for clarifying. I will update the commit message making this clearer in V2,
along with your other comments on 3/4 and 4/4.

Thanks! // John Ernberg