[PATCH] power_supply: Adjust devm usage

[PATCH] power_supply: Adjust devm usage

[Vaishali Thakkar]

Use devm_kasprintf instead of kasprintf. Also, remove various
gotos by direct returns and drop unneeded label err_free_name.

Signed-off-by: Vaishali Thakkar <vthakkar1994@gmail.com>
---
 drivers/power/bq24735-charger.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/drivers/power/bq24735-charger.c b/drivers/power/bq24735-charger.c
index b017437..b2bb67e 100644
--- a/drivers/power/bq24735-charger.c
+++ b/drivers/power/bq24735-charger.c
@@ -267,8 +267,9 @@ static int bq24735_charger_probe(struct i2c_client *client,
 
 	name = (char *)charger->pdata->name;
 	if (!name) {
-		name = kasprintf(GFP_KERNEL, "bq24735@%s",
-				 dev_name(&client->dev));
+		name = devm_kasprintf(&client->dev, GFP_KERNEL,
+				      "bq24735@%s",
+				      dev_name(&client->dev));
 		if (!name) {
 			dev_err(&client->dev, "Failed to alloc device name\n");
 			return -ENOMEM;
@@ -296,23 +297,21 @@ static int bq24735_charger_probe(struct i2c_client *client,
 	if (ret < 0) {
 		dev_err(&client->dev, "Failed to read manufacturer id : %d\n",
 			ret);
-		goto err_free_name;
+		return ret;
 	} else if (ret != 0x0040) {
 		dev_err(&client->dev,
 			"manufacturer id mismatch. 0x0040 != 0x%04x\n", ret);
-		ret = -ENODEV;
-		goto err_free_name;
+		return -ENODEV;
 	}
 
 	ret = bq24735_read_word(client, BQ24735_DEVICE_ID);
 	if (ret < 0) {
 		dev_err(&client->dev, "Failed to read device id : %d\n", ret);
-		goto err_free_name;
+		return ret;
 	} else if (ret != 0x000B) {
 		dev_err(&client->dev,
 			"device id mismatch. 0x000b != 0x%04x\n", ret);
-		ret = -ENODEV;
-		goto err_free_name;
+		return -ENODEV;
 	}
 
 	if (gpio_is_valid(charger->pdata->status_gpio)) {
@@ -331,7 +330,7 @@ static int bq24735_charger_probe(struct i2c_client *client,
 	ret = bq24735_config_charger(charger);
 	if (ret < 0) {
 		dev_err(&client->dev, "failed in configuring charger");
-		goto err_free_name;
+		return ret;
 	}
 
 	/* check for AC adapter presence */
@@ -339,7 +338,7 @@ static int bq24735_charger_probe(struct i2c_client *client,
 		ret = bq24735_enable_charging(charger);
 		if (ret < 0) {
 			dev_err(&client->dev, "Failed to enable charging\n");
-			goto err_free_name;
+			return ret;
 		}
 	}
 
@@ -349,7 +348,7 @@ static int bq24735_charger_probe(struct i2c_client *client,
 		ret = PTR_ERR(charger->charger);
 		dev_err(&client->dev, "Failed to register power supply: %d\n",
 			ret);
-		goto err_free_name;
+		return ret;
 	}
 
 	if (client->irq) {
@@ -371,10 +370,6 @@ static int bq24735_charger_probe(struct i2c_client *client,
 	return 0;
 err_unregister_supply:
 	power_supply_unregister(charger->charger);
-err_free_name:
-	if (name != charger->pdata->name)
-		kfree(name);
-
 	return ret;
 }
 
-- 
1.9.1

Re: [PATCH] power_supply: Adjust devm usage

[Frans Klaver]

Hi,

On Fri, Jul 24, 2015 at 1:58 PM, Vaishali Thakkar
<vthakkar1994@gmail.com> wrote:
> Use devm_kasprintf instead of kasprintf. Also, remove various
> gotos by direct returns and drop unneeded label err_free_name.

If there's to be a respin, reword this so that it becomes clearer that
removing the various gotos and the label is an effect of using
devm_kasprintf here. I started out thinking that this patch did two
things.

Frans

Re: [PATCH] power_supply: Adjust devm usage

[Krzysztof Kozlowski]

2015-07-24 21:17 GMT+09:00 Frans Klaver <fransklaver@gmail.com>:
> Hi,
>
> On Fri, Jul 24, 2015 at 1:58 PM, Vaishali Thakkar
> <vthakkar1994@gmail.com> wrote:
>> Use devm_kasprintf instead of kasprintf. Also, remove various
>> gotos by direct returns and drop unneeded label err_free_name.
>
> If there's to be a respin, reword this so that it becomes clearer that
> removing the various gotos and the label is an effect of using
> devm_kasprintf here. I started out thinking that this patch did two
> things.

And please put a driver prefix in the subject (like in rest of
commits). Beside of that:
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof

Re: [PATCH] power_supply: Adjust devm usage

[Vaishali Thakkar]

On Fri, Jul 24, 2015 at 5:59 PM, Krzysztof Kozlowski
<k.kozlowski@samsung.com> wrote:
> 2015-07-24 21:17 GMT+09:00 Frans Klaver <fransklaver@gmail.com>:
>> Hi,

Hi

>> On Fri, Jul 24, 2015 at 1:58 PM, Vaishali Thakkar
>> <vthakkar1994@gmail.com> wrote:
>>> Use devm_kasprintf instead of kasprintf. Also, remove various
>>> gotos by direct returns and drop unneeded label err_free_name.
>>
>> If there's to be a respin, reword this so that it becomes clearer that
>> removing the various gotos and the label is an effect of using
>> devm_kasprintf here. I started out thinking that this patch did two
>> things.

Ok. Sure. I will change the commit log.

> And please put a driver prefix in the subject (like in rest of
> commits). Beside of that:
> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Ok.

> Best regards,
> Krzysztof



-- 
Vaishali

Re: [PATCH] power_supply: Adjust devm usage

[Sebastian Reichel]

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

Hi,

Thanks for the cleanup patch.
I have a couple of comments inlined.

> Subject: Re: [PATCH] power_supply: Adjust devm usage

Please make this "power_supply: bq24735: ...".

On Fri, Jul 24, 2015 at 05:28:13PM +0530, Vaishali Thakkar wrote:
> Use devm_kasprintf instead of kasprintf. Also, remove various
> gotos by direct returns and drop unneeded label err_free_name.

Please also use devm_power_supply_unregister() instead
of power_supply_unregister() to further simplify the driver.

> @@ -267,8 +267,9 @@ static int bq24735_charger_probe() {}
> [...]

Your patch is missing removal of the
kfree(charger->charger_desc.name) in bq24735_charger_remove().

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] power_supply: Adjust devm usage

[Vaishali Thakkar]

On Fri, Jul 24, 2015 at 5:56 PM, Sebastian Reichel <sre@kernel.org> wrote:
> Hi,

Hi

> Thanks for the cleanup patch.
> I have a couple of comments inlined.
>
>> Subject: Re: [PATCH] power_supply: Adjust devm usage
>
> Please make this "power_supply: bq24735: ...".

Ok. Sure.

> On Fri, Jul 24, 2015 at 05:28:13PM +0530, Vaishali Thakkar wrote:
>> Use devm_kasprintf instead of kasprintf. Also, remove various
>> gotos by direct returns and drop unneeded label err_free_name.
>
> Please also use devm_power_supply_unregister() instead
> of power_supply_unregister() to further simplify the driver.

Ok.

>> @@ -267,8 +267,9 @@ static int bq24735_charger_probe() {}
>> [...]
>
> Your patch is missing removal of the
> kfree(charger->charger_desc.name) in bq24735_charger_remove().

Yes. Because it seems that this kfree is freeing some other data which
is not related to devm_kzalloc. I was not sure about removing it.
So, I was about to discuss it in a separate thread. Also, in the remove function
we have devm_free_irq. I am unsure it too. Because normally remove functions
do not use devm counterparts.

> -- Sebastian



-- 
Vaishali

Re: [PATCH] power_supply: Adjust devm usage

[Sebastian Reichel]

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

Hi,

On Fri, Jul 24, 2015 at 06:03:38PM +0530, Vaishali Thakkar wrote:
> On Fri, Jul 24, 2015 at 5:56 PM, Sebastian Reichel <sre@kernel.org> wrote:
> > On Fri, Jul 24, 2015 at 05:28:13PM +0530, Vaishali Thakkar wrote:
> >> @@ -267,8 +267,9 @@ static int bq24735_charger_probe() {}
> >> [...]
> >
> > Your patch is missing removal of the
> > kfree(charger->charger_desc.name) in bq24735_charger_remove().
> 
> Yes. Because it seems that this kfree is freeing some other data which
> is not related to devm_kzalloc. I was not sure about removing it.
> So, I was about to discuss it in a separate thread.s

it's assigned in the probe function:

    name = kasprintf(...);
    ...
    supply_desc->name = name;
    ...
    power_supply_register(..., supply_desc, ...);


> Also, in the remove function we have devm_free_irq. I am unsure it
> too. Because normally remove functions do not use devm
> counterparts.

It's required to free the irq before removing the power supply
device.

If the power supply is registered with devm, that should happen
automatically, since it is requested before the irq.  Thus the
remove function can be removed completely at that point :)

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] power_supply: Adjust devm usage

[Vaishali Thakkar]

On Fri, Jul 24, 2015 at 6:29 PM, Sebastian Reichel <sre@kernel.org> wrote:
> Hi,
>
> On Fri, Jul 24, 2015 at 06:03:38PM +0530, Vaishali Thakkar wrote:
>> On Fri, Jul 24, 2015 at 5:56 PM, Sebastian Reichel <sre@kernel.org> wrote:
>> > On Fri, Jul 24, 2015 at 05:28:13PM +0530, Vaishali Thakkar wrote:
>> >> @@ -267,8 +267,9 @@ static int bq24735_charger_probe() {}
>> >> [...]
>> >
>> > Your patch is missing removal of the
>> > kfree(charger->charger_desc.name) in bq24735_charger_remove().
>>
>> Yes. Because it seems that this kfree is freeing some other data which
>> is not related to devm_kzalloc. I was not sure about removing it.
>> So, I was about to discuss it in a separate thread.s
>
> it's assigned in the probe function:
>
>     name = kasprintf(...);
>     ...
>     supply_desc->name = name;
>     ...
>     power_supply_register(..., supply_desc, ...);
>

Oh. Yes. I missed that.

>> Also, in the remove function we have devm_free_irq. I am unsure it
>> too. Because normally remove functions do not use devm
>> counterparts.
>
> It's required to free the irq before removing the power supply
> device.
>
> If the power supply is registered with devm, that should happen
> automatically, since it is requested before the irq.  Thus the
> remove function can be removed completely at that point :)

This makes sense. Thanks for explanation and review :)
So, can I send all changes along with getting rid of remove
function here in a single patch?

> -- Sebastian



-- 
Vaishali

Re: [PATCH] power_supply: Adjust devm usage

[Sebastian Reichel]

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

Hi,

On Fri, Jul 24, 2015 at 06:44:41PM +0530, Vaishali Thakkar wrote:
> [...]
> 
> This makes sense. Thanks for explanation and review :)
> So, can I send all changes along with getting rid of remove
> function here in a single patch?

A single patch is fine for me. Use something like the
following patch subject then:

"power_supply: bq24735: Convert to using managed resources"

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] power_supply: Adjust devm usage

