Re: [PATCH] hwmon: (aspeed-g6-pwm-tach): fix driver data handling in probe/remove

[Guenter Roeck <linux@roeck-us.net>]

On Fri, Mar 06, 2026 at 02:44:31PM +0800, Billy Tsai wrote:
> Ensure proper association of driver data by setting and retrieving
> the platform device's driver data during probe and remove.
> 
> Fixes: 7e1449cd15d1 ("hwmon: (aspeed-g6-pwm-tacho): Support for ASPEED g6 PWM/Fan tach")
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> ---
>  drivers/hwmon/aspeed-g6-pwm-tach.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/aspeed-g6-pwm-tach.c b/drivers/hwmon/aspeed-g6-pwm-tach.c
> index 44e1ecba205d..0c03d272279a 100644
> --- a/drivers/hwmon/aspeed-g6-pwm-tach.c
> +++ b/drivers/hwmon/aspeed-g6-pwm-tach.c
> @@ -490,6 +490,7 @@ static int aspeed_pwm_tach_probe(struct platform_device *pdev)
>  	if (IS_ERR(chip))
>  		return PTR_ERR(chip);
>  
> +	platform_set_drvdata(pdev, chip);
>  	pwmchip_set_drvdata(chip, priv);
>  	chip->ops = &aspeed_pwm_ops;
>  
> @@ -519,7 +520,8 @@ static int aspeed_pwm_tach_probe(struct platform_device *pdev)
>  
>  static void aspeed_pwm_tach_remove(struct platform_device *pdev)
>  {
> -	struct aspeed_pwm_tach_data *priv = platform_get_drvdata(pdev);
> +	struct pwm_chip *chip = platform_get_drvdata(pdev);
> +	struct aspeed_pwm_tach_data *priv = aspeed_pwm_chip_to_data(chip);
>  
>  	reset_control_assert(priv->reset);

AI review has the same (and more) concerns. Here is what it has to say:

Does asserting the reset here place the hardware into reset while the sysfs
interfaces are still active?

Since probe uses devm_pwmchip_add() and
devm_hwmon_device_register_with_info(), those devices are unregistered
during devres cleanup, which happens after this remove function returns. If
userspace accesses the devices before devres cleanup finishes, it could
access an IP block that is in reset and cause a system hang.

Also, since probe already registers a devres action
(devm_add_action_or_reset()) to call aspeed_pwm_tach_reset_assert(),
this explicit assert appears to be redundant. Can the remove function be
dropped entirely?

This isn't a bug, but probe calls of_platform_populate() without a
corresponding of_platform_depopulate(). Using
devm_of_platform_populate() in probe would fix the leak and allow
deleting this remove function.

I think all those points are valid.

Thanks,
Guenter