[PATCH 0/5] ASoC: intel: fix to sst driver series

[PATCH 0/5] ASoC: intel: fix to sst driver series

[Vinod Koul]

Hi,

This series fixes comments observed while applying the sst series last week.


Vinod Koul (5):
  ASoC: intel: use __iowrite32_copy for 32 bit copy
  ASoC: intel: log an error on double free
  ASoC: intel: fix the kernldoc comment
  ASoC: intel: explain why block not found isn't error always
  ASoC: intel: use __iowrite32_copy for 32 bit copy

 sound/soc/intel/sst-firmware.c          |    7 ++-----
 sound/soc/intel/sst/sst_drv_interface.c |    4 +++-
 sound/soc/intel/sst/sst_ipc.c           |   15 +++++++++++++++
 sound/soc/intel/sst/sst_loader.c        |    9 +++------
 4 files changed, 23 insertions(+), 12 deletions(-)

[PATCH 1/5] ASoC: intel: use __iowrite32_copy for 32 bit copy

[Vinod Koul]

The driver was using own method to do 32bit copy, turns out we have a kernel
API so use that instead

Tested-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/soc/intel/sst/sst_loader.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/sound/soc/intel/sst/sst_loader.c b/sound/soc/intel/sst/sst_loader.c
index b6d27c1..40e501b 100644
--- a/sound/soc/intel/sst/sst_loader.c
+++ b/sound/soc/intel/sst/sst_loader.c
@@ -41,12 +41,9 @@
 
 static void memcpy32_toio(void __iomem *dst, const void *src, int count)
 {
-	int i;
-	const u32 *src_32 = src;
-	u32 *dst_32 = dst;
-
-	for (i = 0; i < count/sizeof(u32); i++)
-		writel(*src_32++, dst_32++);
+	/* __iowrite32_copy uses 32-bit count values so dev by 4 for right
+	 * count in words */
+	__iowrite32_copy(dst, src, count/4);
 }
 
 /**
-- 
1.7.0.4

[PATCH 2/5] ASoC: intel: log an error on double free

[Vinod Koul]

the stream context should be freed only once on stream cleanup. If we ever
hit a chance that stream context is getting double freed, though not an
cause of panic as memory allocator can deal with this, we should still log
this to help in finding issues and debugging

Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/soc/intel/sst/sst_drv_interface.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/sound/soc/intel/sst/sst_drv_interface.c b/sound/soc/intel/sst/sst_drv_interface.c
index aadb0db..423c5ff 100644
--- a/sound/soc/intel/sst/sst_drv_interface.c
+++ b/sound/soc/intel/sst/sst_drv_interface.c
@@ -55,6 +55,8 @@ int free_stream_context(struct intel_sst_drv *ctx, unsigned int str_id)
 		if (ret)
 			sst_clean_stream(&ctx->streams[str_id]);
 		return ret;
+	} else {
+		dev_err(ctx->dev, "we tried to free stream context %d which was freed!!!", str_id);
 	}
 	return ret;
 }
-- 
1.7.0.4

[PATCH 3/5] ASoC: intel: fix the kernldoc comment

[Vinod Koul]

copypaste error on function sst_get_num_channel caused the comment to be
wrong, so fix it here

Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/soc/intel/sst/sst_drv_interface.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/sound/soc/intel/sst/sst_drv_interface.c b/sound/soc/intel/sst/sst_drv_interface.c
index 423c5ff..0ee471d 100644
--- a/sound/soc/intel/sst/sst_drv_interface.c
+++ b/sound/soc/intel/sst/sst_drv_interface.c
@@ -94,7 +94,7 @@ int sst_get_sfreq(struct snd_sst_params *str_param)
 }
 
 /*
- * sst_get_sfreq - this function returns the frequency of the stream
+ * sst_get_num_channel - get number of channels for the stream
  *
  * @str_param : stream params
  */
-- 
1.7.0.4

[PATCH 4/5] ASoC: intel: explain why block not found isn't error always

[Vinod Koul]

