Re: [PATCH 6/9] pinctrl: ultrarisc: Add UltraRISC DP1000 pinctrl driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Memory leak of temporary DT parsing arrays on consumer probe paths
- [Medium] Silent ignoring of unsupported pin configurations
- [Low] Build failure due to dynamic mask in `field_prep()`
- [Low] Signed integer overflow during pin mux configuration
- [Low] Unnecessary Kconfig dependencies selected
--

commit 03445610d24a54757505c7f2def35cc4894dd19f
Author: Jia Wang <wangjia@ultrarisc.com>

pinctrl: ultrarisc: Add UltraRISC DP1000 pinctrl driver

This commit adds the pinctrl driver for the UltraRISC DP1000 pinctrl
controller.

> diff --git a/drivers/pinctrl/ultrarisc/Kconfig b/drivers/pinctrl/ultrarisc/Kconfig
> new file mode 100644
> index 0000000000000..ba8747b90127d
> --- /dev/null
> +++ b/drivers/pinctrl/ultrarisc/Kconfig
> @@ -0,0 +1,23 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +config PINCTRL_ULTRARISC
> +	tristate
> +	depends on OF
> +	select PINMUX
> +	select GENERIC_PINCTRL_GROUPS
> +	select GENERIC_PINCONF
> +	select GENERIC_PINMUX_FUNCTIONS
> +	select GPIOLIB
> +	select IRQ_DOMAIN_HIERARCHY
> +	select MFD_SYSCON

This isn't a bug, but is it necessary to select IRQ_DOMAIN_HIERARCHY and
MFD_SYSCON here? The driver code doesn't seem to include the corresponding
headers or use their APIs.

