All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ASoC: SOF: add error handling to snd_sof_ipc_msg_data()
@ 2021-09-28 10:35 Peter Ujfalusi
  2021-09-28 16:23 ` Mark Brown
  0 siblings, 1 reply; 2+ messages in thread
From: Peter Ujfalusi @ 2021-09-28 10:35 UTC (permalink / raw)
  To: lgirdwood, broonie, pierre-louis.bossart, guennadi.liakhovetski,
	daniel.baluta
  Cc: alsa-devel, ranjani.sridharan, kai.vehmanen

From: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

If an invalid stream is passed to snd_sof_ipc_msg_data() it won't
fill the provided object with data. The caller has to be able to
recognise such cases to avoid handling invalid data. Make the
function return an error when failing.

Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Reviewed-by: Peter Ujfalusi <peter.ujfalusi@intel.com>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
---
 sound/soc/sof/imx/imx8.c        |  7 +++---
 sound/soc/sof/imx/imx8m.c       |  7 +++---
 sound/soc/sof/intel/hda-ipc.c   | 15 +++++++-----
 sound/soc/sof/intel/hda.h       |  6 ++---
 sound/soc/sof/intel/intel-ipc.c | 14 +++++++----
 sound/soc/sof/ipc.c             | 41 +++++++++++++++++++++++++--------
 sound/soc/sof/ops.h             |  8 +++----
 sound/soc/sof/sof-priv.h        | 12 +++++-----
 8 files changed, 71 insertions(+), 39 deletions(-)

diff --git a/sound/soc/sof/imx/imx8.c b/sound/soc/sof/imx/imx8.c
index 82ca920a69ab..e6eed53f8644 100644
--- a/sound/soc/sof/imx/imx8.c
+++ b/sound/soc/sof/imx/imx8.c
@@ -375,11 +375,12 @@ static int imx8_get_bar_index(struct snd_sof_dev *sdev, u32 type)
 	}
 }
 
-static void imx8_ipc_msg_data(struct snd_sof_dev *sdev,
-			      struct snd_pcm_substream *substream,
-			      void *p, size_t sz)
+static int imx8_ipc_msg_data(struct snd_sof_dev *sdev,
+			     struct snd_pcm_substream *substream,
+			     void *p, size_t sz)
 {
 	sof_mailbox_read(sdev, sdev->dsp_box.offset, p, sz);
+	return 0;
 }
 
 static int imx8_ipc_pcm_params(struct snd_sof_dev *sdev,
diff --git a/sound/soc/sof/imx/imx8m.c b/sound/soc/sof/imx/imx8m.c
index 76f4ffd9d540..5579cbef4a93 100644
--- a/sound/soc/sof/imx/imx8m.c
+++ b/sound/soc/sof/imx/imx8m.c
@@ -238,11 +238,12 @@ static int imx8m_get_bar_index(struct snd_sof_dev *sdev, u32 type)
 	}
 }
 
-static void imx8m_ipc_msg_data(struct snd_sof_dev *sdev,
-			       struct snd_pcm_substream *substream,
-			       void *p, size_t sz)
+static int imx8m_ipc_msg_data(struct snd_sof_dev *sdev,
+			      struct snd_pcm_substream *substream,
+			      void *p, size_t sz)
 {
 	sof_mailbox_read(sdev, sdev->dsp_box.offset, p, sz);
+	return 0;
 }
 
 static int imx8m_ipc_pcm_params(struct snd_sof_dev *sdev,
diff --git a/sound/soc/sof/intel/hda-ipc.c b/sound/soc/sof/intel/hda-ipc.c
index acfeca42604c..11f20a5a62df 100644
--- a/sound/soc/sof/intel/hda-ipc.c
+++ b/sound/soc/sof/intel/hda-ipc.c
@@ -253,9 +253,9 @@ int hda_dsp_ipc_get_window_offset(struct snd_sof_dev *sdev, u32 id)
 	return SRAM_WINDOW_OFFSET(id);
 }
 
-void hda_ipc_msg_data(struct snd_sof_dev *sdev,
-		      struct snd_pcm_substream *substream,
-		      void *p, size_t sz)
+int hda_ipc_msg_data(struct snd_sof_dev *sdev,
+		     struct snd_pcm_substream *substream,
+		     void *p, size_t sz)
 {
 	if (!substream || !sdev->stream_box.size) {
 		sof_mailbox_read(sdev, sdev->dsp_box.offset, p, sz);
@@ -268,10 +268,13 @@ void hda_ipc_msg_data(struct snd_sof_dev *sdev,
 					  hda_stream.hstream);
 
 		/* The stream might already be closed */
-		if (hstream)
-			sof_mailbox_read(sdev, hda_stream->stream.posn_offset,
-					 p, sz);
+		if (!hstream)
+			return -ESTRPIPE;
+
+		sof_mailbox_read(sdev, hda_stream->stream.posn_offset, p, sz);
 	}
+
+	return 0;
 }
 
 int hda_ipc_pcm_params(struct snd_sof_dev *sdev,
diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h
index 087fa06d5210..cb5a1b17f153 100644
--- a/sound/soc/sof/intel/hda.h
+++ b/sound/soc/sof/intel/hda.h
@@ -563,9 +563,9 @@ int hda_dsp_stream_spib_config(struct snd_sof_dev *sdev,
 			       struct hdac_ext_stream *stream,
 			       int enable, u32 size);
 
-void hda_ipc_msg_data(struct snd_sof_dev *sdev,
-		      struct snd_pcm_substream *substream,
-		      void *p, size_t sz);
+int hda_ipc_msg_data(struct snd_sof_dev *sdev,
+		     struct snd_pcm_substream *substream,
+		     void *p, size_t sz);
 int hda_ipc_pcm_params(struct snd_sof_dev *sdev,
 		       struct snd_pcm_substream *substream,
 		       const struct sof_ipc_pcm_params_reply *reply);
diff --git a/sound/soc/sof/intel/intel-ipc.c b/sound/soc/sof/intel/intel-ipc.c
index de66f8a82a07..df37187c8427 100644
--- a/sound/soc/sof/intel/intel-ipc.c
+++ b/sound/soc/sof/intel/intel-ipc.c
@@ -25,9 +25,9 @@ struct intel_stream {
 };
 
 /* Mailbox-based Intel IPC implementation */
-void intel_ipc_msg_data(struct snd_sof_dev *sdev,
-			struct snd_pcm_substream *substream,
-			void *p, size_t sz)
+int intel_ipc_msg_data(struct snd_sof_dev *sdev,
+		       struct snd_pcm_substream *substream,
+		       void *p, size_t sz)
 {
 	if (!substream || !sdev->stream_box.size) {
 		sof_mailbox_read(sdev, sdev->dsp_box.offset, p, sz);
@@ -35,9 +35,13 @@ void intel_ipc_msg_data(struct snd_sof_dev *sdev,
 		struct intel_stream *stream = substream->runtime->private_data;
 
 		/* The stream might already be closed */
-		if (stream)
-			sof_mailbox_read(sdev, stream->posn_offset, p, sz);
+		if (!stream)
+			return -ESTRPIPE;
+
+		sof_mailbox_read(sdev, stream->posn_offset, p, sz);
 	}
+
+	return 0;
 }
 EXPORT_SYMBOL_NS(intel_ipc_msg_data, SND_SOC_SOF_INTEL_HIFI_EP_IPC);
 
diff --git a/sound/soc/sof/ipc.c b/sound/soc/sof/ipc.c
index 5d41924f37a6..152d36a6253d 100644
--- a/sound/soc/sof/ipc.c
+++ b/sound/soc/sof/ipc.c
@@ -394,6 +394,7 @@ static void ipc_comp_notification(struct snd_sof_dev *sdev,
 {
 	u32 msg_type = hdr->cmd & SOF_CMD_TYPE_MASK;
 	struct sof_ipc_ctrl_data *cdata;
+	int ret;
 
 	switch (msg_type) {
 	case SOF_IPC_COMP_GET_VALUE:
@@ -403,7 +404,12 @@ static void ipc_comp_notification(struct snd_sof_dev *sdev,
 			return;
 
 		/* read back full message */
-		snd_sof_ipc_msg_data(sdev, NULL, cdata, hdr->size);
+		ret = snd_sof_ipc_msg_data(sdev, NULL, cdata, hdr->size);
+		if (ret < 0) {
+			dev_err(sdev->dev,
+				"error: failed to read component event: %d\n", ret);
+			goto err;
+		}
 		break;
 	default:
 		dev_err(sdev->dev, "error: unhandled component message %#x\n", msg_type);
@@ -412,6 +418,7 @@ static void ipc_comp_notification(struct snd_sof_dev *sdev,
 
 	snd_sof_control_notify(sdev, cdata);
 
+err:
 	kfree(cdata);
 }
 
@@ -420,10 +427,14 @@ void snd_sof_ipc_msgs_rx(struct snd_sof_dev *sdev)
 {
 	struct sof_ipc_cmd_hdr hdr;
 	u32 cmd, type;
-	int err = 0;
+	int err;
 
 	/* read back header */
-	snd_sof_ipc_msg_data(sdev, NULL, &hdr, sizeof(hdr));
+	err = snd_sof_ipc_msg_data(sdev, NULL, &hdr, sizeof(hdr));
+	if (err < 0) {
+		dev_warn(sdev->dev, "failed to read IPC header: %d\n", err);
+		return;
+	}
 	ipc_log_header(sdev->dev, "ipc rx", hdr.cmd);
 
 	cmd = hdr.cmd & SOF_GLB_TYPE_MASK;
@@ -477,12 +488,16 @@ EXPORT_SYMBOL(snd_sof_ipc_msgs_rx);
 static void ipc_trace_message(struct snd_sof_dev *sdev, u32 msg_type)
 {
 	struct sof_ipc_dma_trace_posn posn;
+	int ret;
 
 	switch (msg_type) {
 	case SOF_IPC_TRACE_DMA_POSITION:
 		/* read back full message */
-		snd_sof_ipc_msg_data(sdev, NULL, &posn, sizeof(posn));
-		snd_sof_trace_update_pos(sdev, &posn);
+		ret = snd_sof_ipc_msg_data(sdev, NULL, &posn, sizeof(posn));
+		if (ret < 0)
+			dev_warn(sdev->dev, "failed to read trace position: %d\n", ret);
+		else
+			snd_sof_trace_update_pos(sdev, &posn);
 		break;
 	default:
 		dev_err(sdev->dev, "error: unhandled trace message %#x\n", msg_type);
@@ -500,7 +515,7 @@ static void ipc_period_elapsed(struct snd_sof_dev *sdev, u32 msg_id)
 	struct snd_sof_pcm_stream *stream;
 	struct sof_ipc_stream_posn posn;
 	struct snd_sof_pcm *spcm;
-	int direction;
+	int direction, ret;
 
 	spcm = snd_sof_find_spcm_comp(scomp, msg_id, &direction);
 	if (!spcm) {
@@ -511,7 +526,11 @@ static void ipc_period_elapsed(struct snd_sof_dev *sdev, u32 msg_id)
 	}
 
 	stream = &spcm->stream[direction];
-	snd_sof_ipc_msg_data(sdev, stream->substream, &posn, sizeof(posn));
+	ret = snd_sof_ipc_msg_data(sdev, stream->substream, &posn, sizeof(posn));
+	if (ret < 0) {
+		dev_warn(sdev->dev, "failed to read stream position: %d\n", ret);
+		return;
+	}
 
 	dev_vdbg(sdev->dev, "posn : host 0x%llx dai 0x%llx wall 0x%llx\n",
 		 posn.host_posn, posn.dai_posn, posn.wallclock);
@@ -530,7 +549,7 @@ static void ipc_xrun(struct snd_sof_dev *sdev, u32 msg_id)
 	struct snd_sof_pcm_stream *stream;
 	struct sof_ipc_stream_posn posn;
 	struct snd_sof_pcm *spcm;
-	int direction;
+	int direction, ret;
 
 	spcm = snd_sof_find_spcm_comp(scomp, msg_id, &direction);
 	if (!spcm) {
@@ -540,7 +559,11 @@ static void ipc_xrun(struct snd_sof_dev *sdev, u32 msg_id)
 	}
 
 	stream = &spcm->stream[direction];
-	snd_sof_ipc_msg_data(sdev, stream->substream, &posn, sizeof(posn));
+	ret = snd_sof_ipc_msg_data(sdev, stream->substream, &posn, sizeof(posn));
+	if (ret < 0) {
+		dev_warn(sdev->dev, "failed to read overrun position: %d\n", ret);
+		return;
+	}
 
 	dev_dbg(sdev->dev,  "posn XRUN: host %llx comp %d size %d\n",
 		posn.host_posn, posn.xrun_comp_id, posn.xrun_size);
diff --git a/sound/soc/sof/ops.h b/sound/soc/sof/ops.h
index d40ce2581d94..a93aa5b943b2 100644
--- a/sound/soc/sof/ops.h
+++ b/sound/soc/sof/ops.h
@@ -422,11 +422,11 @@ static inline int snd_sof_load_firmware(struct snd_sof_dev *sdev)
 }
 
 /* host DSP message data */
-static inline void snd_sof_ipc_msg_data(struct snd_sof_dev *sdev,
-					struct snd_pcm_substream *substream,
-					void *p, size_t sz)
+static inline int snd_sof_ipc_msg_data(struct snd_sof_dev *sdev,
+				       struct snd_pcm_substream *substream,
+				       void *p, size_t sz)
 {
-	sof_ops(sdev)->ipc_msg_data(sdev, substream, p, sz);
+	return sof_ops(sdev)->ipc_msg_data(sdev, substream, p, sz);
 }
 
 /* host configure DSP HW parameters */
diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h
index 6b1dbae3344c..a6ded5bd0674 100644
--- a/sound/soc/sof/sof-priv.h
+++ b/sound/soc/sof/sof-priv.h
@@ -210,9 +210,9 @@ struct snd_sof_dsp_ops {
 #endif
 
 	/* host read DSP stream data */
-	void (*ipc_msg_data)(struct snd_sof_dev *sdev,
-			     struct snd_pcm_substream *substream,
-			     void *p, size_t sz); /* mandatory */
+	int (*ipc_msg_data)(struct snd_sof_dev *sdev,
+			    struct snd_pcm_substream *substream,
+			    void *p, size_t sz); /* mandatory */
 
 	/* host configure DSP HW parameters */
 	int (*ipc_pcm_params)(struct snd_sof_dev *sdev,
@@ -567,9 +567,9 @@ int sof_block_read(struct snd_sof_dev *sdev, enum snd_sof_fw_blk_type blk_type,
 
 int sof_fw_ready(struct snd_sof_dev *sdev, u32 msg_id);
 
-void intel_ipc_msg_data(struct snd_sof_dev *sdev,
-			struct snd_pcm_substream *substream,
-			void *p, size_t sz);
+int intel_ipc_msg_data(struct snd_sof_dev *sdev,
+		       struct snd_pcm_substream *substream,
+		       void *p, size_t sz);
 int intel_ipc_pcm_params(struct snd_sof_dev *sdev,
 			 struct snd_pcm_substream *substream,
 			 const struct sof_ipc_pcm_params_reply *reply);
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 2+ messages in thread
* Re: [PATCH] ASoC: SOF: add error handling to snd_sof_ipc_msg_data()
  2021-09-28 10:35 [PATCH] ASoC: SOF: add error handling to snd_sof_ipc_msg_data() Peter Ujfalusi
@ 2021-09-28 16:23 ` Mark Brown
  0 siblings, 0 replies; 2+ messages in thread
From: Mark Brown @ 2021-09-28 16:23 UTC (permalink / raw)
  To: daniel.baluta, Peter Ujfalusi, lgirdwood, guennadi.liakhovetski,
	pierre-louis.bossart
  Cc: alsa-devel, Mark Brown, ranjani.sridharan, kai.vehmanen

On Tue, 28 Sep 2021 13:35:16 +0300, Peter Ujfalusi wrote:
> From: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
> 
> If an invalid stream is passed to snd_sof_ipc_msg_data() it won't
> fill the provided object with data. The caller has to be able to
> recognise such cases to avoid handling invalid data. Make the
> function return an error when failing.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/1] ASoC: SOF: add error handling to snd_sof_ipc_msg_data()
      commit: 6a0ba071b71c44bc905522b77e96afad464e8aac

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

^ permalink raw reply	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-09-28 16:27 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-28 10:35 [PATCH] ASoC: SOF: add error handling to snd_sof_ipc_msg_data() Peter Ujfalusi
2021-09-28 16:23 ` Mark Brown

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.