Re: [PATCH v9 6/6] arm64: dts: amlogic: t7: khadas-vim4: Add i2c MCU fan node

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The khadas-mcu-fan driver leaks regulator references and enable counts due to binding devres to the parent device and missing cleanup paths.
--

--- Patch [6]: [PATCH v9 6/6] arm64: dts: amlogic: t7: khadas-vim4: Add i2c MCU fan node ---
commit 69aa0768107f9fa2d0c3f9c0b9a761cb13f3890b
Author: Ronald Claveau <[email protected]>

arm64: dts: amlogic: t7: khadas-vim4: Add i2c MCU fan node
    
Enable and configure i2c MCU node to get fan working on Khadas VIM4.
    
Reviewed-by: Neil Armstrong <[email protected]>
Reviewed-by: Martin Blumenstingl <[email protected]>
Signed-off-by: Ronald Claveau <[email protected]>

> diff --git a/arch/arm64/boot/dts/amlogic/amlogic-t7-a311d2-khadas-vim4.dts b/arch/arm64/boot/dts/amlogic/amlogic-t7-a311d2-khadas-vim4.dts
> index c41525a34b721..7d95aadcf6a66 100644
> --- a/arch/arm64/boot/dts/amlogic/amlogic-t7-a311d2-khadas-vim4.dts
> +++ b/arch/arm64/boot/dts/amlogic/amlogic-t7-a311d2-khadas-vim4.dts
> @@ -167,6 +167,19 @@ xtal: xtal-clk {
>  	};
>  };
>  
> +&i2c_m_ao_a {
> +	status = "okay";
> +	pinctrl-0 = <&i2c0_ao_d_pins>;
> +	pinctrl-names = "default";
> +
> +	khadas_mcu: system-controller@18 {
> +		compatible = "khadas,vim4-mcu";
> +		reg = <0x18>;
> +		fan-supply = <&vcc5v>;

[Severity: High]
Does enabling this fan node expose a regulator leak in the khadas-mcu-fan
driver?

When the driver probes, it appears to attach the regulator resource
lifecycle to the parent device rather than the child:

drivers/thermal/khadas_mcu_fan.c:khadas_mcu_fan_probe() {
    ...
    ctx->power = devm_regulator_get(dev->parent, "fan");
    ...
}

If devm_thermal_of_child_cooling_device_register() fails, the probe
function returns without calling regulator_disable():

drivers/thermal/khadas_mcu_fan.c:khadas_mcu_fan_probe() {
    ...
    return ret;
}

Furthermore, the driver struct lacks a .remove callback entirely:

drivers/thermal/khadas_mcu_fan.c:
static struct platform_driver khadas_mcu_fan_driver = {
    .probe      = khadas_mcu_fan_probe,
    .shutdown   = khadas_mcu_fan_shutdown,
    .driver = {
        .name       = "khadas-mcu-fan-ctrl",
        .pm         = &khadas_mcu_fan_pm,
    },
    .id_table   = khadas_mcu_fan_id_table,
};

Since devres is attached to dev->parent, the driver core will not
automatically clean up the regulator when the child device probe fails or
when the child driver is unbound. 

Will this cause a monotonically increasing enable count and memory leak
on the parent device during unbind/rebind cycles?

> +		#cooling-cells = <2>;
> +	};
> +};

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic