Bug 217539
| Summary: | snd_usb_audio: Device stuck in D1 state after resume | ||
|---|---|---|---|
| Product: | Drivers | Reporter: | Tatsuyuki Ishi (ishitatsuyuki) |
| Component: | Sound(ALSA) | Assignee: | Jaroslav Kysela (perex) |
| Status: | RESOLVED CODE_FIX | ||
| Severity: | normal | CC: | tiwai |
| Priority: | P3 | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Kernel Version: | Subsystem: | ||
| Regression: | No | Bisected commit-id: | |
| Attachments: | Hack to confirm that PM is the root cause | ||
|
Description
Tatsuyuki Ishi
2023-06-11 09:51:44 UTC
Created attachment 304395 [details]
Hack to confirm that PM is the root cause
So, simply changing the power state at prepare fixes the issue?
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -650,6 +650,10 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream)
goto unlock;
}
+ ret = snd_usb_pcm_change_state(subs, UAC3_PD_STATE_D0);
+ if (ret < 0)
+ goto unlock;
+
again:
if (subs->sync_endpoint) {
ret = snd_usb_endpoint_prepare(chip, subs->sync_endpoint);
hw_params isn't called in a usual resume path, hence the current code failed.
Looks good to me, will test later. Patch in comment #2 works fine, thanks. Additionally it seems we can remove this call from hw_params since all the setup URBs are flushed only once prepare is called. diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index eec5232f9fb2..b7428a23647e 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -520,10 +520,6 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream, if (ret < 0) goto stop_pipeline; - ret = snd_usb_pcm_change_state(subs, UAC3_PD_STATE_D0); - if (ret < 0) - goto unlock; - if (subs->data_endpoint) { if (snd_usb_endpoint_compatible(chip, subs->data_endpoint, fmt, hw_params)) Well, that's another code path, and in theory, application can issue the hw_params step before the prepare step, and that'll hit a similar problem. So it's still safer to keep the power state change code there, too. We may optimize the power state change by caching the value and checking it before actually executing the USB command, if it really matters, too. > Well, that's another code path, and in theory, application can issue the > hw_params step before the prepare step, and that'll hit a similar problem. > So it's still safer to keep the power state change code there, too. A call to hw_params() put the stream in SND_PCM_STATE_SETUP state, and this needs to always be followed by a prepare() in order to proceed to starting the stream. Hence, I don't see how we will "hit a similar problem". > We may optimize the power state change by caching the value and checking it > before actually executing the USB command, if it really matters, too. Looking at debug logs it seems it already avoids redundant state changes. So this is more or less a code flow clarity thing. I don't really mind keeping the code if you want to be absolutely safe, so up to you. (In reply to Tatsuyuki Ishi from comment #6) > > Well, that's another code path, and in theory, application can issue the > > hw_params step before the prepare step, and that'll hit a similar problem. > > So it's still safer to keep the power state change code there, too. > > A call to hw_params() put the stream in SND_PCM_STATE_SETUP state, and this > needs to always be followed by a prepare() in order to proceed to starting > the stream. Hence, I don't see how we will "hit a similar problem". In general, hw_params does set up the hardware parameters, and it implies that the power gets turned on at this stage. Although USB-audio driver tries to avoid touching the hardware parameters at hw_parms call as much as possible, it'd be error-prone to leave in an unknown power state. So yes, I'd like to be in the safer side and keep the power up explicitly there on purpose. The fix submitted to the upstream: https://lore.kernel.org/all/20230612132818.29486-1-tiwai@suse.de/ Fix live with 6.3.9 or 6.4. |