[PATCH] ASoC don't clobber platform DMA driver ops

[PATCH] ASoC don't clobber platform DMA driver ops

[Alan Tull]

Previously, only one static struct for ops existed for all
platform DMA drivers to share.  Half of the ops are shared
functions and the other half are initialized to point directly
to ops in the platform driver.  This creates problems where
each time soc_new_pcm is called, the new platform driver's
ops would overwrite a subset of the ops.

This patch creates and populates a ops struct for each call
to soc_pcm_new.

Signed-off-by: Alan Tull <alan.tull@freescale.com>
---
 sound/soc/soc-core.c |    1 +
 sound/soc/soc-pcm.c  |   48 ++++++++++++++++++++++++++++--------------------
 2 files changed, 29 insertions(+), 20 deletions(-)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index b085d8e..1b48fc6 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -1547,6 +1547,7 @@ static int soc_cleanup_card_resources(struct snd_soc_card *card)
 
 	snd_soc_dapm_free(&card->dapm);
 
+	kfree(card->rtd->ops);
 	kfree(card->rtd);
 	snd_card_free(card->snd_card);
 	return 0;
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 2879c88..091c074 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -567,17 +567,6 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream)
 	return offset;
 }
 
-/* ASoC PCM operations */
-static struct snd_pcm_ops soc_pcm_ops = {
-	.open		= soc_pcm_open,
-	.close		= soc_pcm_close,
-	.hw_params	= soc_pcm_hw_params,
-	.hw_free	= soc_pcm_hw_free,
-	.prepare	= soc_pcm_prepare,
-	.trigger	= soc_pcm_trigger,
-	.pointer	= soc_pcm_pointer,
-};
-
 /* create a new pcm */
 int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
 {
@@ -586,9 +575,17 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
 	struct snd_soc_dai *codec_dai = rtd->codec_dai;
 	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
 	struct snd_pcm *pcm;
+	struct snd_pcm_ops *soc_pcm_ops;
 	char new_name[64];
 	int ret = 0, playback = 0, capture = 0;
 
+	/* ASoC PCM operations */
+	soc_pcm_ops = kzalloc(sizeof(struct snd_pcm_ops), GFP_KERNEL);
+	if (soc_pcm_ops == NULL) {
+		printk(KERN_ERR "asoc: can't create pcm ops for codec %s\n", codec->name);
+		return -ENOMEM;
+	}
+
 	/* check client and interface hw capabilities */
 	snprintf(new_name, sizeof(new_name), "%s %s-%d",
 			rtd->dai_link->stream_name, codec_dai->name, num);
@@ -603,34 +600,45 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
 			num, playback, capture, &pcm);
 	if (ret < 0) {
 		printk(KERN_ERR "asoc: can't create pcm for codec %s\n", codec->name);
+		kfree(soc_pcm_ops);
 		return ret;
 	}
 
 	/* DAPM dai link stream work */
 	INIT_DELAYED_WORK(&rtd->delayed_work, close_delayed_work);
 
+	soc_pcm_ops->open = soc_pcm_open;
+	soc_pcm_ops->close = soc_codec_close;
+	soc_pcm_ops->hw_params = soc_pcm_hw_params;
+	soc_pcm_ops->hw_free = soc_pcm_hw_free;
+	soc_pcm_ops->prepare = soc_pcm_prepare;
+	soc_pcm_ops->pointer = soc_pcm_pointer;
+	soc_pcm_ops->trigger = soc_pcm_trigger;
+
 	rtd->pcm = pcm;
 	pcm->private_data = rtd;
 	if (platform->driver->ops) {
-		soc_pcm_ops.mmap = platform->driver->ops->mmap;
-		soc_pcm_ops.pointer = platform->driver->ops->pointer;
-		soc_pcm_ops.ioctl = platform->driver->ops->ioctl;
-		soc_pcm_ops.copy = platform->driver->ops->copy;
-		soc_pcm_ops.silence = platform->driver->ops->silence;
-		soc_pcm_ops.ack = platform->driver->ops->ack;
-		soc_pcm_ops.page = platform->driver->ops->page;
+		soc_pcm_ops->mmap = platform->driver->ops->mmap;
+		soc_pcm_ops->ioctl = platform->driver->ops->ioctl;
+		soc_pcm_ops->copy = platform->driver->ops->copy;
+		soc_pcm_ops->silence = platform->driver->ops->silence;
+		soc_pcm_ops->ack = platform->driver->ops->ack;
+		soc_pcm_ops->page = platform->driver->ops->page;
 	}
 