[Vaishali Thakkar]

On Fri, Jul 24, 2015 at 7:08 PM, Sebastian Reichel <sre@kernel.org> wrote:
> Hi,

Hi,

> On Fri, Jul 24, 2015 at 06:44:41PM +0530, Vaishali Thakkar wrote:
>> [...]
>>
>> This makes sense. Thanks for explanation and review :)
>> So, can I send all changes along with getting rid of remove
>> function here in a single patch?
>
> A single patch is fine for me. Use something like the
> following patch subject then:
>
> "power_supply: bq24735: Convert to using managed resources"

Ok. Sure. I'll send version 2 of a patch with this subject line and detailed
commit log.

Thank You.

> -- Sebastian



-- 
Vaishali

Re: [PATCH] power_supply: Adjust devm usage

[Krzysztof Kozlowski]

2015-07-24 21:26 GMT+09:00 Sebastian Reichel <sre@kernel.org>:
> Hi,
>
> Thanks for the cleanup patch.
> I have a couple of comments inlined.
>
>> Subject: Re: [PATCH] power_supply: Adjust devm usage
>
> Please make this "power_supply: bq24735: ...".
>
> On Fri, Jul 24, 2015 at 05:28:13PM +0530, Vaishali Thakkar wrote:
>> Use devm_kasprintf instead of kasprintf. Also, remove various
>> gotos by direct returns and drop unneeded label err_free_name.
>
> Please also use devm_power_supply_unregister() instead
> of power_supply_unregister() to further simplify the driver.
>
>> @@ -267,8 +267,9 @@ static int bq24735_charger_probe() {}
>> [...]
>
> Your patch is missing removal of the
> kfree(charger->charger_desc.name) in bq24735_charger_remove().

