| Message ID | 1490691532-2086-3-git-send-email-daniel.baluta@nxp.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | db22d189453cee666f8da2e67419f14f4b2fd9d1 |
| Headers | show |
On Tue, Mar 28, 2017 at 11:58:52AM +0300, Daniel Baluta wrote: > Resource managed devm_clk_get only works with platform's device dev. > I feel like this could use an explaination of why not using devm is the correct fix, rather than just using the platform device for the call. Its not obvious to me, that using the platform device would be an issue. Thanks, Charles > Reported-by: Nicolin Chen <nicoleotsuka@gmail.com> > Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com> > --- > sound/soc/fsl/imx-wm8962.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/sound/soc/fsl/imx-wm8962.c b/sound/soc/fsl/imx-wm8962.c > index 3d894d9..52659fa 100644 > --- a/sound/soc/fsl/imx-wm8962.c > +++ b/sound/soc/fsl/imx-wm8962.c > @@ -231,7 +231,7 @@ static int imx_wm8962_probe(struct platform_device *pdev) > goto fail; > } > > - codec_clk = devm_clk_get(&codec_dev->dev, NULL); > + codec_clk = clk_get(&codec_dev->dev, NULL); > if (IS_ERR(codec_clk)) { > ret = PTR_ERR(codec_clk); > dev_err(&codec_dev->dev, "failed to get codec clk: %d\n", ret); > @@ -239,6 +239,7 @@ static int imx_wm8962_probe(struct platform_device *pdev) > } > > data->clk_frequency = clk_get_rate(codec_clk); > + clk_put(codec_clk); > > data->dai.name = "HiFi"; > data->dai.stream_name = "HiFi"; > -- > 2.7.4 > > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Tue, Mar 28, 2017 at 12:21 PM, Charles Keepax <ckeepax@opensource.wolfsonmicro.com> wrote: > On Tue, Mar 28, 2017 at 11:58:52AM +0300, Daniel Baluta wrote: >> Resource managed devm_clk_get only works with platform's device dev. >> > > I feel like this could use an explaination of why not using devm > is the correct fix, rather than just using the platform device > for the call. Its not obvious to me, that using the platform > device would be an issue. Hi Charles, I see where the confusion comes from :) and I thought the explanation is obvious from the code, see inline comments. Would an explanation like the one below, work better? " We cannot use devm_clk_get with &codec_dev->dev device because the kernel uses pdev->dev to free the managed resources, so we will end up with a leaking codec_clk reference" >> @@ -231,7 +231,7 @@ static int imx_wm8962_probe(struct platform_device *pdev) >> goto fail; >> } pdev->dev from here: >> >> - codec_clk = devm_clk_get(&codec_dev->dev, NULL); >> + codec_clk = clk_get(&codec_dev->dev, NULL); is different from &codec_dev->dev. >> if (IS_ERR(codec_clk)) { >> ret = PTR_ERR(codec_clk); >> dev_err(&codec_dev->dev, "failed to get codec clk: %d\n", ret); >> @@ -239,6 +239,7 @@ static int imx_wm8962_probe(struct platform_device *pdev) >> } >> >> data->clk_frequency = clk_get_rate(codec_clk); >> + clk_put(codec_clk); >> >> data->dai.name = "HiFi"; >> data->dai.stream_name = "HiFi"; thanks, Daniel.
On Tue, Mar 28, 2017 at 01:47:04PM +0300, Daniel Baluta wrote: > On Tue, Mar 28, 2017 at 12:21 PM, Charles Keepax > <ckeepax@opensource.wolfsonmicro.com> wrote: > > On Tue, Mar 28, 2017 at 11:58:52AM +0300, Daniel Baluta wrote: > >> Resource managed devm_clk_get only works with platform's device dev. > >> > > > > I feel like this could use an explaination of why not using devm > > is the correct fix, rather than just using the platform device > > for the call. Its not obvious to me, that using the platform > > device would be an issue. > > Hi Charles, > > I see where the confusion comes from :) and I thought the explanation > is obvious from the code, see inline comments. > > Would an explanation like the one below, work better? > > " We cannot use devm_clk_get with &codec_dev->dev device because > the kernel uses pdev->dev to free the managed resources, so we will end > up with a leaking codec_clk reference" > > >> @@ -231,7 +231,7 @@ static int imx_wm8962_probe(struct platform_device *pdev) > >> goto fail; > >> } > > pdev->dev from here: > > >> > >> - codec_clk = devm_clk_get(&codec_dev->dev, NULL); > >> + codec_clk = clk_get(&codec_dev->dev, NULL); > > is different from &codec_dev->dev. > I get that they are different, I just don't get why changing from a devm_clk_get to a clk_get is a better fix than changing &codec->dev to &pdev->dev. Thanks, Charles
On Tue, Mar 28, 2017 at 12:53:06PM +0100, Charles Keepax wrote: > On Tue, Mar 28, 2017 at 01:47:04PM +0300, Daniel Baluta wrote: > > >> - codec_clk = devm_clk_get(&codec_dev->dev, NULL); > > >> + codec_clk = clk_get(&codec_dev->dev, NULL); > > is different from &codec_dev->dev. > I get that they are different, I just don't get why changing > from a devm_clk_get to a clk_get is a better fix than changing > &codec->dev to &pdev->dev. This should be clear from the semantics of clk_get(): you're looking up the clock in the context of the supplied device and the clock is attached to the CODEC so you need to look up in the CODEC context. What would be even better would be to move the allocation of the clock into the CODEC driver...
On Tue, Mar 28, 2017 at 04:24:57PM +0100, Mark Brown wrote: > On Tue, Mar 28, 2017 at 12:53:06PM +0100, Charles Keepax wrote: > > On Tue, Mar 28, 2017 at 01:47:04PM +0300, Daniel Baluta wrote: > > > > >> - codec_clk = devm_clk_get(&codec_dev->dev, NULL); > > > >> + codec_clk = clk_get(&codec_dev->dev, NULL); > > > > is different from &codec_dev->dev. > > > I get that they are different, I just don't get why changing > > from a devm_clk_get to a clk_get is a better fix than changing > > &codec->dev to &pdev->dev. > > This should be clear from the semantics of clk_get(): you're looking up > the clock in the context of the supplied device and the clock is > attached to the CODEC so you need to look up in the CODEC context. What > would be even better would be to move the allocation of the clock into > the CODEC driver... Ah yes thats what I was missing. Thanks, Charles
On Tue, Mar 28, 2017 at 6:24 PM, Mark Brown <broonie@kernel.org> wrote: > On Tue, Mar 28, 2017 at 12:53:06PM +0100, Charles Keepax wrote: >> On Tue, Mar 28, 2017 at 01:47:04PM +0300, Daniel Baluta wrote: > >> > >> - codec_clk = devm_clk_get(&codec_dev->dev, NULL); >> > >> + codec_clk = clk_get(&codec_dev->dev, NULL); > >> > is different from &codec_dev->dev. > >> I get that they are different, I just don't get why changing >> from a devm_clk_get to a clk_get is a better fix than changing >> &codec->dev to &pdev->dev. > > This should be clear from the semantics of clk_get(): you're looking up > the clock in the context of the supplied device and the clock is > attached to the CODEC so you need to look up in the CODEC context. What > would be even better would be to move the allocation of the clock into > the CODEC driver... If I read the code correctly, both machine and codec driver are doing: * codec_clk = clk_get(...) I guess that the codec driver is the first to be loaded and it will call: * __clk_create_clk which will do the allocation. Then when machine driver is loaded, it will only take a reference to the allocated clock. I might be very wrong on this. Can you clarify what does moving allocation of the clock into codec driver implies? Machine driver only needs a reference to codec_clk to just get the clock's rate. Daniel.
On Wed, Mar 29, 2017 at 02:38:23PM +0300, Daniel Baluta wrote: > If I read the code correctly, both machine and codec driver are doing: > * codec_clk = clk_get(...) > I guess that the codec driver is the first to be loaded and it will call: > * __clk_create_clk > which will do the allocation. Then when machine driver is loaded, it > will only take a reference to the allocated clock. > I might be very wrong on this. Can you clarify what does moving allocation > of the clock into codec driver implies? Machine driver only needs a reference > to codec_clk to just get the clock's rate. Oh, so the CODEC is already requesting the clock. Then what I'm saying is work out a way to use the handle to the clock that the CODEC driver created. In effect the machine driver is telling the CODEC driver to do things with the input clock.
diff --git a/sound/soc/fsl/imx-wm8962.c b/sound/soc/fsl/imx-wm8962.c index 3d894d9..52659fa 100644 --- a/sound/soc/fsl/imx-wm8962.c +++ b/sound/soc/fsl/imx-wm8962.c @@ -231,7 +231,7 @@ static int imx_wm8962_probe(struct platform_device *pdev) goto fail; } - codec_clk = devm_clk_get(&codec_dev->dev, NULL); + codec_clk = clk_get(&codec_dev->dev, NULL); if (IS_ERR(codec_clk)) { ret = PTR_ERR(codec_clk); dev_err(&codec_dev->dev, "failed to get codec clk: %d\n", ret); @@ -239,6 +239,7 @@ static int imx_wm8962_probe(struct platform_device *pdev) } data->clk_frequency = clk_get_rate(codec_clk); + clk_put(codec_clk); data->dai.name = "HiFi"; data->dai.stream_name = "HiFi";
Resource managed devm_clk_get only works with platform's device dev. Reported-by: Nicolin Chen <nicoleotsuka@gmail.com> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com> --- sound/soc/fsl/imx-wm8962.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)