+	rtd->ops = soc_pcm_ops;
+
 	if (playback)
-		snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &soc_pcm_ops);
+		snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, soc_pcm_ops);
 
 	if (capture)
-		snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE, &soc_pcm_ops);
+		snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE, soc_pcm_ops);
 
 	if (platform->driver->pcm_new) {
 		ret = platform->driver->pcm_new(rtd);
 		if (ret < 0) {
 			pr_err("asoc: platform pcm constructor failed\n");
+			kfree(rtd->ops);
 			return ret;
 		}
 	}
-- 
1.6.0.4

Re: [PATCH] ASoC don't clobber platform DMA driver ops

[Mark Brown]

On Tue, Nov 01, 2011 at 06:46:52PM +0000, Tull Alan-R80115 wrote:

> This patch creates and populates a ops struct for each call to soc_pcm_new.

This isn't very idiomatic - a much more idiomatic solution would be to
add new indirection functions which do the lookups.  While it's probably
not going to make much difference for most things there's operations
like ioctl() which definitely don't look at all DMA specific in there.

Re: [PATCH] ASoC don't clobber platform DMA driver ops

[Mark Brown]

On Tue, Nov 01, 2011 at 06:57:47PM +0000, Tull Alan-R80115 wrote:

On public Linux lists you should never top post (so people have context
and know what you're talking about) and you should fix your mail client
to word wrap within paragraphs.

> I originally started down the path you are describing, but quickly ran
> into problems.  I started by adding a soc_pcm_copy and soc_pcm_mmap
> and found that I needed to pull code from ALSA core in to handle the
> case where copy or mmap was NULL for a given platform driver.  

In that case your changelog needs to explain this and I'd still use a
function for those ops that don't have stubs in the core.  Things like
ioctl() could usefully be routed to all the drivers, for example.

[PATCH] ASoC don't clobber platform DMA driver ops

[Alan Tull]

Previously, only one static struct for ops existed for all
platform DMA drivers to share.  Half of the ops are shared
functions which don't have stubs in the ALSA core.  The
other half are initialized to point directly to ops in the
platform driver.  This creates problems where each time
soc_new_pcm is called, the new platform driver's ops would
overwrite a subset of the ops.

This patch creates and populates a ops struct for each call
to soc_pcm_new.  Also creates a common soc_pcm_ioctl function
that calls all the driver components for ioctl calls.

Signed-off-by: Alan Tull <alan.tull@freescale.com>
---
 include/sound/soc-dai.h |    2 +
 include/sound/soc.h     |    1 +
 sound/soc/soc-core.c    |    1 +
 sound/soc/soc-pcm.c     |   80 +++++++++++++++++++++++++++++++++++-----------
 4 files changed, 65 insertions(+), 19 deletions(-)

diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
index 5ad5f3a..b3f880e 100644
--- a/include/sound/soc-dai.h
+++ b/include/sound/soc-dai.h
@@ -172,6 +172,8 @@ struct snd_soc_dai_ops {
 		struct snd_soc_dai *);
 	int (*trigger)(struct snd_pcm_substream *, int,
 		struct snd_soc_dai *);
+	int (*ioctl)(struct snd_pcm_substream *, unsigned int, void *,
+		struct snd_soc_dai *);
 	/*
 	 * For hardware based FIFO caused delay reporting.
 	 * Optional.
diff --git a/include/sound/soc.h b/include/sound/soc.h
index aa19f5a..1574325 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -517,6 +517,7 @@ struct snd_soc_ops {
 	int (*hw_free)(struct snd_pcm_substream *);
 	int (*prepare)(struct snd_pcm_substream *);
 	int (*trigger)(struct snd_pcm_substream *, int);
+	int (*ioctl)(struct snd_pcm_substream *, unsigned int, void *);
 };
 
 /* SoC cache ops */
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index b085d8e..1b48fc6 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -1547,6 +1547,7 @@ static int soc_cleanup_card_resources(struct snd_soc_card *card)
 
 	snd_soc_dapm_free(&card->dapm);
 
+	kfree(card->rtd->ops);
 	kfree(card->rtd);
 	snd_card_free(card->snd_card);
 	return 0;
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 2879c88..2bd7504 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -567,16 +567,39 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream)
 	return offset;
 }
 
