Re: [PATCH v2 5/5] pinctrl: pinctrl-scmi: Log number of pins, groups, functions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Low] Unnecessary structure bloat by adding `nr_groups` to persistent driver state.
- [Low] Missing error handling for firmware query result before assignment to an unsigned integer.
- [Low] Format specifier type mismatch in `dev_dbg`.
--

commit 1afe1feb7c661d03b99df0a5046eac6639ef2fe6
Author: Alex Tran <alex.tran@oss.qualcomm.com>

pinctrl: pinctrl-scmi: Log number of pins, groups, functions

The SCMI pinctrl driver does not currently log the number of pins, groups,
and functions discovered from firmware. This commit adds a debug log statement
to print these counts after a successful probe.

> diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c
> index f22be6b7b82a4..999c2061ddc38 100644
> --- a/drivers/pinctrl/pinctrl-scmi.c
> +++ b/drivers/pinctrl/pinctrl-scmi.c
> @@ -40,6 +40,7 @@ struct scmi_pinctrl {
>  	struct pinctrl_desc pctl_desc;
>  	struct pinfunction *functions;
>  	unsigned int nr_functions;
> +	unsigned int nr_groups;

Is it necessary to add nr_groups to struct scmi_pinctrl?

Unlike nr_functions which is used later, nr_groups is only written and read
sequentially within scmi_pinctrl_probe() to populate the dev_dbg message.
Could this simply be a local variable in the probe function to avoid bloating
the persistent device data structure?

>  };
>  
>  static int pinctrl_scmi_get_groups_count(struct pinctrl_dev *pctldev)

[ ... ]

> @@ -578,7 +579,15 @@ static int scmi_pinctrl_probe(struct scmi_device *sdev)
>  	if (!pmx->functions)
>  		return -ENOMEM;
>  
> -	return pinctrl_enable(pmx->pctldev);
> +	pmx->nr_groups = pinctrl_scmi_get_groups_count(pmx->pctldev);

What happens if pinctrl_scmi_get_groups_count() returns a negative error code
due to an SCMI firmware query failure?

Since nr_groups is an unsigned int, assigning a negative error code will
result in a silent integer underflow. While this isn't used for memory
allocation or bounds checking, it will result in incorrect values being
logged.

> +
> +	ret = pinctrl_enable(pmx->pctldev);
> +	if (ret)
> +		return ret;
> +
> +	dev_dbg(dev, "Initialized %d pins, %d groups, %d functions\n",
> +		 pmx->pctl_desc.npins, pmx->nr_groups, pmx->nr_functions);

Should we use %u instead of %d for these format specifiers?

pmx->pctl_desc.npins, nr_groups, and nr_functions are all unsigned integers.
Using %d causes any underflowed error codes to print deceptively as negative
numbers in the logs rather than revealing the implicit cast.

> +	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513-scmi-client-probe-log-v2-0-36607e9dd540@oss.qualcomm.com?part=5