Re: [PATCH v2 04/13] pinctrl: berlin: add optional pinconf support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [Medium] pinctrl: berlin: memory leak on error in
  berlin_pinctrl_dt_node_to_map()
- [Medium] pinctrl: berlin: wrong errno -EOPNOTSUPP in
  berlin_pinconf_group_get()
- [Low] pinctrl: berlin: map over-allocation in
  berlin_pinctrl_dt_node_to_map()
- [Low] pinctrl: berlin: berlin_pinconf_group_get() fails when slew rate
  is 0
--

commit 8d9a060e7649872ae247085353d00ffa248ae051
Author: Jisheng Zhang <jszhang@kernel.org>

pinctrl: berlin: add optional pinconf support

This commit adds generic pin configuration support (bias, drive strength,
input enable, Schmitt trigger, and slew rate) to the Berlin pinctrl driver
for 8-bit and 10-bit group configuration registers.

> diff --git a/drivers/pinctrl/berlin/berlin.c b/drivers/pinctrl/berlin/berlin.c
> index a0ae75b1d705d..0edf9d5a2a7b7 100644
> --- a/drivers/pinctrl/berlin/berlin.c
> +++ b/drivers/pinctrl/berlin/berlin.c
[ ... ]
> @@ -75,11 +95,19 @@ static int berlin_pinctrl_dt_node_to_map(struct pinctrl_dev *pctrl_dev,
>  		return -EINVAL;
>  	}
>  
> +	ret = pinconf_generic_parse_dt_config(node, pctrl_dev, &configs,
> +					      &num_configs);
> +	if (ret < 0) {
> +		dev_err(pctrl->dev, "%s: could not parse node property\n",
> +			node->name);
> +		return ret;
> +	}
> +
>  	ret = pinctrl_utils_reserve_map(pctrl_dev, map, &reserved_maps,
> -					num_maps, ngroups);
> +					num_maps, ngroups * (num_configs + 1));

[Severity: Low]
Does this over-allocate *map when num_configs > 1?
pinctrl_utils_add_map_configs() packs all num_configs entries for a group
into a single PIN_MAP_TYPE_CONFIGS_GROUP map slot, so each group uses at
most two map entries (ngroups * (!!num_configs + 1) in total), leaving
ngroups * (num_configs - 1) reserved entries unused.

>  	if (ret) {
>  		dev_err(pctrl->dev, "can't reserve map: %d\n", ret);
> -		return ret;
> +		goto exit;
>  	}
>  
>  	of_property_for_each_string(node, "groups", prop, group_name) {
> @@ -88,11 +116,22 @@ static int berlin_pinctrl_dt_node_to_map(struct pinctrl_dev *pctrl_dev,
>  						function_name);
>  		if (ret) {
>  			dev_err(pctrl->dev, "can't add map: %d\n", ret);
> -			return ret;
> +			goto exit;
> +		}
> +
> +		if (num_configs) {
> +			ret = pinctrl_utils_add_map_configs(pctrl_dev, map,
> +					&reserved_maps, num_maps, group_name,
> +					configs, num_configs,
> +					PIN_MAP_TYPE_CONFIGS_GROUP);
> +			if (ret < 0)
> +				goto exit;
>  		}
>  	}
>  
> -	return 0;
> +exit:
> +	kfree(configs);
> +	return ret;
>  }

[Severity: Medium]
Does the error path leak *map and any per-group config arrays already
duplicated by pinctrl_utils_add_map_configs()?

When ops->dt_node_to_map() returns a negative error code, the caller
dt_to_map_one_config() in drivers/pinctrl/devicetree.c returns immediately
without freeing the partially constructed map.  If
pinctrl_utils_add_map_mux() or pinctrl_utils_add_map_configs() fails in the
loop after pinctrl_utils_reserve_map() allocated *map, jumping to exit only
frees the temporary configs array and leaks *map along with any configs
duplicated for earlier groups.  Calling pinctrl_utils_free_map(pctrl_dev,
*map, *num_maps) when ret < 0 would clean these up.

> @@ -177,6 +216,190 @@ static const struct pinmux_ops berlin_pinmux_ops = {
>  	.set_mux		= &berlin_pinmux_set,
>  };
>  
> +static int berlin_pinconf_group_get(struct pinctrl_dev *pctrl_dev,
> +				    unsigned int group, unsigned long *configs)
> +{
> +	enum pin_config_param param = pinconf_to_config_param(*configs);
> +	struct berlin_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctrl_dev);
> +	const struct berlin_desc_group *group_desc = pctrl->desc->groups + group;
> +	bool enabled;
> +	u32 val;
> +	int ret;
> +	u8 conf_bits = group_desc->conf_bits;
> +
> +	if (!conf_bits)
> +		return -EOPNOTSUPP;
[ ... ]
> +	case PIN_CONFIG_SLEW_RATE:
> +		if (conf_bits == 10)
> +			enabled = val & PINCFG_SL_10BIT;
> +		else
> +			enabled = val & PINCFG_SL_8BIT;
> +		val = enabled ? 1 : 0;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	*configs = pinconf_to_config_packed(param, val);
> +	return enabled ? 0 : -EINVAL;
> +}

[Severity: Low]
Does querying PIN_CONFIG_SLEW_RATE fail with -EINVAL whenever the slew rate
bit is 0?

berlin_pinconf_group_set() accepts 0 as a valid slew-rate argument to clear
PINCFG_SL_10BIT or PINCFG_SL_8BIT, but berlin_pinconf_group_get() sets
enabled from that bit and returns -EINVAL when enabled is false, so a slew
rate of 0 cannot be read back (for example via debugfs pinconf-groups).
Setting enabled = true as in the PIN_CONFIG_DRIVE_STRENGTH case would allow
both 0 and 1 to be reported.

> +
> +static int berlin_pinconf_group_set(struct pinctrl_dev *pctrl_dev,
> +				    unsigned int group, unsigned long *configs,
> +				    unsigned int nconfigs)
> +{
> +	struct berlin_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctrl_dev);
> +	const struct berlin_desc_group *group_desc = pctrl->desc->groups + group;
> +	u8 conf_bits = group_desc->conf_bits;
> +	u32 mask = 0, val = 0;
> +	int i;
> +
> +	if (!conf_bits)
> +		return -EOPNOTSUPP;
[ ... ]
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +	}

[Severity: Medium]
Should berlin_pinconf_group_get() and berlin_pinconf_group_set() return
-ENOTSUPP instead of -EOPNOTSUPP when !conf_bits or when param is not
supported?

With .is_generic = true, reading debugfs pinconf-groups calls
pinconf_generic_dump_one(), which queries every generic parameter in
conf_items[] and only treats -EINVAL and -ENOTSUPP as legal responses:

drivers/pinctrl/pinconf-generic.c:pinconf_generic_dump_one() {
	...
		ret = pin_config_group_get(dev_name(pctldev->dev),
					   pindesc->name, &config);
		/* These are legal errors */
		if (ret == -EINVAL || ret == -ENOTSUPP)
			continue;
		if (ret) {
			seq_printf(s, "ERROR READING CONFIG SETTING %d ", i);
			continue;
		}
	...
}

Returning -EOPNOTSUPP causes debugfs pinconf-groups to print "ERROR READING
CONFIG SETTING" for every unsupported parameter and for any group with
conf_bits == 0.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261115102401.3520-1-jszhang@kernel.org?part=4