-/* ASoC PCM operations */
-static struct snd_pcm_ops soc_pcm_ops = {
-	.open		= soc_pcm_open,
-	.close		= soc_pcm_close,
-	.hw_params	= soc_pcm_hw_params,
-	.hw_free	= soc_pcm_hw_free,
-	.prepare	= soc_pcm_prepare,
-	.trigger	= soc_pcm_trigger,
-	.pointer	= soc_pcm_pointer,
-};
+static int soc_pcm_ioctl(struct snd_pcm_substream *substream, unsigned int cmd, void *arg)
+{
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct snd_soc_platform *platform = rtd->platform;
+	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+	struct snd_soc_dai *codec_dai = rtd->codec_dai;
+	int ret;
+
+	if (rtd->dai_link->ops && rtd->dai_link->ops->ioctl) {
+		ret = rtd->dai_link->ops->ioctl(substream, cmd, arg);
+		if (ret < 0)
+			return ret;
+	}
+
+	if (codec_dai->driver->ops->ioctl) {
+		ret = codec_dai->driver->ops->ioctl(substream, cmd, arg, codec_dai);
+		if (ret < 0)
+			return ret;
+	}
+
+	if (platform->driver->ops->ioctl) {
+		ret = platform->driver->ops->ioctl(substream, cmd, arg);
+		if (ret < 0)
+			return ret;
+	}
+
+	if (cpu_dai->driver->ops->ioctl) {
+		ret = cpu_dai->driver->ops->ioctl(substream, cmd, arg, cpu_dai);
+		if (ret < 0)
+			return ret;
+	}
+	return 0;
+}
 
 /* create a new pcm */
 int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
@@ -586,9 +609,17 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
 	struct snd_soc_dai *codec_dai = rtd->codec_dai;
 	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
 	struct snd_pcm *pcm;
+	struct snd_pcm_ops *soc_pcm_ops;
 	char new_name[64];
 	int ret = 0, playback = 0, capture = 0;
 
+	/* ASoC PCM operations */
+	soc_pcm_ops = kzalloc(sizeof(struct snd_pcm_ops), GFP_KERNEL);
+	if (soc_pcm_ops == NULL) {
+		printk(KERN_ERR "asoc: can't create pcm ops for codec %s\n", codec->name);
+		return -ENOMEM;
+	}
+
 	/* check client and interface hw capabilities */
 	snprintf(new_name, sizeof(new_name), "%s %s-%d",
 			rtd->dai_link->stream_name, codec_dai->name, num);
@@ -603,34 +634,45 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
 			num, playback, capture, &pcm);
 	if (ret < 0) {
 		printk(KERN_ERR "asoc: can't create pcm for codec %s\n", codec->name);
+		kfree(soc_pcm_ops);
 		return ret;
 	}
 
 	/* DAPM dai link stream work */
 	INIT_DELAYED_WORK(&rtd->delayed_work, close_delayed_work);
 
