On 17.05.2024 11:24:46, Pankaj Gupta wrote:
> > > new file mode 100644
> > > index 000000000000..0463f26d93c7
> > > --- /dev/null
> > > +++ b/drivers/firmware/imx/ele_base_msg.c
> > > @@ -0,0 +1,287 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Copyright 2024 NXP
> > > + */
> > > +
> > > +#include <linux/types.h>
> > > +#include <linux/completion.h>
> > > +#include <linux/dma-mapping.h>
> > > +
> > > +#include "ele_base_msg.h"
> > > +#include "ele_common.h"
> > > +
> > > +int ele_get_info(struct device *dev, struct soc_info *s_info)
> > > +{
> > > + struct se_if_priv *priv = dev_get_drvdata(dev);
> > > + struct se_api_msg *tx_msg __free(kfree);
> > > + struct se_api_msg *rx_msg __free(kfree);
> > > + phys_addr_t get_info_addr;
> > > + u32 *get_info_data;
> > > + u32 status;
> > > + int ret;
> > > +
> > > + if (!priv || !s_info)
> > > + goto exit;
> >
> > You should code properly, so that this doesn't happen, your cleanup is
> > broken, it will work on uninitialized data, as Sascha already mentioned.
>
> The API(s) part of this file will be later exported and might get used by driver/crypto/ele/*.c.
> Still if you think, this check should be removed, I will do it in v2.
It makes no sense to call these functions with NULL pointers, if you do
so, it's a mistake by the caller. If it's used by some other part of the
ele driver that should be coded properly.
> > > +
> > > + memset(s_info, 0x0, sizeof(*s_info));
> > > +
> > > + if (priv->mem_pool_name)
> > > + get_info_data = get_phy_buf_mem_pool(dev,
> > > + priv->mem_pool_name,
> > > + &get_info_addr,
> > > + ELE_GET_INFO_BUFF_SZ);
> > > + else
> > > + get_info_data = dmam_alloc_coherent(dev,
> > > + ELE_GET_INFO_BUFF_SZ,
> > > + &get_info_addr,
> > > + GFP_KERNEL);
> >
> > It's better style to move the init of the dma memory into the probe
> > function.
>
> It is not DMA init. It is DMA allocation.
It's better style to move the allocation of the dma memory into the
probe function.
[...]
> > > + priv->rx_msg = rx_msg;
> > > + ret = imx_ele_msg_send_rcv(priv, tx_msg);
> >
> > This API looks strange, why put the tx_msg as a parameter the rx_msg
> > into the private struct?
>
> The rx_msg is the populated in the interrupt context. Hence, it kept
> as part of private structure; which is in-turn associated with
> mbox_client.
These are implementation details, it just feels strange to pass one
parameter via an arguments and put the other in the private pointer.
> Though, in v2 moving the rx_msg setting to imx_ele_msg_send_rcv(priv,
> tx_msg, rx_msg);
fine
[...]
> > > + if (status != priv->success_tag) {
> > > + dev_err(dev, "Command Id[%d], Response Failure = 0x%x",
> > > + ELE_GET_INFO_REQ, status);
> > > + ret = -1;
> > > + }
> > > +
> > > + s_info->imem_state = (get_info_data[ELE_IMEM_STATE_WORD]
> > > + & ELE_IMEM_STATE_MASK) >> 16;
> >
> > can you use a struct for get_info_data and use FIELD_GET() (if needed)
>
> Re-write the structure soc_info, matching the information provided in
> response to this api.
Looks better. Please compile the driver and check with "pahole" that the
layout of these structures doesn't contain any unwanted padding.
Otherwise add "__packed" and if you can guarantee "__aligned(4)".
> struct dev_info {
> uint8_t cmd;
> uint8_t ver;
> uint16_t length;
> uint16_t soc_id;
> uint16_t soc_rev;
> uint16_t lmda_val;
> uint8_t ssm_state;
> uint8_t dev_atts_api_ver;
> uint8_t uid[MAX_UID_SIZE];
> uint8_t sha_rom_patch[DEV_GETINFO_ROM_PATCH_SHA_SZ];
> uint8_t sha_fw[DEV_GETINFO_FW_SHA_SZ];
> };
>
> struct dev_addn_info {
> uint8_t oem_srkh[DEV_GETINFO_OEM_SRKH_SZ];
> uint8_t trng_state;
> uint8_t csal_state;
> uint8_t imem_state;
> uint8_t reserved2;
> };
>
> struct soc_info {
> struct dev_info d_info;
> struct dev_addn_info d_addn_info;
> };
[...]
> > > +int imx_ele_msg_send(struct se_if_priv *priv, void *mssg)
> > > +{
> > > + bool is_cmd_lock_tobe_taken = false;
> > > + int err;
> > > +
> > > + if (!priv->waiting_rsp_dev || priv->no_dev_ctx_used) {
> > > + is_cmd_lock_tobe_taken = true;
> > > + mutex_lock(&priv->se_if_cmd_lock);
> > > + }
> > > + scoped_guard(mutex, &priv->se_if_lock);
> > > +
> > > + err = mbox_send_message(priv->tx_chan, mssg);
> > > + if (err < 0) {
> > > + dev_err(priv->dev, "Error: mbox_send_message failure.\n");
> > > + if (is_cmd_lock_tobe_taken)
> > > + mutex_unlock(&priv->se_if_cmd_lock);
> >
> > Only dropping the lock in case of failure doesn't look right to me.
>
> The callers of this function, takes the execution flow to aborting the
> operation on getting return code < 0. No next action is expected under
> this aborted operation. Unlocking the lock here is not an issue
>
> > It seems you should better move the lock to the callers of this function.
>
> Accepted, and moved to the caller of the function for:
> - locking
> - unlocking in case of error.
>
> Unlocking in the read API, once response is successfully received and
> read.
A better design would be: imx_ele_msg_rcv() imx_ele_msg_send() are
expected to be called locked. Add lockdep_assert_held() to these
function to document/check this.
The callers of imx_ele_msg_rcv() and imx_ele_msg_send() have to take
care of the locking.
[...]
> > > +static const struct imx_se_node_info_list imx8ulp_info = {
> > > + .num_mu = 1,
> > > + .soc_id = SOC_ID_OF_IMX8ULP,
> > > + .info = {
> > > + {
> > > + .se_if_id = 2,
> > > + .se_if_did = 7,
> > > + .max_dev_ctx = 4,
> > > + .cmd_tag = 0x17,
> > > + .rsp_tag = 0xe1,
> > > + .success_tag = 0xd6,
> > > + .base_api_ver = MESSAGING_VERSION_6,
> > > + .fw_api_ver = MESSAGING_VERSION_7,
> > > + .se_name = "hsm1",
> > > + .mbox_tx_name = "tx",
> > > + .mbox_rx_name = "rx",
> > > + .pool_name = "sram",
> > > + .fw_name_in_rfs = IMX_ELE_FW_DIR\
> > ^
> > not needed
>
> It is needed for i.MX8ULP, dual FW support.
The backslash is not needed.
>
> > > + "mx8ulpa2ext-ahab- container.img",
>
>
> > > + .soc_register = true,
> > > + .reserved_dma_ranges = true,
> > > + .imem_mgmt = true,
> > > + },
> > > + },
> > > +};
> > > +
> > > +static const struct imx_se_node_info_list imx93_info = {
> > > + .num_mu = 1,
> > > + .soc_id = SOC_ID_OF_IMX93,
> > > + .info = {
> > > + {
> > > + .se_if_id = 2,
> > > + .se_if_did = 3,
> > > + .max_dev_ctx = 4,
> > > + .cmd_tag = 0x17,
> > > + .rsp_tag = 0xe1,
> > > + .success_tag = 0xd6,
> > > + .base_api_ver = MESSAGING_VERSION_6,
> > > + .fw_api_ver = MESSAGING_VERSION_7,
> > > + .se_name = "hsm1",
> > > + .mbox_tx_name = "tx",
> > > + .mbox_rx_name = "rx",
> > > + .reserved_dma_ranges = true,
> > > + .imem_mgmt = true,
> > > + .soc_register = true,
> > > + },
> > > + },
> >
> >
> > Some (most?) members of these structs are the same. Why do you have this
> > abstraction if it's not needed right now?
>
> It is needed as the values is different for different NXP SoC
> compatible. It will be needed for NXP i.MX95 platform, whose code will
> be next in pipeline.
How does the imx95 .info look like?
[...]
> > > +static int imx_fetch_soc_info(struct device *dev)
> > > +{
> > > + struct se_if_priv *priv = dev_get_drvdata(dev);
> > > + struct imx_se_node_info_list *info_list;
> > > + const struct imx_se_node_info *info;
> > > + struct soc_device_attribute *attr;
> > > + struct soc_device *sdev;
> > > + struct soc_info s_info;
> > > + int err = 0;
> > > +
> > > + info = priv->info;
> > > + info_list = (struct imx_se_node_info_list *)
> > > + device_get_match_data(dev->parent);
> >
> > I think cast is not needed.
>
> It returns memory reference with const attribute. SoC revision member
> of 'info_list', is required to be updated. Thus type casted.
Have you considered that this memory is marked as const for a reason?
It's const, you cannot change it. Place any values that have to changed
into your priv.
> > > + if (info_list->soc_rev)
> > > + return err;
> >
> > What does this check do? You'll only get data you put in the info_list
> > in the first place.
> info_list->soc_rev, is equal to zero for the first call to this
> function. To return from this function if this function is already
> executed.
This looks wrong, see above.
> > > + err = ele_get_info(dev, &s_info);
> > > + if (err)
> > > + s_info.major_ver = DEFAULT_IMX_SOC_VER;
> >
> > Why continue here in case of error?
>
> To continue with SoC registration for the default values (without
> fetching information from ELE).
Have you tested the driver that it will work, if this fails?
> > > +
> > > + info_list->soc_rev = s_info.soc_rev;
> > > +
> > > + if (!info->soc_register)
> > > + return 0;
> > > +
> > > + attr = devm_kzalloc(dev, sizeof(*attr), GFP_KERNEL);
> > > + if (!attr)
> > > + return -ENOMEM;
> > > +
> > > + if (s_info.minor_ver)
> > > + attr->revision = devm_kasprintf(dev, GFP_KERNEL, "%x.%x",
> > > + s_info.major_ver,
> > > + s_info.minor_ver);
> > > + else
> > > + attr->revision = devm_kasprintf(dev, GFP_KERNEL, "%x",
> > > + s_info.major_ver);
> > > +
> > > + switch (s_info.soc_id) {
> > > + case SOC_ID_OF_IMX8ULP:
> > > + attr->soc_id = devm_kasprintf(dev, GFP_KERNEL,
> > > + "i.MX8ULP");
> > > + break;
> > > + case SOC_ID_OF_IMX93:
> > > + attr->soc_id = devm_kasprintf(dev, GFP_KERNEL,
> > > + "i.MX93");
> > > + break;
> > > + }
> > > +
> > > + err = of_property_read_string(of_root, "model",
> > > + &attr->machine);
> > > + if (err) {
> > > + devm_kfree(dev, attr);
> >
> > Why do you do a manual cleanup of devm managed resources? Same applies
> > to the other devm managed resources, too.
> >
> Used devm managed memory, as this function is called as part probe.
> Post device registration, this devm managed memory is un-necessarily
> blocked. It is better to release it as part of clean-up, under this
> function only.
Why do you allocate the memory with devm in the first place, if it's not
needed after probe?
> Other devm managed memory clean-up, under se_probe_cleanup, will be
> removed, as suggested.
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
Attachment:
signature.asc
Description: PGP signature
