Message-ID:
 <AM9PR04MB86049FFFCBB357E3BC1E4C90959C2@AM9PR04MB8604.eurprd04.prod.outlook.com>
Date: Wed, 4 Sep 2024 16:09:50 +0000
From: Pankaj Gupta <pankaj.gupta@....com>
To: Frank Li <frank.li@....com>
CC: Jonathan Corbet <corbet@....net>, Rob Herring <robh@...nel.org>, Krzysztof
 Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Shawn Guo
	<shawnguo@...nel.org>, Sascha Hauer <s.hauer@...gutronix.de>, Pengutronix
 Kernel Team <kernel@...gutronix.de>, Fabio Estevam <festevam@...il.com>, Rob
 Herring <robh+dt@...nel.org>, "linux-doc@...r.kernel.org"
	<linux-doc@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "imx@...ts.linux.dev" <imx@...ts.linux.dev>,
	"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>
Subject: RE: [PATCH v7 0/5] Changes in v7:

> 
> On Wed, Sep 04, 2024 at 04:21:16PM +0530, Pankaj Gupta wrote:
> 
> Can you fix subject "[PATCH v7 0/5] Changes in v7"? I remember someone
> already complain this in previous version.

Thanks for pointing it out. Will ensure not to repeat it.

> 
> Frank
> 
> > 5/5
> > - struct se_clbk_handle, is added with a member struct se_if_device_ctx
> *dev_ctx.
> > - func call to ele_miscdev_msg_rcv() & ele_miscdev_msg_send(), are
> removed.
> > - func se_ioctl_cmd_snd_rcv_rsp_handler(), is modified to remove the
> > func call to ele_miscdev_msg_rcv() & ele_miscdev_msg_send()
> > - func se_ioctl_cmd_snd_rcv_rsp_handler is callig func ele_msg_send_rcv(),
> instead.
> > - Mutext "se_cmd_if_lock", handling is removed from this patch.
> > - func ele_miscdev_msg_send() is replaced with func ele_msg_send(), in
> fops_write.
> > - func ele_miscdev_msg_rcv() is replaced with func ele_msg_rcv(), in
> fops_read.
> > - fops_open is modified to create the new dev_ctx instance (using func
> init_device_context()), which is not registered as miscdev.
> > - Only one dev_ctx is registered as miscdev and its reference is stored in the
> struct se_if_priv, as priv_dev_ctx.
> > - Separate func cleanup_se_shared_mem() & func init_se_shared_mem(),
> for shared memory handling part of struct dev_ctx.
> > - Input param for func(s) ele_msg_rcv(), ele_msg_send() &
> ele_msg_send_rcv(), is replaced from struct se_if_priv to struct
> se_if_device_ctx.
> >
> > 4/5
> > - A new structure is defined name struct "se_clbk_handle", to contain
> members processed in mailbox call-back function.
> > - "struct se_if_priv" is modified to contain the two structures of
> "se_clbk_handle" - waiting_rsp_clbk_hdl & cmd_receiver_clbk_hdl.
> > - func ele_msg_rcv() is modified to take a new additional input reference
> param "struct se_clbk_handle *se_clbk_hdl".
> > - func ele_msg_send() is modified to take a new additional input tx_msg_sz.
> > - func ele_msg_send_rcv(), is modified to take 2 more inputs - tx_msg_sz &
> exp_rx_msg_sz.
> > - func se_val_rsp_hdr_n_status(), is modified to take input of rx_msg buffer,
> instead of header value, as input param.
> > - each caller of the func ele_msg_send_rcv(), is sending these two
> additional input params.
> > - func se_if_callback(), is modified to work on two structures of
> "se_clbk_handle" - waiting_rsp_clbk_hdl & cmd_receiver_clbk_hdl.
> > - Variable "max_dev_ctx", is removed from info & priv struture, as well its
> usage.
> > - New member variable "se_img_file_to_load", is added to structure "priv".
> > - Other member variables - rx_msg(ptr), rx_msg_sz, completion done & list
> of dev_ctxs, is removed from priv struture, along with their usage.
> > - func se_resume(), updated to wakeup the two "wq", part of "struct
> se_clbk_handle": priv->waiting_rsp_clbk_hdl & priv->cmd_receiver_clbk_hdl.
> >
> > 3/5
> > - Node name is changed from senclave-firmware@0 to "secure-enclave"
> >
> > 2/5
> > - Node name is changed to "secure-enclave".
> >
> > - Link to v6:
> > https://lore.kernel.org/r/20240722-imx-se-if-v6-0-ee26a87b824a@nxp.com
> >
> > v6: firmware: imx: driver for NXP secure-enclave
> >
> > 5/5
> > - replaced scope_gaurd with gaurd.
> >
> > 4/5
> > - replaced scope_gaurd with gaurd.
> > - remove reading the regs property from dtb.
> > - Added NULL check for priv data fetched from device, as a sanity
> > check, for ele_base_msg apis)
> >
> > 3/5
> > - replace firmware with senclave-firmware.
> >
> > 2/5
> > - replace firmware with senclave-firmware.
> > - drop description for mbox
> > - Replaced "items:" with maxItems:1 for "memory-region"
> > - Replaced "items:" with maxItems:1 for "sram"
> > - remove regs property.
> > - remove "$nodename"
> >
> > - Link to v5:
> > https://lore.kernel.org/r/20240712-imx-se-if-v5-0-66a79903a872@nxp.com
> >
> > Changes in v5:
> >
> > 2/5
> > - updated the description of mboxes
> > - updated the description & items for mbox-names.
> > - updated the description of memory-region
> > - move "additional properties: false" after allOf block.
> > - removed other example except one.
> >
> > 4/5
> > - Corrected the indentation in Kconfig.
> > - info members:mbox_tx_name & mbox_rx_name, are replaced with
> macros.
> >
> > 5/5
> > - Replaced "for  secure enclaves", with "for secure enclaves"
> > - Replaced "user space" with "userspace".
> > - End the line "[include]<linux/firmware/imx/ele_mu_ioctl.h>" with a
> period.
> >
> > - Link to v4:
> > https://lore.kernel.org/r/20240705-imx-se-if-v4-0-52d000e18a1d@nxp.com
> >
> > Changes in v4:
> >
> > 1/5
> > a. Removed - from EdgeLock Enclave.
> >
> > b. Removed , after "Each of the above feature,"
> >
> > c. replace "can exists" with "can exist".
> >
> > d.
> > -messaging units(MU) per SE. Each co-existing 'se' can have one or
> > multiple exclusive -MU(s), dedicated to itself. None of the MU is shared
> between two SEs.
> > +messaging units(MU) per SE. Each co-existing SE can have one or
> > +multiple exclusive MUs, dedicated to itself. None of the MU is shared
> between two SEs.
> >  Communication of the MU is realized using the Linux mailbox driver.
> >
> > e.
> > -All those SE interfaces 'se-if' that is/are dedicated to a particular
> > SE, will be -enumerated and provisioned under the very single 'SE' node.
> > +Although MU(s) is/are not shared between SE(s). But for SoC like
> > +i.MX95 which has multiple SE(s) like HSM, V2X-HSM, V2X-SHE; all the SE(s)
> and their interfaces 'se-if'
> > +that is/are dedicated to a particular SE will be enumerated and
> > +provisioned using the single compatible node("fsl,imx95-se").
> >
> > f. Removed ",". Replaced for "Each 'se-if'," with "Each se-if'.
> >
> > g. removed ","
> > -  This layer is responsible for ensuring the communication protocol,
> > that is defined
> > +  This layer is responsible for ensuring the communication protocol
> > + that is defined
> >
> > h. removed "-"
> > -  - FW can handle one command-message at a time.
> > +  - FW can handle one command message at a time.
> >
> > i.
> > -  Using these multiple device contexts, that are getting multiplexed
> > over a single MU,
> > -  user-space application(s) can call fops like write/read to send the
> > command-message,
> > -  and read back the command-response-message to/from Firmware.
> > -  fops like read & write uses the above defined service layer API(s)
> > to communicate with
> > +  Using these multiple device contexts that are getting multiplexed
> > + over a single MU,  userspace application(s) can call fops like
> > + write/read to send the command message,  and read back the command
> response message to/from Firmware.
> > +  fops like read & write use the above defined service layer API(s)
> > + to communicate with
> >    Firmware.
> >
> > j. Uppercase for word "Linux".
> >
> > 2/5
> > a. Rephrased the description to remove list of phandles.
> >
> > b. Moved required before allOf:
> > +required:
> > +  - compatible
> > +  - reg
> > +  - mboxes
> > +  - mbox-names
> > +
> > +additionalProperties: false
> > +
> >  allOf:
> >
> > c. replaced not: required: with properties: <property-name>: false.
> >    # memory-region
> > -      not:
> > -        required:
> > -          - memory-region
> > +      properties:
> > +        memory-region: false
> >
> >    # sram
> > -    else:
> > -      not:
> > -        required:
> > -          - sram
> >
> > d. Reduced examples. keeping example of i.MX95.
> > e. node-name is changed to "firmware@<hex>"
> >
> > 3/5
> > - node name changed to "firmware@<hex>".
> >
> > 4/5
> > - used sizeof(*s_info)
> > - return early, rather than doing goto exit, in ele_get_info().
> > - Use upper_32_bits() and lower_32_bits()
> > - use rx_msg here instead of priv->rx_msg
> > - Moved the status check to validate_rsp_hdr. Rename the function to
> "se_val_rsp_hdr_n_status"
> > - typecasting removed header = (struct se_msg_hdr *) msg;
> > - Converted the API name with prefix imx_ele_* or imx_se_*, to ele_* and
> se_*, respectively.
> > - Removed the functions definition & declaration for:
> > free_phybuf_mem_pool() & get_phybuf_mem_pool()
> > - removed the mbox_free_channel() calls from clean-up.
> > - Flag "priv->flags" is removed.
> > - Converted the int se_if_probe_cleanup() to void se_if_probe_cleanup().
> > - Replaced NULL initialization of structure members: priv-
> >cmd_receiver_dev & priv->waiting_rsp_dev , with comments.
> > - Removed the function's declaration get_phy_buf_mem_pool1
> >
> > 5/5
> > Changes to Documentation/ABI/testing/se-cdev.
> > a. Removed "-" from "secure-enclave" and "file-descriptor".
> >
> > b. Removed "-" from "shared-library"
> >
> > c. Replaced "get" with "getting".
> >
> > d. Added description for the new IOCTL "send command and receive
> command response"
> >
> > e. Replaced "wakeup_intruptible" with "wait_event_interruptible"
> >
> > f. Removed ";"
> >
> > g. Removd "," from "mailbox_lock,"
> >
> > h. Replaced "free" with "frees"
> >
> > i. In mailbox callback function, checking the buffer size before
> > copying.
> >
> > - Link to v3:
> > https://lore.kernel.org/r/20240617-imx-se-if-v3-0-a7d28dea5c4a@nxp.com
> >
> > Communication Interface to NXP secure-enclave HW IP like Edgelock
> > Enclave
> >
> > Hardware interface of the NXP Secure Enclave  HW IP(s) like EdgeLock
> > Enclave, V2X, SHE etc, is based on the Messaging Unit module that
> > enables processing elements like ARMv8 core, RISC V core, within the
> > SoC to communicate and coordinate by passing messages (e.g., data,
> > status and control) through these interfaces.
> >
> > The NXP i.MX secure enclaves hardware interface kernel driver, is
> > specifically targeted for use between application core and NXP
> > secure-enclave(s) HW. It allows to send/receive messages to/from the
> secure-enclave.
> >
> > Patch-set adds the kernel driver for communication interface to
> > secure-enclave, for exchanging messages with NXP secure enclave HW
> > IP(s) like EdgeLock Enclave, both from:
> > - User-Space Applications via character driver.
> > - Kernel-space, used by kernel management layers like DM-Crypt.
> >
> > Signed-off-by: Pankaj Gupta <pankaj.gupta@....com>
> >
> > Changes in v3:
> > 5/5:
> > - Initialize tx_msg with NULL.
> > - memdup_user() returns an error pointer, not NULL. correct it by adding
> check for err_ptr.
> > - new IOCTL is added to send & recieve the message.
> > - replaced the while loop till list is empty, with list_for_each_entry.
> > - replaced __list_del_entry, with list_del.
> > - Removed the dev_err message from copy to user.
> > - Removed the casting of void *.
> > - corrected the typcasting in copy to user.
> > - removed un-necessary goto statement.
> > - Removed dead code for clean-up of memory.
> > - Removed un-mapping of secured memory
> > - Passing se_if_priv structure to init_device_context.
> > - Updated the below check to replace io.length with round_up(io.length).
> > 	if (shared_mem->size < shared_mem->pos|| io.length >=
> > shared_mem->size - shared_mem->pos)
> > - Created a function to cleanup the list of shared memory buffers.
> > - Used list_for_each_entry_safe(). created a separate functions:
> > se_dev_ctx_cpy_out_data() & se_dev_ctx_shared_mem_cleanup()
> >
> > 4/5
> > - Changed the compatible string to replace "-ele", to "-se".
> > - Declaration of imx_se_node_info, is done as const in the whole file
> > - Remove the unused macros from ele_base_msg.h
> > - Remove the function declaration get_phy_buf_mem_pool1, from the
> header file.
> > - Replace the use of dmam_alloc_coherent to dma_alloc_coherent
> > - Check for function pointer, before calling the fucntion pointer in
> > imx_fetch_se_soc_info
> > - Removed the unused flag for SE_MU_IO_FLAGS_USE_SEC_MEM.
> > -  Removed the unused macros WORD_SZ
> > - instead of struct device *dev, struct se_if_priv *priv, is used as
> > argument to the funtions:se_save_imem_state, se_restore_imem_state,
> > imx_fetch_se_soc_info
> > - Removed ret from validate_rsp_hdr.
> > - changed the prefix of the funtion: plat_add_msg_crc and
> plat_fill_cmd_msg_hdr.
> > - indentation correction for info structures.
> > - remove the check for priv not null from se_if_probe_cleanup
> > - Removed the casting of void *.
> > - se_load_firmware function is corrected for not freeing the buffer when
> allocation fails.
> > - Checking if get_imx_se_node_info() can return NULL, in se_if_probe()
> > - imem.size has type u32. return value from se_save_imem_state() will be
> assigned to imem.size in case of success only.
> > - removed the flag un-setting in case of failure. priv->flags &=
> > (~RESERVED_DMA_POOL);
> > - removed the function call for devm_of_platform_populate(dev);
> > - Checking for not-NULL,  before calling the funtion pointer
> se_fetch_soc_info.
> > - Removed the checking for reserved memory flag, before freeing up the
> reserved memory, in se_probe_if_cleanup.
> >
> > 3/5
> > - Changed the compatible string to replace "-ele", to "-se".
> >
> > 2/5
> > - to fix the warning error, replaced the "-ele" & "-v2x" in compatible string,
> to "-se".
> > - Added an example for ele@0 for compatible string "fsl,imx95-se"
> >
> > Changes in v2:
> >
> > 4/4
> > - Split this patch into two: 1. base driver & 2. Miscdev
> > - Initialize the return variable "err" as 0, before calling 'return
> > err', in the file ele_common.c
> > - Fix the usage of un-iniitialized pointer variable, by initializing them with
> NULL, in ele_base_msg.c.
> > - Fix initializing the ret variable, to return the correct error code in case of
> issue.
> > - replaced dmam_alloc_coherent with dma_alloc_coherent.
> > - Replace the use of ELE_GET_INFO_READ_SZ, with sizeof(soc_info).
> > - Replaced -1 with -EPERM
> > - Removed the safety check on func-input param, in ele_get_info().
> > - fix the assigning data[1] with lower 32 address, rather than zero, for
> ele_fw_authenticate API.
> > - Correctly initializing the function's return error code, for file
> ele_base_msg.c.
> > - replaced 'return' with 'goto'.
> > - Use length in bytes.
> > - Corrected the structure se_msg_hdr.
> > - Moved setting of rx_msg  to priv, into the function
> > imx_ele_msg_send_rcv
> > - Will add lockdep_assert_held, to receive path, in v2.
> > - corrected the spacing at "ret  = validate_rsp_hdr"
> > - FIELD_GET() used for RES_STATUS
> > - Re-write the structure soc_info, matching the information provided in
> response to this api.
> > - The "|" goes to the end of the previous line.
> > - Moved the locking and unlocking of the command lock to the caller of the
> function.
> > - removed the safety check for device private data.
> > - Structure memory reference, used to read message header.
> > - In the interrupt call back function, remove assigning waiting_rsp_dev to
> NULL, in case of response message rcv from FW.
> > - do while removed.
> > - replaced BIT(1) for RESERVED_DMA_POOL, to BIT(0)
> > - The backslash is removed while assigning the file name with absolute path
> to structure variable.fw_name_in_rfs =.
> > - Update the 'if' condition by removing "idx < 0".
> > - mbox_request_channel_byname() uses a "char" for the name not a u8.
> Corrected.
> > - devm managed resources, are not cleaned now, in function
> > se_probe_if_cleanup
> > - Used dev_err_probe().
> > - Used %pe to print error string.
> > - remove "__maybe_unused" for "struct platform_device *enum_plat_dev
> __maybe_unused;"
> > - used FIELD_GET(), for  RES_STATUS. Removed the use of MSG_TAG,
> MSG_COMMAND, MSG_SIZE, MSG_VER.
> > - Depricated the used of member of struct se_if_priv, bool
> > no_dev_ctx_used;
> > - Moved the text explaing the synchronization logic via mutexes, from patch
> 1/4 to se_ctrl.h.
> > - removed the type casting of info_list = (struct
> > imx_se_node_info_list *) device_get_match_data(dev->parent);
> > - Used static variable priv->soc_rev in the se_ctrl.c, replaced the following
> condition: if (info_list->soc_rev) to if (priv->soc_rev) for checking if this flow is
> already executed or not.
> > - imx_fetch_soc_info will return failure if the get_info function fails.
> > - Removed devm_free from imx_fetch_soc_info too.
> >
> > 3/3
> > - Made changes to move all the properties to parent node, without any
> child node.
> >
> > 2/4
> > - Use Hex pattern string.
> > - Move the properties to parent node, with no child node.
> > - Add i.MX95-ele to compatible nodes to fix the warning "/example-2/v2x:
> failed to match any schema with compatible: ['fsl,imx95-v2x']"
> >
> > 1/1
> > - Corrected the spelling from creats to creates.
> > - drop the braces around the plural 's' for interfaces
> > - written se in upper case SE.
> > - Replace "multiple message(s)" with messages.
> > - Removed too much details about locks.
> >
> > Testing
> > - make CHECK_DTBS=y freescale/imx8ulp-evk.dtb;
> > - make ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- -j8
> > dt_binding_check DT_SCHEMA_FILES=fsl,imx-se.yaml
> > - make C=1 CHECK=scripts/coccicheck drivers/firmware/imx/*.* W=1 >
> > r.txt
> > - ./scripts/checkpatch.pl --git <>..HEAD
> > - Tested the Image and .dtb, on the i.MX8ULP.
> >
> > Reference
> > - Link to v1:
> > https://lore.kernel.org/r/20240510-imx-se-if-v1-0-27c5a674916d@nxp.com
> > - Link to v2:
> > https://lore.kernel.org/r/20240523-imx-se-if-v2-0-5a6fd189a539@nxp.com
> >
> > ---
> > Pankaj Gupta (5):
> >       Documentation/firmware: add imx/se to other_interfaces
> >       dt-bindings: arm: fsl: add imx-se-fw binding doc
> >       arm64: dts: imx8ulp-evk: add nxp secure enclave firmware
> >       firmware: imx: add driver for NXP EdgeLock Enclave
> >       firmware: imx: adds miscdev
> >
> >  Documentation/ABI/testing/se-cdev                  |   43 +
> >  .../devicetree/bindings/firmware/fsl,imx-se.yaml   |   91 ++
> >  .../driver-api/firmware/other_interfaces.rst       |  121 ++
> >  arch/arm64/boot/dts/freescale/imx8ulp-evk.dts      |   17 +-
> >  arch/arm64/boot/dts/freescale/imx8ulp.dtsi         |   13 +-
> >  drivers/firmware/imx/Kconfig                       |   12 +
> >  drivers/firmware/imx/Makefile                      |    2 +
> >  drivers/firmware/imx/ele_base_msg.c                |  286 +++++
> >  drivers/firmware/imx/ele_base_msg.h                |   95 ++
> >  drivers/firmware/imx/ele_common.c                  |  318 +++++
> >  drivers/firmware/imx/ele_common.h                  |   51 +
> >  drivers/firmware/imx/se_ctrl.c                     | 1305 ++++++++++++++++++++
> >  drivers/firmware/imx/se_ctrl.h                     |  151 +++
> >  include/linux/firmware/imx/se_api.h                |   14 +
> >  include/uapi/linux/se_ioctl.h                      |   94 ++
> >  15 files changed, 2610 insertions(+), 3 deletions(-)
> > ---
> > base-commit: b63ff26648537a5600cf79bd62f916792c53e015
> > change-id: 20240507-imx-se-if-a40055093dc6
> >
> > Best regards,
> > --
> > Pankaj Gupta <pankaj.gupta@....com>
> >

Powered by blists - more mailing lists