+	soc_pcm_ops->open = soc_pcm_open;
+	soc_pcm_ops->close = soc_codec_close;
+	soc_pcm_ops->hw_params = soc_pcm_hw_params;
+	soc_pcm_ops->hw_free = soc_pcm_hw_free;
+	soc_pcm_ops->prepare = soc_pcm_prepare;
+	soc_pcm_ops->pointer = soc_pcm_pointer;
+	soc_pcm_ops->trigger = soc_pcm_trigger;
+	soc_pcm_ops->ioctl = soc_pcm_ioctl;
+
 	rtd->pcm = pcm;
 	pcm->private_data = rtd;
 	if (platform->driver->ops) {
-		soc_pcm_ops.mmap = platform->driver->ops->mmap;
-		soc_pcm_ops.pointer = platform->driver->ops->pointer;
-		soc_pcm_ops.ioctl = platform->driver->ops->ioctl;
-		soc_pcm_ops.copy = platform->driver->ops->copy;
-		soc_pcm_ops.silence = platform->driver->ops->silence;
-		soc_pcm_ops.ack = platform->driver->ops->ack;
-		soc_pcm_ops.page = platform->driver->ops->page;
+		soc_pcm_ops->mmap = platform->driver->ops->mmap;
+		soc_pcm_ops->copy = platform->driver->ops->copy;
+		soc_pcm_ops->silence = platform->driver->ops->silence;
+		soc_pcm_ops->ack = platform->driver->ops->ack;
+		soc_pcm_ops->page = platform->driver->ops->page;
 	}
 
+	rtd->ops = soc_pcm_ops;
+
 	if (playback)
-		snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &soc_pcm_ops);
+		snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, soc_pcm_ops);
 
 	if (capture)
-		snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE, &soc_pcm_ops);
+		snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE, soc_pcm_ops);
 
 	if (platform->driver->pcm_new) {
 		ret = platform->driver->pcm_new(rtd);
 		if (ret < 0) {
 			pr_err("asoc: platform pcm constructor failed\n");
+			kfree(rtd->ops);
 			return ret;
 		}
 	}
-- 
1.6.0.4

Re: [PATCH] ASoC don't clobber platform DMA driver ops

[Tabi Timur-B04825]

On Tue, Nov 1, 2011 at 4:05 PM, Alan Tull <alan.tull@freescale.com> wrote:
>
> This patch creates and populates a ops struct for each call
> to soc_pcm_new.  Also creates a common soc_pcm_ioctl function
> that calls all the driver components for ioctl calls.

Maybe I'm missing something, but it looks like the ioctl stuff is not
related to the ops stuff.  In other words, it looks like this patch
does two completely different things.  Shouldn't it be broken into two
patches?

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] ASoC don't clobber platform DMA driver ops

[Mark Brown]

On Wed, Nov 02, 2011 at 03:40:48AM +0000, Tabi Timur-B04825 wrote:
> On Tue, Nov 1, 2011 at 4:05 PM, Alan Tull <alan.tull@freescale.com> wrote:

> > This patch creates and populates a ops struct for each call
> > to soc_pcm_new. ?Also creates a common soc_pcm_ioctl function
> > that calls all the driver components for ioctl calls.

> Maybe I'm missing something, but it looks like the ioctl stuff is not
> related to the ops stuff.  In other words, it looks like this patch
> does two completely different things.  Shouldn't it be broken into two
> patches?

Ideally.  The relationship is that it's making all the ops per-PCM
overridable rather than just a few as is the case currently.

Re: [PATCH] ASoC don't clobber platform DMA driver ops

[Mark Brown]

On Tue, Nov 01, 2011 at 04:05:16PM -0500, Alan Tull wrote:

This looks mostly good, though as someone pointed out earlier it'd be
nicer to split the ioctl() operation addition out into a separate patch.
One issue, though.

> @@ -1547,6 +1547,7 @@ static int soc_cleanup_card_resources(struct snd_soc_card *card)
>  
>  	snd_soc_dapm_free(&card->dapm);
>  
> +	kfree(card->rtd->ops);

This is going to leak - rtd is an array per PCM, not a single value, so
we need to free the ops for each element.  This is so we can have
multiple DMA controllers in a single card.

