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>

diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
index 0cc7e9c01263f5ae193f3b80e40caefca412f67f..7012fdafc3d80207f78a21c3ac33e57e2aa0d974 100644 (file)
--- a/sound/usb/endpoint.c
+++ b/sound/usb/endpoint.c
@@ -169,11 +169,20 @@ int snd_usb_endpoint_next_packet_size(struct snd_usb_endpoint *ep)
        return ret;
 }
 
+static void call_retire_callback(struct snd_usb_endpoint *ep,
+                                struct urb *urb)
+{
+       struct snd_usb_substream *data_subs;
+
+       data_subs = READ_ONCE(ep->data_subs);
+       if (data_subs && ep->retire_data_urb)
+               ep->retire_data_urb(data_subs, urb);
+}
+
 static void retire_outbound_urb(struct snd_usb_endpoint *ep,
                                struct snd_urb_ctx *urb_ctx)
 {
-       if (ep->retire_data_urb)
-               ep->retire_data_urb(ep->data_subs, urb_ctx->urb);
+       call_retire_callback(ep, urb_ctx->urb);
 }
 
 static void retire_inbound_urb(struct snd_usb_endpoint *ep,
@@ -189,8 +198,7 @@ static void retire_inbound_urb(struct snd_usb_endpoint *ep,
        if (ep->sync_slave)
                snd_usb_handle_sync_urb(ep->sync_slave, ep, urb);
 
-       if (ep->retire_data_urb)
-               ep->retire_data_urb(ep->data_subs, urb);
+       call_retire_callback(ep, urb);
 }
 
 static void prepare_silent_urb(struct snd_usb_endpoint *ep,
@@ -244,17 +252,17 @@ static void prepare_outbound_urb(struct snd_usb_endpoint *ep,
 {
        struct urb *urb = ctx->urb;
        unsigned char *cp = urb->transfer_buffer;
+       struct snd_usb_substream *data_subs;
 
        urb->dev = ep->chip->dev; /* we need to set this at each time */
 
        switch (ep->type) {
        case SND_USB_ENDPOINT_TYPE_DATA:
-               if (ep->prepare_data_urb) {
-                       ep->prepare_data_urb(ep->data_subs, urb);
-               } else {
-                       /* no data provider, so send silence */
+               data_subs = READ_ONCE(ep->data_subs);
+               if (data_subs && ep->prepare_data_urb)
+                       ep->prepare_data_urb(data_subs, urb);
+               else /* no data provider, so send silence */
                        prepare_silent_urb(ep, ctx);
-               }
                break;
 
        case SND_USB_ENDPOINT_TYPE_SYNC:
@@ -381,7 +389,7 @@ static void snd_complete_urb(struct urb *urb)
 {
        struct snd_urb_ctx *ctx = urb->context;
        struct snd_usb_endpoint *ep = ctx->ep;
-       struct snd_pcm_substream *substream;
+       struct snd_usb_substream *data_subs;
        unsigned long flags;
        int err;
 
@@ -430,10 +438,9 @@ static void snd_complete_urb(struct urb *urb)
                return;
 
        usb_audio_err(ep->chip, "cannot submit urb (err = %d)\n", err);
-       if (ep->data_subs && ep->data_subs->pcm_substream) {
-               substream = ep->data_subs->pcm_substream;
-               snd_pcm_stop_xrun(substream);
-       }
+       data_subs = READ_ONCE(ep->data_subs);
+       if (data_subs && data_subs->pcm_substream)
+               snd_pcm_stop_xrun(data_subs->pcm_substream);
 
 exit_clear:
        clear_bit(ctx->index, &ep->active_mask);
@@ -532,6 +539,24 @@ void snd_usb_endpoint_set_syncinterval(struct snd_usb_audio *chip,
        }
 }
 
+/*
+ * Set data endpoint callbacks and the assigned data stream
+ *
+ * Called at PCM trigger and cleanups.
+ * Pass NULL to deactivate each callback.
+ */
+void snd_usb_endpoint_set_callback(struct snd_usb_endpoint *ep,
+                                  void (*prepare)(struct snd_usb_substream *subs,
+                                                  struct urb *urb),
+                                  void (*retire)(struct snd_usb_substream *subs,
+                                                 struct urb *urb),
+                                  struct snd_usb_substream *data_subs)
+{
+       ep->prepare_data_urb = prepare;
+       ep->retire_data_urb = retire;
+       WRITE_ONCE(ep->data_subs, data_subs);
+}
+
 /*
  *  wait until all urbs are processed.
  */
@@ -554,10 +579,8 @@ static int wait_clear_urbs(struct snd_usb_endpoint *ep)
                        alive, ep->ep_num);
        clear_bit(EP_FLAG_STOPPING, &ep->flags);
 
-       ep->data_subs = NULL;
        ep->sync_slave = NULL;
-       ep->retire_data_urb = NULL;
-       ep->prepare_data_urb = NULL;
+       snd_usb_endpoint_set_callback(ep, NULL, NULL, NULL);
 
        return 0;
 }
@@ -607,8 +630,7 @@ static void release_urbs(struct snd_usb_endpoint *ep, int force)
        int i;
 
        /* route incoming urbs to nirvana */
-       ep->retire_data_urb = NULL;
-       ep->prepare_data_urb = NULL;
+       snd_usb_endpoint_set_callback(ep, NULL, NULL, NULL);
 
        /* stop urbs */
        deactivate_urbs(ep, force);

diff --git a/sound/usb/endpoint.h b/sound/usb/endpoint.h
index 76b6de7de991f6d992a3bc7ef0c337aa88857962..e2fddb3dcf7ad7ef8e9d8ed96b6a574acace7c6e 100644 (file)
--- a/sound/usb/endpoint.h
+++ b/sound/usb/endpoint.h
@@ -20,6 +20,13 @@ int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep,
                                struct audioformat *fmt,
                                struct snd_usb_endpoint *sync_ep);
 
+void snd_usb_endpoint_set_callback(struct snd_usb_endpoint *ep,
+                                  void (*prepare)(struct snd_usb_substream *subs,
+                                                  struct urb *urb),
+                                  void (*retire)(struct snd_usb_substream *subs,
+                                                 struct urb *urb),
+                                  struct snd_usb_substream *data_subs);
+
 int  snd_usb_endpoint_start(struct snd_usb_endpoint *ep);
 void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep);
 void snd_usb_endpoint_sync_pending_stop(struct snd_usb_endpoint *ep);

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index c4e39aa92a84d2f967345d0b80cd36f05745a1de..32237623de96bbd63b5f55d6d91dd28140b9a044 100644 (file)
--- 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);