diff mbox series

[v2] ASoC: amd: acp: Initialize list to store acp_stream during pcm_open

Message ID 20220722133530.3314087-1-venkataprasad.potturu@amd.com (mailing list archive)
State Superseded
Headers show
Series [v2] ASoC: amd: acp: Initialize list to store acp_stream during pcm_open | expand

Commit Message

Venkata Prasad Potturu July 22, 2022, 1:35 p.m. UTC 
From: Ajit Kumar Pandey <AjitKumar.Pandey@amd.com>

We are currently allocating acp_stream during pcm_open and saving
it in static array corresponds to array index calculated based on
cpu dai->driver id. This approach will fail if we have single dai
linked to multiple pcm device as we will have same dai->driver id
or array index for multiple pcm open. Initialize new linked list
stream_list to store opened pcm stream info dynamically.

Signed-off-by: Ajit Kumar Pandey <AjitKumar.Pandey@amd.com>
Signed-off-by: Venkata Prasad Potturu <venkataprasad.potturu@amd.com>
Reviewed-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---

Changes since v1:
    -- Fix compile error and remove unused variable.

 sound/soc/amd/acp/acp-platform.c | 33 ++++++++++++--------------------
 sound/soc/amd/acp/amd.h          |  3 ++-
 2 files changed, 14 insertions(+), 22 deletions(-)

Comments

Takashi Iwai July 22, 2022, 1:41 p.m. UTC | #1 
On Fri, 22 Jul 2022 15:35:22 +0200,
Venkata Prasad Potturu wrote:
> 
> From: Ajit Kumar Pandey <AjitKumar.Pandey@amd.com>
> 
> We are currently allocating acp_stream during pcm_open and saving
> it in static array corresponds to array index calculated based on
> cpu dai->driver id. This approach will fail if we have single dai
> linked to multiple pcm device as we will have same dai->driver id
> or array index for multiple pcm open. Initialize new linked list
> stream_list to store opened pcm stream info dynamically.

If an IRQ handler refers to the linked list, make sure that no list
change will happen concurrently during the IRQ handling.  It seems
that you have no protection for it yet.


thanks,

Takashi

