diff mbox series

[v2,5/5] ASoC: da7219: properly get clk from the provider

Message ID 20210421120512.413057-6-jbrunet@baylibre.com (mailing list archive)
State Accepted
Commit 12f8127fe9e6154dd4197df97e44f3fd67583071
Headers show
Series ASoC: clock provider clean-up | expand

Commit Message

Jerome Brunet April 21, 2021, 12:05 p.m. UTC
Instead of using the clk embedded in the clk_hw (which is meant to go
away), a clock provider which need to interact with its own clock should
request clk reference through the clock provider API.

Reviewed-by: Stephen Boyd <sboyd@kernel.org>
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 sound/soc/codecs/da7219.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Pierre-Louis Bossart April 26, 2021, 6:10 p.m. UTC | #1
On 4/21/21 7:05 AM, Jerome Brunet wrote:
> Instead of using the clk embedded in the clk_hw (which is meant to go
> away), a clock provider which need to interact with its own clock should
> request clk reference through the clock provider API.
> 
> Reviewed-by: Stephen Boyd <sboyd@kernel.org>
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>

This patch seems to introduce a regression in our modprobe/rmmod tests

https://github.com/thesofproject/linux/pull/2870

RMMOD	snd_soc_da7219
rmmod: ERROR: Module snd_soc_da7219 is in use

Reverting this patch restores the ability to remove the module.

Wondering if devm_ increases a module/device refcount somehow?

> ---
>   sound/soc/codecs/da7219.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
> index 13009d08b09a..bd3c523a8617 100644
> --- a/sound/soc/codecs/da7219.c
> +++ b/sound/soc/codecs/da7219.c
> @@ -2181,7 +2181,10 @@ static int da7219_register_dai_clks(struct snd_soc_component *component)
>   				 ret);
>   			goto err;
>   		}
> -		da7219->dai_clks[i] = dai_clk_hw->clk;
> +
> +		da7219->dai_clks[i] = devm_clk_hw_get_clk(dev, dai_clk_hw, NULL);
> +		if (IS_ERR(da7219->dai_clks[i]))
> +			return PTR_ERR(da7219->dai_clks[i]);
>   
>   		/* For DT setup onecell data, otherwise create lookup */
>   		if (np) {
>
Pierre-Louis Bossart April 26, 2021, 6:39 p.m. UTC | #2
> On 4/21/21 7:05 AM, Jerome Brunet wrote:
>> Instead of using the clk embedded in the clk_hw (which is meant to go
>> away), a clock provider which need to interact with its own clock should
>> request clk reference through the clock provider API.
>>
>> Reviewed-by: Stephen Boyd <sboyd@kernel.org>
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> 
> This patch seems to introduce a regression in our modprobe/rmmod tests
> 
> https://github.com/thesofproject/linux/pull/2870
> 
> RMMOD    snd_soc_da7219
> rmmod: ERROR: Module snd_soc_da7219 is in use
> 
> Reverting this patch restores the ability to remove the module.
> 
> Wondering if devm_ increases a module/device refcount somehow?

the following diff fixes the issue for me

There is an explicit try_module_get() in clk_hw_create_clk, so you 
end-up increasing the refcount of your own module.

devm_ doesn't seem like a good idea here, I think we have to release the 
clk and its implicit module reference when the component is freed, no?

I can send a proper fix if there is consensus.


diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
index bd3c523a8617..8696ac749af3 100644
--- a/sound/soc/codecs/da7219.c
+++ b/sound/soc/codecs/da7219.c
@@ -2182,7 +2182,7 @@ static int da7219_register_dai_clks(struct 
snd_soc_component *component)
                         goto err;
                 }

-               da7219->dai_clks[i] = devm_clk_hw_get_clk(dev, 
dai_clk_hw, NULL);
+               da7219->dai_clks[i] = clk_hw_get_clk(dai_clk_hw, NULL);
                 if (IS_ERR(da7219->dai_clks[i]))
                         return PTR_ERR(da7219->dai_clks[i]);

@@ -2218,6 +2218,8 @@ static int da7219_register_dai_clks(struct 
snd_soc_component *component)
                 if (da7219->dai_clks_lookup[i])
                         clkdev_drop(da7219->dai_clks_lookup[i]);

+               clk_put(da7219->dai_clks[i]);
+
                 clk_hw_unregister(&da7219->dai_clks_hw[i]);
         } while (i-- > 0);

@@ -2240,6 +2242,8 @@ static void da7219_free_dai_clks(struct 
snd_soc_component *component)
                 if (da7219->dai_clks_lookup[i])
                         clkdev_drop(da7219->dai_clks_lookup[i]);

+               clk_put(da7219->dai_clks[i]);
+
                 clk_hw_unregister(&da7219->dai_clks_hw[i]);
         }
Jerome Brunet April 26, 2021, 7:35 p.m. UTC | #3
On Mon 26 Apr 2021 at 20:10, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote:

> On 4/21/21 7:05 AM, Jerome Brunet wrote:
>> Instead of using the clk embedded in the clk_hw (which is meant to go
>> away), a clock provider which need to interact with its own clock should
>> request clk reference through the clock provider API.
>> Reviewed-by: Stephen Boyd <sboyd@kernel.org>
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>
> This patch seems to introduce a regression in our modprobe/rmmod tests

