Re: [PATCH v50 1/7] Documentation/firmware: add imx/se to other_interfaces

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The documented signal handling design drops successful responses for completed hardware commands, breaking non-idempotent operations.
- [Low] The ASCII sequence diagram for concurrent ELE_STORAGE_OPEN_REQ ioctls uses confusing and incorrectly directed arrows, misrepresenting the flow of operations.
--

commit c9bb0d9967cd15b4acd5ef648fe93b131b0a638a
Author: Pankaj Gupta <pankaj.gupta@nxp.com>

Documentation/firmware: add imx/se to other_interfaces

This commit documents the i.MX SoC Service layer and C_DEV driver for 
secure enclaves like the NXP EdgeLock Enclave. It details MU 
communication, character device usage, and synchronization protocols.

> diff --git a/Documentation/driver-api/firmware/other_interfaces.rst b/Documentation/driver-api/firmware/other_interfaces.rst
> index 06ac89adaafba..984ee3ecc8dc2 100644
> --- a/Documentation/driver-api/firmware/other_interfaces.rst
> +++ b/Documentation/driver-api/firmware/other_interfaces.rst

[ ... ]

> +      |                    |                      |  [B gets lock]   |
> +      |                    |                      |--STORAGE_OPEN_REQ->
> +      |                    |                      |  [FW rejects:    |
> +      |                    |                      |   one storage    |
> +      |                    |                      |   at a time]     |
> +      |                    |                      |<--ERROR_RSP------|

[Severity: Low]
Is this diagram accurate? The arrows seem to originate from FW and point
directly to Userspace B, bypassing the Kernel column. Does this misrepresent
the flow of operations by implying firmware directly interacts with userspace?

> +      |                    |  se_val_rsp_hdr_n_status: -EPERM        |
> +      |                    |  fw_api_specific_ops not called         |
> +      |                    |<--ioctl -EPERM-to B------------------->|

[ ... ]

> +      |                    |<--CMD_RSP------------|
> +      |  wait_for_completion_interruptible()      |
> +      |  wakes: signal seen -> -ERESTARTSYS       |
> +      |                    |                      |
> +      |            [response is in rx_msg:        |
> +      |             validate with                 |
> +      |             se_val_rsp_hdr_n_status()]    |
> +      |            [fw_api_specific_ops()         |
> +      |             (is_cmd_interrupted=true):    |
> +      |             for SESSION_OPEN: record      |
> +      |             handle, close session via     |
> +      |             se_close_session(), clear;    |
> +      |             for STORAGE_OPEN: record      |
> +      |             handle, close storage via     |
> +      |             se_close_storage(), return 0] |
> +      |            err = -EINTR                   |
> +      |            (not -ERESTARTSYS:             |
> +      |             prevents VFS auto-restart)    |
> +      |<--ioctl -EINTR-----|                      |

[Severity: High]
Could returning an error (-EINTR) to userspace for a hardware command that
successfully completed cause userspace to incorrectly assume the operation
was aborted?

For non-idempotent operations (like incrementing a monotonic counter or
generating a key), could this lead to userspace erroneously retrying and
double-executing the operation in hardware?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260912-imx-se-if-v50-0-80834ef510d3@nxp.com?part=1