The IPC blocking can be error when we don't find block or a short message,
explain that by adding a comment about this scenario

Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/soc/intel/sst/sst_ipc.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/sound/soc/intel/sst/sst_ipc.c b/sound/soc/intel/sst/sst_ipc.c
index 41a2b41..fbda009 100644
--- a/sound/soc/intel/sst/sst_ipc.c
+++ b/sound/soc/intel/sst/sst_ipc.c
@@ -54,6 +54,21 @@ struct sst_block *sst_create_block(struct intel_sst_drv *ctx,
 	return msg;
 }
 
+/*
+ * while handling the interrupts, we need to check for message status and
+ * then if we are blocking for a message
+ *
+ * here we are unblocking the blocked ones, this is based on id we have
+ * passed and search that for block threads.
+ * We will not find block in two cases
+ *  a) when its small message and block in not there, so sliently ignore
+ *  them
+ *  b) when we are actually not able to find the block (bug perhpas)
+ *
+ *  Since we have bit of small messages we can spam kernel log with err
+ *  print on above so need to keep as debug prints which should be enabled
+ *  via dybnamic debug while debuuging IPC issues
+ */
 int sst_wake_up_block(struct intel_sst_drv *ctx, int result,
 		u32 drv_id, u32 ipc, void *data, u32 size)
 {
-- 
1.7.0.4

[PATCH 5/5] ASoC: intel: use __iowrite32_copy for 32 bit copy

[Vinod Koul]

The sst-firmware was also using own method to do 32bit copy, turns out we have a
kernel API so use that instead

[For BYT]
Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/soc/intel/sst-firmware.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/sound/soc/intel/sst-firmware.c b/sound/soc/intel/sst-firmware.c
index 3bb43da..44090ac 100644
--- a/sound/soc/intel/sst-firmware.c
+++ b/sound/soc/intel/sst-firmware.c
@@ -34,11 +34,8 @@ static void block_module_remove(struct sst_module *module);
 
 static void sst_memcpy32(volatile void __iomem *dest, void *src, u32 bytes)
 {
-	u32 i;
-
-	/* copy one 32 bit word at a time as 64 bit access is not supported */
-	for (i = 0; i < bytes; i += 4)
-		memcpy_toio(dest + i, src + i, 4);
+	/* __iowrite32_copy use 32bit size values so divide by 4 */
+	__iowrite32_copy(dest, src, bytes/4);
 }
 
 /* create new generic firmware object */
-- 
1.7.0.4

Re: [PATCH 1/5] ASoC: intel: use __iowrite32_copy for 32 bit copy

[Takashi Iwai]

At Mon, 20 Oct 2014 15:29:04 +0530,
Vinod Koul wrote:
> 
> The driver was using own method to do 32bit copy, turns out we have a kernel
> API so use that instead
> 
> Tested-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> ---
>  sound/soc/intel/sst/sst_loader.c |    9 +++------
>  1 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/sound/soc/intel/sst/sst_loader.c b/sound/soc/intel/sst/sst_loader.c
> index b6d27c1..40e501b 100644
> --- a/sound/soc/intel/sst/sst_loader.c
> +++ b/sound/soc/intel/sst/sst_loader.c
> @@ -41,12 +41,9 @@
>  
>  static void memcpy32_toio(void __iomem *dst, const void *src, int count)
>  {
> -	int i;
> -	const u32 *src_32 = src;
> -	u32 *dst_32 = dst;
> -
> -	for (i = 0; i < count/sizeof(u32); i++)
> -		writel(*src_32++, dst_32++);
> +	/* __iowrite32_copy uses 32-bit count values so dev by 4 for right
> +	 * count in words */

s/dev/div/

Also, it's worth to make it inline, although the compiler would do it
automatically in most cases like this.


Takashi

> +	__iowrite32_copy(dst, src, count/4);
>  }
>  
>  /**
> -- 
> 1.7.0.4
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: [PATCH 2/5] ASoC: intel: log an error on double free

[Takashi Iwai]

At Mon, 20 Oct 2014 15:29:05 +0530,
Vinod Koul wrote:
> 
> the stream context should be freed only once on stream cleanup. If we ever
> hit a chance that stream context is getting double freed, though not an
> cause of panic as memory allocator can deal with this, we should still log
> this to help in finding issues and debugging
> 
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> ---
>  sound/soc/intel/sst/sst_drv_interface.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/sound/soc/intel/sst/sst_drv_interface.c b/sound/soc/intel/sst/sst_drv_interface.c
> index aadb0db..423c5ff 100644
> --- a/sound/soc/intel/sst/sst_drv_interface.c
> +++ b/sound/soc/intel/sst/sst_drv_interface.c
> @@ -55,6 +55,8 @@ int free_stream_context(struct intel_sst_drv *ctx, unsigned int str_id)
>  		if (ret)
>  			sst_clean_stream(&ctx->streams[str_id]);
>  		return ret;
> +	} else {
> +		dev_err(ctx->dev, "we tried to free stream context %d which was freed!!!", str_id);

Missing line feed at the end of the string.


Takashi


>  	}
>  	return ret;
>  }
> -- 
> 1.7.0.4
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: [PATCH 2/5] ASoC: intel: log an error on double free

[Vinod Koul]

On Mon, Oct 20, 2014 at 01:30:05PM +0200, Takashi Iwai wrote:
> At Mon, 20 Oct 2014 15:29:05 +0530,
> Vinod Koul wrote:
> > 
> > the stream context should be freed only once on stream cleanup. If we ever
> > hit a chance that stream context is getting double freed, though not an
> > cause of panic as memory allocator can deal with this, we should still log
> > this to help in finding issues and debugging
> > 
> > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > ---
> >  sound/soc/intel/sst/sst_drv_interface.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > diff --git a/sound/soc/intel/sst/sst_drv_interface.c b/sound/soc/intel/sst/sst_drv_interface.c
> > index aadb0db..423c5ff 100644
> > --- a/sound/soc/intel/sst/sst_drv_interface.c
> > +++ b/sound/soc/intel/sst/sst_drv_interface.c
> > @@ -55,6 +55,8 @@ int free_stream_context(struct intel_sst_drv *ctx, unsigned int str_id)
> >  		if (ret)
> >  			sst_clean_stream(&ctx->streams[str_id]);
> >  		return ret;
> > +	} else {
> > +		dev_err(ctx->dev, "we tried to free stream context %d which was freed!!!", str_id);
> 
> Missing line feed at the end of the string.
Thanks for pointing :)

-- 
~Vinod

> 
> 
> Takashi
> 
> 
> >  	}
> >  	return ret;
> >  }
> > -- 
> > 1.7.0.4
> > 
> > _______________________________________________
> > Alsa-devel mailing list
> > Alsa-devel@alsa-project.org
> > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> > 

-- 

Re: [PATCH 1/5] ASoC: intel: use __iowrite32_copy for 32 bit copy

[Vinod Koul]

On Mon, Oct 20, 2014 at 01:29:33PM +0200, Takashi Iwai wrote:
> At Mon, 20 Oct 2014 15:29:04 +0530,
> Vinod Koul wrote:
> > 
> > The driver was using own method to do 32bit copy, turns out we have a kernel
> > API so use that instead
> > 
> > Tested-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > ---
> >  sound/soc/intel/sst/sst_loader.c |    9 +++------
> >  1 files changed, 3 insertions(+), 6 deletions(-)
> > 
> > diff --git a/sound/soc/intel/sst/sst_loader.c b/sound/soc/intel/sst/sst_loader.c
> > index b6d27c1..40e501b 100644
> > --- a/sound/soc/intel/sst/sst_loader.c
> > +++ b/sound/soc/intel/sst/sst_loader.c
> > @@ -41,12 +41,9 @@
> >  
> >  static void memcpy32_toio(void __iomem *dst, const void *src, int count)
> >  {
> > -	int i;
> > -	const u32 *src_32 = src;
> > -	u32 *dst_32 = dst;
> > -
> > -	for (i = 0; i < count/sizeof(u32); i++)
> > -		writel(*src_32++, dst_32++);
> > +	/* __iowrite32_copy uses 32-bit count values so dev by 4 for right
> > +	 * count in words */
> 
> s/dev/div/
> 
> Also, it's worth to make it inline, although the compiler would do it
> automatically in most cases like this.
Yes it did, although it would make sense to do so here as well

-- 
~Vinod

> 
> 
> Takashi
> 
> > +	__iowrite32_copy(dst, src, count/4);
> >  }
> >  
> >  /**
> > -- 
> > 1.7.0.4
> > 
> > _______________________________________________
> > Alsa-devel mailing list
> > Alsa-devel@alsa-project.org
> > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> > 

--