Really sorry about that :/

>
> https://github.com/thesofproject/linux/pull/2870
>
> RMMOD	snd_soc_da7219
> rmmod: ERROR: Module snd_soc_da7219 is in use
>
> Reverting this patch restores the ability to remove the module.
>
> Wondering if devm_ increases a module/device refcount somehow?

The driver is the provider and consumer, so it is consuming itself.
This was the intent, I think the patch should be correct like
this. Maybe I overlooked something on the clock side. I'll check.

I'm not sure the problem is devm_ variant, it might be 
clk_hw_get_clk() simpler variant which also plays with module ref counts.

I don't have this particular HW to check but I'll try to replicate the
test with a dummy module and report ASAP.

Of course, I suppose the same problem applies to PATCH 1 of the series

>
>> ---
>>   sound/soc/codecs/da7219.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>> diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
>> index 13009d08b09a..bd3c523a8617 100644
>> --- a/sound/soc/codecs/da7219.c
>> +++ b/sound/soc/codecs/da7219.c
>> @@ -2181,7 +2181,10 @@ static int da7219_register_dai_clks(struct snd_soc_component *component)
>>   				 ret);
>>   			goto err;
>>   		}
>> -		da7219->dai_clks[i] = dai_clk_hw->clk;
>> +
>> +		da7219->dai_clks[i] = devm_clk_hw_get_clk(dev, dai_clk_hw, NULL);
>> +		if (IS_ERR(da7219->dai_clks[i]))
>> +			return PTR_ERR(da7219->dai_clks[i]);
>>     		/* For DT setup onecell data, otherwise create lookup */
>>   		if (np) {
>>
Jerome Brunet April 27, 2021, 9:16 a.m. UTC | #4
On Mon 26 Apr 2021 at 21:35, Jerome Brunet <jbrunet@baylibre.com> wrote:

> On Mon 26 Apr 2021 at 20:10, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote:
>
>> On 4/21/21 7:05 AM, Jerome Brunet wrote:
>>> Instead of using the clk embedded in the clk_hw (which is meant to go
>>> away), a clock provider which need to interact with its own clock should
>>> request clk reference through the clock provider API.
>>> Reviewed-by: Stephen Boyd <sboyd@kernel.org>
>>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>>
>> This patch seems to introduce a regression in our modprobe/rmmod tests
>
> Really sorry about that :/
>
>>
>> https://github.com/thesofproject/linux/pull/2870
>>
>> RMMOD	snd_soc_da7219
>> rmmod: ERROR: Module snd_soc_da7219 is in use
>>
>> Reverting this patch restores the ability to remove the module.
>>
>> Wondering if devm_ increases a module/device refcount somehow?
>
> The driver is the provider and consumer, so it is consuming itself.
> This was the intent, I think the patch should be correct like
> this. Maybe I overlooked something on the clock side. I'll check.
>
> I'm not sure the problem is devm_ variant, it might be 
> clk_hw_get_clk() simpler variant which also plays with module ref counts.
>
> I don't have this particular HW to check but I'll try to replicate the
> test with a dummy module and report ASAP.
>
> Of course, I suppose the same problem applies to PATCH 1 of the series

So I can confirm that the problem is in CCF and the function
clk_hw_get_clk() which pins the clocks provider to itself.

Not that many clock providers are modules and even less are getting
removed. This is probably why this fundamental issue has not been
detected before. Thanks a lot for reporting it.

Mark, at this point I think it would be best to revert patches 1 and 5
while we work this out in CCF. The other patches are not affected.
Sorry for the mess.
Mark Brown April 27, 2021, 10:27 a.m. UTC | #5
On Tue, Apr 27, 2021 at 11:16:25AM +0200, Jerome Brunet wrote:

> Mark, at this point I think it would be best to revert patches 1 and 5
> while we work this out in CCF. The other patches are not affected.
> Sorry for the mess.

Sure, can someone send a patch with a changelog explaining the issue
please?
Jerome Brunet April 27, 2021, 11:33 a.m. UTC | #6
On Tue 27 Apr 2021 at 12:27, Mark Brown <broonie@kernel.org> wrote:

> On Tue, Apr 27, 2021 at 11:16:25AM +0200, Jerome Brunet wrote:
>
>> Mark, at this point I think it would be best to revert patches 1 and 5
>> while we work this out in CCF. The other patches are not affected.
>> Sorry for the mess.
>
> Sure, can someone send a patch with a changelog explaining the issue
> please?

Will do
diff mbox series

Patch

diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
index 13009d08b09a..bd3c523a8617 100644
--- a/sound/soc/codecs/da7219.c
+++ b/sound/soc/codecs/da7219.c
@@ -2181,7 +2181,10 @@  static int da7219_register_dai_clks(struct snd_soc_component *component)
 				 ret);
 			goto err;
 		}
-		da7219->dai_clks[i] = dai_clk_hw->clk;
+
+		da7219->dai_clks[i] = devm_clk_hw_get_clk(dev, dai_clk_hw, NULL);
+		if (IS_ERR(da7219->dai_clks[i]))
+			return PTR_ERR(da7219->dai_clks[i]);
 
 		/* For DT setup onecell data, otherwise create lookup */
 		if (np) {