Actually what would be even easier would be to just embed the ops in the
struct, we need to allocate one per runtime anyway and it makes for less
allocation and cleanup code.

Re: [PATCH] ASoC don't clobber platform DMA driver ops

[r80115]

On 11/10/2011 5:43 AM, Mark Brown wrote:
> On Tue, Nov 01, 2011 at 04:05:16PM -0500, Alan Tull wrote:
>
> This looks mostly good, though as someone pointed out earlier it'd be
> nicer to split the ioctl() operation addition out into a separate patch.
> One issue, though.
>
>> @@ -1547,6 +1547,7 @@ static int soc_cleanup_card_resources(struct snd_soc_card *card)
>>
>>   	snd_soc_dapm_free(&card->dapm);
>>
>> +	kfree(card->rtd->ops);
>
> This is going to leak - rtd is an array per PCM, not a single value, so
> we need to free the ops for each element.  This is so we can have
> multiple DMA controllers in a single card.
>
> Actually what would be even easier would be to just embed the ops in the
> struct, we need to allocate one per runtime anyway and it makes for less
> allocation and cleanup code.
>

In soc-pcm.c we allocate:
soc_pcm_ops = kzalloc(sizeof(struct snd_pcm_ops), GFP_KERNEL);

and set it to the pointer

rtd->ops = soc_pcm_ops;

So that's what is freed there.

-- 
Regards,
Alan

Re: [PATCH] ASoC don't clobber platform DMA driver ops

[Mark Brown]

On Thu, Nov 10, 2011 at 09:14:25AM -0600, r80115 wrote:
> On 11/10/2011 5:43 AM, Mark Brown wrote:

> >This is going to leak - rtd is an array per PCM, not a single value, so
> >we need to free the ops for each element.  This is so we can have
> >multiple DMA controllers in a single card.

> >Actually what would be even easier would be to just embed the ops in the
> >struct, we need to allocate one per runtime anyway and it makes for less
> >allocation and cleanup code.

> In soc-pcm.c we allocate:
> soc_pcm_ops = kzalloc(sizeof(struct snd_pcm_ops), GFP_KERNEL);

> and set it to the pointer

> rtd->ops = soc_pcm_ops;

> So that's what is freed there.

You're not getting the point here.  The point is that you're only
freeing a single copy of the ops no matter how many are allocated.

Re: [PATCH] ASoC don't clobber platform DMA driver ops

[r80115]

On 11/10/2011 9:16 AM, Mark Brown wrote:
> On Thu, Nov 10, 2011 at 09:14:25AM -0600, r80115 wrote:
>> On 11/10/2011 5:43 AM, Mark Brown wrote:
>
>>> This is going to leak - rtd is an array per PCM, not a single value, so
>>> we need to free the ops for each element.  This is so we can have
>>> multiple DMA controllers in a single card.
>
>>> Actually what would be even easier would be to just embed the ops in the
>>> struct, we need to allocate one per runtime anyway and it makes for less
>>> allocation and cleanup code.
>
>> In soc-pcm.c we allocate:
>> soc_pcm_ops = kzalloc(sizeof(struct snd_pcm_ops), GFP_KERNEL);
>
>> and set it to the pointer
>
>> rtd->ops = soc_pcm_ops;
>
>> So that's what is freed there.
>
> You're not getting the point here.  The point is that you're only
> freeing a single copy of the ops no matter how many are allocated.
>

I see.  This is sounding like a significant rewrite.  I'll have to 
revisit this then.  Thanks for catching that.

-- 
Regards,
Alan

Re: [PATCH] ASoC don't clobber platform DMA driver ops

[Mark Brown]

On Thu, Nov 10, 2011 at 09:20:35AM -0600, r80115 wrote:

> I see.  This is sounding like a significant rewrite.  I'll have to
> revisit this then.  Thanks for catching that.

It should be pretty trivial - just a for loop over all rtds, or even
simpler should be to just embed the ops within the rtd.

[PATCH] ASoC don't clobber platform DMA driver ops

