On Thu, Mar 14, 2024 at 09:35:21PM +0800, Peng Fan (OSS) wrote:
> +static int pinctrl_scmi_get_function_groups(struct pinctrl_dev *pctldev,
> + unsigned int selector,
> + const char * const **groups,
> + unsigned int * const num_groups)
> +{
> + const unsigned int *group_ids;
> + int ret, i;
> + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + if (!groups || !num_groups)
> + return -EINVAL;
> +
> + if (selector < pmx->nr_functions &&
> + pmx->functions[selector].num_groups) {
If pmx->functions[selector].num_groups is set then we assume that
functions[selector].groups has been allocated.
> + *groups = (const char * const *)pmx->functions[selector].groups;
> + *num_groups = pmx->functions[selector].num_groups;
> + return 0;
> + }
> +
> + ret = pinctrl_ops->function_groups_get(pmx->ph, selector,
> + &pmx->functions[selector].num_groups,
> + &group_ids);
However, pmx->functions[selector].num_groups is set here and not cleared
on the error paths. Or instead of clearing the .num_groups it would be
nice to pass a local variable and only do the
pmx->functions[selector].num_groups = local assignment right before the
success return.
regards,
dan carpenter
> + if (ret) {
> + dev_err(pmx->dev, "Unable to get function groups, err %d", ret);
> + return ret;
> + }
> +
> + *num_groups = pmx->functions[selector].num_groups;
> + if (!*num_groups)
> + return -EINVAL;
> +
> + pmx->functions[selector].groups =
> + devm_kcalloc(pmx->dev, *num_groups,
> + sizeof(*pmx->functions[selector].groups),
> + GFP_KERNEL);
> + if (!pmx->functions[selector].groups)
> + return -ENOMEM;
> +
> + for (i = 0; i < *num_groups; i++) {
> + pmx->functions[selector].groups[i] =
> + pinctrl_scmi_get_group_name(pmx->pctldev,
> + group_ids[i]);
> + if (!pmx->functions[selector].groups[i]) {
> + ret = -ENOMEM;
> + goto err_free;
> + }
> + }
> +
> + *groups = (const char * const *)pmx->functions[selector].groups;
> +
> + return 0;
> +
> +err_free:
> + devm_kfree(pmx->dev, pmx->functions[selector].groups);
> +
> + return ret;
> +}
