ALSA: Don't share mixer->urb with the FCP/scarlett2 notification endpoint

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

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.

---
 sound/usb/fcp.c | 38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

index 6f5dcd35e1d4..6c7da5508bc3 100644
@@ -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

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.

---
 sound/usb/mixer.c           |  6 +++
 sound/usb/mixer.h           |  2 +
 sound/usb/mixer_scarlett2.c | 98 ++++++++++++++++++++++++-------------
 3 files changed, 71 insertions(+), 35 deletions(-)

index 703c118f9d4e..5de182181ede 100644
@@ -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) {

index 3fa1bd96f858..037b446d8b6f 100644
@@ -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 */

2.55.0

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

thanks,

Takashi