| Message ID | 20140706070800.GA2927@himangi-Dell (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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 <fabio.estevam@freescale.com> Date: Thu Apr 24 14:13:08 2014 -0300 It seems that that patch should be reverted? julia
On Mon, Jul 7, 2014 at 11:58 AM, Julia Lawall <julia.lawall@lip6.fr> 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 <fabio.estevam@freescale.com> > 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
On Mon, 7 Jul 2014, Fabio Estevam wrote: > On Mon, Jul 7, 2014 at 11:58 AM, Julia Lawall <julia.lawall@lip6.fr> 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 <fabio.estevam@freescale.com> > > 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
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 <julia.lawall@lip6.fr> 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 <fabio.estevam@freescale.com> >>> 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
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.
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
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.
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
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.
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
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; }
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 <himangi774@gmail.com> --- To send to: Liam Girdwood <lgirdwood@gmail.com>,Mark Brown <broonie@kernel.org>,Jaroslav Kysela <perex@perex.cz>,Takashi Iwai <tiwai@suse.de>,alsa-devel@alsa-project.org,linux-kernel@vger.kernel.org sound/soc/codecs/sgtl5000.c | 46 +++++++-------------------------------------- 1 file changed, 7 insertions(+), 39 deletions(-)