[Alan Tull]

Previously, only one static struct for ops existed for all
platform DMA drivers to share.  Half of the ops are shared
functions and the other half are initialized to point directly
to ops in the platform driver.  This creates problems where
each time soc_new_pcm is called, the new platform driver's
ops would overwrite a subset of the ops.

This patch creates and populates a ops struct for each call
to soc_pcm_new.

Signed-off-by: Alan Tull <alan.tull@freescale.com>
---
 sound/soc/soc-core.c |    6 ++++++
 sound/soc/soc-pcm.c  |   48 ++++++++++++++++++++++++++++--------------------
 2 files changed, 34 insertions(+), 20 deletions(-)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index b085d8e..216803c 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -1541,6 +1541,12 @@ static int soc_cleanup_card_resources(struct snd_soc_card *card)
 
 	soc_cleanup_card_debugfs(card);
 
+	/* free the ops */
+	for (i = 0; i < card->num_rtd; i++) {
+		struct snd_soc_pcm_runtime *rtd = &card->rtd[i];
+		kfree(rtd->ops);
+	}
+
 	/* remove the card */
 	if (card->remove)
 		card->remove(card);
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 2879c88..091c074 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -567,17 +567,6 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream)
 	return offset;
 }
 
-/* ASoC PCM operations */
-static struct snd_pcm_ops soc_pcm_ops = {
-	.open		= soc_pcm_open,
-	.close		= soc_pcm_close,
-	.hw_params	= soc_pcm_hw_params,
-	.hw_free	= soc_pcm_hw_free,
-	.prepare	= soc_pcm_prepare,
-	.trigger	= soc_pcm_trigger,
-	.pointer	= soc_pcm_pointer,
-};
-
 /* create a new pcm */
 int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
 {
@@ -586,9 +575,17 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
 	struct snd_soc_dai *codec_dai = rtd->codec_dai;
 	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
 	struct snd_pcm *pcm;
+	struct snd_pcm_ops *soc_pcm_ops;
 	char new_name[64];
 	int ret = 0, playback = 0, capture = 0;
 
+	/* ASoC PCM operations */
+	soc_pcm_ops = kzalloc(sizeof(struct snd_pcm_ops), GFP_KERNEL);
+	if (soc_pcm_ops == NULL) {
+		printk(KERN_ERR "asoc: can't create pcm ops for codec %s\n", codec->name);
+		return -ENOMEM;
+	}
+
 	/* check client and interface hw capabilities */
 	snprintf(new_name, sizeof(new_name), "%s %s-%d",
 			rtd->dai_link->stream_name, codec_dai->name, num);
@@ -603,34 +600,45 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
 			num, playback, capture, &pcm);
 	if (ret < 0) {
 		printk(KERN_ERR "asoc: can't create pcm for codec %s\n", codec->name);
+		kfree(soc_pcm_ops);
 		return ret;
 	}
 
 	/* DAPM dai link stream work */
 	INIT_DELAYED_WORK(&rtd->delayed_work, close_delayed_work);
 
+	soc_pcm_ops->open = soc_pcm_open;
+	soc_pcm_ops->close = soc_codec_close;
+	soc_pcm_ops->hw_params = soc_pcm_hw_params;
+	soc_pcm_ops->hw_free = soc_pcm_hw_free;
+	soc_pcm_ops->prepare = soc_pcm_prepare;
+	soc_pcm_ops->pointer = soc_pcm_pointer;
+	soc_pcm_ops->trigger = soc_pcm_trigger;
+
 	rtd->pcm = pcm;
 	pcm->private_data = rtd;
 	if (platform->driver->ops) {
-		soc_pcm_ops.mmap = platform->driver->ops->mmap;
-		soc_pcm_ops.pointer = platform->driver->ops->pointer;
-		soc_pcm_ops.ioctl = platform->driver->ops->ioctl;
-		soc_pcm_ops.copy = platform->driver->ops->copy;
-		soc_pcm_ops.silence = platform->driver->ops->silence;
-		soc_pcm_ops.ack = platform->driver->ops->ack;
-		soc_pcm_ops.page = platform->driver->ops->page;
+		soc_pcm_ops->mmap = platform->driver->ops->mmap;
+		soc_pcm_ops->ioctl = platform->driver->ops->ioctl;
+		soc_pcm_ops->copy = platform->driver->ops->copy;
+		soc_pcm_ops->silence = platform->driver->ops->silence;
+		soc_pcm_ops->ack = platform->driver->ops->ack;
+		soc_pcm_ops->page = platform->driver->ops->page;
 	}
 
