2014-07-06 07:08:10

by Himangi Saraogi

[permalink] [raw]
Subject: [PATCH] ASoC: sgtl5000: Use devm_ functions

This patch introduces the use of managed interfaces like devm_kzalloc,
devm_kstrdup and devm_regulator_register and does avay with the calls to
the functions to free the allocated memory in ldo_regulator_register and
ldo_regulator_remove. The ldo_regulator_remove function is completely
removed as it is no longer required. ldo_regulator_register is called
from a probe function and on failure its value is returned as the
result.

Signed-off-by: Himangi Saraogi <[email protected]>
---
To send to: Liam Girdwood <[email protected]>,Mark Brown <[email protected]>,Jaroslav Kysela <[email protected]>,Takashi Iwai <[email protected]>,[email protected],[email protected]
sound/soc/codecs/sgtl5000.c | 46 +++++++--------------------------------------
1 file changed, 7 insertions(+), 39 deletions(-)

diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
index 249fadb..0efd6d6 100644
--- a/sound/soc/codecs/sgtl5000.c
+++ b/sound/soc/codecs/sgtl5000.c
@@ -841,14 +841,15 @@ static int ldo_regulator_register(struct snd_soc_codec *codec,
struct sgtl5000_priv *sgtl5000 = snd_soc_codec_get_drvdata(codec);
struct regulator_config config = { };

- ldo = kzalloc(sizeof(struct ldo_regulator), GFP_KERNEL);
+ ldo = devm_kzalloc(codec->dev, sizeof(struct ldo_regulator),
+ GFP_KERNEL);

if (!ldo)
return -ENOMEM;

- ldo->desc.name = kstrdup(dev_name(codec->dev), GFP_KERNEL);
+ ldo->desc.name = devm_kstrdup(codec->dev, dev_name(codec->dev),
+ GFP_KERNEL);
if (!ldo->desc.name) {
- kfree(ldo);
dev_err(codec->dev, "failed to allocate decs name memory\n");
return -ENOMEM;
}
@@ -865,35 +866,17 @@ static int ldo_regulator_register(struct snd_soc_codec *codec,
config.driver_data = ldo;
config.init_data = init_data;

- ldo->dev = regulator_register(&ldo->desc, &config);
+ ldo->dev = devm_regulator_register(codec->dev, &ldo->desc, &config);
if (IS_ERR(ldo->dev)) {
int ret = PTR_ERR(ldo->dev);

dev_err(codec->dev, "failed to register regulator\n");
- kfree(ldo->desc.name);
- kfree(ldo);
-
return ret;
}
sgtl5000->ldo = ldo;

return 0;
}
-
-static int ldo_regulator_remove(struct snd_soc_codec *codec)
-{
- struct sgtl5000_priv *sgtl5000 = snd_soc_codec_get_drvdata(codec);
- struct ldo_regulator *ldo = sgtl5000->ldo;
-
- if (!ldo)
- return 0;
-
- regulator_unregister(ldo->dev);
- kfree(ldo->desc.name);
- kfree(ldo);
-
- return 0;
-}
#else
static int ldo_regulator_register(struct snd_soc_codec *codec,
struct regulator_init_data *init_data,
@@ -902,11 +885,6 @@ static int ldo_regulator_register(struct snd_soc_codec *codec,
dev_err(codec->dev, "this setup needs regulator support in the kernel\n");
return -EINVAL;
}
-
-static int ldo_regulator_remove(struct snd_soc_codec *codec)
-{
- return 0;
-}
#endif

/*
@@ -1278,23 +1256,17 @@ static int sgtl5000_enable_regulators(struct snd_soc_codec *codec)
ret = devm_regulator_bulk_get(codec->dev, ARRAY_SIZE(sgtl5000->supplies),
sgtl5000->supplies);
if (ret)
- goto err_ldo_remove;
+ return ret;

ret = regulator_bulk_enable(ARRAY_SIZE(sgtl5000->supplies),
sgtl5000->supplies);
if (ret)
- goto err_ldo_remove;
+ return ret;

/* wait for all power rails bring up */
udelay(10);

return 0;
-
-err_ldo_remove:
- if (!external_vddd)
- ldo_regulator_remove(codec);
- return ret;
-
}

static int sgtl5000_probe(struct snd_soc_codec *codec)
@@ -1359,8 +1331,6 @@ static int sgtl5000_probe(struct snd_soc_codec *codec)
err:
regulator_bulk_disable(ARRAY_SIZE(sgtl5000->supplies),
sgtl5000->supplies);
- ldo_regulator_remove(codec);
-
return ret;
}