Right, I missed that... My review was not sufficient.

Best regards,
Krzysztof

Re: [PATCH] power_supply: Adjust devm usage

[Frans Klaver]

On Fri, Jul 24, 2015 at 2:26 PM, Sebastian Reichel <sre@kernel.org> wrote:
> Hi,
>
> Thanks for the cleanup patch.
> I have a couple of comments inlined.
>
>> Subject: Re: [PATCH] power_supply: Adjust devm usage
>
> Please make this "power_supply: bq24735: ...".
>
> On Fri, Jul 24, 2015 at 05:28:13PM +0530, Vaishali Thakkar wrote:
>> Use devm_kasprintf instead of kasprintf. Also, remove various
>> gotos by direct returns and drop unneeded label err_free_name.
>
> Please also use devm_power_supply_unregister() instead
> of power_supply_unregister() to further simplify the driver.

Sounds like a separate patch.

Frans

Re: [PATCH] power_supply: Adjust devm usage

[Sebastian Reichel]

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

Hi,

On Fri, Jul 24, 2015 at 02:39:12PM +0200, Frans Klaver wrote:
> On Fri, Jul 24, 2015 at 2:26 PM, Sebastian Reichel <sre@kernel.org> wrote:
> > On Fri, Jul 24, 2015 at 05:28:13PM +0530, Vaishali Thakkar wrote:
> >> Use devm_kasprintf instead of kasprintf. Also, remove various
> >> gotos by direct returns and drop unneeded label err_free_name.
> >
> > Please also use devm_power_supply_unregister() instead
> > of power_supply_unregister() to further simplify the driver.
> 
> Sounds like a separate patch.

