Commit 47ab1545 authored by Takashi Iwai's avatar Takashi Iwai
Browse files

ALSA: usb-audio: Avoid nested autoresume calls 



After the recent fix of runtime PM for USB-audio driver, we got a
lockdep warning like:

  =============================================
  [ INFO: possible recursive locking detected ]
  4.2.0-rc8+ #61 Not tainted
  ---------------------------------------------
  pulseaudio/980 is trying to acquire lock:
   (&chip->shutdown_rwsem){.+.+.+}, at: [<ffffffffa0355dac>] snd_usb_autoresume+0x1d/0x52 [snd_usb_audio]
  but task is already holding lock:
   (&chip->shutdown_rwsem){.+.+.+}, at: [<ffffffffa0355dac>] snd_usb_autoresume+0x1d/0x52 [snd_usb_audio]

This comes from snd_usb_autoresume() invoking down_read() and it's
used in a nested way.  Although it's basically safe, per se (as these
are read locks), it's better to reduce such spurious warnings.

The read lock is needed to guarantee the execution of "shutdown"
(cleanup at disconnection) task after all concurrent tasks are
finished.  This can be implemented in another better way.

Also, the current check of chip->in_pm isn't good enough for
protecting the racy execution of multiple auto-resumes.

This patch rewrites the logic of snd_usb_autoresume() & co; namely,
- The recursive call of autopm is avoided by the new refcount,
  chip->active.  The chip->in_pm flag is removed accordingly.
- Instead of rwsem, another refcount, chip->usage_count, is introduced
  for tracking the period to delay the shutdown procedure.  At
  the last clear of this refcount, wake_up() to the shutdown waiter is
  called.
- The shutdown flag is replaced with shutdown atomic count; this is
  for reducing the lock.
- Two new helpers are introduced to simplify the management of these
  refcounts; snd_usb_lock_shutdown() increases the usage_count, checks
  the shutdown state, and does autoresume.  snd_usb_unlock_shutdown()
  does the opposite.  Most of mixer and other codes just need this,
  and simply returns an error if it receives an error from lock.

Fixes: 9003ebb1 ('ALSA: usb-audio: Fix runtime PM unbalance')
Reported-and-tested-by: default avatarAlexnader Kuleshov <kuleshovmail@gmail.com>
Signed-off-by: default avatarTakashi Iwai <tiwai@suse.de>
parent b25cf30a

sound/usb/card.c

+51 −23
Original line number Diff line number Diff line
@@ -365,13 +365,15 @@ static int snd_usb_audio_create(struct usb_interface *intf, 
	}



	mutex_init(&chip->mutex);

	init_rwsem(&chip->shutdown_rwsem);

	init_waitqueue_head(&chip->shutdown_wait);

	chip->index = idx;

	chip->dev = dev;

	chip->card = card;

	chip->setup = device_setup[idx];

	chip->autoclock = autoclock;

	chip->probing = 1;

	atomic_set(&chip->usage_count, 0);

	atomic_set(&chip->shutdown, 0);



	chip->usb_id = USB_ID(le16_to_cpu(dev->descriptor.idVendor),

			      le16_to_cpu(dev->descriptor.idProduct));