+	rtd->ops = soc_pcm_ops;
+
 	if (playback)
-		snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &soc_pcm_ops);
+		snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, soc_pcm_ops);
 
 	if (capture)
-		snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE, &soc_pcm_ops);
+		snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE, soc_pcm_ops);
 
 	if (platform->driver->pcm_new) {
 		ret = platform->driver->pcm_new(rtd);
 		if (ret < 0) {
 			pr_err("asoc: platform pcm constructor failed\n");
+			kfree(rtd->ops);
 			return ret;
 		}
 	}
-- 
1.6.0.4

Re: [PATCH] ASoC don't clobber platform DMA driver ops

[Tabi Timur-B04825]

On Tue, Nov 15, 2011 at 9:59 AM, Alan Tull <alan.tull@freescale.com> wrote:
> Previously, only one static struct for ops existed for all
> platform DMA drivers to share.  Half of the ops are shared
> functions and the other half are initialized to point directly
> to ops in the platform driver.  This creates problems where
> each time soc_new_pcm is called, the new platform driver's
> ops would overwrite a subset of the ops.
>
> This patch creates and populates a ops struct for each call
> to soc_pcm_new.
>
> Signed-off-by: Alan Tull <alan.tull@freescale.com>
> ---

Something is wrong.  When I apply this patch, I get this:

  CC      sound/soc/soc-core.o
/home/b04825/git/linux.34/sound/soc/soc-core.c: In function
'soc_cleanup_card_resources':
/home/b04825/git/linux.34/sound/soc/soc-core.c:1583:3: error:
incompatible type for argument 1 of 'kfree'
/home/b04825/git/linux.34/include/linux/slab.h:161:6: note: expected
'const void *' but argument is of type 'struct snd_pcm_ops'

	/* free the ops */
	for (i = 0; i < card->num_rtd; i++) {
		struct snd_soc_pcm_runtime *rtd = &card->rtd[i];
		kfree(rtd->ops);
	}

rtd->ops is not a pointer, so it can't be freed.  Also, this:

+	rtd->ops = soc_pcm_ops;

is a memcpy.  Why not just write to rtd->ops directly?

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] ASoC don't clobber platform DMA driver ops

[r80115]

On 11/15/2011 10:56 AM, Tabi Timur-B04825 wrote:
> Something is wrong.  When I apply this patch, I get this:
>
>    CC      sound/soc/soc-core.o
> /home/b04825/git/linux.34/sound/soc/soc-core.c: In function
> 'soc_cleanup_card_resources':
> /home/b04825/git/linux.34/sound/soc/soc-core.c:1583:3: error:
> incompatible type for argument 1 of 'kfree'
> /home/b04825/git/linux.34/include/linux/slab.h:161:6: note: expected
> 'const void *' but argument is of type 'struct snd_pcm_ops'
>
> 	/* free the ops */
> 	for (i = 0; i<  card->num_rtd; i++) {
> 		struct snd_soc_pcm_runtime *rtd =&card->rtd[i];
> 		kfree(rtd->ops);
> 	}
>
> rtd->ops is not a pointer, so it can't be freed.  Also, this:
>
> +	rtd->ops = soc_pcm_ops;
>
> is a memcpy.  Why not just write to rtd->ops directly?
>


I see the problem.  I implemented this under 2.6.38.  rtd->ops was a 
pointer under 2.6.38.  This could change much then.

-- 
Regards,
Alan