@@ -1372,8 +1342,6 @@ static int sgtl5000_remove(struct snd_soc_codec *codec)

regulator_bulk_disable(ARRAY_SIZE(sgtl5000->supplies),
sgtl5000->supplies);
- ldo_regulator_remove(codec);
-
return 0;
}

--
1.9.1


2014-07-07 14:51:16

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH] ASoC: sgtl5000: Use devm_ functions

On Sun, Jul 06, 2014 at 12:38:00PM +0530, Himangi Saraogi wrote:

> diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
> index 249fadb..0efd6d6 100644
> --- a/sound/soc/codecs/sgtl5000.c
> +++ b/sound/soc/codecs/sgtl5000.c
> @@ -841,14 +841,15 @@ static int ldo_regulator_register(struct snd_soc_codec *codec,
> struct sgtl5000_priv *sgtl5000 = snd_soc_codec_get_drvdata(codec);
> struct regulator_config config = { };
>
> - ldo = kzalloc(sizeof(struct ldo_regulator), GFP_KERNEL);
> + ldo = devm_kzalloc(codec->dev, sizeof(struct ldo_regulator),
> + GFP_KERNEL);

You're using the managed functions within the ASoC level probe functions
which doesn't work - devm_ is only usable as part of the driver model
binding and unbinding. All this resource allocation needs to be moved
into the device level probe (which is a better thing anyway) before it
is converted to devm.


Attachments:
(No filename) (903.00 B)
signature.asc (836.00 B)
Digital signature
Download all attachments

2014-07-07 14:58:30

by Julia Lawall

[permalink] [raw]
Subject: Re: [PATCH] ASoC: sgtl5000: Use devm_ functions



On Mon, 7 Jul 2014, Mark Brown wrote:

> On Sun, Jul 06, 2014 at 12:38:00PM +0530, Himangi Saraogi wrote:
>
> > diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
> > index 249fadb..0efd6d6 100644
> > --- a/sound/soc/codecs/sgtl5000.c
> > +++ b/sound/soc/codecs/sgtl5000.c
> > @@ -841,14 +841,15 @@ static int ldo_regulator_register(struct snd_soc_codec *codec,
> > struct sgtl5000_priv *sgtl5000 = snd_soc_codec_get_drvdata(codec);
> > struct regulator_config config = { };
> >
> > - ldo = kzalloc(sizeof(struct ldo_regulator), GFP_KERNEL);
> > + ldo = devm_kzalloc(codec->dev, sizeof(struct ldo_regulator),
> > + GFP_KERNEL);
>
> You're using the managed functions within the ASoC level probe functions
> which doesn't work - devm_ is only usable as part of the driver model
> binding and unbinding. All this resource allocation needs to be moved
> into the device level probe (which is a better thing anyway) before it
> is converted to devm.

Nevertheless, there is already a call to devm_regulator_bulk_get in
sgtl5000_enable_regulators which calls sgtl5000_replace_vddd_with_ldo
which calls ldo_regulator_register. That call was introduced by

commit 63e54cd9caa3ce03635810608519e2b37d8bc706
Author: Fabio Estevam <[email protected]>
Date: Thu Apr 24 14:13:08 2014 -0300

It seems that that patch should be reverted?

julia

2014-07-07 15:20:34

by Fabio Estevam

[permalink] [raw]
Subject: Re: [alsa-devel] [PATCH] ASoC: sgtl5000: Use devm_ functions

On Mon, Jul 7, 2014 at 11:58 AM, Julia Lawall <[email protected]> wrote:

> Nevertheless, there is already a call to devm_regulator_bulk_get in
> sgtl5000_enable_regulators which calls sgtl5000_replace_vddd_with_ldo
> which calls ldo_regulator_register. That call was introduced by
>
> commit 63e54cd9caa3ce03635810608519e2b37d8bc706
> Author: Fabio Estevam <[email protected]>
> Date: Thu Apr 24 14:13:08 2014 -0300
>
> It seems that that patch should be reverted?

I think so. Russell also reported a kernel oops when unbinding this
module, so I will prepare a patch reverting it.

Thanks

2014-07-07 15:23:46

by Julia Lawall

[permalink] [raw]
Subject: Re: [alsa-devel] [PATCH] ASoC: sgtl5000: Use devm_ functions



On Mon, 7 Jul 2014, Fabio Estevam wrote:

> On Mon, Jul 7, 2014 at 11:58 AM, Julia Lawall <[email protected]> wrote:
>
> > Nevertheless, there is already a call to devm_regulator_bulk_get in
> > sgtl5000_enable_regulators which calls sgtl5000_replace_vddd_with_ldo
> > which calls ldo_regulator_register. That call was introduced by
> >
> > commit 63e54cd9caa3ce03635810608519e2b37d8bc706
> > Author: Fabio Estevam <[email protected]>
> > Date: Thu Apr 24 14:13:08 2014 -0300
> >
> > It seems that that patch should be reverted?
>
> I think so. Russell also reported a kernel oops when unbinding this
> module, so I will prepare a patch reverting it.

There is documentation about what kinds of devm functions exist, but it is
too bad that there is no documentation about where they can be used.
Often there are several levels of function pointers involved, so it can be
hard to figure out whether they can be used just by looking at the code.
I have only taken the strategy of using them in kinds of functions where
someone else has alreadyy figured out that they can be used.

julia

2014-07-07 15:35:22

by Lars-Peter Clausen

[permalink] [raw]
Subject: Re: [alsa-devel] [PATCH] ASoC: sgtl5000: Use devm_ functions

On 07/07/2014 05:23 PM, Julia Lawall wrote:
>
>
> On Mon, 7 Jul 2014, Fabio Estevam wrote:
>
>> On Mon, Jul 7, 2014 at 11:58 AM, Julia Lawall <[email protected]> wrote:
>>
>>> Nevertheless, there is already a call to devm_regulator_bulk_get in
>>> sgtl5000_enable_regulators which calls sgtl5000_replace_vddd_with_ldo
>>> which calls ldo_regulator_register. That call was introduced by
>>>
>>> commit 63e54cd9caa3ce03635810608519e2b37d8bc706
>>> Author: Fabio Estevam <[email protected]>
>>> Date: Thu Apr 24 14:13:08 2014 -0300
>>>
>>> It seems that that patch should be reverted?
>>
>> I think so. Russell also reported a kernel oops when unbinding this
>> module, so I will prepare a patch reverting it.
>
> There is documentation about what kinds of devm functions exist, but it is
> too bad that there is no documentation about where they can be used.
> Often there are several levels of function pointers involved, so it can be
> hard to figure out whether they can be used just by looking at the code.
> I have only taken the strategy of using them in kinds of functions where
> someone else has alreadyy figured out that they can be used.

Yes, it is probably a bit underdocumented. The rule of thumb is don't use it
anywhere else except for device probe callbacks (and functions that are only
called from a device probe function) and only for the device that is being
probed. There are probably a couple of instances in the kernel where the
manged resource allocators are used in places where they shouldn't be used.

- Lars

2014-07-08 08:08:52

by Mark Brown

[permalink] [raw]
Subject: Re: [alsa-devel] [PATCH] ASoC: sgtl5000: Use devm_ functions

On Mon, Jul 07, 2014 at 05:23:39PM +0200, Julia Lawall wrote:

> There is documentation about what kinds of devm functions exist, but it is
> too bad that there is no documentation about where they can be used.
> Often there are several levels of function pointers involved, so it can be
> hard to figure out whether they can be used just by looking at the code.
> I have only taken the strategy of using them in kinds of functions where
> someone else has alreadyy figured out that they can be used.

It should be fairly clear given what they do I'd have thought - the
devm_ functions tie the deallocation of a resource to the unbinding of
a driver from a device so they can only be used to replace things that
get cleaned up in a device model unbind path. There's not usually a
great deal of indirection going on in those.


Attachments:
(No filename) (826.00 B)
signature.asc (836.00 B)
Digital signature
Download all attachments

2014-07-08 08:15:29

by Julia Lawall

[permalink] [raw]
Subject: Re: [alsa-devel] [PATCH] ASoC: sgtl5000: Use devm_ functions



On Tue, 8 Jul 2014, Mark Brown wrote:

> On Mon, Jul 07, 2014 at 05:23:39PM +0200, Julia Lawall wrote:
>
> > There is documentation about what kinds of devm functions exist, but it is
> > too bad that there is no documentation about where they can be used.
> > Often there are several levels of function pointers involved, so it can be
> > hard to figure out whether they can be used just by looking at the code.
> > I have only taken the strategy of using them in kinds of functions where
> > someone else has alreadyy figured out that they can be used.
>
> It should be fairly clear given what they do I'd have thought - the
> devm_ functions tie the deallocation of a resource to the unbinding of
> a driver from a device so they can only be used to replace things that
> get cleaned up in a device model unbind path. There's not usually a
> great deal of indirection going on in those.

It is completely clear what they do. What is not clear is what device
libraries are set up to call the freeing functions at what point. For
example, I know that that platform drivers are set up for this, but once I
tried to find the lines of code that would justify that, but I could not.
Perhaps I was not patient enough or missed something.

julia

2014-07-08 14:56:06

by Mark Brown

[permalink] [raw]
Subject: Re: [alsa-devel] [PATCH] ASoC: sgtl5000: Use devm_ functions

On Tue, Jul 08, 2014 at 10:15:20AM +0200, Julia Lawall wrote:
> On Tue, 8 Jul 2014, Mark Brown wrote:

> > It should be fairly clear given what they do I'd have thought - the
> > devm_ functions tie the deallocation of a resource to the unbinding of
> > a driver from a device so they can only be used to replace things that
> > get cleaned up in a device model unbind path. There's not usually a
> > great deal of indirection going on in those.

> It is completely clear what they do. What is not clear is what device
> libraries are set up to call the freeing functions at what point. For
> example, I know that that platform drivers are set up for this, but once I
> tried to find the lines of code that would justify that, but I could not.
> Perhaps I was not patient enough or missed something.

All devices do this - it's done as part of the driver model core code so
there is no need for individual buses to do anything.


Attachments:
(No filename) (931.00 B)
signature.asc (836.00 B)
Digital signature
Download all attachments

2014-07-09 05:30:45

by Julia Lawall

[permalink] [raw]
Subject: Re: [alsa-devel] [PATCH] ASoC: sgtl5000: Use devm_ functions



On Tue, 8 Jul 2014, Mark Brown wrote:

> On Tue, Jul 08, 2014 at 10:15:20AM +0200, Julia Lawall wrote:
> > On Tue, 8 Jul 2014, Mark Brown wrote:
>
> > > It should be fairly clear given what they do I'd have thought - the
> > > devm_ functions tie the deallocation of a resource to the unbinding of
> > > a driver from a device so they can only be used to replace things that
> > > get cleaned up in a device model unbind path. There's not usually a
> > > great deal of indirection going on in those.
>
> > It is completely clear what they do. What is not clear is what device
> > libraries are set up to call the freeing functions at what point. For
> > example, I know that that platform drivers are set up for this, but once I
> > tried to find the lines of code that would justify that, but I could not.
> > Perhaps I was not patient enough or missed something.
>
> All devices do this - it's done as part of the driver model core code so
> there is no need for individual buses to do anything.

How should one realize that this does not apply to the original file
under discussion, sound/soc/codecs/sgtl5000.c? The associated structure
is snd_soc_codec_driver. What code could one look for at the call sites
of the probe and remove functions to know that managed memory can be used?

thanks,
julia

2014-07-09 08:03:30

by Mark Brown

[permalink] [raw]
Subject: Re: [alsa-devel] [PATCH] ASoC: sgtl5000: Use devm_ functions

On Wed, Jul 09, 2014 at 07:30:40AM +0200, Julia Lawall wrote:
> On Tue, 8 Jul 2014, Mark Brown wrote:

> > All devices do this - it's done as part of the driver model core code so
> > there is no need for individual buses to do anything.

> How should one realize that this does not apply to the original file
> under discussion, sound/soc/codecs/sgtl5000.c? The associated structure
> is snd_soc_codec_driver. What code could one look for at the call sites
> of the probe and remove functions to know that managed memory can be used?

That's not a device model driver; the way you should realise this is
that there's no device being registered on a bus which is matched by the
driver core.


Attachments:
(No filename) (696.00 B)
signature.asc (836.00 B)
Digital signature
Download all attachments

2014-07-09 08:10:17

by Julia Lawall

[permalink] [raw]
Subject: Re: [alsa-devel] [PATCH] ASoC: sgtl5000: Use devm_ functions



On Wed, 9 Jul 2014, Mark Brown wrote:

> On Wed, Jul 09, 2014 at 07:30:40AM +0200, Julia Lawall wrote:
> > On Tue, 8 Jul 2014, Mark Brown wrote:
>
> > > All devices do this - it's done as part of the driver model core code so
> > > there is no need for individual buses to do anything.
>
> > How should one realize that this does not apply to the original file
> > under discussion, sound/soc/codecs/sgtl5000.c? The associated structure
> > is snd_soc_codec_driver. What code could one look for at the call sites
> > of the probe and remove functions to know that managed memory can be used?
>
> That's not a device model driver; the way you should realise this is
> that there's no device being registered on a bus which is matched by the
> driver core.

OK, thanks for the clarification.

julia