| Message ID | 20221110141330.740916-3-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: Intel: avs: DSP recovery and resume fixes | expand |
On 11/10/22 08:13, Cezary Rojewski wrote: > To improve performance and overall system stability, suspend/resume > operations for ASoC cards always return success status and defer the > actual work. > > Because of that, if a substream fails to resume, userspace may still > attempt to invoke commands on it as from their perspective the operation > completed successfully. Set substream's state to DISCONNECTED to ensure > no further commands are attempted. > > Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> > --- > sound/soc/intel/avs/pcm.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/sound/soc/intel/avs/pcm.c b/sound/soc/intel/avs/pcm.c > index ca624fbb5c0d..f95c530ffeb1 100644 > --- a/sound/soc/intel/avs/pcm.c > +++ b/sound/soc/intel/avs/pcm.c > @@ -934,8 +934,11 @@ static int avs_component_pm_op(struct snd_soc_component *component, bool be, > rtd = snd_pcm_substream_chip(data->substream); > if (rtd->dai_link->no_pcm == be && !rtd->dai_link->ignore_suspend) { > ret = op(dai, data); > - if (ret < 0) > + if (ret < 0) { > + data->substream->runtime->status->state = > + SNDRV_PCM_STATE_DISCONNECTED; should runtime->state be used instead of runtime->status->state? A quick grep shows the former seems more popular in drivers, status->seems to be only used in pcm_native.c? Another plumbing question is whether it's actually ok to change the state of the runtime in a driver, are you not going to have locking issues? Very few drivers change the internal state and I wonder how legit/safe this is. > return ret; > + } > } > } > > @@ -944,8 +947,11 @@ static int avs_component_pm_op(struct snd_soc_component *component, bool be, > rtd = snd_pcm_substream_chip(data->substream); > if (rtd->dai_link->no_pcm == be && !rtd->dai_link->ignore_suspend) { > ret = op(dai, data); > - if (ret < 0) > + if (ret < 0) { > + data->substream->runtime->status->state = > + SNDRV_PCM_STATE_DISCONNECTED; > return ret; > + } > } > } > }
On 2022-11-10 4:39 PM, Pierre-Louis Bossart wrote: > On 11/10/22 08:13, Cezary Rojewski wrote: >> --- a/sound/soc/intel/avs/pcm.c >> +++ b/sound/soc/intel/avs/pcm.c >> @@ -934,8 +934,11 @@ static int avs_component_pm_op(struct snd_soc_component *component, bool be, >> rtd = snd_pcm_substream_chip(data->substream); >> if (rtd->dai_link->no_pcm == be && !rtd->dai_link->ignore_suspend) { >> ret = op(dai, data); >> - if (ret < 0) >> + if (ret < 0) { >> + data->substream->runtime->status->state = >> + SNDRV_PCM_STATE_DISCONNECTED; > > should runtime->state be used instead of runtime->status->state? > > A quick grep shows the former seems more popular in drivers, > status->seems to be only used in pcm_native.c? > > Another plumbing question is whether it's actually ok to change the > state of the runtime in a driver, are you not going to have locking > issues? Very few drivers change the internal state and I wonder how > legit/safe this is. Unless something new has been added/updated, there is no runtime->state field available. All the PCM code works with runtime->status->state. component->resume() gets fired before any PCMs have a chance to be resumed. Userspace cannot access us _yet_. Similarly for suspend, component->suspend() is called once all the streams receive TRIGGER_SUSPEND and from then on, we're on device-pm level already. Well, in regard to grep, very few drivers enter the recovery jungle. All of this is to help improve user experience when things go south. Unfortunately for us, restoring regmap is insufficient to get AudioDSP happy. Right now if things do go south, userspace still performs all of the PCM commands on the stream as nothing has happened - snd_soc_suspend/resume() return 0 in all cases. Regards, Czarek
On 11/10/22 10:06, Cezary Rojewski wrote: > On 2022-11-10 4:39 PM, Pierre-Louis Bossart wrote: >> On 11/10/22 08:13, Cezary Rojewski wrote: > >>> --- a/sound/soc/intel/avs/pcm.c >>> +++ b/sound/soc/intel/avs/pcm.c >>> @@ -934,8 +934,11 @@ static int avs_component_pm_op(struct >>> snd_soc_component *component, bool be, >>> rtd = snd_pcm_substream_chip(data->substream); >>> if (rtd->dai_link->no_pcm == be && >>> !rtd->dai_link->ignore_suspend) { >>> ret = op(dai, data); >>> - if (ret < 0) >>> + if (ret < 0) { >>> + data->substream->runtime->status->state = >>> + SNDRV_PCM_STATE_DISCONNECTED; >> >> should runtime->state be used instead of runtime->status->state? >> >> A quick grep shows the former seems more popular in drivers, >> status->seems to be only used in pcm_native.c? >> >> Another plumbing question is whether it's actually ok to change the >> state of the runtime in a driver, are you not going to have locking >> issues? Very few drivers change the internal state and I wonder how >> legit/safe this is. > > > Unless something new has been added/updated, there is no runtime->state > field available. All the PCM code works with runtime->status->state. cd sound/ git grep -c 'runtime->state' core/compress_offload.c:27 core/oss/pcm_oss.c:21 core/pcm.c:2 core/pcm_compat.c:2 core/pcm_lib.c:8 core/pcm_native.c:50 drivers/aloop.c:2 firewire/bebob/bebob_pcm.c:2 firewire/dice/dice-pcm.c:2 firewire/digi00x/digi00x-pcm.c:2 firewire/fireface/ff-pcm.c:2 firewire/fireworks/fireworks_pcm.c:2 firewire/motu/motu-pcm.c:2 firewire/oxfw/oxfw-pcm.c:4 firewire/tascam/tascam-pcm.c:2 hda/hdmi_chmap.c:1 isa/gus/gus_pcm.c:1 soc/intel/skylake/skl-pcm.c:2 soc/sh/rz-ssi.c:1 usb/pcm.c:2 usb/usx2y/usbusx2yaudio.c:1 usb/usx2y/usx2yhwdeppcm.c:1 git grep -c 'status->state' core/pcm_compat.c:2 core/pcm_native.c:4
On 2022-11-10 5:18 PM, Pierre-Louis Bossart wrote: > On 11/10/22 10:06, Cezary Rojewski wrote: >> Unless something new has been added/updated, there is no runtime->state >> field available. All the PCM code works with runtime->status->state. > > cd sound/ > > git grep -c 'runtime->state' > core/compress_offload.c:27 ... > git grep -c 'status->state' > core/pcm_compat.c:2 > core/pcm_native.c:4 I see now, thanks. Commit from late September "ALSA: pcm: Avoid reference to status->state" add a new field. Will update the code to use __snd_pcm_set_state() instead. My base did not have it yet.
On 11/10/22 10:29, Cezary Rojewski wrote: > On 2022-11-10 5:18 PM, Pierre-Louis Bossart wrote: >> On 11/10/22 10:06, Cezary Rojewski wrote: > >>> Unless something new has been added/updated, there is no runtime->state >>> field available. All the PCM code works with runtime->status->state. >> >> cd sound/ >> >> git grep -c 'runtime->state' >> core/compress_offload.c:27 > > ... > >> git grep -c 'status->state' >> core/pcm_compat.c:2 >> core/pcm_native.c:4 > > I see now, thanks. Commit from late September "ALSA: pcm: Avoid > reference to status->state" add a new field. Will update the code to use > __snd_pcm_set_state() instead. > > My base did not have it yet. Right, it's relatively recent, and my point is that the helper does use stream locking when testing the same state you modify. Maybe that's a red herring but I thought I'd ask. static void snd_pcm_set_state(struct snd_pcm_substream *substream, snd_pcm_state_t state) { snd_pcm_stream_lock_irq(substream); if (substream->runtime->state != SNDRV_PCM_STATE_DISCONNECTED) __snd_pcm_set_state(substream->runtime, state); snd_pcm_stream_unlock_irq(substream); }
On 2022-11-10 5:36 PM, Pierre-Louis Bossart wrote: > On 11/10/22 10:29, Cezary Rojewski wrote: >> On 2022-11-10 5:18 PM, Pierre-Louis Bossart wrote: >>> On 11/10/22 10:06, Cezary Rojewski wrote: >> >>>> Unless something new has been added/updated, there is no runtime->state >>>> field available. All the PCM code works with runtime->status->state. >>> >>> cd sound/ >>> >>> git grep -c 'runtime->state' >>> core/compress_offload.c:27 >> >> ... >> >>> git grep -c 'status->state' >>> core/pcm_compat.c:2 >>> core/pcm_native.c:4 >> >> I see now, thanks. Commit from late September "ALSA: pcm: Avoid >> reference to status->state" add a new field. Will update the code to use >> __snd_pcm_set_state() instead. >> >> My base did not have it yet. > > Right, it's relatively recent, and my point is that the helper does use > stream locking when testing the same state you modify. Maybe that's a > red herring but I thought I'd ask. > > static void snd_pcm_set_state(struct snd_pcm_substream *substream, > snd_pcm_state_t state) > { > snd_pcm_stream_lock_irq(substream); > if (substream->runtime->state != SNDRV_PCM_STATE_DISCONNECTED) > __snd_pcm_set_state(substream->runtime, state); > snd_pcm_stream_unlock_irq(substream); > } > Your question is a right one and this is the right time to talk about locking. Our current test results and my analysis show that locking is not needed (what isn't the case for the first patch in the series) but races such as this one are hard to reproduce. If I'm proven wrong, no problem updating the code on my side.
diff --git a/sound/soc/intel/avs/pcm.c b/sound/soc/intel/avs/pcm.c index ca624fbb5c0d..f95c530ffeb1 100644 --- a/sound/soc/intel/avs/pcm.c +++ b/sound/soc/intel/avs/pcm.c @@ -934,8 +934,11 @@ static int avs_component_pm_op(struct snd_soc_component *component, bool be, rtd = snd_pcm_substream_chip(data->substream); if (rtd->dai_link->no_pcm == be && !rtd->dai_link->ignore_suspend) { ret = op(dai, data); - if (ret < 0) + if (ret < 0) { + data->substream->runtime->status->state = + SNDRV_PCM_STATE_DISCONNECTED; return ret; + } } } @@ -944,8 +947,11 @@ static int avs_component_pm_op(struct snd_soc_component *component, bool be, rtd = snd_pcm_substream_chip(data->substream); if (rtd->dai_link->no_pcm == be && !rtd->dai_link->ignore_suspend) { ret = op(dai, data); - if (ret < 0) + if (ret < 0) { + data->substream->runtime->status->state = + SNDRV_PCM_STATE_DISCONNECTED; return ret; + } } } }
To improve performance and overall system stability, suspend/resume operations for ASoC cards always return success status and defer the actual work. Because of that, if a substream fails to resume, userspace may still attempt to invoke commands on it as from their perspective the operation completed successfully. Set substream's state to DISCONNECTED to ensure no further commands are attempted. Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> --- sound/soc/intel/avs/pcm.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)