Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1191767 > unrolled thread

[PATCH] power_supply: Adjust devm usage

Back to article view | Back to linux.kernel

Contents

  [PATCH] power_supply: Adjust devm usage Vaishali Thakkar <vthakkar1994@gmail.com> - 2015-07-24 14:00 +0200
    Re: [PATCH] power_supply: Adjust devm usage Frans Klaver <fransklaver@gmail.com> - 2015-07-24 14:20 +0200
      Re: [PATCH] power_supply: Adjust devm usage Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2015-07-24 14:30 +0200
        Re: [PATCH] power_supply: Adjust devm usage Vaishali Thakkar <vthakkar1994@gmail.com> - 2015-07-24 14:40 +0200
    Re: [PATCH] power_supply: Adjust devm usage Sebastian Reichel <sre@kernel.org> - 2015-07-24 14:30 +0200
      Re: [PATCH] power_supply: Adjust devm usage Vaishali Thakkar <vthakkar1994@gmail.com> - 2015-07-24 14:40 +0200
        Re: [PATCH] power_supply: Adjust devm usage Sebastian Reichel <sre@kernel.org> - 2015-07-24 15:10 +0200
          Re: [PATCH] power_supply: Adjust devm usage Vaishali Thakkar <vthakkar1994@gmail.com> - 2015-07-24 15:20 +0200
            Re: [PATCH] power_supply: Adjust devm usage Sebastian Reichel <sre@kernel.org> - 2015-07-24 15:40 +0200
              Re: [PATCH] power_supply: Adjust devm usage Vaishali Thakkar <vthakkar1994@gmail.com> - 2015-07-24 15:50 +0200
      Re: [PATCH] power_supply: Adjust devm usage Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2015-07-24 14:40 +0200
      Re: [PATCH] power_supply: Adjust devm usage Frans Klaver <fransklaver@gmail.com> - 2015-07-24 14:40 +0200
        Re: [PATCH] power_supply: Adjust devm usage Sebastian Reichel <sre@kernel.org> - 2015-07-24 15:10 +0200
    Re: [PATCH] power_supply: Adjust devm usage Pavel Machek <pavel@ucw.cz> - 2015-08-02 09:00 +0200
      Re: [PATCH] power_supply: Adjust devm usage Sebastian Reichel <sre@kernel.org> - 2015-08-03 19:40 +0200

#1191767 — [PATCH] power_supply: Adjust devm usage

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1191775

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1191779

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1191785

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1191781

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1191783

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1191808

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1191823

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1191843

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1191848

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1191788

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1191790

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1191805

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1198393

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1199119

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web