[PATCH v5 1/2] HID: input: Convert battery code to devm_*

Lucas Zampieri posted 2 patches 2 months, 2 weeks ago
There is a newer version of this series
[PATCH v5 1/2] HID: input: Convert battery code to devm_*
Posted by Lucas Zampieri 2 months, 2 weeks ago
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@redhat.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;
-}
-
-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
Re: [PATCH v5 1/2] HID: input: Convert battery code to devm_*
Posted by Benjamin Tissoires 2 months ago
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@redhat.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
>