| Message ID | 20230321022642.1426611-1-yung-chuan.liao@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: intel: don't save hw_params for use in prepare | expand |
On 21-03-23, 10:26, Bard Liao wrote: > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > > The existing code copies the hw_params pointer and reuses it later in > .prepare, specifically to re-initialize the ALH DMA channel > information that's lost in suspend-resume cycles. > > This is not needed, we can directly access the information from the > substream/rtd - as done for the HDAudio DAIs in > sound/soc/sof/intel/hda-dai.c > > In addition, using the saved pointer causes the suspend-resume test > cases to fail on specific platforms, depending on which version of GCC > is used. Péter Ujfalusi and I have spent long hours to root-cause this > problem that was reported by the Intel CI first with 6.2-rc1 and again > v6.3-rc1. In the latter case we were lucky that the problem was 100% > reproducible on local test devices, and found out that adding a > dev_dbg() or adding a call to usleep_range() just before accessing the > saved pointer "fixed" the issue. With errors appearing just by > changing the compiler version or minor changes in the code generated, > clearly we have a memory management Heisenbug. > > The root-cause seems to be that the hw_params pointer is not > persistent. The soc-pcm code allocates the hw_params structure on the > stack, and passes it to the BE dailink hw_params and DAIs > hw_params. Saving such a pointer and reusing it later during the > .prepare stage cannot possibly work reliably, it's broken-by-design > since v5.10. It's astonishing that the problem was not seen earlier. > > This simple fix will have to be back-ported to -stable, due to changes > to avoid the use of the get/set_dmadata routines this patch will only > apply on kernels older than v6.1. Applied, thanks
diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h index b653734085d9..27c56274217f 100644 --- a/drivers/soundwire/cadence_master.h +++ b/drivers/soundwire/cadence_master.h @@ -84,7 +84,6 @@ struct sdw_cdns_stream_config { * @bus: Bus handle * @stream_type: Stream type * @link_id: Master link id - * @hw_params: hw_params to be applied in .prepare step * @suspended: status set when suspended, to be used in .prepare * @paused: status set in .trigger, to be used in suspend * @direction: stream direction @@ -96,7 +95,6 @@ struct sdw_cdns_dai_runtime { struct sdw_bus *bus; enum sdw_stream_type stream_type; int link_id; - struct snd_pcm_hw_params *hw_params; bool suspended; bool paused; int direction; diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 046c67a2a39b..238acf5c97a9 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -725,7 +725,6 @@ static int intel_hw_params(struct snd_pcm_substream *substream, dai_runtime->paused = false; dai_runtime->suspended = false; dai_runtime->pdi = pdi; - dai_runtime->hw_params = params; /* Inform DSP about PDI stream number */ ret = intel_params_stream(sdw, substream->stream, dai, params, @@ -778,6 +777,11 @@ static int intel_prepare(struct snd_pcm_substream *substream, } if (dai_runtime->suspended) { + struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); + struct snd_pcm_hw_params *hw_params; + + hw_params = &rtd->dpcm[substream->stream].hw_params; + dai_runtime->suspended = false; /* @@ -789,7 +793,7 @@ static int intel_prepare(struct snd_pcm_substream *substream, */ /* configure stream */ - ch = params_channels(dai_runtime->hw_params); + ch = params_channels(hw_params); if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) dir = SDW_DATA_DIR_RX; else @@ -801,7 +805,7 @@ static int intel_prepare(struct snd_pcm_substream *substream, /* Inform DSP about PDI stream number */ ret = intel_params_stream(sdw, substream->stream, dai, - dai_runtime->hw_params, + hw_params, sdw->instance, dai_runtime->pdi->intel_alh_id); } @@ -840,7 +844,6 @@ intel_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) return ret; } - dai_runtime->hw_params = NULL; dai_runtime->pdi = NULL; return 0;