[alsa-devel] [PATCH 0/5] ASoC: intel: fix to sst driver series
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(-)
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); }
/**
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
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
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; }
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
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 :)
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 */
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) {
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 */
participants (2)
-
Takashi Iwai -
Vinod Koul