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

From: Jerome Brunet <jbrunet@baylibre.com>
To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
	Mark Brown <broonie@kernel.org>
Cc: Stephen Boyd <sboyd@kernel.org>,
	alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 5/5] ASoC: da7219: properly get clk from the provider
Date: Tue, 27 Apr 2021 11:16:25 +0200	[thread overview]
Message-ID: <1jim48jdee.fsf@starbuckisacylon.baylibre.com> (raw)
In-Reply-To: <1j7dkon8jy.fsf@starbuckisacylon.baylibre.com>

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.