[EXT] Re: [PATCH v12 4/5] firmware: imx: add driver for NXP EdgeLock Enclave

[Krzysztof Kozlowski]

On 22/01/2025 12:13, Pankaj Gupta wrote:
> 
> 
> -----Original Message----- From: Krzysztof Kozlowski
> <krzk at kernel.org>> Sent: Monday, January 20, 2025 5:54 PM To: Pankaj
> Gupta <pankaj.gupta at nxp.com>>; Jonathan Corbet <corbet at lwn.net>>; 
> Rob Herring <robh at kernel.org>>; Krzysztof Kozlowski
> <krzk+dt at kernel.org>>; Conor Dooley <conor+dt at kernel.org>>; Shawn
> Guo <shawnguo at kernel.org>>; Sascha Hauer <s.hauer at pengutronix.de>>;
> Pengutronix Kernel Team <kernel at pengutronix.de>>; Fabio Estevam
> <festevam at gmail.com>> Cc: linux-doc at vger.kernel.org; linux-
> kernel at vger.kernel.org; devicetree at vger.kernel.org;
> imx at lists.linux.dev; linux-arm-kernel at lists.infradead.org Subject:
> [EXT] Re: [PATCH v12 4/5] firmware: imx: add driver for NXP EdgeLock
> Enclave
> 
> Caution: This is an external email. Please take care when clicking
> links or opening attachments. When in doubt, report the message
> using the 'Report this email' button


You got comment on this, more than once.

Rest of the email is poorly formatted, so I am just skimming through it.
Fix your email program to send readable content if you want some answers
for stuff I missed. I expect all my comments fully addressed, not just
some of them.


>>> + +static int se_if_probe(struct platform_device *pdev) { +
>>> const struct se_if_node_info_list *info_list; +     const struct
>>> se_if_node_info *info; +     struct device *dev = &pdev->>dev; 
>>> +     struct se_fw_load_info *load_fw; +     struct se_if_priv
>>> *priv; +     u32 idx; +     int ret; +q +     idx =
>>> GET_IDX_FROM_DEV_NODE_NAME(dev->>of_node);
> 
>> NAK. Node can be called firmware and your entire driver collapes.
> The macro is updated to verify the correct-ness of node-name.

NAK, do you understand the term? I provided the reasons for NAK.

> 
> +               (!memcmp(dev_of_node->full_name, NODE_NAME, 
> strlen(NODE_NAME)) ?\ ((strlen(dev_of_node->full_name) >
> strlen(NODE_NAME)) ?\ GET_ASCII_TO_U8((strlen(dev_of_node-
> >full_name) - strlen(NODE_NAME)),\ dev_of_node-
> >full_name[strlen(NODE_NAME) + 1], \ -
> dev_of_node->full_name[strlen(NODE_NAME) + 2]) : 0) 
> +                               dev_of_node-
> >full_name[strlen(NODE_NAME) + 2]) : 0) : -EINVAL)
> 
>>> +     info_list = device_get_match_data(dev); +     if (idx >>=
>>> info_list->>num_mu) { +             dev_err(dev, 
>>> +                     "Incorrect node name :%s\n", 
>>> +                     dev->>of_node->>full_name);
> 
>> Nope. "firmware" or "secure" are correct node names.
> New check is added to validate the correctness of the node name for
> this driver. Replaced the message of " Incorrect node name..", with
> the help message.

You did not resolve the NAK.
1. You cannot reject correct names.
2. You cannot add undocumented ABI. You could try to document it, but it
will not solve the first problem.


Best regards,
Krzysztof