> 
> Signed-off-by: Ajit Kumar Pandey <AjitKumar.Pandey@amd.com>
> Signed-off-by: Venkata Prasad Potturu <venkataprasad.potturu@amd.com>
> Reviewed-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
> ---
> 
> Changes since v1:
>     -- Fix compile error and remove unused variable.
> 
>  sound/soc/amd/acp/acp-platform.c | 33 ++++++++++++--------------------
>  sound/soc/amd/acp/amd.h          |  3 ++-
>  2 files changed, 14 insertions(+), 22 deletions(-)
> 
> diff --git a/sound/soc/amd/acp/acp-platform.c b/sound/soc/amd/acp/acp-platform.c
> index 10730d33c3b0..20c0f75f7c97 100644
> --- a/sound/soc/amd/acp/acp-platform.c
> +++ b/sound/soc/amd/acp/acp-platform.c
> @@ -94,7 +94,7 @@ static irqreturn_t i2s_irq_handler(int irq, void *data)
>  	struct acp_resource *rsrc = adata->rsrc;
>  	struct acp_stream *stream;
>  	u16 i2s_flag = 0;
> -	u32 ext_intr_stat, ext_intr_stat1, i;
> +	u32 ext_intr_stat, ext_intr_stat1;
>  
>  	if (!adata)
>  		return IRQ_NONE;
> @@ -104,8 +104,7 @@ static irqreturn_t i2s_irq_handler(int irq, void *data)
>  
>  	ext_intr_stat = readl(ACP_EXTERNAL_INTR_STAT(adata, rsrc->irqp_used));
>  
> -	for (i = 0; i < ACP_MAX_STREAM; i++) {
> -		stream = adata->stream[i];
> +	list_for_each_entry(stream, &adata->stream_list, list) {
>  		if (stream && (ext_intr_stat & stream->irq_bit)) {
>  			writel(stream->irq_bit,
>  			       ACP_EXTERNAL_INTR_STAT(adata, rsrc->irqp_used));
> @@ -146,9 +145,8 @@ static void config_pte_for_stream(struct acp_dev_data *adata, struct acp_stream
>  	writel(0x01, adata->acp_base + ACPAXI2AXI_ATU_CTRL);
>  }
>  
> -static void config_acp_dma(struct acp_dev_data *adata, int cpu_id, int size)
> +static void config_acp_dma(struct acp_dev_data *adata, struct acp_stream *stream, int size)
>  {
> -	struct acp_stream *stream = adata->stream[cpu_id];
>  	struct snd_pcm_substream *substream = stream->substream;
>  	struct acp_resource *rsrc = adata->rsrc;
>  	dma_addr_t addr = substream->dma_buffer.addr;
> @@ -174,13 +172,10 @@ static void config_acp_dma(struct acp_dev_data *adata, int cpu_id, int size)
>  
>  static int acp_dma_open(struct snd_soc_component *component, struct snd_pcm_substream *substream)
>  {
> -	struct snd_soc_pcm_runtime *soc_runtime = asoc_substream_to_rtd(substream);
> -	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(soc_runtime, 0);
>  	struct snd_pcm_runtime *runtime = substream->runtime;
>  	struct device *dev = component->dev;
>  	struct acp_dev_data *adata = dev_get_drvdata(dev);
>  	struct acp_stream *stream;
> -	int stream_id = cpu_dai->driver->id * 2 + substream->stream;
>  	int ret;
>  
>  	stream = kzalloc(sizeof(*stream), GFP_KERNEL);
> @@ -188,7 +183,8 @@ static int acp_dma_open(struct snd_soc_component *component, struct snd_pcm_subs
>  		return -ENOMEM;
>  
>  	stream->substream = substream;
> -	adata->stream[stream_id] = stream;
> +
> +	list_add_tail(&stream->list, &adata->stream_list);
>  
>  	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>  		runtime->hw = acp_pcm_hardware_playback;
> @@ -212,16 +208,13 @@ static int acp_dma_hw_params(struct snd_soc_component *component,
>  			     struct snd_pcm_substream *substream,
>  			     struct snd_pcm_hw_params *params)
>  {
> -	struct snd_soc_pcm_runtime *soc_runtime = asoc_substream_to_rtd(substream);
>  	struct acp_dev_data *adata = snd_soc_component_get_drvdata(component);
> -	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(soc_runtime, 0);
>  	struct acp_stream *stream = substream->runtime->private_data;
> -	int stream_id = cpu_dai->driver->id * 2 + substream->stream;
>  	u64 size = params_buffer_bytes(params);
>  
>  	/* Configure ACP DMA block with params */
>  	config_pte_for_stream(adata, stream);
> -	config_acp_dma(adata, stream_id, size);
> +	config_acp_dma(adata, stream, size);
>  
>  	return 0;
>  }
> @@ -261,16 +254,11 @@ static int acp_dma_new(struct snd_soc_component *component,
>  static int acp_dma_close(struct snd_soc_component *component,
>  			 struct snd_pcm_substream *substream)
>  {
> -	struct snd_soc_pcm_runtime *soc_runtime = asoc_substream_to_rtd(substream);
> -	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(soc_runtime, 0);
> -	struct device *dev = component->dev;
> -	struct acp_dev_data *adata = dev_get_drvdata(dev);
> -	struct acp_stream *stream;
> -	int stream_id = cpu_dai->driver->id * 2 + substream->stream;
> +	struct acp_stream *stream = substream->runtime->private_data;
>  
> -	stream = adata->stream[stream_id];
> +	/* Remove entry from list */
> +	list_del(&stream->list);
>  	kfree(stream);
> -	adata->stream[stream_id] = NULL;
>  
>  	return 0;
>  }
> @@ -305,6 +293,9 @@ int acp_platform_register(struct device *dev)
>  		dev_err(dev, "Fail to register acp i2s component\n");
>  		return status;
>  	}
> +
> +	INIT_LIST_HEAD(&adata->stream_list);
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_NS_GPL(acp_platform_register, SND_SOC_ACP_COMMON);
> diff --git a/sound/soc/amd/acp/amd.h b/sound/soc/amd/acp/amd.h
> index af9603724a68..148a9ab6206d 100644
> --- a/sound/soc/amd/acp/amd.h
> +++ b/sound/soc/amd/acp/amd.h
> @@ -91,6 +91,7 @@ struct acp_chip_info {
>  };
>  
>  struct acp_stream {
> +	struct list_head list;
>  	struct snd_pcm_substream *substream;
>  	int irq_bit;
>  	int dai_id;
> @@ -123,7 +124,7 @@ struct acp_dev_data {
>  	struct snd_soc_dai_driver *dai_driver;
>  	int num_dai;
>  
> -	struct acp_stream *stream[ACP_MAX_STREAM];
> +	struct list_head stream_list;
>  
>  	struct snd_soc_acpi_mach *machines;
>  	struct platform_device *mach_dev;
> -- 
> 2.25.1
>
Venkata Prasad Potturu July 28, 2022, 7:52 a.m. UTC | #2 
[AMD Official Use Only - General]



On 7/22/2022 7:11 PM, Takashi Iwai wrote:
Thanks for your time.

[CAUTION: External Email]



On Fri, 22 Jul 2022 15:35:22 +0200,

Venkata Prasad Potturu wrote:



From: Ajit Kumar Pandey <AjitKumar.Pandey@amd.com><mailto:AjitKumar.Pandey@amd.com>



We are currently allocating acp_stream during pcm_open and saving

it in static array corresponds to array index calculated based on

cpu dai->driver id. This approach will fail if we have single dai

linked to multiple pcm device as we will have same dai->driver id

or array index for multiple pcm open. Initialize new linked list

stream_list to store opened pcm stream info dynamically.



If an IRQ handler refers to the linked list, make sure that no list

change will happen concurrently during the IRQ handling.  It seems

that you have no protection for it yet.
Yes, linked list may changes concurrently during IRQ handling, we will use spin locks in the next version of patch.








thanks,



Takashi





Signed-off-by: Ajit Kumar Pandey <AjitKumar.Pandey@amd.com><mailto:AjitKumar.Pandey@amd.com>

Signed-off-by: Venkata Prasad Potturu <venkataprasad.potturu@amd.com><mailto:venkataprasad.potturu@amd.com>

Reviewed-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com><mailto:Vijendar.Mukunda@amd.com>

---



Changes since v1:

    -- Fix compile error and remove unused variable.



 sound/soc/amd/acp/acp-platform.c | 33 ++++++++++++--------------------

 sound/soc/amd/acp/amd.h          |  3 ++-

 2 files changed, 14 insertions(+), 22 deletions(-)



diff --git a/sound/soc/amd/acp/acp-platform.c b/sound/soc/amd/acp/acp-platform.c

index 10730d33c3b0..20c0f75f7c97 100644

--- a/sound/soc/amd/acp/acp-platform.c

+++ b/sound/soc/amd/acp/acp-platform.c

@@ -94,7 +94,7 @@ static irqreturn_t i2s_irq_handler(int irq, void *data)

      struct acp_resource *rsrc = adata->rsrc;

      struct acp_stream *stream;

      u16 i2s_flag = 0;

-     u32 ext_intr_stat, ext_intr_stat1, i;

+     u32 ext_intr_stat, ext_intr_stat1;



      if (!adata)

              return IRQ_NONE;

@@ -104,8 +104,7 @@ static irqreturn_t i2s_irq_handler(int irq, void *data)



      ext_intr_stat = readl(ACP_EXTERNAL_INTR_STAT(adata, rsrc->irqp_used));



-     for (i = 0; i < ACP_MAX_STREAM; i++) {

-             stream = adata->stream[i];

+     list_for_each_entry(stream, &adata->stream_list, list) {

              if (stream && (ext_intr_stat & stream->irq_bit)) {

                      writel(stream->irq_bit,

                             ACP_EXTERNAL_INTR_STAT(adata, rsrc->irqp_used));

@@ -146,9 +145,8 @@ static void config_pte_for_stream(struct acp_dev_data *adata, struct acp_stream

      writel(0x01, adata->acp_base + ACPAXI2AXI_ATU_CTRL);

 }



-static void config_acp_dma(struct acp_dev_data *adata, int cpu_id, int size)

+static void config_acp_dma(struct acp_dev_data *adata, struct acp_stream *stream, int size)

 {

-     struct acp_stream *stream = adata->stream[cpu_id];

      struct snd_pcm_substream *substream = stream->substream;

      struct acp_resource *rsrc = adata->rsrc;

      dma_addr_t addr = substream->dma_buffer.addr;

@@ -174,13 +172,10 @@ static void config_acp_dma(struct acp_dev_data *adata, int cpu_id, int size)



 static int acp_dma_open(struct snd_soc_component *component, struct snd_pcm_substream *substream)

 {

-     struct snd_soc_pcm_runtime *soc_runtime = asoc_substream_to_rtd(substream);

-     struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(soc_runtime, 0);

      struct snd_pcm_runtime *runtime = substream->runtime;

      struct device *dev = component->dev;

      struct acp_dev_data *adata = dev_get_drvdata(dev);

      struct acp_stream *stream;

-     int stream_id = cpu_dai->driver->id * 2 + substream->stream;

      int ret;



      stream = kzalloc(sizeof(*stream), GFP_KERNEL);

@@ -188,7 +183,8 @@ static int acp_dma_open(struct snd_soc_component *component, struct snd_pcm_subs

              return -ENOMEM;



      stream->substream = substream;

-     adata->stream[stream_id] = stream;

+

+     list_add_tail(&stream->list, &adata->stream_list);



      if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)

              runtime->hw = acp_pcm_hardware_playback;

@@ -212,16 +208,13 @@ static int acp_dma_hw_params(struct snd_soc_component *component,

                           struct snd_pcm_substream *substream,

                           struct snd_pcm_hw_params *params)

 {

-     struct snd_soc_pcm_runtime *soc_runtime = asoc_substream_to_rtd(substream);

      struct acp_dev_data *adata = snd_soc_component_get_drvdata(component);

-     struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(soc_runtime, 0);

      struct acp_stream *stream = substream->runtime->private_data;

-     int stream_id = cpu_dai->driver->id * 2 + substream->stream;

      u64 size = params_buffer_bytes(params);



      /* Configure ACP DMA block with params */

      config_pte_for_stream(adata, stream);

-     config_acp_dma(adata, stream_id, size);

+     config_acp_dma(adata, stream, size);



      return 0;

 }

@@ -261,16 +254,11 @@ static int acp_dma_new(struct snd_soc_component *component,

 static int acp_dma_close(struct snd_soc_component *component,

                       struct snd_pcm_substream *substream)

 {

-     struct snd_soc_pcm_runtime *soc_runtime = asoc_substream_to_rtd(substream);

-     struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(soc_runtime, 0);

-     struct device *dev = component->dev;

-     struct acp_dev_data *adata = dev_get_drvdata(dev);

-     struct acp_stream *stream;

-     int stream_id = cpu_dai->driver->id * 2 + substream->stream;

+     struct acp_stream *stream = substream->runtime->private_data;



-     stream = adata->stream[stream_id];

+     /* Remove entry from list */

+     list_del(&stream->list);

      kfree(stream);

-     adata->stream[stream_id] = NULL;



      return 0;

 }

@@ -305,6 +293,9 @@ int acp_platform_register(struct device *dev)

              dev_err(dev, "Fail to register acp i2s component\n");

              return status;

      }

+

+     INIT_LIST_HEAD(&adata->stream_list);

+

      return 0;

 }

 EXPORT_SYMBOL_NS_GPL(acp_platform_register, SND_SOC_ACP_COMMON);

diff --git a/sound/soc/amd/acp/amd.h b/sound/soc/amd/acp/amd.h

index af9603724a68..148a9ab6206d 100644

--- a/sound/soc/amd/acp/amd.h

+++ b/sound/soc/amd/acp/amd.h

@@ -91,6 +91,7 @@ struct acp_chip_info {

 };



 struct acp_stream {

+     struct list_head list;

      struct snd_pcm_substream *substream;

      int irq_bit;

      int dai_id;

@@ -123,7 +124,7 @@ struct acp_dev_data {

      struct snd_soc_dai_driver *dai_driver;

      int num_dai;



-     struct acp_stream *stream[ACP_MAX_STREAM];

+     struct list_head stream_list;



      struct snd_soc_acpi_mach *machines;

      struct platform_device *mach_dev;

--

2.25.1
Mark Brown July 28, 2022, 11:14 a.m. UTC | #3 
On Thu, Jul 28, 2022 at 07:52:25AM +0000, Prasad, Prasad wrote:

> linked to multiple pcm device as we will have same dai->driver id
> 
> or array index for multiple pcm open. Initialize new linked list
> 
> stream_list to store opened pcm stream info dynamically.
> 
> 
> 
> If an IRQ handler refers to the linked list, make sure that no list
> 
> change will happen concurrently during the IRQ handling.  It seems
> 
> that you have no protection for it yet.
> Yes, linked list may changes concurrently during IRQ handling, we will use spin locks in the next version of patch.

Please fix your mail client to clearly identify quoted text, it's very
hard to read your mails since (among other things) it's hard to tell
what text you added.
diff mbox series

Patch

diff --git a/sound/soc/amd/acp/acp-platform.c b/sound/soc/amd/acp/acp-platform.c
index 10730d33c3b0..20c0f75f7c97 100644
--- a/sound/soc/amd/acp/acp-platform.c
+++ b/sound/soc/amd/acp/acp-platform.c
@@ -94,7 +94,7 @@  static irqreturn_t i2s_irq_handler(int irq, void *data)
 	struct acp_resource *rsrc = adata->rsrc;
 	struct acp_stream *stream;
 	u16 i2s_flag = 0;
-	u32 ext_intr_stat, ext_intr_stat1, i;
+	u32 ext_intr_stat, ext_intr_stat1;
 
 	if (!adata)
 		return IRQ_NONE;
@@ -104,8 +104,7 @@  static irqreturn_t i2s_irq_handler(int irq, void *data)
 
 	ext_intr_stat = readl(ACP_EXTERNAL_INTR_STAT(adata, rsrc->irqp_used));
 
-	for (i = 0; i < ACP_MAX_STREAM; i++) {
-		stream = adata->stream[i];
+	list_for_each_entry(stream, &adata->stream_list, list) {
 		if (stream && (ext_intr_stat & stream->irq_bit)) {
 			writel(stream->irq_bit,
 			       ACP_EXTERNAL_INTR_STAT(adata, rsrc->irqp_used));
@@ -146,9 +145,8 @@  static void config_pte_for_stream(struct acp_dev_data *adata, struct acp_stream
 	writel(0x01, adata->acp_base + ACPAXI2AXI_ATU_CTRL);
 }
 
-static void config_acp_dma(struct acp_dev_data *adata, int cpu_id, int size)
+static void config_acp_dma(struct acp_dev_data *adata, struct acp_stream *stream, int size)
 {
-	struct acp_stream *stream = adata->stream[cpu_id];
 	struct snd_pcm_substream *substream = stream->substream;
 	struct acp_resource *rsrc = adata->rsrc;
 	dma_addr_t addr = substream->dma_buffer.addr;
@@ -174,13 +172,10 @@  static void config_acp_dma(struct acp_dev_data *adata, int cpu_id, int size)
 
 static int acp_dma_open(struct snd_soc_component *component, struct snd_pcm_substream *substream)
 {
-	struct snd_soc_pcm_runtime *soc_runtime = asoc_substream_to_rtd(substream);
-	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(soc_runtime, 0);
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct device *dev = component->dev;
 	struct acp_dev_data *adata = dev_get_drvdata(dev);
 	struct acp_stream *stream;
-	int stream_id = cpu_dai->driver->id * 2 + substream->stream;
 	int ret;
 
 	stream = kzalloc(sizeof(*stream), GFP_KERNEL);
@@ -188,7 +183,8 @@  static int acp_dma_open(struct snd_soc_component *component, struct snd_pcm_subs
 		return -ENOMEM;
 
 	stream->substream = substream;
-	adata->stream[stream_id] = stream;
+
+	list_add_tail(&stream->list, &adata->stream_list);
 
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 		runtime->hw = acp_pcm_hardware_playback;
@@ -212,16 +208,13 @@  static int acp_dma_hw_params(struct snd_soc_component *component,
 			     struct snd_pcm_substream *substream,
 			     struct snd_pcm_hw_params *params)
 {
-	struct snd_soc_pcm_runtime *soc_runtime = asoc_substream_to_rtd(substream);
 	struct acp_dev_data *adata = snd_soc_component_get_drvdata(component);
-	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(soc_runtime, 0);
 	struct acp_stream *stream = substream->runtime->private_data;
-	int stream_id = cpu_dai->driver->id * 2 + substream->stream;
 	u64 size = params_buffer_bytes(params);
 
 	/* Configure ACP DMA block with params */
 	config_pte_for_stream(adata, stream);
-	config_acp_dma(adata, stream_id, size);
+	config_acp_dma(adata, stream, size);
 
 	return 0;
 }
@@ -261,16 +254,11 @@  static int acp_dma_new(struct snd_soc_component *component,
 static int acp_dma_close(struct snd_soc_component *component,
 			 struct snd_pcm_substream *substream)
 {
-	struct snd_soc_pcm_runtime *soc_runtime = asoc_substream_to_rtd(substream);
-	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(soc_runtime, 0);
-	struct device *dev = component->dev;
-	struct acp_dev_data *adata = dev_get_drvdata(dev);
-	struct acp_stream *stream;
-	int stream_id = cpu_dai->driver->id * 2 + substream->stream;
+	struct acp_stream *stream = substream->runtime->private_data;
 
-	stream = adata->stream[stream_id];
+	/* Remove entry from list */
+	list_del(&stream->list);
 	kfree(stream);
-	adata->stream[stream_id] = NULL;
 
 	return 0;
 }
@@ -305,6 +293,9 @@  int acp_platform_register(struct device *dev)
 		dev_err(dev, "Fail to register acp i2s component\n");
 		return status;
 	}
+
+	INIT_LIST_HEAD(&adata->stream_list);
+
 	return 0;
 }
 EXPORT_SYMBOL_NS_GPL(acp_platform_register, SND_SOC_ACP_COMMON);
diff --git a/sound/soc/amd/acp/amd.h b/sound/soc/amd/acp/amd.h
index af9603724a68..148a9ab6206d 100644
--- a/sound/soc/amd/acp/amd.h
+++ b/sound/soc/amd/acp/amd.h
@@ -91,6 +91,7 @@  struct acp_chip_info {
 };
 
 struct acp_stream {
+	struct list_head list;
 	struct snd_pcm_substream *substream;
 	int irq_bit;
 	int dai_id;
@@ -123,7 +124,7 @@  struct acp_dev_data {
 	struct snd_soc_dai_driver *dai_driver;
 	int num_dai;
 
-	struct acp_stream *stream[ACP_MAX_STREAM];
+	struct list_head stream_list;
 
 	struct snd_soc_acpi_mach *machines;
 	struct platform_device *mach_dev;