Feedback-ID: ie8e14432:Fastmail
Date:   Sat, 18 Mar 2023 09:20:05 +0900
From:   Takashi Sakamoto <o-takashi@sakamocchi.jp>
To:     John Keeping <john@metanate.com>
Cc:     Takashi Iwai <tiwai@suse.com>,
        "moderated list:SOUND" <alsa-devel@alsa-project.org>,
        open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] ALSA: usb-audio: Fix recursive locking on XRUN
Message-ID: <20230318002005.GA84781@workstation>
Mail-Followup-To: John Keeping <john@metanate.com>,
        Takashi Iwai <tiwai@suse.com>,
        "moderated list:SOUND" <alsa-devel@alsa-project.org>,
        open list <linux-kernel@vger.kernel.org>
References: <20230317195128.3911155-1-john@metanate.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230317195128.3911155-1-john@metanate.com>
Precedence: bulk

Hi,

On Fri, Mar 17, 2023 at 07:51:27PM +0000, John Keeping wrote:
> snd_usb_queue_pending_output_urbs() may be called from
> snd_pcm_ops::ack() which means the PCM stream is locked.
> 
> For the normal case where the call back into the PCM core is via
> prepare_output_urb() the "_under_stream_lock" variant of
> snd_pcm_period_elapsed() is called, but when an error occurs and the
> stream is stopped as XRUN then snd_pcm_xrun() tries to recursively lock
> the stream which results in deadlock.
> 
> Follow the example of snd_pcm_period_elapsed() by adding
> snd_pcm_xrun_under_stream_lock() and use this when the PCM substream
> lock is already held.
> 
> Signed-off-by: John Keeping <john@metanate.com>
> ---
>  include/sound/pcm.h     |  1 +
>  sound/core/pcm_native.c | 28 ++++++++++++++++++++++++----
>  sound/usb/endpoint.c    | 18 +++++++++++-------
>  3 files changed, 36 insertions(+), 11 deletions(-)
 
The name of added kernel API implies me that you refer to existent
'snd_pcm_period_elapsed_under_stream_lock()' which I added to Linux
v5.14.

In my opinion, unlike the version of period elapsed API, the version of
XRUN API seems not to be necessarily required to ALSA PCM core, since PCM
device drivers can implement .pointer callback in the part of PCM operation.
When the callback returns SNDRV_PCM_POS_XRUN, ALSA PCM application get
occurence of XRUN as a result of any operation relevant to hwptr movement
(e.g. SNDRV_PCM_IOCTL_HWSYNC).

Therefore I think it possible to fix the issue without the proposed
kernel API. I can assume some scenario:

1. Failure at tasklet for URB completion

It is softIRQ context. The stream lock is not acquired. It doesn't
matter to call current XRUN API.

2. Failure at PCM operation called by ALSA PCM application

It is process context. The stream lock is acquired before calling driver
code. When detecting any type of failure, driver code stores the state.
Then .pointer callback should return SNDRV_PCM_IOCTL_HWSYNC refering to
the state.

Of course, I'm not a developer for USB audio devices. I'm just a developer
for the other type of packet-oriented drivers (IEC 61883-1/6 packet
streaming engine for audio and music unit in IEEE 1394 bus). So I do not
get every part of USB driver. However, from my experience for the
packet-oriented drivers, I have the above concern about adding the new
XRUN API.

I apologize if miss-hitting the point for your issue.


Regards

Takashi Sakamoto