| Message ID | 20200303101858.ytehbrivocyp3cnf@kili.mountain (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 613cea5935e83cb5a7d182ee3f98d54620e102e2 |
| Headers | show |
| Series | ASoC: SOF: Fix snd_sof_ipc_stream_posn() | expand |
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
>> 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.
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
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 >
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",
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(-)