I would be fine with either one or two patches. It's common, that
devm conversion happens in one commit instead of one patch per
function change.

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] power_supply: Adjust devm usage

[Pavel Machek]

On Fri 2015-07-24 17:28:13, Vaishali Thakkar wrote:
> Use devm_kasprintf instead of kasprintf. Also, remove various
> gotos by direct returns and drop unneeded label err_free_name.

What happens if some /sys file is still open when the device is
removed?



> Signed-off-by: Vaishali Thakkar <vthakkar1994@gmail.com>
> ---
>  drivers/power/bq24735-charger.c | 25 ++++++++++---------------
>  1 file changed, 10 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/power/bq24735-charger.c b/drivers/power/bq24735-charger.c
> index b017437..b2bb67e 100644
> --- a/drivers/power/bq24735-charger.c
> +++ b/drivers/power/bq24735-charger.c
> @@ -267,8 +267,9 @@ static int bq24735_charger_probe(struct i2c_client *client,
>  
>  	name = (char *)charger->pdata->name;
>  	if (!name) {
> -		name = kasprintf(GFP_KERNEL, "bq24735@%s",
> -				 dev_name(&client->dev));
> +		name = devm_kasprintf(&client->dev, GFP_KERNEL,
> +				      "bq24735@%s",
> +				      dev_name(&client->dev));
>  		if (!name) {
>  			dev_err(&client->dev, "Failed to alloc device name\n");
>  			return -ENOMEM;
> @@ -296,23 +297,21 @@ static int bq24735_charger_probe(struct i2c_client *client,
>  	if (ret < 0) {
>  		dev_err(&client->dev, "Failed to read manufacturer id : %d\n",
>  			ret);
> -		goto err_free_name;
> +		return ret;
>  	} else if (ret != 0x0040) {
>  		dev_err(&client->dev,
>  			"manufacturer id mismatch. 0x0040 != 0x%04x\n", ret);
> -		ret = -ENODEV;
> -		goto err_free_name;
> +		return -ENODEV;
>  	}
>  
>  	ret = bq24735_read_word(client, BQ24735_DEVICE_ID);
>  	if (ret < 0) {
>  		dev_err(&client->dev, "Failed to read device id : %d\n", ret);
> -		goto err_free_name;
> +		return ret;
>  	} else if (ret != 0x000B) {
>  		dev_err(&client->dev,
>  			"device id mismatch. 0x000b != 0x%04x\n", ret);
> -		ret = -ENODEV;
> -		goto err_free_name;
> +		return -ENODEV;
>  	}
>  
>  	if (gpio_is_valid(charger->pdata->status_gpio)) {
> @@ -331,7 +330,7 @@ static int bq24735_charger_probe(struct i2c_client *client,
>  	ret = bq24735_config_charger(charger);
>  	if (ret < 0) {
>  		dev_err(&client->dev, "failed in configuring charger");
> -		goto err_free_name;
> +		return ret;
>  	}
>  
>  	/* check for AC adapter presence */
> @@ -339,7 +338,7 @@ static int bq24735_charger_probe(struct i2c_client *client,
>  		ret = bq24735_enable_charging(charger);
>  		if (ret < 0) {
>  			dev_err(&client->dev, "Failed to enable charging\n");
> -			goto err_free_name;
> +			return ret;
>  		}
>  	}
>  
> @@ -349,7 +348,7 @@ static int bq24735_charger_probe(struct i2c_client *client,
>  		ret = PTR_ERR(charger->charger);
>  		dev_err(&client->dev, "Failed to register power supply: %d\n",
>  			ret);
> -		goto err_free_name;
> +		return ret;
>  	}
>  
>  	if (client->irq) {
> @@ -371,10 +370,6 @@ static int bq24735_charger_probe(struct i2c_client *client,
>  	return 0;
>  err_unregister_supply:
>  	power_supply_unregister(charger->charger);
> -err_free_name:
> -	if (name != charger->pdata->name)
> -		kfree(name);
> -
>  	return ret;
>  }
>  

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] power_supply: Adjust devm usage

[Sebastian Reichel]

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

Hi,

On Sun, Aug 02, 2015 at 08:53:43AM +0200, Pavel Machek wrote:
> On Fri 2015-07-24 17:28:13, Vaishali Thakkar wrote:
> > Use devm_kasprintf instead of kasprintf. Also, remove various
> > gotos by direct returns and drop unneeded label err_free_name.
> 
> What happens if some /sys file is still open when the device is
> removed?

There is currently discussion about this on LKML:

https://lkml.org/lkml/2015/7/15/731

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]