ALSA: usb-audio: Set callbacks via snd_usb_endpoint_set_callback()

The prepare_data_urb and retire_data_urb fields of the endpoint object
are set dynamically at PCM trigger start/stop.  Those are evaluated in
the endpoint handler, but there can be a race, especially if two
different PCM substreams are handling the same endpoint for the
implicit feedback case.  Also, the data_subs field of the endpoint is
set and accessed dynamically, too, which has the same risk.

As a slight improvement for the concurrency, this patch introduces the
function to set the callbacks and the data in a shot with the memory
barrier.  In the reader side, it's also fetched with the memory
barrier.

There is still a room of race if prepare and retire callbacks are set
during executing the URB completion.  But such an inconsistency may
happen only for the implicit fb source, i.e. it's only about the
capture stream.  And luckily, the capture stream never sets the
prepare callback, hence the problem doesn't happen practically.

Tested-by: Keith Milner <kamilner@superlative.org>
Tested-by: Dylan Robinson <dylan_robinson@motu.com>
Link: https://lore.kernel.org/r/20201123085347.19667-23-tiwai@suse.de
Signed-off-by: Takashi Iwai <tiwai@suse.de>

1 files changed, 18 insertions, 15 deletions

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index c4e39aa92a84..32237623de96 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -232,7 +232,6 @@ static int start_endpoints(struct snd_usb_substream *subs)
 	if (!test_and_set_bit(SUBSTREAM_FLAG_DATA_EP_STARTED, &subs->flags)) {
 		struct snd_usb_endpoint *ep = subs->data_endpoint;
 
-		ep->data_subs = subs;
 		err = snd_usb_endpoint_start(ep);
 		if (err < 0) {
 			clear_bit(SUBSTREAM_FLAG_DATA_EP_STARTED, &subs->flags);
@@ -1830,18 +1829,24 @@ static int snd_usb_substream_playback_trigger(struct snd_pcm_substream *substrea
 		subs->trigger_tstamp_pending_update = true;
 		fallthrough;
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
-		subs->data_endpoint->prepare_data_urb = prepare_playback_urb;
-		subs->data_endpoint->retire_data_urb = retire_playback_urb;
+		snd_usb_endpoint_set_callback(subs->data_endpoint,
+					      prepare_playback_urb,
+					      retire_playback_urb,
+					      subs);
 		subs->running = 1;
 		return 0;
 	case SNDRV_PCM_TRIGGER_STOP:
 		stop_endpoints(subs);
+		snd_usb_endpoint_set_callback(subs->data_endpoint,
+					      NULL, NULL, NULL);
 		subs->running = 0;
 		return 0;
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
-		subs->data_endpoint->prepare_data_urb = NULL;
 		/* keep retire_data_urb for delay calculation */
-		subs->data_endpoint->retire_data_urb = retire_playback_urb;
+		snd_usb_endpoint_set_callback(subs->data_endpoint,
+					      NULL,
+					      retire_playback_urb,
+					      subs);
 		subs->running = 0;
 		return 0;
 	case SNDRV_PCM_TRIGGER_SUSPEND:
@@ -1867,23 +1872,21 @@ static int snd_usb_substream_capture_trigger(struct snd_pcm_substream *substream
 		err = start_endpoints(subs);
 		if (err < 0)
 			return err;
-
-		subs->data_endpoint->retire_data_urb = retire_capture_urb;
+		fallthrough;
+	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+		snd_usb_endpoint_set_callback(subs->data_endpoint,
+					      NULL, retire_capture_urb,
+					      subs);
 		subs->running = 1;
 		return 0;
 	case SNDRV_PCM_TRIGGER_STOP:
 		stop_endpoints(subs);
-		subs->data_endpoint->retire_data_urb = NULL;
-		subs->running = 0;
-		return 0;
+		fallthrough;
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
-		subs->data_endpoint->retire_data_urb = NULL;
+		snd_usb_endpoint_set_callback(subs->data_endpoint,
+					      NULL, NULL, NULL);
 		subs->running = 0;
 		return 0;
-	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
-		subs->data_endpoint->retire_data_urb = retire_capture_urb;
-		subs->running = 1;
-		return 0;
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 		if (subs->stream->chip->setup_fmt_after_resume_quirk) {
 			stop_endpoints(subs);