[alsa-devel] [PATCH 1/3] ASoC: Intel: Skylake: Fix IPC rx_list corruption
From: Pardha Saradhi K pardha.saradhi.kesapragada@intel.com
In SKL+ platforms, all IPC commands are serialised, i.e. the driver sends a new IPC to DSP, only after receiving a reply from the firmware for the current IPC.
Hence it seems apparent that there is only a single modifier of the IPC RX List. However, during an IPC timeout case in a multithreaded environment, there is a possibility of the list element being deleted two times if not properly protected.
So, use spin lock save/restore to prevent rx_list corruption.
Signed-off-by: Pardha Saradhi K pardha.saradhi.kesapragada@intel.com Signed-off-by: Subhransu S. Prusty subhransu.s.prusty@intel.com --- sound/soc/intel/skylake/skl-sst-ipc.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/sound/soc/intel/skylake/skl-sst-ipc.c b/sound/soc/intel/skylake/skl-sst-ipc.c index 58c525096a7c..498b15345b1a 100644 --- a/sound/soc/intel/skylake/skl-sst-ipc.c +++ b/sound/soc/intel/skylake/skl-sst-ipc.c @@ -413,8 +413,11 @@ static void skl_ipc_process_reply(struct sst_generic_ipc *ipc, u32 reply = header.primary & IPC_GLB_REPLY_STATUS_MASK; u64 *ipc_header = (u64 *)(&header); struct skl_sst *skl = container_of(ipc, struct skl_sst, ipc); + unsigned long flags;
+ spin_lock_irqsave(&ipc->dsp->spinlock, flags); msg = skl_ipc_reply_get_msg(ipc, *ipc_header); + spin_unlock_irqrestore(&ipc->dsp->spinlock, flags); if (msg == NULL) { dev_dbg(ipc->dev, "ipc: rx list is empty\n"); return; @@ -456,8 +459,10 @@ static void skl_ipc_process_reply(struct sst_generic_ipc *ipc, } }
+ spin_lock_irqsave(&ipc->dsp->spinlock, flags); list_del(&msg->list); sst_ipc_tx_msg_reply_complete(ipc, msg); + spin_unlock_irqrestore(&ipc->dsp->spinlock, flags); }
irqreturn_t skl_dsp_irq_thread_handler(int irq, void *context)
From: Shreyas NC shreyas.nc@intel.com
Element size in the manifest should be updated for each token, so that the loop can parse all the string elements in the manifest. This was not happening when more than two string elements appear consecutively, as it is not updated with correct string element size. Fixed with this patch.
Signed-off-by: Shreyas NC shreyas.nc@intel.com Signed-off-by: Subhransu S. Prusty subhransu.s.prusty@intel.com --- sound/soc/intel/skylake/skl-topology.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c index b28199a5348c..43ca39d61559 100644 --- a/sound/soc/intel/skylake/skl-topology.c +++ b/sound/soc/intel/skylake/skl-topology.c @@ -2502,7 +2502,7 @@ static int skl_tplg_get_manifest_tkn(struct device *dev,
if (ret < 0) return ret; - tkn_count += ret; + tkn_count = ret;
tuple_size += tkn_count * sizeof(struct snd_soc_tplg_vendor_string_elem);
The patch
ASoC: Intel: Skylake: Fix to parse consecutive string tkns in manifest
has been applied to the asoc tree at
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From 0a716776914ed9d7ca90b48041e6767693bfb672 Mon Sep 17 00:00:00 2001
From: Shreyas NC shreyas.nc@intel.com Date: Mon, 15 May 2017 19:44:30 +0530 Subject: [PATCH] ASoC: Intel: Skylake: Fix to parse consecutive string tkns in manifest
Element size in the manifest should be updated for each token, so that the loop can parse all the string elements in the manifest. This was not happening when more than two string elements appear consecutively, as it is not updated with correct string element size. Fixed with this patch.
Signed-off-by: Shreyas NC shreyas.nc@intel.com Signed-off-by: Subhransu S. Prusty subhransu.s.prusty@intel.com Acked-by: Vinod Koul vinod.koul@intel.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/intel/skylake/skl-topology.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c index 3a99712e44a8..64a0f8ed33e1 100644 --- a/sound/soc/intel/skylake/skl-topology.c +++ b/sound/soc/intel/skylake/skl-topology.c @@ -2502,7 +2502,7 @@ static int skl_tplg_get_manifest_tkn(struct device *dev,
if (ret < 0) return ret; - tkn_count += ret; + tkn_count = ret;
tuple_size += tkn_count * sizeof(struct snd_soc_tplg_vendor_string_elem);
From: Shreyas NC shreyas.nc@intel.com
Module init params are additional data block in the module private data. Skylake driver doesn't yet have support to parse multiple data blocks if it appears in private data. Add support for parsing of multiple data blocks and module init params.
Signed-off-by: Shreyas NC shreyas.nc@intel.com Signed-off-by: Subhransu S. Prusty subhransu.s.prusty@intel.com --- sound/soc/intel/skylake/skl-topology.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c index 43ca39d61559..b687ae455a61 100644 --- a/sound/soc/intel/skylake/skl-topology.c +++ b/sound/soc/intel/skylake/skl-topology.c @@ -2070,6 +2070,16 @@ static int skl_tplg_get_token(struct device *dev,
break;
+ case SKL_TKN_U32_CAPS_SET_PARAMS: + mconfig->formats_config.set_params = + tkn_elem->value; + break; + + case SKL_TKN_U32_CAPS_PARAMS_ID: + mconfig->formats_config.param_id = + tkn_elem->value; + break; + case SKL_TKN_U32_PROC_DOMAIN: mconfig->domain = tkn_elem->value; @@ -2147,7 +2157,7 @@ static int skl_tplg_get_tokens(struct device *dev, tuple_size += tkn_count * sizeof(*tkn_elem); }
- return 0; + return off; }
/* @@ -2198,10 +2208,11 @@ static int skl_tplg_get_pvt_data(struct snd_soc_tplg_dapm_widget *tplg_w, num_blocks = ret;
off += array->size; - array = (struct snd_soc_tplg_vendor_array *)(tplg_w->priv.data + off); - /* Read the BLOCK_TYPE and BLOCK_SIZE descriptor */ while (num_blocks > 0) { + array = (struct snd_soc_tplg_vendor_array *) + (tplg_w->priv.data + off); + ret = skl_tplg_get_desc_blocks(dev, array);
if (ret < 0) @@ -2237,7 +2248,9 @@ static int skl_tplg_get_pvt_data(struct snd_soc_tplg_dapm_widget *tplg_w, memcpy(mconfig->formats_config.caps, data, mconfig->formats_config.caps_size); --num_blocks; + ret = mconfig->formats_config.caps_size; } + off += ret; }
return 0;
The patch
ASoC: Intel: Skylake: Support for multiple data blocks
has been applied to the asoc tree at
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From 133e6e5c27340fe2205537373e50d43881a0f745 Mon Sep 17 00:00:00 2001
From: Shreyas NC shreyas.nc@intel.com Date: Mon, 15 May 2017 19:44:31 +0530 Subject: [PATCH] ASoC: Intel: Skylake: Support for multiple data blocks
Module init params are additional data block in the module private data. Skylake driver doesn't yet have support to parse multiple data blocks if it appears in private data. Add support for parsing of multiple data blocks and module init params.
Signed-off-by: Shreyas NC shreyas.nc@intel.com Signed-off-by: Subhransu S. Prusty subhransu.s.prusty@intel.com Acked-by: Vinod Koul vinod.koul@intel.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/intel/skylake/skl-topology.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c index b28199a5348c..4c3bdff092bd 100644 --- a/sound/soc/intel/skylake/skl-topology.c +++ b/sound/soc/intel/skylake/skl-topology.c @@ -2070,6 +2070,16 @@ static int skl_tplg_get_token(struct device *dev,
break;
+ case SKL_TKN_U32_CAPS_SET_PARAMS: + mconfig->formats_config.set_params = + tkn_elem->value; + break; + + case SKL_TKN_U32_CAPS_PARAMS_ID: + mconfig->formats_config.param_id = + tkn_elem->value; + break; + case SKL_TKN_U32_PROC_DOMAIN: mconfig->domain = tkn_elem->value; @@ -2147,7 +2157,7 @@ static int skl_tplg_get_tokens(struct device *dev, tuple_size += tkn_count * sizeof(*tkn_elem); }
- return 0; + return off; }
/* @@ -2198,10 +2208,11 @@ static int skl_tplg_get_pvt_data(struct snd_soc_tplg_dapm_widget *tplg_w, num_blocks = ret;
off += array->size; - array = (struct snd_soc_tplg_vendor_array *)(tplg_w->priv.data + off); - /* Read the BLOCK_TYPE and BLOCK_SIZE descriptor */ while (num_blocks > 0) { + array = (struct snd_soc_tplg_vendor_array *) + (tplg_w->priv.data + off); + ret = skl_tplg_get_desc_blocks(dev, array);
if (ret < 0) @@ -2237,7 +2248,9 @@ static int skl_tplg_get_pvt_data(struct snd_soc_tplg_dapm_widget *tplg_w, memcpy(mconfig->formats_config.caps, data, mconfig->formats_config.caps_size); --num_blocks; + ret = mconfig->formats_config.caps_size; } + off += ret; }
return 0;
On Mon, May 15, 2017 at 07:44:29PM +0530, Subhransu S. Prusty wrote:
From: Pardha Saradhi K pardha.saradhi.kesapragada@intel.com
In SKL+ platforms, all IPC commands are serialised, i.e. the driver sends a new IPC to DSP, only after receiving a reply from the firmware for the current IPC.
Hence it seems apparent that there is only a single modifier of the IPC RX List. However, during an IPC timeout case in a multithreaded environment, there is a possibility of the list element being deleted two times if not properly protected.
So, use spin lock save/restore to prevent rx_list corruption.
Looks good, all three:
Acked-by: Vinod Koul vinod.koul@intel.com
The patch
ASoC: Intel: Skylake: Fix IPC rx_list corruption
has been applied to the asoc tree at
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From 5cd1f5c32132101955d7f0e1955249a84f9b6fd9 Mon Sep 17 00:00:00 2001
From: Pardha Saradhi K pardha.saradhi.kesapragada@intel.com Date: Mon, 15 May 2017 19:44:29 +0530 Subject: [PATCH] ASoC: Intel: Skylake: Fix IPC rx_list corruption
In SKL+ platforms, all IPC commands are serialised, i.e. the driver sends a new IPC to DSP, only after receiving a reply from the firmware for the current IPC.
Hence it seems apparent that there is only a single modifier of the IPC RX List. However, during an IPC timeout case in a multithreaded environment, there is a possibility of the list element being deleted two times if not properly protected.
So, use spin lock save/restore to prevent rx_list corruption.
Signed-off-by: Pardha Saradhi K pardha.saradhi.kesapragada@intel.com Signed-off-by: Subhransu S. Prusty subhransu.s.prusty@intel.com Acked-by: Vinod Koul vinod.koul@intel.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/intel/skylake/skl-sst-ipc.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/sound/soc/intel/skylake/skl-sst-ipc.c b/sound/soc/intel/skylake/skl-sst-ipc.c index 58c525096a7c..498b15345b1a 100644 --- a/sound/soc/intel/skylake/skl-sst-ipc.c +++ b/sound/soc/intel/skylake/skl-sst-ipc.c @@ -413,8 +413,11 @@ static void skl_ipc_process_reply(struct sst_generic_ipc *ipc, u32 reply = header.primary & IPC_GLB_REPLY_STATUS_MASK; u64 *ipc_header = (u64 *)(&header); struct skl_sst *skl = container_of(ipc, struct skl_sst, ipc); + unsigned long flags;
+ spin_lock_irqsave(&ipc->dsp->spinlock, flags); msg = skl_ipc_reply_get_msg(ipc, *ipc_header); + spin_unlock_irqrestore(&ipc->dsp->spinlock, flags); if (msg == NULL) { dev_dbg(ipc->dev, "ipc: rx list is empty\n"); return; @@ -456,8 +459,10 @@ static void skl_ipc_process_reply(struct sst_generic_ipc *ipc, } }
+ spin_lock_irqsave(&ipc->dsp->spinlock, flags); list_del(&msg->list); sst_ipc_tx_msg_reply_complete(ipc, msg); + spin_unlock_irqrestore(&ipc->dsp->spinlock, flags); }
irqreturn_t skl_dsp_irq_thread_handler(int irq, void *context)
participants (3)
-
Mark Brown
-
Subhransu S. Prusty
-
Vinod Koul