Re: [RESEND PATCH] power: supply: bq25890: Fix power_supply reference leak

[Sebastian Reichel <sre@kernel.org>]

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

Hi,

On Tue, Jun 30, 2026 at 04:33:52PM +0800, Ma Ke wrote:
> bq25890_fw_probe() acquires a reference to a secondary charger via
> power_supply_get_by_name(). This helper internally calls
> class_find_device() which gets a reference on the found device.
> However, the driver does not release this reference on error paths.
> The devm cleanup callback also does not put the reference, so normal
> driver unload leaks it as well.
> 
> Fix the leak by adding a power_supply_put() call in the new error
> label of bq25890_fw_probe() and in the devm cleanup routine.
> 
> Found by code review.
> 
> Signed-off-by: Ma Ke <make_ruc2021@163.com>
> Cc: stable@vger.kernel.org
> Fixes: d54bf877fd87 ("power: supply: bq25890: Add support for having a secondary charger IC")
> ---
>  drivers/power/supply/bq25890_charger.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
> index c1c12a447178..b50be3fca77c 100644
> --- a/drivers/power/supply/bq25890_charger.c
> +++ b/drivers/power/supply/bq25890_charger.c
> @@ -1411,7 +1411,8 @@ static int bq25890_fw_probe(struct bq25890_device *bq)
>  	if (ret == 0) {
>  		if (val > 100) {
>  			dev_err(bq->dev, "Error linux,iinlim-percentage %u > 100\n", val);
> -			return -EINVAL;
> +			ret = -EINVAL;
> +			goto err_put_charger;
>  		}
>  		bq->iinlim_percentage = val;
>  	} else {
> @@ -1426,12 +1427,19 @@ static int bq25890_fw_probe(struct bq25890_device *bq)
>  
>  	ret = bq25890_fw_read_u32_props(bq);
>  	if (ret < 0)
> -		return ret;
> +		goto err_put_charger;
>  
>  	init->ilim_en = device_property_read_bool(bq->dev, "ti,use-ilim-pin");
>  	init->boostf = device_property_read_bool(bq->dev, "ti,boost-low-freq");
>  
>  	return 0;
> +
> +err_put_charger:
> +	if (bq->secondary_chrg) {
> +		power_supply_put(bq->secondary_chrg);
> +		bq->secondary_chrg = NULL;
> +	}
> +	return ret;
>  }

The fix is incomplete. It only handles errors in bq25890_fw_probe,
but does not release if bq25890_hw_init() fails. It's a lot simpler
to register a new cleanup function:

static void bq25890_release_secondary_chrg(void *data)
{
	struct bq25890_device *bq = data;

    power_supply_put(bq->secondary_chrg);
	bq->secondary_chrg = NULL;
}

	if (ret == 0) {
		bq->secondary_chrg = power_supply_get_by_name(str);
		if (!bq->secondary_chrg)
			return -EPROBE_DEFER;
        ret = devm_add_action_or_reset(dev, bq25890_release_secondary_chrg, bq);
        if (ret)
            return ret;
	}

Greetings,

-- Sebastian

>  static void bq25890_non_devm_cleanup(void *data)
> @@ -1440,6 +1448,11 @@ static void bq25890_non_devm_cleanup(void *data)
>  
>  	cancel_delayed_work_sync(&bq->pump_express_work);
>  
> +	if (bq->secondary_chrg) {
> +		power_supply_put(bq->secondary_chrg);
> +		bq->secondary_chrg = NULL;
> +	}
> +
>  	if (bq->id >= 0) {
>  		mutex_lock(&bq25890_id_mutex);
>  		idr_remove(&bq25890_id, bq->id);
> -- 
> 2.43.0
> 

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