Re: [PATCH v4 1/4] firmware: arm_scmi: Add optional flags to extended names helper

[Cristian Marussi]

On Tue, Aug 08, 2023 at 06:25:35PM +0000, Oleksii Moisieiev wrote:
> From: Cristian Marussi <cristian.marussi@xxxxxxx>
> 
> Some recently added SCMI protocols needs an additional flags parameter to
> be able to properly configure the command used to query the extended name
> of a resource.
> 

Hi Oleksii,

> Signed-off-by: Cristian Marussi <cristian.marussi@xxxxxxx>
> ---

You need to apply also your Signed-off as last in V5 for this to be
accepted AND beside this I spotted a bug while re-testing this. (my bad)
See down below a possible fix.

>  drivers/firmware/arm_scmi/clock.c     | 2 +-
>  drivers/firmware/arm_scmi/driver.c    | 7 +++++--
>  drivers/firmware/arm_scmi/perf.c      | 3 ++-
>  drivers/firmware/arm_scmi/power.c     | 2 +-
>  drivers/firmware/arm_scmi/powercap.c  | 2 +-
>  drivers/firmware/arm_scmi/protocols.h | 3 ++-
>  drivers/firmware/arm_scmi/reset.c     | 3 ++-
>  drivers/firmware/arm_scmi/sensors.c   | 2 +-
>  drivers/firmware/arm_scmi/voltage.c   | 2 +-
>  9 files changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> index 96060bf90a24..e6e087686e8c 100644
> --- a/drivers/firmware/arm_scmi/clock.c
> +++ b/drivers/firmware/arm_scmi/clock.c
> @@ -169,7 +169,7 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
>  	if (!ret && PROTOCOL_REV_MAJOR(version) >= 0x2) {
>  		if (SUPPORTS_EXTENDED_NAMES(attributes))
>  			ph->hops->extended_name_get(ph, CLOCK_NAME_GET, clk_id,
> -						    clk->name,
> +						    NULL, clk->name,
>  						    SCMI_MAX_STR_SIZE);
>  
>  		if (SUPPORTS_RATE_CHANGED_NOTIF(attributes))
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index e7d97b59963b..729201d8f935 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -1438,6 +1438,7 @@ struct scmi_msg_resp_domain_name_get {
>   * @ph: A protocol handle reference.
>   * @cmd_id: The specific command ID to use.
>   * @res_id: The specific resource ID to use.
> + * @flags: A pointer to specific flags to use, if any.
>   * @name: A pointer to the preallocated area where the retrieved name will be
>   *	  stored as a NULL terminated string.
>   * @len: The len in bytes of the @name char array.
> @@ -1445,8 +1446,8 @@ struct scmi_msg_resp_domain_name_get {
>   * Return: 0 on Succcess
>   */
>  static int scmi_common_extended_name_get(const struct scmi_protocol_handle *ph,
> -					 u8 cmd_id, u32 res_id, char *name,
> -					 size_t len)
> +					 u8 cmd_id, u32 res_id, u32 *flags,
> +					 char *name, size_t len)
>  {
>  	int ret;
>  	struct scmi_xfer *t;
> @@ -1458,6 +1459,8 @@ static int scmi_common_extended_name_get(const struct scmi_protocol_handle *ph,
>  		goto out;
>  
>  	put_unaligned_le32(res_id, t->tx.buf);
> +	if (flags)
> +		put_unaligned_le32(*flags, t->tx.buf + sizeof(res_id));
>  	resp = t->rx.buf;
>  
>  	ret = ph->xops->do_xfer(ph, t);

Here my patch is buggy since when you provide the optional flags they
are after the res_id parameter BUT the previous xfer_get_init still
requires an xfer with a tx_len size of sizeof(res_id) ONLY, so while the
flags will fit into tx.buf that second optional flags field won't be
sent because the core SCMI stack will see t->tx.len == 4 :<
(so this flags extension wont work when needed in Pinctrl)

A possible fix that I tested consist in changing this snippet with:

-->8---
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index b5957cc12fee..06c101edba7f 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -1438,6 +1438,7 @@ struct scmi_msg_resp_domain_name_get {
  * @ph: A protocol handle reference.
  * @cmd_id: The specific command ID to use.
  * @res_id: The specific resource ID to use.
+ * @flags: A pointer to specific flags to use, if any.
  * @name: A pointer to the preallocated area where the retrieved name will be
  *       stored as a NULL terminated string.
  * @len: The len in bytes of the @name char array.
@@ -1445,19 +1446,22 @@ struct scmi_msg_resp_domain_name_get {
  * Return: 0 on Succcess
  */
 static int scmi_common_extended_name_get(const struct scmi_protocol_handle *ph,
-                                        u8 cmd_id, u32 res_id, char *name,
-                                        size_t len)
+                                        u8 cmd_id, u32 res_id, u32 *flags,
+                                        char *name, size_t len)
 {
        int ret;
+       size_t txlen;
        struct scmi_xfer *t;
        struct scmi_msg_resp_domain_name_get *resp;
 
-       ret = ph->xops->xfer_get_init(ph, cmd_id, sizeof(res_id),
-                                     sizeof(*resp), &t);
+       txlen = !flags ? sizeof(res_id) : sizeof(res_id) + sizeof(*flags);
+       ret = ph->xops->xfer_get_init(ph, cmd_id, txlen, sizeof(*resp), &t);
        if (ret)
                goto out;
 
        put_unaligned_le32(res_id, t->tx.buf);
+       if (flags)
+               put_unaligned_le32(*flags, t->tx.buf + sizeof(res_id));
        resp = t->rx.buf;
 
        ret = ph->xops->do_xfer(ph, t);


---8<---

My bad,

Thanks,
Cristian