Re: [PATCH v9 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:

New issues:
- [High] Missing regulator_disable() in error paths and on driver unbind.
- [High] Regulator devres allocation is incorrectly attached to the parent device.
- [Medium] In suspend and shutdown, the code bypasses the newly introduced `levels` abstraction and contains dead state-saving logic.

Pre-existing issues:
- [Critical] Thermal cooling device devres allocation is incorrectly attached to the parent device, leading to UAF on child unbind.
--

--- Patch [4]: [PATCH v9 4/6] thermal: khadas-mcu-fan: Add fan config from platform data Add regulator support ---
commit cdd1fb53966220cb2785c705c998afe04ac55405
Author: Ronald Claveau <linux-kernel-dev@aliel.fr>

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

This commit updates the khadas-mcu-fan driver to read fan configuration
(register and levels) from platform data instead of using hardcoded values.
The cooling device's state is stored as an index into levels[]. It also
optionally acquires and enables a "fan" regulator supply during probe and
resume.

> diff --git a/drivers/thermal/khadas_mcu_fan.c b/drivers/thermal/khadas_mcu_fan.c
> index 21b3d0a71bd0d..c7f74394b823f 100644
> --- a/drivers/thermal/khadas_mcu_fan.c
> +++ b/drivers/thermal/khadas_mcu_fan.c
> @@ -13,34 +13,30 @@
>  static int khadas_mcu_fan_set_level(struct khadas_mcu_fan_ctx *ctx,
>  				    unsigned int level)
>  {
> -	int ret;
> -
> -	ret = regmap_write(ctx->mcu->regmap, KHADAS_MCU_CMD_FAN_STATUS_CTRL_REG,
> -			   level);
> -	if (ret)
> -		return ret;
> -
> -	ctx->level = level;
> -
> -	return 0;
> +	return regmap_write(ctx->mcu->regmap, ctx->fan_reg, level);
>  }

[Severity: Medium]
Since this function no longer updates ctx->level and now takes a raw register
value instead of a state index, does this break khadas_mcu_fan_suspend() and
khadas_mcu_fan_shutdown()?

Both functions still call khadas_mcu_fan_set_level(ctx, 0), which now writes a
raw 0 to the hardware instead of ctx->levels[0]. Additionally, the state-saving
logic in suspend is now dead code since khadas_mcu_fan_set_level() no longer
mutates ctx->level.

[ ... ]

> @@ -87,7 +91,22 @@ static int khadas_mcu_fan_probe(struct platform_device *pdev)
>  	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
>  	if (!ctx)
>  		return -ENOMEM;
> +
>  	ctx->mcu = mcu;
> +	ctx->fan_reg = pdata->fan_reg;
> +	ctx->levels = pdata->levels;
> +	ctx->nlevels = pdata->nlevels;
> +
> +	ctx->power = devm_regulator_get(dev->parent, "fan");

[Severity: High]
Does devm_regulator_get() incorrectly attach the devres release action to
the parent device instead of the child device?

If the child driver is unbound or probe defers repeatedly, the memory and
regulator handle could be permanently leaked on the parent device's devres
list. Should this use dev instead of dev->parent?

> +	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;
> +	}
> +
>  	platform_set_drvdata(pdev, ctx);
>  
>  	cdev = devm_thermal_of_child_cooling_device_register(dev->parent,

[Severity: Critical]
This is a pre-existing issue, but does using dev->parent for the thermal
cooling device devres allocation mean the cooling device will outlive the
child device?

If the child driver is unbound via sysfs, ctx is freed, but the cooling
device remains active on the parent. Could reading the cooling device state
via sysfs then cause a use-after-free by dereferencing the freed ctx pointer?

>  							     dev->parent->of_node,
>  							     "khadas-mcu-fan", ctx,
>  							     &khadas_mcu_fan_cooling_ops);
>  	if (IS_ERR(cdev)) {
>  		ret = PTR_ERR(cdev);
>  		dev_err(dev, "Failed to register khadas-mcu-fan as cooling device: %d\n",
>  			ret);
>  		return ret;

[Severity: High]
If devm_thermal_of_child_cooling_device_register() fails here, does the
probe function return without disabling the manually enabled regulator?

>  	}

[ ... ]

> @@ -125,14 +144,19 @@ static int khadas_mcu_fan_suspend(struct device *dev)
>  static int khadas_mcu_fan_resume(struct device *dev)
>  {
>  	struct khadas_mcu_fan_ctx *ctx = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = regulator_enable(ctx->power);
> +	if (ret)
> +		return ret;
>  
> -	return khadas_mcu_fan_set_level(ctx, ctx->level);
> +	return khadas_mcu_fan_set_level(ctx, ctx->levels[ctx->level]);
>  }

[Severity: High]
If khadas_mcu_fan_set_level() fails here and returns an error to the PM core,
does the regulator remain permanently enabled?

Additionally, it appears the driver is missing a .remove callback in the
platform_driver definition to symmetrically disable the regulator on unbind.
Could this lead to a leak of the regulator enable count?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805-add-mcu-fan-khadas-vim4-v9-0-75df78710b9e@aliel.fr?part=4

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic