[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(-)

[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 <***@intel.com>
Signed-off-by: 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);
}

/**

[Takashi Iwai]

At Mon, 20 Oct 2014 15:29:04 +0530,

Post by Vinod Koul
The driver was using own method to do 32bit copy, turns out we have a kernel
API so use that instead
---
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

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

[Vinod Koul]

Post by Takashi Iwai
At Mon, 20 Oct 2014 15:29:04 +0530,

Post by Vinod Koul
The driver was using own method to do 32bit copy, turns out we have a kernel
API so use that instead
---
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

Post by Takashi Iwai
Takashi

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

--

[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 <***@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
*/

[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 <***@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;
}

[Takashi Iwai]

At Mon, 20 Oct 2014 15:29:05 +0530,

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

Post by Vinod Koul
}
return ret;
}
--
1.7.0.4
_______________________________________________
Alsa-devel mailing list
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[Vinod Koul]

Post by Takashi Iwai
At Mon, 20 Oct 2014 15:29:05 +0530,

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

Post by Takashi Iwai
Takashi

Post by Vinod Koul
}
return ret;
}
--
1.7.0.4
_______________________________________________
Alsa-devel mailing list
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

--

[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 <***@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)
{

[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 <***@linux.intel.com>
Signed-off-by: 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 */