Re: [PATCH v10 4/6] thermal: khadas-mcu-fan: Add fan config from platform data Add regulator support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Direct assignment of `dev->of_node` without `of_node_get()` causes a reference count underflow and use-after-free on device removal.
- [Medium] State desynchronization on `khadas_mcu_fan_suspend` error path leaves the fan permanently disabled.
- [Medium] Unchecked platform data pointer dereference causes a potential kernel panic.
- [Medium] Rolling back `regulator_enable` in the `khadas_mcu_fan_resume` error path causes a regulator enable count underflow on driver unbind.
--

commit 48a15d3c23acef32291aa52062459e22677349e6
Author: Ronald Claveau <linux-kernel-dev@aliel.fr>

thermal: khadas-mcu-fan: Add fan config from platform data Add regulator support

This commit reads the fan register and level table from platform data instead
of hardcoding them. It also introduces regulator support for the fan's power
supply during probe, resume, and suspend.

> diff --git a/drivers/thermal/khadas_mcu_fan.c b/drivers/thermal/khadas_mcu_fan.c
> index 21b3d0a71bd0d..512a6f0ff7c26 100644
> --- a/drivers/thermal/khadas_mcu_fan.c
> +++ b/drivers/thermal/khadas_mcu_fan.c

[ ... ]

> @@ -76,22 +82,53 @@ static const struct thermal_cooling_device_ops khadas_mcu_fan_cooling_ops = {
>  	.set_cur_state = khadas_mcu_fan_set_cur_state,
>  };
>  
> +static void khadas_mcu_fan_regulator_disable(void *data)
> +{
> +	struct regulator *power = data;
> +
> +	regulator_disable(power);
> +}
> +
>  static int khadas_mcu_fan_probe(struct platform_device *pdev)
>  {
> +	const struct khadas_mcu_fan_pdata *pdata = dev_get_platdata(&pdev->dev);
>  	struct khadas_mcu *mcu = dev_get_drvdata(pdev->dev.parent);
>  	struct thermal_cooling_device *cdev;
>  	struct device *dev = &pdev->dev;
>  	struct khadas_mcu_fan_ctx *ctx;
>  	int ret;
>  
> +
> +	if (!dev->of_node)
> +		dev->of_node = dev->parent->of_node;

[Severity: High]
Does this code cause a reference count underflow and use-after-free on device
removal?  By assigning dev->of_node without incrementing the reference count
via of_node_get(), the driver core's unconditional of_node_put(dev->of_node)
on unbind could underflow the parent's reference count.

> +
>  	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
>  	if (!ctx)
>  		return -ENOMEM;
> +
>  	ctx->mcu = mcu;
> +	ctx->fan_reg = pdata->fan_reg;

[Severity: Medium]
Could this code cause a NULL pointer dereference?  If the device is manually
instantiated (e.g. via sysfs or a DT overlay) without providing the required
platform data, dev_get_platdata() returns NULL, which is then unconditionally
dereferenced here.

> +	ctx->levels = pdata->levels;
> +	ctx->nlevels = pdata->nlevels;
> +
> +	ctx->power = devm_regulator_get(dev, "fan");
> +	if (IS_ERR(ctx->power))
> +		return PTR_ERR(ctx->power);
> +
> +	ret = regulator_enable(ctx->power);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable fan power supply: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = devm_add_action_or_reset(dev, khadas_mcu_fan_regulator_disable, ctx->power);
> +	if (ret)
> +		return ret;
> +
>  	platform_set_drvdata(pdev, ctx);

[ ... ]

>  #ifdef CONFIG_PM_SLEEP
>  static int khadas_mcu_fan_suspend(struct device *dev)
>  {
>  	struct khadas_mcu_fan_ctx *ctx = dev_get_drvdata(dev);
> -	unsigned int level_save = ctx->level;
>  	int ret;
>  
> -	ret = khadas_mcu_fan_set_level(ctx, 0);
> +	ret = khadas_mcu_fan_set_level(ctx, ctx->levels[0]);
>  	if (ret)
>  		return ret;
>  
> -	ctx->level = level_save;
> -
> -	return 0;
> +	return regulator_disable(ctx->power);

[Severity: Medium]
If regulator_disable() fails here, the PM core aborts system suspend, and the
resume callback will not be executed.  Because the fan was successfully turned
off via khadas_mcu_fan_set_level(), but ctx->level was not updated to reflect
this, will the thermal framework later fail to restore the correct fan state
due to evaluating state == ctx->level in khadas_mcu_fan_set_cur_state()?

>  }
>  
>  static int khadas_mcu_fan_resume(struct device *dev)
>  {
>  	struct khadas_mcu_fan_ctx *ctx = dev_get_drvdata(dev);
> +	int ret, ret2;
> +
> +	ret = regulator_enable(ctx->power);
> +	if (ret)
> +		return ret;
>  
> -	return khadas_mcu_fan_set_level(ctx, ctx->level);
> +	ret = khadas_mcu_fan_set_level(ctx, ctx->levels[ctx->level]);
> +	if (ret) {
> +		ret2 = regulator_disable(ctx->power);

[Severity: Medium]
Does rolling back the regulator state here cause an enable count underflow
on driver unbind?

Because khadas_mcu_fan_probe() registered an unconditional cleanup action
via devm_add_action_or_reset(dev, khadas_mcu_fan_regulator_disable, ctx->power),
calling regulator_disable() here leaves the regulator disabled while the device
remains bound.  When the device is later unbound, the devm action will call
regulator_disable() again on the already disabled regulator.

> +		if (ret2)
> +			dev_err(dev, "failed to disable regulator after error %d: %d\n", ret,
> +				ret2);
> +		return ret;
> +	}
> +
> +	return 0;
>  }
>  #endif

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901-add-mcu-fan-khadas-vim4-v10-0-aecdb9955a30@aliel.fr?part=4