Re: [PATCH 09/20] reset: add Synaptics SL261x reset support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] reset: sl261x: potential NULL pointer dereference in probe
- [Medium] reset: sl261x: unsafe read-modify-write on non-sticky pulse triggers
- [Low] reset: sl261x: typo 'sl261x2' in driver name and probe function
--

commit 78616d8a7b66b0ac665b311775dd566f2469412a
Author: Jisheng Zhang <jszhang@kernel.org>

reset: add Synaptics SL261x reset support

This commit adds support for the reset controller in Synaptics SL261X SoCs.

> diff --git a/drivers/reset/reset-sl261x.c b/drivers/reset/reset-sl261x.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/reset/reset-sl261x.c

[ ... ]

> +static int sl261x_reset_reset(struct reset_controller_dev *rcdev,
> +			      unsigned long id)
> +{
> +	struct sl261x_reset_priv *priv = to_sl261x_reset_priv(rcdev);
> +	const struct sl261x_reset_cfg *cfg = &priv->cfgs[id];
> +
> +	if (cfg->sticky) {
> +		regmap_clear_bits(priv->regmap, cfg->reg, cfg->bit);
> +		usleep_range(10, 15);
> +		return regmap_set_bits(priv->regmap, cfg->reg, cfg->bit);
> +	}
> +
> +	return regmap_set_bits(priv->regmap, cfg->reg, cfg->bit);

[Severity: Medium]
Could this read-modify-write operation disrupt concurrent resets? 

If the non-sticky hardware registers are active-high self-clearing pulse
triggers that read as 1 while active, the regmap_set_bits() operation
might read 1s for concurrently active resets and write them back,
unintentionally extending or re-triggering them.

[ ... ]

> +static const struct reset_control_ops sl261x_reset_ops = {
> +	.reset	= sl261x_reset_reset,
> +	.assert = sl261x_reset_assert,
> +	.deassert = sl261x_reset_deassert,
> +};
> +
> +static int sl261x2_reset_probe(struct platform_device *pdev)

[Severity: Low]
Is the "sl261x2" in the probe function name intentional? It looks like a
typo.

> +{
> +	struct device_node *parent_np;
> +	struct sl261x_reset_priv *priv;
> +	const struct sl261x_reset_data *data = device_get_match_data(&pdev->dev);
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	parent_np = of_get_parent(pdev->dev.of_node);
> +	priv->regmap = syscon_node_to_regmap(parent_np);
> +	of_node_put(parent_np);
> +	if (IS_ERR(priv->regmap))
> +		return PTR_ERR(priv->regmap);
> +
> +	priv->rcdev.owner = THIS_MODULE;
> +	priv->rcdev.ops = &sl261x_reset_ops;
> +	priv->rcdev.of_node = pdev->dev.of_node;
> +	priv->rcdev.of_reset_n_cells = 1;
> +	priv->rcdev.nr_resets = data->num;

[Severity: Medium]
Can data be NULL here? 

If the driver is bound via sysfs to a device that has a valid syscon parent
but does not match the OF compatible strings, device_get_match_data() will
return NULL. Unconditionally dereferencing data->num in this case would
cause a NULL pointer dereference.

> +	priv->cfgs = data->cfgs;
> +
> +	return devm_reset_controller_register(&pdev->dev, &priv->rcdev);
> +}
> +
> +static const struct of_device_id sl261x_reset_dt_match[] = {
> +	{ .compatible = "syna,sl261x-reset", .data = &sl261x_reset_data },
> +	{ .compatible = "syna,sl261x-system-reset", .data = &sl261x_system_reset_data },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, sl261x_reset_dt_match);
> +
> +static struct platform_driver sl261x_reset_driver = {
> +	.probe	= sl261x2_reset_probe,
> +	.driver	= {
> +		.name = "sl261x2-reset",

[Severity: Low]
Does this driver name intentionally use "sl261x2-reset"? Exposing a confusing
name to the platform bus and sysfs might break userspace scripts expecting a
consistent "sl261x-reset" name.

> +		.of_match_table = sl261x_reset_dt_match,
> +	},
> +};

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