[ ... ]
> diff --git a/drivers/pinctrl/ultrarisc/pinctrl-ultrarisc.c b/drivers/pinctrl/ultrarisc/pinctrl-ultrarisc.c
> new file mode 100644
> index 0000000000000..774746943e283
> --- /dev/null
> +++ b/drivers/pinctrl/ultrarisc/pinctrl-ultrarisc.c
[ ... ]
> @@ -0,0 +1,746 @@
[ ... ]
> +static int ur_write_pin_conf(struct ur_pinctrl *pctldata, unsigned int pin, u32 conf)
> +{
> +	const struct ur_port_desc *port_desc;
> +	struct ur_pin_val pin_val;
> +	unsigned long flags;
> +	void __iomem *reg;
> +	u32 reg_offset;
> +	u32 val;
> +	u32 shift;
> +	u32 mask;
> +
> +	if (ur_pin_num_to_port_pin(pctldata->match_data, &pin_val, pin))
> +		return -EINVAL;
> +
> +	port_desc = &pctldata->match_data->ports[pin_val.port];
> +	reg_offset = ur_get_pin_conf_offset(port_desc, pin_val.pin);
> +	reg = pctldata->base + reg_offset;
> +	shift = (pin_val.pin % UR_CONF_PIN_PER_REG) * UR_CONF_BIT_PER_PIN;
> +	mask = GENMASK(UR_CONF_BIT_PER_PIN - 1, 0) << shift;
> +
> +	raw_spin_lock_irqsave(&pctldata->lock, flags);
> +	val = readl_relaxed(reg);
> +	val = (val & ~mask) | field_prep(mask, conf);

Does this cause a build failure? The field_prep() macro strictly enforces
that its mask argument must be a compile-time constant. Because mask is
calculated dynamically based on pin_val.pin, the driver will fail to compile.

> +	writel_relaxed(val, reg);
> +	raw_spin_unlock_irqrestore(&pctldata->lock, flags);
> +
> +	return 0;
> +}
> +
> +static int ur_set_pin_mux(struct ur_pinctrl *pctldata, struct ur_pin_val *pin_val)
> +{
> +	const struct ur_port_desc *port_desc = &pctldata->match_data->ports[pin_val->port];
> +	void __iomem *reg = pctldata->base + port_desc->func_offset;
> +	unsigned long flags;
> +	u32 val;
> +
> +	raw_spin_lock_irqsave(&pctldata->lock, flags);
> +	val = readl_relaxed(reg);
> +	val &= ~((UR_FUNC0 | UR_FUNC1) << pin_val->pin);

Could this shift result in a signed integer overflow?

UR_FUNC0 is 1 and UR_FUNC1 is 0x10000, so their bitwise OR is 0x10001. If
pin_val->pin is 15, left-shifting 0x10001 by 15 results in 0x80008000.
This shifts a bit into the sign bit of a 32-bit signed integer.

Should these macros be defined with unsigned literals, such as 1U and
0x10000U?

> +	val |= pin_val->mode << pin_val->pin;
> +	writel_relaxed(val, reg);
> +	raw_spin_unlock_irqrestore(&pctldata->lock, flags);
> +
> +	return 0;
> +}
[ ... ]
> +static int ur_config_to_hw(unsigned long config, u32 *conf)
> +{
> +	enum pin_config_param param = pinconf_to_config_param(config);
> +	u32 arg = pinconf_to_config_argument(config);
> +
> +	switch (param) {
> +	case PIN_CONFIG_BIAS_DISABLE:
> +	case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
> +		FIELD_MODIFY(UR_PULL_MASK, conf, UR_PULL_DIS);
> +		return 0;
> +	case PIN_CONFIG_BIAS_PULL_UP:
> +		FIELD_MODIFY(UR_PULL_MASK, conf, UR_PULL_UP);
> +		return 0;
> +	case PIN_CONFIG_BIAS_PULL_DOWN:
> +	case PIN_CONFIG_BIAS_PULL_PIN_DEFAULT:
> +		FIELD_MODIFY(UR_PULL_MASK, conf, UR_PULL_DOWN);
> +		return 0;
> +	case PIN_CONFIG_DRIVE_STRENGTH:
> +		for (u32 i = 0; i < ARRAY_SIZE(ur_drive_strengths); i++) {
> +			if (ur_drive_strengths[i] != arg)
> +				continue;
> +			FIELD_MODIFY(UR_DRIVE_MASK, conf, i);
> +			return 0;
> +		}
> +		return -EINVAL;
> +	case PIN_CONFIG_DRIVE_PUSH_PULL:
> +	case PIN_CONFIG_INPUT_ENABLE:
> +	case PIN_CONFIG_OUTPUT_ENABLE:
> +	case PIN_CONFIG_PERSIST_STATE:
> +		return 0;

Does this code silently ignore unsupported configurations?

Returning 0 here tells the pinctrl framework that the configurations were
successfully applied, even though no hardware state was modified. Should
this return -ENOTSUPP for configurations the hardware doesn't support?

> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
[ ... ]
> +static int ur_legacy_parse_prop(struct pinctrl_dev *pctldev,
> +				struct device_node *np,
> +				const char *propname,
> +				struct ur_legacy_prop_data *prop)
> +{
> +	struct ur_pinctrl *pctldata = pinctrl_dev_get_drvdata(pctldev);
> +	int rows;
> +
> +	rows = pinctrl_count_index_with_args(np, propname);
> +	if (rows < 0)
> +		return dev_err_probe(pctldev->dev, rows, "%pOF: invalid %s count\n",
> +				     np, propname);
> +
> +	prop->pin_vals = devm_kcalloc(pctldev->dev, rows, sizeof(*prop->pin_vals),
> +				      GFP_KERNEL);
> +	if (!prop->pin_vals)
> +		return -ENOMEM;
> +
> +	prop->group_pins = devm_kcalloc(pctldev->dev, rows, sizeof(*prop->group_pins),
> +					GFP_KERNEL);

Will this leak memory when consumer devices defer probe?

The devm_kcalloc() allocations are tied to pctldev->dev, which is the pinctrl
provider device. Since ur_legacy_parse_prop() is called from dt_node_to_map
during the consumer's probe sequence, these allocations will remain on the
provider's devres list and accumulate each time the consumer defers probe.

Should standard kcalloc() be used here instead and explicitly freed at the
end of ur_legacy_dt_node_to_map()?

> +	if (!prop->group_pins)
> +		return -ENOMEM;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515-ultrarisc-pinctrl-v1-0-bf559589ea8a@ultrarisc.com?part=6