Re: [PATCH 3/4] power: supply: bq24190: Avoid rescheduling after cancelling work

[Hans de Goede <hansg@kernel.org>]

Hi,

On 20-Feb-26 18:49, Krzysztof Kozlowski wrote:
> Driver initializes delayed work and then registers interrupt handler
> with devm interface.  This means that device removal will not use a
> reversed order, but first cancel pending work items and then, via devm
> release handlers, free the interrupt.
> 
> The interrupt handler does not directly use/schedule work
> items on the workqueue, however it updates the status of the battery
> charger which might lead to calling power_supply_changed() and trigger
> chain of calls leading to scheduling the work items.  If this happens
> during short time window after cancel_delayed_work_sync() in remove()
> callback, the work would be rescheduled.
> 
> Avoid this by using devm interface to initialize and cancel work item,
> thus having exactly reverse order during remove() in respect to rest of
> the probe/cleanup paths.  This is also more logical and readable code.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>

Regards,

Hans



> ---
>  drivers/power/supply/bq24190_charger.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index ed0ceae8d90b..55da91bacc3e 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -9,6 +9,7 @@
>  #include <linux/module.h>
>  #include <linux/interrupt.h>
>  #include <linux/delay.h>
> +#include <linux/devm-helpers.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/power_supply.h>
>  #include <linux/power/bq24190_charger.h>
> @@ -2087,8 +2088,11 @@ static int bq24190_probe(struct i2c_client *client)
>  	bdi->charge_type = POWER_SUPPLY_CHARGE_TYPE_FAST;
>  	bdi->f_reg = 0;
>  	bdi->ss_reg = BQ24190_REG_SS_VBUS_STAT_MASK; /* impossible state */
> -	INIT_DELAYED_WORK(&bdi->input_current_limit_work,
> -			  bq24190_input_current_limit_work);
> +
> +	ret = devm_delayed_work_autocancel(dev, &bdi->input_current_limit_work,
> +					   bq24190_input_current_limit_work);
> +	if (ret)
> +		return ret;
>  
>  	i2c_set_clientdata(client, bdi);
>  
> @@ -2198,7 +2202,6 @@ static void bq24190_remove(struct i2c_client *client)
>  	struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
>  	int error;
>  
> -	cancel_delayed_work_sync(&bdi->input_current_limit_work);
>  	error = pm_runtime_resume_and_get(bdi->dev);
>  	if (error < 0)
>  		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);