| Message ID | 20210924192417.169243-1-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | ALSA: hda: potential hdac_stream locking issues? | expand |
On Fri, 24 Sep 2021 21:24:13 +0200, Pierre-Louis Bossart wrote: > > While reviewing the HDAudio DMA handling, I found a number of > inconsistencies in how spin_locks are used. It's not clear what the > HDaudio bus->reg_lock is supposed to protect. In most cases only the > writes to specific boolean status flags are protected, and there are > multiple cases of taking the lock after testing a status flag. > > This patchset suggests a more consistent locking pattern, but it's > entirely possible that the bus->reg_lock is only intented to protect > register read/write access on the HDaudio bus, and not the status > flags, and that this entire piece of code is completely > over-engineered. > > On the Intel side no one knows why this spinlock was used, the reasons > are lost to history. I set the 'RFC' status on purpose in the hope > that Takashi might recall what this lock is supposed to protect. The > diff format makes this patchset difficult to review, it's recommended > to apply the patches and look at entire functions with changes to get > a better idea of the suggested changes. Oh well, the missing piece was the lock inside the loop in *_stream_assign(). It was lost while factoring out and converting to the HD-audio core code. (The lock wasn't taken on the whole loop at that time because the list itself was supposed to be static.) In anyway, I applied all patches except for patch#2, as the fixes for spinlock look correct. thanks, Takashi
While reviewing the HDAudio DMA handling, I found a number of inconsistencies in how spin_locks are used. It's not clear what the HDaudio bus->reg_lock is supposed to protect. In most cases only the writes to specific boolean status flags are protected, and there are multiple cases of taking the lock after testing a status flag. This patchset suggests a more consistent locking pattern, but it's entirely possible that the bus->reg_lock is only intented to protect register read/write access on the HDaudio bus, and not the status flags, and that this entire piece of code is completely over-engineered. On the Intel side no one knows why this spinlock was used, the reasons are lost to history. I set the 'RFC' status on purpose in the hope that Takashi might recall what this lock is supposed to protect. The diff format makes this patchset difficult to review, it's recommended to apply the patches and look at entire functions with changes to get a better idea of the suggested changes. Pierre-Louis Bossart (4): ALSA: hda: hdac_stream: fix potential locking issue in snd_hdac_stream_assign() ALSA: hda: hdac_stream: reset assigned_key in stream_release() ALSA: hda: hdac_ext_stream: fix potential locking issues ASoC: SOF: Intel: hda-dai: fix potential locking issue include/sound/hdaudio_ext.h | 2 ++ sound/hda/ext/hdac_ext_stream.c | 46 ++++++++++++++++++++------------- sound/hda/hdac_stream.c | 5 ++-- sound/soc/sof/intel/hda-dai.c | 7 ++--- 4 files changed, 37 insertions(+), 23 deletions(-)