@@ -495,7 +497,7 @@ static int usb_audio_probe(struct usb_interface *intf, 
	mutex_lock(&register_mutex);

	for (i = 0; i < SNDRV_CARDS; i++) {

		if (usb_chip[i] && usb_chip[i]->dev == dev) {

			if (usb_chip[i]->shutdown) {

			if (atomic_read(&usb_chip[i]->shutdown)) {

				dev_err(&dev->dev, "USB device is in the shutdown state, cannot create a card instance\n");

				err = -EIO;

				goto __error;
@@ -585,23 +587,23 @@ static void usb_audio_disconnect(struct usb_interface *intf) 
	struct snd_usb_audio *chip = usb_get_intfdata(intf);

	struct snd_card *card;

	struct list_head *p;

	bool was_shutdown;



	if (chip == (void *)-1L)

		return;



	card = chip->card;

	down_write(&chip->shutdown_rwsem);

	was_shutdown = chip->shutdown;

	chip->shutdown = 1;

	up_write(&chip->shutdown_rwsem);



	mutex_lock(&register_mutex);

	if (!was_shutdown) {

	if (atomic_inc_return(&chip->shutdown) == 1) {

		struct snd_usb_stream *as;

		struct snd_usb_endpoint *ep;

		struct usb_mixer_interface *mixer;



		/* wait until all pending tasks done;

		 * they are protected by snd_usb_lock_shutdown()

		 */

		wait_event(chip->shutdown_wait,

			   !atomic_read(&chip->usage_count));

		snd_card_disconnect(card);

		/* release the pcm resources */

		list_for_each_entry(as, &chip->pcm_list, list) {
@@ -631,28 +633,54 @@ static void usb_audio_disconnect(struct usb_interface *intf) 
	}

}



#ifdef CONFIG_PM



int snd_usb_autoresume(struct snd_usb_audio *chip)

/* lock the shutdown (disconnect) task and autoresume */

int snd_usb_lock_shutdown(struct snd_usb_audio *chip)

{

	int err = -ENODEV;

	int err;



	down_read(&chip->shutdown_rwsem);

	if (chip->probing || chip->in_pm)

		err = 0;

	else if (!chip->shutdown)

		err = usb_autopm_get_interface(chip->pm_intf);

	up_read(&chip->shutdown_rwsem);

	atomic_inc(&chip->usage_count);

	if (atomic_read(&chip->shutdown)) {

		err = -EIO;

		goto error;

	}

	err = snd_usb_autoresume(chip);

	if (err < 0)

		goto error;

	return 0;



 error:

	if (atomic_dec_and_test(&chip->usage_count))

		wake_up(&chip->shutdown_wait);

	return err;

}



/* autosuspend and unlock the shutdown */

void snd_usb_unlock_shutdown(struct snd_usb_audio *chip)

{

	snd_usb_autosuspend(chip);

	if (atomic_dec_and_test(&chip->usage_count))

		wake_up(&chip->shutdown_wait);

}



#ifdef CONFIG_PM



int snd_usb_autoresume(struct snd_usb_audio *chip)

{

	if (atomic_read(&chip->shutdown))

		return -EIO;

	if (chip->probing)

		return 0;

	if (atomic_inc_return(&chip->active) == 1)

		return usb_autopm_get_interface(chip->pm_intf);

	return 0;

}



void snd_usb_autosuspend(struct snd_usb_audio *chip)

{

	down_read(&chip->shutdown_rwsem);

	if (!chip->shutdown && !chip->probing && !chip->in_pm)

	if (chip->probing)

		return;

	if (atomic_dec_and_test(&chip->active))

		usb_autopm_put_interface(chip->pm_intf);

	up_read(&chip->shutdown_rwsem);

}



static int usb_audio_suspend(struct usb_interface *intf, pm_message_t message)
@@ -705,7 +733,7 @@ static int __usb_audio_resume(struct usb_interface *intf, bool reset_resume) 
	if (--chip->num_suspended_intf)

		return 0;



	chip->in_pm = 1;

	atomic_inc(&chip->active); /* avoid autopm */

	/*

	 * ALSA leaves material resumption to user space

	 * we just notify and restart the mixers
@@ -725,7 +753,7 @@ static int __usb_audio_resume(struct usb_interface *intf, bool reset_resume) 
	chip->autosuspended = 0;



err_out:

	chip->in_pm = 0;

	atomic_dec(&chip->active); /* allow autopm after this point */

	return err;

}

sound/usb/endpoint.c

+6 −4
Original line number Diff line number Diff line
@@ -355,8 +355,10 @@ static void snd_complete_urb(struct urb *urb) 
	if (unlikely(urb->status == -ENOENT ||		/* unlinked */

		     urb->status == -ENODEV ||		/* device removed */

		     urb->status == -ECONNRESET ||	/* unlinked */

		     urb->status == -ESHUTDOWN ||	/* device disabled */

		     ep->chip->shutdown))		/* device disconnected */

		     urb->status == -ESHUTDOWN))	/* device disabled */

		goto exit_clear;

	/* device disconnected */

	if (unlikely(atomic_read(&ep->chip->shutdown)))

		goto exit_clear;



	if (usb_pipeout(ep->pipe)) {
@@ -529,7 +531,7 @@ static int deactivate_urbs(struct snd_usb_endpoint *ep, bool force) 
{

	unsigned int i;



	if (!force && ep->chip->shutdown) /* to be sure... */

	if (!force && atomic_read(&ep->chip->shutdown)) /* to be sure... */

		return -EBADFD;



	clear_bit(EP_FLAG_RUNNING, &ep->flags);
@@ -868,7 +870,7 @@ int snd_usb_endpoint_start(struct snd_usb_endpoint *ep, bool can_sleep) 
	int err;

	unsigned int i;



	if (ep->chip->shutdown)

	if (atomic_read(&ep->chip->shutdown))

		return -EBADFD;



	/* already running? */

sound/usb/mixer.c

+10 −22
Original line number Diff line number Diff line
@@ -311,14 +311,11 @@ static int get_ctl_value_v1(struct usb_mixer_elem_info *cval, int request, 
	int timeout = 10;

	int idx = 0, err;



	err = snd_usb_autoresume(chip);

	err = snd_usb_lock_shutdown(chip);

	if (err < 0)

		return -EIO;



	down_read(&chip->shutdown_rwsem);

	while (timeout-- > 0) {

		if (chip->shutdown)

			break;

		idx = snd_usb_ctrl_intf(chip) | (cval->head.id << 8);

		if (snd_usb_ctl_msg(chip->dev, usb_rcvctrlpipe(chip->dev, 0), request,

				    USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
@@ -334,8 +331,7 @@ static int get_ctl_value_v1(struct usb_mixer_elem_info *cval, int request, 
	err = -EINVAL;



 out:

	up_read(&chip->shutdown_rwsem);

	snd_usb_autosuspend(chip);

	snd_usb_unlock_shutdown(chip);

	return err;

}
@@ -358,21 +354,15 @@ static int get_ctl_value_v2(struct usb_mixer_elem_info *cval, int request, 


	memset(buf, 0, sizeof(buf));



	ret = snd_usb_autoresume(chip) ? -EIO : 0;

	ret = snd_usb_lock_shutdown(chip) ? -EIO : 0;

	if (ret)

		goto error;



	down_read(&chip->shutdown_rwsem);

	if (chip->shutdown) {

		ret = -ENODEV;

	} else {

	idx = snd_usb_ctrl_intf(chip) | (cval->head.id << 8);

	ret = snd_usb_ctl_msg(chip->dev, usb_rcvctrlpipe(chip->dev, 0), bRequest,

			      USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,

			      validx, idx, buf, size);

	}

	up_read(&chip->shutdown_rwsem);

	snd_usb_autosuspend(chip);

	snd_usb_unlock_shutdown(chip);



	if (ret < 0) {

error:
@@ -485,13 +475,12 @@ int snd_usb_mixer_set_ctl_value(struct usb_mixer_elem_info *cval, 
	buf[1] = (value_set >> 8) & 0xff;

	buf[2] = (value_set >> 16) & 0xff;

	buf[3] = (value_set >> 24) & 0xff;

	err = snd_usb_autoresume(chip);



	err = snd_usb_lock_shutdown(chip);

	if (err < 0)

		return -EIO;

	down_read(&chip->shutdown_rwsem);



	while (timeout-- > 0) {

		if (chip->shutdown)

			break;

		idx = snd_usb_ctrl_intf(chip) | (cval->head.id << 8);

		if (snd_usb_ctl_msg(chip->dev,

				    usb_sndctrlpipe(chip->dev, 0), request,
@@ -506,8 +495,7 @@ int snd_usb_mixer_set_ctl_value(struct usb_mixer_elem_info *cval, 
	err = -EINVAL;



 out:

	up_read(&chip->shutdown_rwsem);

	snd_usb_autosuspend(chip);

	snd_usb_unlock_shutdown(chip);

	return err;

}

sound/usb/mixer_quirks.c

+54 −72
Original line number Diff line number Diff line
@@ -308,11 +308,10 @@ static int snd_audigy2nx_led_update(struct usb_mixer_interface *mixer, 
	struct snd_usb_audio *chip = mixer->chip;

	int err;



	down_read(&chip->shutdown_rwsem);

	if (chip->shutdown) {

		err = -ENODEV;

		goto out;

	}

	err = snd_usb_lock_shutdown(chip);

	if (err < 0)

		return err;



	if (chip->usb_id == USB_ID(0x041e, 0x3042))

		err = snd_usb_ctl_msg(chip->dev,

			      usb_sndctrlpipe(chip->dev, 0), 0x24,
@@ -329,8 +328,7 @@ static int snd_audigy2nx_led_update(struct usb_mixer_interface *mixer, 
			      usb_sndctrlpipe(chip->dev, 0), 0x24,

			      USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_OTHER,

			      value, index + 2, NULL, 0);

 out:

	up_read(&chip->shutdown_rwsem);

	snd_usb_unlock_shutdown(chip);

	return err;

}
@@ -441,16 +439,15 @@ static void snd_audigy2nx_proc_read(struct snd_info_entry *entry, 


	for (i = 0; jacks[i].name; ++i) {

		snd_iprintf(buffer, "%s: ", jacks[i].name);

		down_read(&mixer->chip->shutdown_rwsem);

		if (mixer->chip->shutdown)

			err = 0;

		else

		err = snd_usb_lock_shutdown(mixer->chip);

		if (err < 0)

			return;

		err = snd_usb_ctl_msg(mixer->chip->dev,

				      usb_rcvctrlpipe(mixer->chip->dev, 0),

				      UAC_GET_MEM, USB_DIR_IN | USB_TYPE_CLASS |

				      USB_RECIP_INTERFACE, 0,

				      jacks[i].unitid << 8, buf, 3);

		up_read(&mixer->chip->shutdown_rwsem);

		snd_usb_unlock_shutdown(mixer->chip);

		if (err == 3 && (buf[0] == 3 || buf[0] == 6))

			snd_iprintf(buffer, "%02x %02x\n", buf[1], buf[2]);

		else
@@ -481,11 +478,9 @@ static int snd_emu0204_ch_switch_update(struct usb_mixer_interface *mixer, 
	int err;

	unsigned char buf[2];



	down_read(&chip->shutdown_rwsem);

	if (mixer->chip->shutdown) {

		err = -ENODEV;

		goto out;

	}

	err = snd_usb_lock_shutdown(chip);

	if (err < 0)

		return err;



	buf[0] = 0x01;

	buf[1] = value ? 0x02 : 0x01;
@@ -493,8 +488,7 @@ static int snd_emu0204_ch_switch_update(struct usb_mixer_interface *mixer, 
		      usb_sndctrlpipe(chip->dev, 0), UAC_SET_CUR,

		      USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_OUT,

		      0x0400, 0x0e00, buf, 2);

 out:

	up_read(&chip->shutdown_rwsem);

	snd_usb_unlock_shutdown(chip);

	return err;

}
@@ -554,15 +548,14 @@ static int snd_xonar_u1_switch_update(struct usb_mixer_interface *mixer, 
	struct snd_usb_audio *chip = mixer->chip;

	int err;



	down_read(&chip->shutdown_rwsem);

	if (chip->shutdown)

		err = -ENODEV;

	else

	err = snd_usb_lock_shutdown(chip);

	if (err < 0)

		return err;

	err = snd_usb_ctl_msg(chip->dev,

			      usb_sndctrlpipe(chip->dev, 0), 0x08,

			      USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_OTHER,

			      50, 0, &status, 1);

	up_read(&chip->shutdown_rwsem);

	snd_usb_unlock_shutdown(chip);

	return err;

}
@@ -623,11 +616,9 @@ static int snd_mbox1_switch_update(struct usb_mixer_interface *mixer, int val) 
	int err;

	unsigned char buff[3];



	down_read(&chip->shutdown_rwsem);

	if (chip->shutdown) {

		err = -ENODEV;

		goto err;

	}

	err = snd_usb_lock_shutdown(chip);

	if (err < 0)

		return err;



	/* Prepare for magic command to toggle clock source */

	err = snd_usb_ctl_msg(chip->dev,
@@ -683,7 +674,7 @@ static int snd_mbox1_switch_update(struct usb_mixer_interface *mixer, int val) 
		goto err;



err:

	up_read(&chip->shutdown_rwsem);

	snd_usb_unlock_shutdown(chip);

	return err;

}
@@ -778,15 +769,14 @@ static int snd_ni_update_cur_val(struct usb_mixer_elem_list *list) 
	unsigned int pval = list->kctl->private_value;

	int err;



	down_read(&chip->shutdown_rwsem);

	if (chip->shutdown)

		err = -ENODEV;

	else

	err = snd_usb_lock_shutdown(chip);

	if (err < 0)

		return err;

	err = usb_control_msg(chip->dev, usb_sndctrlpipe(chip->dev, 0),

			      (pval >> 16) & 0xff,

			      USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT,

			      pval >> 24, pval & 0xffff, NULL, 0, 1000);

	up_read(&chip->shutdown_rwsem);

	snd_usb_unlock_shutdown(chip);

	return err;

}
@@ -944,10 +934,9 @@ static int snd_ftu_eff_switch_update(struct usb_mixer_elem_list *list) 
	value[0] = pval >> 24;

	value[1] = 0;



	down_read(&chip->shutdown_rwsem);

	if (chip->shutdown)

		err = -ENODEV;

	else

	err = snd_usb_lock_shutdown(chip);

	if (err < 0)

		return err;

	err = snd_usb_ctl_msg(chip->dev,

			      usb_sndctrlpipe(chip->dev, 0),

			      UAC_SET_CUR,
@@ -955,7 +944,7 @@ static int snd_ftu_eff_switch_update(struct usb_mixer_elem_list *list) 
			      pval & 0xff00,

			      snd_usb_ctrl_intf(chip) | ((pval & 0xff) << 8),

			      value, 2);

	up_read(&chip->shutdown_rwsem);

	snd_usb_unlock_shutdown(chip);

	return err;

}
@@ -1519,11 +1508,9 @@ static int snd_microii_spdif_default_get(struct snd_kcontrol *kcontrol, 
	unsigned char data[3];

	int rate;



	down_read(&chip->shutdown_rwsem);

	if (chip->shutdown) {

		err = -ENODEV;

		goto end;

	}

	err = snd_usb_lock_shutdown(chip);

	if (err < 0)

		return err;



	ucontrol->value.iec958.status[0] = kcontrol->private_value & 0xff;

	ucontrol->value.iec958.status[1] = (kcontrol->private_value >> 8) & 0xff;
@@ -1551,7 +1538,7 @@ static int snd_microii_spdif_default_get(struct snd_kcontrol *kcontrol, 


	err = 0;

 end:

	up_read(&chip->shutdown_rwsem);

	snd_usb_unlock_shutdown(chip);

	return err;

}
@@ -1562,11 +1549,9 @@ static int snd_microii_spdif_default_update(struct usb_mixer_elem_list *list) 
	u8 reg;

	int err;



	down_read(&chip->shutdown_rwsem);

	if (chip->shutdown) {

		err = -ENODEV;

		goto end;

	}

	err = snd_usb_lock_shutdown(chip);

	if (err < 0)

		return err;



	reg = ((pval >> 4) & 0xf0) | (pval & 0x0f);

	err = snd_usb_ctl_msg(chip->dev,
@@ -1594,7 +1579,7 @@ static int snd_microii_spdif_default_update(struct usb_mixer_elem_list *list) 
		goto end;



 end:

	up_read(&chip->shutdown_rwsem);

	snd_usb_unlock_shutdown(chip);

	return err;

}
@@ -1650,11 +1635,9 @@ static int snd_microii_spdif_switch_update(struct usb_mixer_elem_list *list) 
	u8 reg = list->kctl->private_value;

	int err;



	down_read(&chip->shutdown_rwsem);

	if (chip->shutdown) {

		err = -ENODEV;

		goto end;

	}

	err = snd_usb_lock_shutdown(chip);

	if (err < 0)

		return err;



	err = snd_usb_ctl_msg(chip->dev,

			usb_sndctrlpipe(chip->dev, 0),
@@ -1665,8 +1648,7 @@ static int snd_microii_spdif_switch_update(struct usb_mixer_elem_list *list) 
			NULL,

			0);



 end:

	up_read(&chip->shutdown_rwsem);

	snd_usb_unlock_shutdown(chip);

	return err;

}

sound/usb/pcm.c

+15 −17
Original line number Diff line number Diff line
@@ -80,7 +80,7 @@ static snd_pcm_uframes_t snd_usb_pcm_pointer(struct snd_pcm_substream *substream 
	unsigned int hwptr_done;



	subs = (struct snd_usb_substream *)substream->runtime->private_data;

	if (subs->stream->chip->shutdown)

	if (atomic_read(&subs->stream->chip->shutdown))

		return SNDRV_PCM_POS_XRUN;

	spin_lock(&subs->lock);

	hwptr_done = subs->hwptr_done;
@@ -735,12 +735,11 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream, 
		return -EINVAL;

	}



	down_read(&subs->stream->chip->shutdown_rwsem);

	if (subs->stream->chip->shutdown)

		ret = -ENODEV;

	else

	ret = snd_usb_lock_shutdown(subs->stream->chip);

	if (ret < 0)

		return ret;

	ret = set_format(subs, fmt);

	up_read(&subs->stream->chip->shutdown_rwsem);

	snd_usb_unlock_shutdown(subs->stream->chip);

	if (ret < 0)

		return ret;
@@ -763,13 +762,12 @@ static int snd_usb_hw_free(struct snd_pcm_substream *substream) 
	subs->cur_audiofmt = NULL;

	subs->cur_rate = 0;

	subs->period_bytes = 0;

	down_read(&subs->stream->chip->shutdown_rwsem);

	if (!subs->stream->chip->shutdown) {

	if (!snd_usb_lock_shutdown(subs->stream->chip)) {

		stop_endpoints(subs, true);

		snd_usb_endpoint_deactivate(subs->sync_endpoint);

		snd_usb_endpoint_deactivate(subs->data_endpoint);

		snd_usb_unlock_shutdown(subs->stream->chip);

	}

	up_read(&subs->stream->chip->shutdown_rwsem);

	return snd_pcm_lib_free_vmalloc_buffer(substream);

}
@@ -791,11 +789,9 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream) 
		return -ENXIO;

	}



	down_read(&subs->stream->chip->shutdown_rwsem);

	if (subs->stream->chip->shutdown) {

		ret = -ENODEV;

		goto unlock;

	}

	ret = snd_usb_lock_shutdown(subs->stream->chip);

	if (ret < 0)

		return ret;

	if (snd_BUG_ON(!subs->data_endpoint)) {

		ret = -EIO;

		goto unlock;
@@ -844,7 +840,7 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream) 
		ret = start_endpoints(subs, true);



 unlock:

	up_read(&subs->stream->chip->shutdown_rwsem);

	snd_usb_unlock_shutdown(subs->stream->chip);

	return ret;

}
@@ -1246,9 +1242,11 @@ static int snd_usb_pcm_close(struct snd_pcm_substream *substream, int direction) 


	stop_endpoints(subs, true);



	if (!as->chip->shutdown && subs->interface >= 0) {

	if (subs->interface >= 0 &&

	    !snd_usb_lock_shutdown(subs->stream->chip)) {

		usb_set_interface(subs->dev, subs->interface, 0);

		subs->interface = -1;

		snd_usb_unlock_shutdown(subs->stream->chip);

	}



	subs->pcm_substream = NULL;

CRA Git | Maintained and supported by SUSTech CRA and CCSE