[PATCH] ASoC: SOF: Fix snd_sof_ipc_stream_posn()

[PATCH] ASoC: SOF: Fix snd_sof_ipc_stream_posn()

[Dan Carpenter]

We're passing "&posn" instead of "posn" so it ends up corrupting
memory instead of doing something useful.

Fixes: 53e0c72d98ba ("ASoC: SOF: Add support for IPC IO between DSP and Host")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 sound/soc/sof/ipc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/sof/ipc.c b/sound/soc/sof/ipc.c
index 22d296f95761..d6bf53dcd80b 100644
--- a/sound/soc/sof/ipc.c
+++ b/sound/soc/sof/ipc.c
@@ -524,7 +524,7 @@ int snd_sof_ipc_stream_posn(struct snd_soc_component *scomp,
 
 	/* send IPC to the DSP */
 	err = sof_ipc_tx_message(sdev->ipc,
-				 stream.hdr.cmd, &stream, sizeof(stream), &posn,
+				 stream.hdr.cmd, &stream, sizeof(stream), posn,
 				 sizeof(*posn));
 	if (err < 0) {
 		dev_err(sdev->dev, "error: failed to get stream %d position\n",
-- 
2.11.0

Re: [PATCH] ASoC: SOF: Fix snd_sof_ipc_stream_posn()

[Kai Vehmanen]

Hi, 

On Tue, 3 Mar 2020, Dan Carpenter wrote:

> We're passing "&posn" instead of "posn" so it ends up corrupting
> memory instead of doing something useful.
[...]
>  	/* send IPC to the DSP */
>  	err = sof_ipc_tx_message(sdev->ipc,
> -				 stream.hdr.cmd, &stream, sizeof(stream), &posn,
> +				 stream.hdr.cmd, &stream, sizeof(stream), posn,
>  				 sizeof(*posn));

ack, thanks, this is clearly wrong. This function is not used by current 
platforms, so the bug has gone unnnoticed. Most platforms either rely on 
direct MMIO queries of the DSP position, or the periodic position updates 
DSPs send after each ALSA period. This function for host to query DSP 
position via IPC is thus not used, although it's part of the generic audio
DSP IPC interface.

For the SOF folks in CC, I wonder should we keep this function at all? 

Anyways, that's probably a longer discussion, so while it's there, 
the code should be correct, so for the patch:
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>

Br, Kai

Re: [PATCH] ASoC: SOF: Fix snd_sof_ipc_stream_posn()

[Pierre-Louis Bossart]

>> We're passing "&posn" instead of "posn" so it ends up corrupting
>> memory instead of doing something useful.
> [...]
>>   	/* send IPC to the DSP */
>>   	err = sof_ipc_tx_message(sdev->ipc,
>> -				 stream.hdr.cmd, &stream, sizeof(stream), &posn,
>> +				 stream.hdr.cmd, &stream, sizeof(stream), posn,
>>   				 sizeof(*posn));
> 
> ack, thanks, this is clearly wrong. This function is not used by current
> platforms, so the bug has gone unnnoticed. Most platforms either rely on
> direct MMIO queries of the DSP position, or the periodic position updates
> DSPs send after each ALSA period. This function for host to query DSP
> position via IPC is thus not used, although it's part of the generic audio
> DSP IPC interface.
> 
> For the SOF folks in CC, I wonder should we keep this function at all?
> 
> Anyways, that's probably a longer discussion, so while it's there,
> the code should be correct, so for the patch:
> Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>

I checked all the way to 5.2 and it was never used, so indeed wondering 
what the purpose of this function was.

Applied "ASoC: SOF: Fix snd_sof_ipc_stream_posn()" to the asoc tree

[Mark Brown]

The patch

   ASoC: SOF: Fix snd_sof_ipc_stream_posn()

has been applied to the asoc tree at

   https://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 613cea5935e83cb5a7d182ee3f98d54620e102e2 Mon Sep 17 00:00:00 2001
From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Tue, 3 Mar 2020 13:18:58 +0300
Subject: [PATCH] ASoC: SOF: Fix snd_sof_ipc_stream_posn()

We're passing "&posn" instead of "posn" so it ends up corrupting
memory instead of doing something useful.

Fixes: 53e0c72d98ba ("ASoC: SOF: Add support for IPC IO between DSP and Host")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Link: https://lore.kernel.org/r/20200303101858.ytehbrivocyp3cnf@kili.mountain
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/sof/ipc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/sof/ipc.c b/sound/soc/sof/ipc.c
index b63fc529b456..78aa1da7c7a9 100644
--- a/sound/soc/sof/ipc.c
+++ b/sound/soc/sof/ipc.c
@@ -499,7 +499,7 @@ int snd_sof_ipc_stream_posn(struct snd_soc_component *scomp,
 
 	/* send IPC to the DSP */
 	err = sof_ipc_tx_message(sdev->ipc,
-				 stream.hdr.cmd, &stream, sizeof(stream), &posn,
+				 stream.hdr.cmd, &stream, sizeof(stream), posn,
 				 sizeof(*posn));
 	if (err < 0) {
 		dev_err(sdev->dev, "error: failed to get stream %d position\n",
-- 
2.20.1

Re: [PATCH] ASoC: SOF: Fix snd_sof_ipc_stream_posn()

[Guennadi Liakhovetski]

On Tue, Mar 03, 2020 at 09:36:29AM -0600, Pierre-Louis Bossart wrote:
> 
> 
> > > We're passing "&posn" instead of "posn" so it ends up corrupting
> > > memory instead of doing something useful.
> > [...]
> > >   	/* send IPC to the DSP */
> > >   	err = sof_ipc_tx_message(sdev->ipc,
> > > -				 stream.hdr.cmd, &stream, sizeof(stream), &posn,
> > > +				 stream.hdr.cmd, &stream, sizeof(stream), posn,
> > >   				 sizeof(*posn));
> > 
> > ack, thanks, this is clearly wrong. This function is not used by current
> > platforms, so the bug has gone unnnoticed. Most platforms either rely on
> > direct MMIO queries of the DSP position, or the periodic position updates
> > DSPs send after each ALSA period. This function for host to query DSP
> > position via IPC is thus not used, although it's part of the generic audio
> > DSP IPC interface.
> > 
> > For the SOF folks in CC, I wonder should we keep this function at all?
> > 
> > Anyways, that's probably a longer discussion, so while it's there,
> > the code should be correct, so for the patch:
> > Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> 
> I checked all the way to 5.2 and it was never used, so indeed wondering what
> the purpose of this function was.

I actually have a patch, removing that function, which I noticed as being unused 
during my VirtIO work. I think it was used by compressed.c before.

Thanks
Guennadi

Re: [PATCH] ASoC: SOF: Fix snd_sof_ipc_stream_posn()

[Keyon Jie]

On 3/4/20 3:07 PM, Guennadi Liakhovetski wrote:
> On Tue, Mar 03, 2020 at 09:36:29AM -0600, Pierre-Louis Bossart wrote:
>>
>>
>>>> We're passing "&posn" instead of "posn" so it ends up corrupting
>>>> memory instead of doing something useful.
>>> [...]
>>>>    	/* send IPC to the DSP */
>>>>    	err = sof_ipc_tx_message(sdev->ipc,
>>>> -				 stream.hdr.cmd, &stream, sizeof(stream), &posn,
>>>> +				 stream.hdr.cmd, &stream, sizeof(stream), posn,
>>>>    				 sizeof(*posn));
>>>
>>> ack, thanks, this is clearly wrong. This function is not used by current
>>> platforms, so the bug has gone unnnoticed. Most platforms either rely on
>>> direct MMIO queries of the DSP position, or the periodic position updates
>>> DSPs send after each ALSA period. This function for host to query DSP
>>> position via IPC is thus not used, although it's part of the generic audio
>>> DSP IPC interface.
>>>
>>> For the SOF folks in CC, I wonder should we keep this function at all?
>>>
>>> Anyways, that's probably a longer discussion, so while it's there,
>>> the code should be correct, so for the patch:
>>> Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
>>
>> I checked all the way to 5.2 and it was never used, so indeed wondering what
>> the purpose of this function was.
> 
> I actually have a patch, removing that function, which I noticed as being unused
> during my VirtIO work. I think it was used by compressed.c before.

The function was designed for getting the full picture of the pipeline 
position inside the FW, including the host position(the closest one to 
the ALSA buffer), the dai rendering position, the system timestamp, etc.

Iirc, the function could be used for precise position control, AV sync, 
hammock harbor, etc.

Handling to this IPC is already implemented in SOF firmware(which means 
we can already get it from the FW), I think nobody is calling the 
function as we haven't had the corresponding requirement yet.

Thanks,
~Keyon

> 
> Thanks
> Guennadi
>