All of lore.kernel.org
 help / color / mirror / Atom feed
From: Takashi Iwai <tiwai@suse.de>
To: alsa-devel@alsa-project.org
Cc: Matwey Kornilov <matwey.kornilov@gmail.com>,
	Dylan Robinson <dylan_robinson@motu.com>,
	Keith Milner <kamilner@superlative.org>
Subject: [PATCH 22/41] ALSA: usb-audio: Set callbacks via snd_usb_endpoint_set_callback()
Date: Mon, 23 Nov 2020 09:53:28 +0100	[thread overview]
Message-ID: <20201123085347.19667-23-tiwai@suse.de> (raw)
In-Reply-To: <20201123085347.19667-1-tiwai@suse.de>

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>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/usb/endpoint.c | 60 +++++++++++++++++++++++++++++++++++-----------------
 sound/usb/endpoint.h |  7 ++++++
 sound/usb/pcm.c      | 33 ++++++++++++++++-------------
 3 files changed, 66 insertions(+), 34 deletions(-)

diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
index 0cc7e9c01263..7012fdafc3d8 100644
--- 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 76b6de7de991..e2fddb3dcf7a 100644
--- 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 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);
-- 
2.16.4


  parent reply	other threads:[~2020-11-23  9:06 UTC|newest]

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-23  8:53 [PATCH 00/41] USB audio refactoring for better implicit feedback support Takashi Iwai
2020-11-23  8:53 ` [PATCH 01/41] ALSA: usb-audio: Handle discrete rates properly in hw constraints Takashi Iwai
2020-11-23  8:53 ` [PATCH 02/41] ALSA: usb-audio: Don't call usb_set_interface() at trigger callback Takashi Iwai
2020-11-23  8:53 ` [PATCH 03/41] ALSA: usb-audio: Check valid altsetting at parsing rates for UAC2/3 Takashi Iwai
2020-11-23  8:53 ` [PATCH 04/41] ALSA: usb-audio: Check implicit feedback EP generically for UAC2 Takashi Iwai
2020-11-23  8:53 ` [PATCH 05/41] ALSA: usb-audio: Add snd_usb_get_endpoint() helper Takashi Iwai
2020-11-23  8:53 ` [PATCH 06/41] ALSA: usb-audio: Set and clear sync EP link properly Takashi Iwai
2020-11-23  8:53 ` [PATCH 07/41] ALSA: usb-audio: Improve some debug prints Takashi Iwai
2020-11-23  8:53 ` [PATCH 08/41] ALSA: usb-audio: Track implicit fb sync endpoint in audioformat list Takashi Iwai
2020-11-23  8:53 ` [PATCH 09/41] ALSA: usb-audio: Move snd_usb_autoresume() call out of setup_hw_info() Takashi Iwai
2020-11-23  8:53 ` [PATCH 10/41] ALSA: usb-audio: Add hw constraint for implicit fb sync Takashi Iwai
2020-11-23  8:53 ` [PATCH 11/41] ALSA: usb-audio: Simplify hw_params rules Takashi Iwai
2020-11-23  8:53 ` [PATCH 12/41] ALSA: usb-audio: Drop debug.h Takashi Iwai
2020-11-23  8:53 ` [PATCH 13/41] ALSA: usb-audio: Avoid doubly initialization for implicit fb Takashi Iwai
2020-11-23  8:53 ` [PATCH 14/41] ALSA: usb-audio: Create endpoint objects at parsing phase Takashi Iwai
2021-01-03 17:09   ` [PATCH 14/41] ALSA: usb-audio: Create endpoint objects at parsing phase - Pioneer DJ DJM-250MK2 stopped working František Kučera
2021-01-03 17:19     ` Takashi Iwai
2021-01-03 18:15       ` František Kučera
2021-01-05  9:29         ` Takashi Iwai
2021-01-05 13:20           ` Takashi Iwai
2021-01-05 22:27             ` František Kučera
2021-01-06  9:03               ` Takashi Iwai
2021-01-06 19:01                 ` František Kučera
2021-01-07 13:30                   ` Takashi Iwai
2020-11-23  8:53 ` [PATCH 15/41] ALSA: usb-audio: Drop keep_interface flag again Takashi Iwai
2020-11-23  8:53 ` [PATCH 16/41] ALSA: usb-audio: Add snd_usb_get_host_interface() helper Takashi Iwai
2020-11-23  8:53 ` [PATCH 17/41] ALSA: usb-audio: Don't set altsetting before initializing sample rate Takashi Iwai
2020-11-23  8:53 ` [PATCH 18/41] ALSA: usb-audio: Pass snd_usb_audio object to quirk functions Takashi Iwai
2020-11-23  8:53 ` [PATCH 19/41] ALSA: usb-audio: Simplify snd_usb_init_sample_rate() arguments Takashi Iwai
2020-11-23  8:53 ` [PATCH 20/41] ALSA: usb-audio: Simplify snd_usb_init_pitch() arguments Takashi Iwai
2020-11-23  8:53 ` [PATCH 21/41] ALSA: usb-audio: Stop both endpoints properly at error Takashi Iwai
2020-11-23  8:53 ` Takashi Iwai [this message]
2020-11-23  8:53 ` [PATCH 23/41] ALSA: usb-audio: Always set up the parameters after resume Takashi Iwai
2020-11-23  8:53 ` [PATCH 24/41] ALSA: usb-audio: Fix EP matching for continuous rates Takashi Iwai
2020-11-23  8:53 ` [PATCH 25/41] ALSA: usb-audio: Refactor endpoint management Takashi Iwai
2020-11-23  8:53 ` [PATCH 26/41] ALSA: usb-audio: Fix possible stall of implicit fb packet ring-buffer Takashi Iwai
2020-11-23  8:53 ` [PATCH 27/41] ALSA: usb-audio: Constify audioformat pointer references Takashi Iwai
2020-11-23  8:53 ` [PATCH 28/41] ALSA: usb-audio: Use atomic_t for endpoint use_count Takashi Iwai
2020-11-23  8:53 ` [PATCH 29/41] ALSA: usb-audio: Refactoring endpoint URB deactivation Takashi Iwai
2020-11-23  8:53 ` [PATCH 30/41] ALSA: usb-audio: Drop unneeded snd_usb_substream fields Takashi Iwai
2020-11-23  8:53 ` [PATCH 31/41] ALSA: usb-audio: Unify the code for the next packet size calculation Takashi Iwai
2020-11-23  8:53 ` [PATCH 32/41] ALSA: usb-audio: Simplify rate_min/max and rates set up Takashi Iwai
2020-11-23  8:53 ` [PATCH 33/41] ALSA: usb-audio: Replace slave/master terms Takashi Iwai
2020-11-23  8:53 ` [PATCH 34/41] ALSA: usb-audio: Use unsigned char for iface and altsettings fields Takashi Iwai
2020-11-23  8:53 ` [PATCH 35/41] ALSA: usb-audio: Show sync endpoint information in proc outputs Takashi Iwai
2020-11-23  8:53 ` [PATCH 36/41] ALSA: usb-audio: Quirk for BOSS GT-001 Takashi Iwai
2020-11-23  8:53 ` [PATCH 37/41] ALSA: usb-audio: Factor out the implicit feedback quirk code Takashi Iwai
2020-11-23  8:53 ` [PATCH 38/41] ALSA: usb-audio: Add generic implicit fb parsing Takashi Iwai
2020-11-23  8:53 ` [PATCH 39/41] ALSA: usb-audio: Add implicit_fb module option Takashi Iwai
2020-11-23  9:46   ` Pavel Hofman
2020-11-23  8:53 ` [PATCH 40/41] ALSA: usb-audio: Fix quirks for other BOSS devices Takashi Iwai
2020-11-23  8:53 ` [PATCH 41/41] ALSA: usb-audio: Fix MOTU M-Series quirks Takashi Iwai

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201123085347.19667-23-tiwai@suse.de \
    --to=tiwai@suse.de \
    --cc=alsa-devel@alsa-project.org \
    --cc=dylan_robinson@motu.com \
    --cc=kamilner@superlative.org \
    --cc=matwey.kornilov@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.