[PATCH 0/2] ALSA: Don't share mixer->urb with the FCP/scarlett2 notification endpoint

[PATCH 0/2] ALSA: Don't share mixer->urb with the FCP/scarlett2 notification endpoint

[Geoffrey D. Bennett]

Hi Takashi,

Both the FCP and scarlett2 drivers used mixer->urb for their
notification endpoint URB, but that field belongs to
snd_usb_mixer_status_create() and is managed by mixer.c. Sharing it
means the "already set up" check in the driver's init_notify() can be
satisfied by the status endpoint's URB, so the driver's own URB is
never submitted and the first command would crash in
wait_for_completion_timeout() on the still-zeroed cmd_done. mixer.c
can also kill, resubmit or free the URB from under the driver.

Give each driver its own URB in its private data. For scarlett2 this
also means taking over the free/suspend/resume handling that mixer.c
was doing for it.

Tested on an ISA C8X and a Scarlett 18i20 Gen 4 (FCP), and a Scarlett
4i4 Gen 3 and 4i4 Gen 4 (scarlett2) on 7.1.7, including suspend/resume
and configuration save on suspend.

Thanks,
Geoffrey

Geoffrey D. Bennett (2):
  ALSA: FCP: Use a private URB for the notification endpoint
  ALSA: scarlett2: Use a private URB for the notification endpoint

 sound/usb/fcp.c             | 38 +++++++-------
 sound/usb/mixer.c           |  6 +++
 sound/usb/mixer.h           |  2 +
 sound/usb/mixer_scarlett2.c | 98 ++++++++++++++++++++++++-------------
 4 files changed, 92 insertions(+), 52 deletions(-)

-- 
2.55.0

[PATCH 1/2] ALSA: FCP: Use a private URB for the notification endpoint

[Geoffrey D. Bennett]

fcp_init_notify() used mixer->urb, which snd_usb_mixer_status_create()
allocates for the optional UAC2 status interrupt endpoint and mixer.c
kills, resubmits and frees. On a device with that endpoint,
fcp_init_notify()'s "already set up" early return fires on the status
URB and returns success without doing anything. No FCP notification
URB is submitted, and cmd_done is left zeroed because it is
initialised past that early return and nowhere else. fcp_init() then
issues init1_opcode and wait_for_completion_timeout() would crash
adding to the zeroed wait.head. fcp_cleanup_urb() would also kill and
free mixer.c's status URB.

Use a separate URB in fcp_data, and initialise cmd_done in
fcp_init_private() where fcp_data is allocated. fcp_init_notify() is
reached again after suspend via fcp_reinit(), and the URB kill path in
fcp_notify() completes cmd_done, leaving a stale count that would
satisfy the next command's wait before the device ACKs. Use
reinit_completion() to clear it.

Fixes: 46757a3e7d50 ("ALSA: FCP: Add Focusrite Control Protocol driver")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5
Signed-off-by: Geoffrey D. Bennett <g@b4.vu>
---
 sound/usb/fcp.c | 38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/sound/usb/fcp.c b/sound/usb/fcp.c
