regarding references taken on platform and codec driver modules

regarding references taken on platform and codec driver modules

[Harsha, Priya]

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

Re: regarding references taken on platform and codec driver modules

[Mark Brown]

On Mon, Jan 03, 2011 at 04:30:35PM +0530, Harsha, Priya wrote:

> 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.

Re: regarding references taken on platform and codec driver modules

[Harsha, Priya]

>> 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

Re: regarding references taken on platform and codec driver modules

[Mark Brown]

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.

Re: regarding references taken on platform and codec driver modules

[Harsha, Priya]

>> 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 Koul<vinod.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

Re: regarding references taken on platform and codec driver modules

[Mark Brown]

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 Koul<vinod.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:
>  	/* 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
> 

Re: regarding references taken on platform and codec driver modules

[Harsha, Priya]

>> 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.

Re: regarding references taken on platform and codec driver modules

[Koul, Vinod]

> 
> >> 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

Re: regarding references taken on platform and codec driver modules

[Koul, 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

Re: regarding references taken on platform and codec driver modules

[Mark Brown]

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.

Re: regarding references taken on platform and codec driver modules

[Harsha, Priya]

>Hi,
>
>In soc-core.c, I see that try_module_get() is used to get reference count on the
>codec and platform drivers. But I don't see try_module_put() to remove the
>reference count after usage.
>
>Because of this, my reference count on codec and platform driver is never
>reduced and I am not able to do rmmod of these drivers. But I can rmmod the
>machine driver as there are no reference count taken on it.
>
>Is this intentional to avoid rmmod of these modules? Or this is to be fixed
>
Please ignore this email.
>-Harsha

regarding references taken on platform and codec driver modules

[Harsha, Priya]

Hi,

In soc-core.c, I see that try_module_get() is used to get reference count on the
codec and platform drivers. But I don't see try_module_put() to remove the
reference count after usage.

Because of this, my reference count on codec and platform driver is never
reduced and I am not able to do rmmod of these drivers. But I can rmmod the
machine driver as there are no reference count taken on it.

Is this intentional to avoid rmmod of these modules? Or this is to be fixed

-Harsha