linux-kernel - Re: [PATCH v5 1/2] HID: input: Convert battery code to devm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <hn2vabg7wvyqzdvp4erwuhfsyahsx35l6ehmo5fmklfnbybqgo@on5mev5x7svd>
Date: Tue, 9 Dec 2025 11:32:37 +0100
From: Benjamin Tissoires <bentiss@...nel.org>
To: Lucas Zampieri <lzampier@...hat.com>
Cc: linux-input@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Jiri Kosina <jikos@...nel.org>, Sebastian Reichel <sre@...nel.org>, 
	Bastien Nocera <hadess@...ess.net>, linux-pm@...r.kernel.org
Subject: Re: [PATCH v5 1/2] HID: input: Convert battery code to devm_*

On Nov 21 2025, Lucas Zampieri wrote:
> Convert the HID battery code to use devm_* managed resource APIs.
> 
> This changes the following allocations:
> - kzalloc() -> devm_kzalloc() for power_supply_desc
> - kasprintf() -> devm_kasprintf() for battery name
> - power_supply_register() -> devm_power_supply_register()
> 
> No functional behavior changes.
> 
> Signed-off-by: Lucas Zampieri <lzampier@...hat.com>
> ---
>  drivers/hid/hid-input.c | 49 +++++++++--------------------------------
>  1 file changed, 10 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index e56e7de53279..5f313c3c35e2 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -530,17 +530,15 @@ static int hidinput_setup_battery(struct hid_device *dev, unsigned report_type,
>  	if (quirks & HID_BATTERY_QUIRK_IGNORE)
>  		return 0;
>  
> -	psy_desc = kzalloc(sizeof(*psy_desc), GFP_KERNEL);
> +	psy_desc = devm_kzalloc(&dev->dev, sizeof(*psy_desc), GFP_KERNEL);
>  	if (!psy_desc)
>  		return -ENOMEM;
>  
> -	psy_desc->name = kasprintf(GFP_KERNEL, "hid-%s-battery",
> -				   strlen(dev->uniq) ?
> -					dev->uniq : dev_name(&dev->dev));
> -	if (!psy_desc->name) {
> -		error = -ENOMEM;
> -		goto err_free_mem;
> -	}
> +	psy_desc->name = devm_kasprintf(&dev->dev, GFP_KERNEL, "hid-%s-battery",
> +					strlen(dev->uniq) ?
> +						dev->uniq : dev_name(&dev->dev));
> +	if (!psy_desc->name)
> +		return -ENOMEM;
>  
>  	psy_desc->type = POWER_SUPPLY_TYPE_BATTERY;
>  	psy_desc->properties = hidinput_battery_props;
> @@ -576,36 +574,15 @@ static int hidinput_setup_battery(struct hid_device *dev, unsigned report_type,
>  	if (quirks & HID_BATTERY_QUIRK_AVOID_QUERY)
>  		dev->battery_avoid_query = true;
>  
> -	dev->battery = power_supply_register(&dev->dev, psy_desc, &psy_cfg);
> +	dev->battery = devm_power_supply_register(&dev->dev, psy_desc, &psy_cfg);
>  	if (IS_ERR(dev->battery)) {
> -		error = PTR_ERR(dev->battery);
> -		hid_warn(dev, "can't register power supply: %d\n", error);
> -		goto err_free_name;
> +		hid_warn(dev, "can't register power supply: %ld\n",
> +			 PTR_ERR(dev->battery));
> +		return PTR_ERR(dev->battery);
>  	}
>  
>  	power_supply_powers(dev->battery, &dev->dev);
>  	return 0;
> -
> -err_free_name:
> -	kfree(psy_desc->name);
> -err_free_mem:
> -	kfree(psy_desc);
> -	dev->battery = NULL;
> -	return error;

As mentioned in my other reply (and this is more of an open question):
what if there is a failure in devm_power_supply_register? Everything
will be allocated and kept until the end of life of the HID device, but
we'll try to recreate the battery for every matching field in the HID
device, meaning we are waiting a lot of space for nothing.

The previous appraoch was cleaning things up, so we were trying a lot,
but at least we were not keeping the memory allocated.

I think we should keep the error path for devm_power_supply_register()
and dealloc (with devm_kfree) the few memories we've done and also clear
dev->battery.  This way we keep the current path and can make use of
devm and get the benefits of removing the hidinput_cleanup_battery()
function.

Cheers,
Benjamin

> -}
> -
> -static void hidinput_cleanup_battery(struct hid_device *dev)
> -{
> -	const struct power_supply_desc *psy_desc;
> -
> -	if (!dev->battery)
> -		return;
> -
> -	psy_desc = dev->battery->desc;
> -	power_supply_unregister(dev->battery);
> -	kfree(psy_desc->name);
> -	kfree(psy_desc);
> -	dev->battery = NULL;
>  }
>  
>  static bool hidinput_update_battery_charge_status(struct hid_device *dev,
> @@ -660,10 +637,6 @@ static int hidinput_setup_battery(struct hid_device *dev, unsigned report_type,
>  	return 0;
>  }
>  
> -static void hidinput_cleanup_battery(struct hid_device *dev)
> -{
> -}
> -
>  static void hidinput_update_battery(struct hid_device *dev, unsigned int usage,
>  				    int value)
>  {
> @@ -2379,8 +2352,6 @@ void hidinput_disconnect(struct hid_device *hid)
>  {
>  	struct hid_input *hidinput, *next;
>  
> -	hidinput_cleanup_battery(hid);
> -
>  	list_for_each_entry_safe(hidinput, next, &hid->inputs, list) {
>  		list_del(&hidinput->list);
>  		if (hidinput->registered)
> -- 
> 2.51.1
>