index 6f5dcd35e1d4..6c7da5508bc3 100644
--- a/sound/usb/fcp.c
+++ b/sound/usb/fcp.c
@@ -82,6 +82,7 @@ struct fcp_data {
 	struct mutex mutex;         /* serialise access to the device */
 	struct completion cmd_done; /* wait for command completion */
 	struct file *file;          /* hwdep file */
+	struct urb *urb;            /* FCP notification endpoint */
 
 	struct fcp_notify notify;
 
@@ -186,7 +187,7 @@ static int fcp_usb(struct usb_mixer_interface *mixer, u32 opcode,
 	const int max_retries = 5;
 	int err;
 
-	if (!mixer->urb)
+	if (!private->urb)
 		return -ENODEV;
 
 	struct fcp_usb_packet *req __free(kfree) = NULL;
@@ -301,7 +302,7 @@ static int fcp_reinit(struct usb_mixer_interface *mixer)
 {
 	struct fcp_data *private = mixer->private_data;
 
-	if (mixer->urb)
+	if (private->urb)
 		return 0;
 
 	void *step0_resp __free(kfree) =
@@ -893,13 +894,15 @@ static int fcp_hwdep_init(struct usb_mixer_interface *mixer)
 
 static void fcp_cleanup_urb(struct usb_mixer_interface *mixer)
 {
-	if (!mixer->urb)
+	struct fcp_data *private = mixer->private_data;
+
+	if (!private->urb)
 		return;
 
-	usb_kill_urb(mixer->urb);
-	kfree(mixer->urb->transfer_buffer);
-	usb_free_urb(mixer->urb);
-	mixer->urb = NULL;
+	usb_kill_urb(private->urb);
+	kfree(private->urb->transfer_buffer);
+	usb_free_urb(private->urb);
+	private->urb = NULL;
 }
 
 static void fcp_private_free(struct usb_mixer_interface *mixer)
@@ -970,37 +973,37 @@ static int fcp_init_notify(struct usb_mixer_interface *mixer)
 	int err;
 
 	/* Already set up */
-	if (mixer->urb)
+	if (private->urb)
 		return 0;
 
 	if (usb_pipe_type_check(dev, pipe))
 		return -EINVAL;
 
-	mixer->urb = usb_alloc_urb(0, GFP_KERNEL);
-	if (!mixer->urb)
+	private->urb = usb_alloc_urb(0, GFP_KERNEL);
+	if (!private->urb)
 		return -ENOMEM;
 
 	transfer_buffer = kmalloc(private->wMaxPacketSize, GFP_KERNEL);
 	if (!transfer_buffer) {
-		usb_free_urb(mixer->urb);
-		mixer->urb = NULL;
+		usb_free_urb(private->urb);
+		private->urb = NULL;
 		return -ENOMEM;
 	}
 
-	usb_fill_int_urb(mixer->urb, dev, pipe,
+	usb_fill_int_urb(private->urb, dev, pipe,
 			 transfer_buffer, private->wMaxPacketSize,
 			 fcp_notify, mixer, private->bInterval);
 
-	init_completion(&private->cmd_done);
+	reinit_completion(&private->cmd_done);
 
-	err = usb_submit_urb(mixer->urb, GFP_KERNEL);
+	err = usb_submit_urb(private->urb, GFP_KERNEL);
 	if (err) {
 		usb_audio_err(mixer->chip,
 			      "%s: usb_submit_urb failed: %d\n",
 			      __func__, err);
 		kfree(transfer_buffer);
-		usb_free_urb(mixer->urb);
-		mixer->urb = NULL;
+		usb_free_urb(private->urb);
+		private->urb = NULL;
 	}
 
 	return err;
@@ -1051,6 +1054,7 @@ static int fcp_init_private(struct usb_mixer_interface *mixer)
 		return -ENOMEM;
 
 	mutex_init(&private->mutex);
+	init_completion(&private->cmd_done);
 	init_waitqueue_head(&private->notify.queue);
 	spin_lock_init(&private->notify.lock);
 
-- 
2.55.0

[PATCH 2/2] ALSA: scarlett2: Use a private URB for the notification endpoint

[Geoffrey D. Bennett]

scarlett2_init_notify() used mixer->urb, which
snd_usb_mixer_status_create() allocates for the UAC2 status interrupt
endpoint and mixer.c manages. On a device with that endpoint, the
"already in use" check fires on the status URB and returns 0 for
success without doing anything. No notification URB is submitted, and
cmd_done is left zeroed because it is initialised past that check and
nowhere else. scarlett2_usb_init() then issues SCARLETT2_USB_INIT_1
and wait_for_completion_timeout() would crash adding to the zeroed
wait.head.

Use a separate URB in scarlett2_data, as done for FCP, and initialise
cmd_done in scarlett2_init_private(). mixer.c was also freeing the URB
in snd_usb_mixer_free() and resubmitting it in
snd_usb_mixer_activate(), so scarlett2 must now do both: add
scarlett2_cleanup_urb(), called from private_free and private_suspend,
and a private_resume callback to re-establish the URB after resume.
scarlett2_init_notify() is reached from there, and the URB kill path
in scarlett2_notify() completes cmd_done, leaving a stale count that
would satisfy the next command's wait before the device ACKs. Use
reinit_completion() to clear it.

Also free the URB if the transfer buffer allocation fails, and both if
usb_submit_urb() fails. Move scarlett2_init_notify() up next to
scarlett2_cleanup_urb() so scarlett2_init_private() can reference it
without a forward declaration.

Fixes: 1b65088958ca ("ALSA: scarlett2: Implement handling of the ACK notification")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5
Signed-off-by: Geoffrey D. Bennett <g@b4.vu>
---
 sound/usb/mixer.c           |  6 +++
 sound/usb/mixer.h           |  2 +
 sound/usb/mixer_scarlett2.c | 98 ++++++++++++++++++++++++-------------
 3 files changed, 71 insertions(+), 35 deletions(-)

diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 703c118f9d4e..5de182181ede 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -3935,6 +3935,12 @@ int snd_usb_mixer_resume(struct usb_mixer_interface *mixer)
 	struct usb_mixer_elem_list *list;
 	int id, err;
 
+	if (mixer->private_resume) {
+		err = mixer->private_resume(mixer);
+		if (err < 0)
+			return err;
+	}
+
 	/* restore cached mixer values */
 	for (id = 0; id < MAX_ID_ELEMS; id++) {
 		for_each_mixer_elem(list, mixer, id) {
diff --git a/sound/usb/mixer.h b/sound/usb/mixer.h
index 3fa1bd96f858..037b446d8b6f 100644
--- a/sound/usb/mixer.h
+++ b/sound/usb/mixer.h
@@ -18,6 +18,7 @@ struct usb_mixer_interface {
 	struct usb_host_interface *hostif;
 	struct list_head list;
 	unsigned int ignore_ctl_error;
+	/* UAC2 status interrupt endpoint; owned by mixer.c */
 	struct urb *urb;
 	/* array[MAX_ID_ELEMS], indexed by unit id */
 	struct usb_mixer_elem_list **id_elems;
@@ -42,6 +43,7 @@ struct usb_mixer_interface {
 	void *private_data;
 	void (*private_free)(struct usb_mixer_interface *mixer);
 	void (*private_suspend)(struct usb_mixer_interface *mixer);
+	int (*private_resume)(struct usb_mixer_interface *mixer);
 };
 
 #define MAX_CHANNELS	64	/* max logical channels */
diff --git a/sound/usb/mixer_scarlett2.c b/sound/usb/mixer_scarlett2.c
index 78fb72e626ca..502854cc9f9f 100644
--- a/sound/usb/mixer_scarlett2.c
+++ b/sound/usb/mixer_scarlett2.c
@@ -1403,6 +1403,7 @@ struct scarlett2_data {
 	struct usb_mixer_interface *mixer;
 	struct mutex usb_mutex; /* prevent sending concurrent USB requests */
 	struct completion cmd_done;
+	struct urb *urb;        /* notification endpoint */
 	struct mutex data_mutex; /* lock access to this data */
 	u8 running;
 	u8 hwdep_in_use;
@@ -8565,13 +8566,70 @@ static void scarlett2_notify(struct urb *urb)
 	}
 }
 
-/*** Cleanup/Suspend Callbacks ***/
+/*** Notification URB and Cleanup/Suspend Callbacks ***/
+
+/* Submit a URB to receive notifications from the device */
+static int scarlett2_init_notify(struct usb_mixer_interface *mixer)
+{
+	struct usb_device *dev = mixer->chip->dev;
+	struct scarlett2_data *private = mixer->private_data;
+	unsigned int pipe = usb_rcvintpipe(dev, private->bEndpointAddress);
+	void *transfer_buffer;
+	int err;
+
+	/* Already set up */
+	if (private->urb)
+		return 0;
+
+	if (usb_pipe_type_check(dev, pipe))
+		return -EINVAL;
+
+	private->urb = usb_alloc_urb(0, GFP_KERNEL);
+	if (!private->urb)
+		return -ENOMEM;
+
+	transfer_buffer = kmalloc(private->wMaxPacketSize, GFP_KERNEL);
+	if (!transfer_buffer) {
+		usb_free_urb(private->urb);
+		private->urb = NULL;
+		return -ENOMEM;
+	}
+
+	usb_fill_int_urb(private->urb, dev, pipe,
+			 transfer_buffer, private->wMaxPacketSize,
+			 scarlett2_notify, mixer, private->bInterval);
+
+	reinit_completion(&private->cmd_done);
+
+	err = usb_submit_urb(private->urb, GFP_KERNEL);
+	if (err) {
+		kfree(transfer_buffer);
+		usb_free_urb(private->urb);
+		private->urb = NULL;
+	}
+
+	return err;
+}
+
+static void scarlett2_cleanup_urb(struct usb_mixer_interface *mixer)
+{
+	struct scarlett2_data *private = mixer->private_data;
+
+	if (!private->urb)
+		return;
+
+	usb_kill_urb(private->urb);
+	kfree(private->urb->transfer_buffer);
+	usb_free_urb(private->urb);
+	private->urb = NULL;
+}
 
 static void scarlett2_private_free(struct usb_mixer_interface *mixer)
 {
 	struct scarlett2_data *private = mixer->private_data;
 
 	cancel_delayed_work_sync(&private->work);
+	scarlett2_cleanup_urb(mixer);
 	kfree(private);
 	mixer->private_data = NULL;
 }
@@ -8582,6 +8640,8 @@ static void scarlett2_private_suspend(struct usb_mixer_interface *mixer)
 
 	if (cancel_delayed_work_sync(&private->work))
 		scarlett2_config_save(private->mixer);
+
+	scarlett2_cleanup_urb(mixer);
 }
 
 /*** Initialisation ***/
@@ -8701,11 +8761,13 @@ static int scarlett2_init_private(struct usb_mixer_interface *mixer,
 
 	mutex_init(&private->usb_mutex);
 	mutex_init(&private->data_mutex);
+	init_completion(&private->cmd_done);
 	INIT_DELAYED_WORK(&private->work, scarlett2_config_save_work);
 
 	mixer->private_data = private;
 	mixer->private_free = scarlett2_private_free;
 	mixer->private_suspend = scarlett2_private_suspend;
+	mixer->private_resume = scarlett2_init_notify;
 
 	private->info = entry->info;
 
@@ -8722,40 +8784,6 @@ static int scarlett2_init_private(struct usb_mixer_interface *mixer,
 	return scarlett2_find_fc_interface(mixer->chip->dev, private);
 }
 
-/* Submit a URB to receive notifications from the device */
-static int scarlett2_init_notify(struct usb_mixer_interface *mixer)
-{
-	struct usb_device *dev = mixer->chip->dev;
-	struct scarlett2_data *private = mixer->private_data;
-	unsigned int pipe = usb_rcvintpipe(dev, private->bEndpointAddress);
-	void *transfer_buffer;
-
-	if (mixer->urb) {
-		usb_audio_err(mixer->chip,
-			      "%s: mixer urb already in use!\n", __func__);
-		return 0;
-	}
-
-	if (usb_pipe_type_check(dev, pipe))
-		return -EINVAL;
-
-	mixer->urb = usb_alloc_urb(0, GFP_KERNEL);
-	if (!mixer->urb)
-		return -ENOMEM;
-
-	transfer_buffer = kmalloc(private->wMaxPacketSize, GFP_KERNEL);
-	if (!transfer_buffer)
-		return -ENOMEM;
-
-	usb_fill_int_urb(mixer->urb, dev, pipe,
-			 transfer_buffer, private->wMaxPacketSize,
-			 scarlett2_notify, mixer, private->bInterval);
-
-	init_completion(&private->cmd_done);
-
-	return usb_submit_urb(mixer->urb, GFP_KERNEL);
-}
-
 /* Cargo cult proprietary initialisation sequence */
 static int scarlett2_usb_init(struct usb_mixer_interface *mixer)
 {
-- 
2.55.0

Re: [PATCH 0/2] ALSA: Don't share mixer->urb with the FCP/scarlett2 notification endpoint

[Takashi Iwai]

On Sun, 09 Aug 2026 20:05:41 +0200,
Geoffrey D. Bennett wrote:
> 
> Hi Takashi,
> 
> Both the FCP and scarlett2 drivers used mixer->urb for their
> notification endpoint URB, but that field belongs to
> snd_usb_mixer_status_create() and is managed by mixer.c. Sharing it
> means the "already set up" check in the driver's init_notify() can be
> satisfied by the status endpoint's URB, so the driver's own URB is
> never submitted and the first command would crash in
> wait_for_completion_timeout() on the still-zeroed cmd_done. mixer.c
> can also kill, resubmit or free the URB from under the driver.
> 
> Give each driver its own URB in its private data. For scarlett2 this
> also means taking over the free/suspend/resume handling that mixer.c
> was doing for it.
> 
> Tested on an ISA C8X and a Scarlett 18i20 Gen 4 (FCP), and a Scarlett
> 4i4 Gen 3 and 4i4 Gen 4 (scarlett2) on 7.1.7, including suspend/resume
> and configuration save on suspend.
> 
> Thanks,
> Geoffrey
> 
> Geoffrey D. Bennett (2):
>   ALSA: FCP: Use a private URB for the notification endpoint
>   ALSA: scarlett2: Use a private URB for the notification endpoint

As those are no new regression, I applied both to for-next branch now.


thanks,

Takashi