[alsa-devel] regarding references taken on platform and codec driver modules
Hi,
I have 4 dai_link instances as below. (the codec_name and platform_name are the same for all the 4 instances)
struct snd_soc_dai_link mfld_msic_dailink[] = { { .name = "Intel MSIC Headset", .stream_name = "Codec", .cpu_dai_name = "Headset-cpu-dai", .codec_dai_name = "Intel MSIC Headset", .codec_name = "mid-msic-codec", .platform_name = "mid-audio-platform", .init = msic_init, }, { .name = "Intel MSIC Speaker", .stream_name = "Speaker", .cpu_dai_name = "Speaker-cpu-dai", .codec_dai_name = "Intel MSIC Speaker", .codec_name = "mid-msic-codec", .platform_name = "mid-audio-platform", .init = NULL, }, { .name = "Intel MSIC Vibra", .stream_name = "Vibra1", .cpu_dai_name = "Vibra 1-cpu-dai", .codec_dai_name = "Intel MSIC Vibra", .codec_name = "mid-msic-codec", .platform_name = "mid-audio-platform", .init = NULL, }, { .name = "Intel MSIC Haptic", .stream_name = "Vibra2", .cpu_dai_name = "Vibra 2-cpu-dai", .codec_dai_name = "Intel MSIC Haptic", .codec_name = "mid-msic-codec", .platform_name = "mid-audio-platform", .init = NULL, }, };
When the soc_bind_dai_link is called For every dai_link instance, reference to codec, platform and cpu_dai are taken
When soc_remove_dai_link is called, the reference is released as follows For cpu_dai after checking if cpu_dai->probed is 1
For codec and platform, it checks for codec->probed and platform->probed So for the first instance of dai_link, the reference of codec and platform are removed correctly but for the other instances, it finds that codec->probed and platform->probed are 0 and does not remove the reference. As a result, the reference count of codec and platform drivers remain as 3 and the rmmod on those driver modules fail.
Is there anything that I am doing wrong here or missing out?
Thanks, Harsha
Is there anything that I am doing wrong here or missing out?
This should all happen automatically based only on the data structures in your driver, there shouldn't be anything to miss.
I have created the required data structures in the driver. When I do a insmod of machine driver, there are 8 references of platform driver taken (4 cpu dai and 4 platform driver) and 4 of codec driver taken When I do rmmod of machine driver, the references of platform driver and codec driver are left at 3. I did a trace back as where all the references are taken and where all they are released. I find that 4 references of codec driver and platform driver are taken but only one of them is released.
How do I fix this problem?
-Harsha
On Mon, Jan 03, 2011 at 09:57:16PM +0530, Harsha, Priya wrote:
driver are left at 3. I did a trace back as where all the references are taken and where all they are released. I find that 4 references of codec driver and platform driver are taken but only one of them is released.
How do I fix this problem?
By understanding where the references should be taken and released then fixing the code to do that.
driver are left at 3. I did a trace back as where all the references are taken and where all they are released. I find that 4 references of codec driver and platform driver are taken but only one of them is released.
How do I fix this problem?
By understanding where the references should be taken and released then fixing the code to do that.
When the dais are binded, the reference to codec and platform driver are taken for every dai link but released only once. Instead, the below patch moves to taking the reference to codec and platform driver when they are probed and release it once. This allows the ASoC driver modules to be inserted and removed without issues.
Please provide your comments on the patch below that makes this change
From: Harsha Priya priya.harsha@intel.com Date: Mon, 3 Jan 2011 22:36:05 +0530 Subject: [PATCH] Fix the reference to codec and platform driver to where they are probed
The soc-core take the platform and codec driver reference and does not free them all. This cause the module unload to fail.
This patch fixes by the taking one reference to platform and codec module and free the same.This allows load/unload properly
Signed-off-by: Harsha Priya priya.harsha@intel.com Signed-off-by: Vinod Koulvinod.koul@intel.com
modified: sound/soc/soc-core.c
--- sound/soc/soc-core.c | 14 +++++++------- 1 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 606281a..97f9d3e 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1259,9 +1259,6 @@ find_codec: if (!strcmp(codec->name, dai_link->codec_name)) { rtd->codec = codec;
- if (!try_module_get(codec->dev->driver->owner)) - return -ENODEV; - /* CODEC found, so find CODEC DAI from registered DAIs from this CODEC*/ list_for_each_entry(codec_dai, &dai_list, list) { if (codec->dev == codec_dai->dev && @@ -1287,10 +1284,6 @@ find_platform: /* no, then find CPU DAI from registered DAIs*/ list_for_each_entry(platform, &platform_list, list) { if (!strcmp(platform->name, dai_link->platform_name)) { - - if (!try_module_get(platform->dev->driver->owner)) - return -ENODEV; - rtd->platform = platform; goto out; } @@ -1425,6 +1418,9 @@ static int soc_probe_codec(struct snd_soc_card *card, soc_init_codec_debugfs(codec);
/* mark codec as probed and add to card codec list */ + if (!try_module_get(codec->dev->driver->owner)) + return -ENODEV; + codec->probed = 1; list_add(&codec->card_list, &card->codec_dev_list); list_add(&codec->dapm.list, &card->dapm_list); @@ -1556,6 +1552,10 @@ static int soc_probe_dai_link(struct snd_soc_card *card, int num) } } /* mark platform as probed and add to card platform list */ + + if (!try_module_get(platform->dev->driver->owner)) + return -ENODEV; + platform->probed = 1; list_add(&platform->card_list, &card->platform_dev_list); } -- 1.7.2.3
On Mon, Jan 03, 2011 at 11:26:34PM +0530, Harsha, Priya wrote:
Please provide your comments on the patch below that makes this change
Please follow Documentation/SubmittingPatches - remember to CC maintainers (I've added Liam) and don't bury your patches in the middle of another e-mail.
From: Harsha Priya priya.harsha@intel.com Date: Mon, 3 Jan 2011 22:36:05 +0530 Subject: [PATCH] Fix the reference to codec and platform driver to where they are probed
The soc-core take the platform and codec driver reference and does not free them all. This cause the module unload to fail.
Please explain in which circumstances this happens in the changelog;
This patch fixes by the taking one reference to platform and codec module and free the same.This allows load/unload properly
This looks reasonable, though we're now not counting CODEC DAIs individually as we do with platform DAIs which is a bit asymmetric, it might be nicer to change to free all the references we're currently taking to the CODEC. I'm not too fussed, though.
free the same.This allows load/unload properly
Signed-off-by: Harsha Priya priya.harsha@intel.com Signed-off-by: Vinod Koulvinod.koul@intel.com
modified: sound/soc/soc-core.c
No need for this in the patch either. Leaving the patch content for Liam:
sound/soc/soc-core.c | 14 +++++++------- 1 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 606281a..97f9d3e 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1259,9 +1259,6 @@ find_codec: if (!strcmp(codec->name, dai_link->codec_name)) { rtd->codec = codec;
if (!try_module_get(codec->dev->driver->owner))return -ENODEV;/* CODEC found, so find CODEC DAI from registered DAIs from this CODEC*/ list_for_each_entry(codec_dai, &dai_list, list) { if (codec->dev == codec_dai->dev && @@ -1287,10 +1284,6 @@ find_platform:if (!try_module_get(platform->dev->driver->owner))return -ENODEV;rtd->platform = platform; goto out;@@ -1425,6 +1418,9 @@ static int soc_probe_codec(struct snd_soc_card *card, soc_init_codec_debugfs(codec);
/* mark codec as probed and add to card codec list */
- if (!try_module_get(codec->dev->driver->owner))
return -ENODEV;- codec->probed = 1; list_add(&codec->card_list, &card->codec_dev_list); list_add(&codec->dapm.list, &card->dapm_list); @@ -1556,6 +1552,10 @@ static int soc_probe_dai_link(struct snd_soc_card *card, int num) } } /* mark platform as probed and add to card platform list */
if (!try_module_get(platform->dev->driver->owner))return -ENODEV;- platform->probed = 1; list_add(&platform->card_list, &card->platform_dev_list); }
-- 1.7.2.3
Please provide your comments on the patch below that makes this change
Please follow Documentation/SubmittingPatches - remember to CC maintainers (I've added Liam) and don't bury your patches in the middle of another e-mail.
I have sent the patch in a separate email.
Please provide your comments on the patch below that makes this change
Please follow Documentation/SubmittingPatches - remember to CC maintainers (I've added Liam) and don't bury your patches in the middle of another e-mail.
I have sent the patch in a separate email.
I seem to have hit another instance of where references are kept wrongly.
During the testing codec driver probe was failing and this results in wrong reference of platform driver .
On investigating the issue is in soc_bind_dai_link()
It takes the reference of cpu_dai and adds the dai in the runtime structure. But since the codec was not found these references are kept and modules cant be unloaded.
Now thinking on this and other reference patch recently applied, IMO we can instead of taking the module reference check if cpu_dai is probed. If probed then only go ahead otherwise return error
Something like this @@ -1238,9 +1238,10 @@ static int soc_bind_dai_link(struct snd_soc_card *card, int num) list_for_each_entry(cpu_dai, &dai_list, list) { if (!strcmp(cpu_dai->name, dai_link->cpu_dai_name)) {
- if (!try_module_get(cpu_dai->dev->driver->owner)) + if (!cpu_dai->probed) return -ENODEV;
Would this be right thing to do? My logic is that we should be relying on probed to see references (as while marking probed we do take the reference). I have tested with both false and positive case worked for me on both counts.
If this looks fine I will send a proper patch
~Vinod
During the testing codec driver probe was failing and this results in wrong reference of platform driver .
On investigating the issue is in soc_bind_dai_link()
It takes the reference of cpu_dai and adds the dai in the runtime structure. But since the codec was not found these references are kept and modules cant be unloaded.
Now thinking on this and other reference patch recently applied, IMO we can instead of taking the module reference check if cpu_dai is probed. If probed then only go ahead otherwise return error
This won't work either as cpu_dai is probed after bind_dai_link()
This issue is on removal of soc_remove() it doesn't free up the reference if the card is not instantiated So in case one of the probes fails we should handle this in soc_remove in else for instantated
if (rtd->codec->probed) module_put(rtd->codec->dev->driver->owner); if (rtd->platform->probed) module_put(rtd->platform->dev->driver->owner); for (i = 0; i < card->num_rtd; i++) module_put(rtd[i].cpu_dai->dev->driver->owner);
Would this be the correct way to solve this?
~Vinod
On Thu, Jan 06, 2011 at 02:41:29PM +0530, Koul, Vinod wrote:
This issue is on removal of soc_remove() it doesn't free up the reference if the card is not instantiated So in case one of the probes fails we should handle this in soc_remove in else for instantated
if (rtd->codec->probed) module_put(rtd->codec->dev->driver->owner); if (rtd->platform->probed) module_put(rtd->platform->dev->driver->owner); for (i = 0; i < card->num_rtd; i++) module_put(rtd[i].cpu_dai->dev->driver->owner);
Would this be the correct way to solve this?
Probably, yes.
participants (3)
-
Harsha, Priya -
Koul, Vinod -
Mark Brown