Re: [PATCH] power: supply: rt9455: quiesce delayed work before teardown

[Sebastian Reichel <sebastian.reichel@collabora.com>]

[-- Attachment #1: Type: text/plain, Size: 4597 bytes --]

Hi,

On Thu, Jul 23, 2026 at 02:53:52PM +0000, Fan Wu wrote:
> The threaded IRQ handler can queue pwr_rdy_work,
> max_charging_time_work and batt_presence_work.  pwr_rdy_work and
> batt_presence_work can also queue max_charging_time_work, while
> batt_presence_work can requeue itself.
> 
> rt9455_remove() cancels max_charging_time_work before
> batt_presence_work.  The latter can therefore queue
> max_charging_time_work after it has already been cancelled:
> 
>   rt9455_remove()                   workqueue
>     cancel pwr_rdy_work
>     cancel max_charging_time_work
>                                       batt_presence_work queues
>                                         max_charging_time_work
>     cancel batt_presence_work
>     return
>     devres frees rt9455_info
>                                       max_charging_time_work dereferences
>                                         rt9455_info
> 
> The IRQ also remains registered until devres cleanup and can queue more
> work after any of the cancellation calls.  If rt9455_hw_init() fails
> after the IRQ has been requested, probe returns without cancelling work
> that may already have been queued.  A pending callback can then access
> rt9455_info after it has been freed.
> 
> Explicitly free the managed IRQ before cancelling the delayed works in
> both paths.  This waits for the threaded handler and prevents it from
> queuing more work.  Cancel pwr_rdy_work and batt_presence_work before
> max_charging_time_work because both can queue the latter.
> 
> This issue was found by an in-house static analysis tool.
> 
> Fixes: e86d69dd786e ("power_supply: Add support for Richtek RT9455 battery charger")
> Cc: stable@vger.kernel.org
> Assisted-by: Codex:gpt-5.6
> Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
> ---
>  drivers/power/supply/rt9455_charger.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/power/supply/rt9455_charger.c b/drivers/power/supply/rt9455_charger.c
> index 7045d2908148..3c64591b5934 100644
> --- a/drivers/power/supply/rt9455_charger.c
> +++ b/drivers/power/supply/rt9455_charger.c
> @@ -1582,6 +1582,17 @@ static const struct regmap_config rt9455_regmap_config = {
>  	.cache_type	= REGCACHE_MAPLE,
>  };
>  
> +static void rt9455_cancel_all_delayed_works(struct rt9455_info *info)
> +{
> +	/*
> +	 * Both pwr_rdy_work and batt_presence_work can queue
> +	 * max_charging_time_work, so cancel them first.
> +	 */
> +	cancel_delayed_work_sync(&info->pwr_rdy_work);
> +	cancel_delayed_work_sync(&info->batt_presence_work);
> +	cancel_delayed_work_sync(&info->max_charging_time_work);
> +}
> +
>  static int rt9455_probe(struct i2c_client *client)
>  {
>  	struct i2c_adapter *adapter = client->adapter;
> @@ -1684,11 +1695,15 @@ static int rt9455_probe(struct i2c_client *client)
>  	ret = rt9455_hw_init(info, ichrg, ieoc_percentage, mivr, iaicr);
>  	if (ret) {
>  		dev_err(dev, "Failed to set charger to its default values\n");
> -		goto put_usb_notifier;
> +		goto free_irq_and_drain;
>  	}
>  
>  	return 0;
>  
> +free_irq_and_drain:
> +	/* Stop new work before draining work queued during probe. */
> +	devm_free_irq(dev, client->irq, info);
> +	rt9455_cancel_all_delayed_works(info);

Manually freeing device managed resources is ugly. Register the
rt9455_cancel_all_delayed_works() function via devm_add_action_or_reset()
after the devm_power_supply_register() call in the probe function
instead.

Greetings,

-- Sebastian

>  put_usb_notifier:
>  #if IS_ENABLED(CONFIG_USB_PHY)
>  	if (info->nb.notifier_call)  {
> @@ -1704,6 +1719,10 @@ static void rt9455_remove(struct i2c_client *client)
>  	int ret;
>  	struct rt9455_info *info = i2c_get_clientdata(client);
>  
> +	/* Stop the IRQ handler from queuing work during teardown. */
> +	devm_free_irq(&client->dev, client->irq, info);
> +	rt9455_cancel_all_delayed_works(info);
> +
>  	ret = rt9455_register_reset(info);
>  	if (ret)
>  		dev_err(&info->client->dev, "Failed to set charger to its default values\n");
> @@ -1712,10 +1731,6 @@ static void rt9455_remove(struct i2c_client *client)
>  	if (info->nb.notifier_call)
>  		usb_unregister_notifier(info->usb_phy, &info->nb);
>  #endif
> -
> -	cancel_delayed_work_sync(&info->pwr_rdy_work);
> -	cancel_delayed_work_sync(&info->max_charging_time_work);
> -	cancel_delayed_work_sync(&info->batt_presence_work);
>  }
>  
>  static const struct i2c_device_id rt9455_i2c_id_table[] = {
> -- 
> 2.34.1
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]