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

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

[Billy Tsai]

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);
 }

---
base-commit: af4e9ef3d78420feb8fe58cd9a1ab80c501b3c08
change-id: 20260306-pwm_fixes-7982ad548f07

Best regards,
-- 
Billy Tsai <billy_tsai@aspeedtech.com>

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

[Guenter Roeck]

On 3/5/26 22:44, 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);
>   }
> 

The probe function registers aspeed_pwm_tach_reset_assert() with
devm_add_action_or_reset(). Why is the remove function needed in
the first place ? It seems redundant to me, and even buggy since
it asserts reset before its users (pwm and hwmon) are disabled/removed.

Thanks,
Guenter

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

[Guenter Roeck]

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

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

[Billy Tsai]

Hi Guenter,

You’re right—the probe path already registers a devres action (devm_add_action_or_reset()) to call aspeed_pwm_tach_reset_assert(). Therefore, there is no need to perform this again. We can simply remove the remove callback, since all resources are already managed by the devm_* APIs.

Thanks