ALSA: control: Track the floating control read/write/tlv accesses

Some control API ioctls may access the hardware and we need to block
such operations during the card power state off.  Although there have
been already a few snd_power_wait() checks in some ioctl handlers,
not only that the snd_power_wait() checks are missing around the
actually needed places for kctl ops accesses, but also that the
snd_power_wait() call itself is basically helpless; if a driver sets
the power state to D3, a pending task that has already entered in the
ioctl may access the hardware afterwards.

For covering those races, this patch introduces a few new things:
- A refcount, power_ref, and a wait head, power_ref_sleep, to the card
  object
- A new helper, snd_power_wait_and_ref(), and snd_power_unref()

In a few code paths that call kctl read/write/tlv ops, we check the
power state again with the new snd_power_want_and_ref() helper, which
also takes the card.power_ref refcount in return.  The refcount is
then released after the kctl ops finishes.  So the driver can sync via
wait_event() with power_ref=0 for assuring all in-flight tasks are
finished.  (Note that the snd_power_wait_and_ref() is written
carefully not to deadlock the power_ref=0 loop by decrementing the
refcount before sleeping.)

Also, this patch changes snd_power_wait() to accept only
SNDRV_CTL_POWER_D0, which is the only value that makes sense.
In later patch, the snd_power_wait() calls will be simplified.

Reader may wonder why a similar stuff isn't needed for PCM; it's
because PCM is stateful unlike the control API and each operation
stage has a proper state check.

Yet another note: the code uses atomic_t because refcount_t doesn't
allow the increment on zero, and it gives no helper for dec-and-test
other than comparison with zero, either.

Signed-off-by: Takashi Iwai <tiwai@suse.de>

3 files changed, 86 insertions, 14 deletions

diff --git a/include/sound/core.h b/include/sound/core.h
index 6b6bb6bc88bb..331c52268c76 100644
--- a/include/sound/core.h
+++ b/include/sound/core.h
@@ -126,6 +126,8 @@ struct snd_card {
 #ifdef CONFIG_PM
 	unsigned int power_state;	/* power state */
 	wait_queue_head_t power_sleep;
+	atomic_t power_ref;
+	wait_queue_head_t power_ref_sleep;
 #endif
 
 #if IS_ENABLED(CONFIG_SND_MIXER_OSS)
@@ -148,8 +150,20 @@ static inline void snd_power_change_state(struct snd_card *card, unsigned int st
 	wake_up(&card->power_sleep);
 }
 
+static inline void snd_power_ref(struct snd_card *card)
+{
+	atomic_inc(&card->power_ref);
+}
+
+static inline void snd_power_unref(struct snd_card *card)
+{
+	if (atomic_dec_and_test(&card->power_ref))
+		wake_up(&card->power_ref_sleep);
+}
+
 /* init.c */
 int snd_power_wait(struct snd_card *card, unsigned int power_state);
+int snd_power_wait_and_ref(struct snd_card *card, bool ref);
 
 #else /* ! CONFIG_PM */
 
@@ -167,6 +181,19 @@ static inline void snd_power_change_state(struct snd_card *card, unsigned int st
 {
 }
 
+static inline int snd_power_wait_and_ref(struct snd_card *card, bool ref)
+{
+	return 0;
+}
+
+static inline void snd_power_ref(struct snd_card *card)
+{
+}
+
+static inline void snd_power_unref(struct snd_card *card)
+{
+}
+
 #endif /* CONFIG_PM */
 
 struct snd_minor {
diff --git a/sound/core/control.c b/sound/core/control.c
index 3b44378b9dec..e1e89fb95f2a 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1060,9 +1060,12 @@ static int snd_ctl_elem_read(struct snd_card *card,
 
 	if (!snd_ctl_skip_validation(&info))
 		fill_remaining_elem_value(control, &info, pattern);
+	ret = snd_power_wait_and_ref(card, true);
+	if (ret < 0)
+		goto out;
 	ret = kctl->get(kctl, control);
 	if (ret < 0)
-		return ret;
+		goto out;
 	if (!snd_ctl_skip_validation(&info) &&
 	    sanity_check_elem_value(card, control, &info, pattern) < 0) {
 		dev_err(card->dev,
@@ -1070,8 +1073,12 @@ static int snd_ctl_elem_read(struct snd_card *card,
 			control->id.iface, control->id.device,
 			control->id.subdevice, control->id.name,
 			control->id.index);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto out;
 	}
+
+ out:
+	snd_power_unref(card);
 	return ret;
 }
 
@@ -1122,16 +1129,22 @@ static int snd_ctl_elem_write(struct snd_card *card, struct snd_ctl_file *file,
 	}
 
 	snd_ctl_build_ioff(&control->id, kctl, index_offset);
+
+	result = snd_power_wait_and_ref(card, true);
+	if (result < 0)
+		goto out;
 	result = kctl->put(kctl, control);
 	if (result < 0)
-		return result;
+		goto out;
 
 	if (result > 0) {
 		struct snd_ctl_elem_id id = control->id;
 		snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_VALUE, &id);
 	}
 
-	return 0;
+ out:
+	snd_power_unref(card);
+	return result < 0 ? result : 0;
 }
 
 static int snd_ctl_elem_write_user(struct snd_ctl_file *file,
@@ -1606,7 +1619,7 @@ static int call_tlv_handler(struct snd_ctl_file *file, int op_flag,
 		{SNDRV_CTL_TLV_OP_CMD,   SNDRV_CTL_ELEM_ACCESS_TLV_COMMAND},
 	};
 	struct snd_kcontrol_volatile *vd = &kctl->vd[snd_ctl_get_ioff(kctl, id)];
-	int i;
+	int i, ret;
 
 	/* Check support of the request for this element. */
 	for (i = 0; i < ARRAY_SIZE(pairs); ++i) {
@@ -1624,7 +1637,11 @@ static int call_tlv_handler(struct snd_ctl_file *file, int op_flag,
 	    vd->owner != NULL && vd->owner != file)
 		return -EPERM;
 
-	return kctl->tlv.c(kctl, op_flag, size, buf);
+	ret = snd_power_wait_and_ref(file->card, true);
+	if (!ret)
+		ret = kctl->tlv.c(kctl, op_flag, size, buf);
+	snd_power_unref(file->card);
+	return ret;
 }
 
 static int read_tlv_buf(struct snd_kcontrol *kctl, struct snd_ctl_elem_id *id,
diff --git a/sound/core/init.c b/sound/core/init.c
index 75aec71c48a8..9dd185f9150f 100644
--- a/sound/core/init.c
+++ b/sound/core/init.c
@@ -215,6 +215,8 @@ int snd_card_new(struct device *parent, int idx, const char *xid,
 	mutex_init(&card->memory_mutex);
 #ifdef CONFIG_PM
 	init_waitqueue_head(&card->power_sleep);
+	atomic_set(&card->power_ref, 0);
+	init_waitqueue_head(&card->power_ref_sleep);
 #endif
 	init_waitqueue_head(&card->remove_sleep);
 	card->sync_irq = -1;
@@ -985,21 +987,26 @@ EXPORT_SYMBOL(snd_card_file_remove);
 
 #ifdef CONFIG_PM
 /**
- *  snd_power_wait - wait until the power-state is changed.
- *  @card: soundcard structure
- *  @power_state: expected power state
+ * snd_power_wait_and_ref - wait until the card gets powered up
+ * @card: soundcard structure
+ * @ref: take power_ref refcount if set
  *
- *  Waits until the power-state is changed.
+ * Waits until the card gets powered up to SNDRV_CTL_POWER_D0 state.
+ * When @ref is set, power_ref refcount is incremented in return.
+ * The caller needs to pull down the refcount via snd_power_unref() later
+ * appropriately.
  *
- *  Return: Zero if successful, or a negative error code.
+ * Return: Zero if successful, or a negative error code.
  */
-int snd_power_wait(struct snd_card *card, unsigned int power_state)
+int snd_power_wait_and_ref(struct snd_card *card, bool ref)
 {
 	wait_queue_entry_t wait;
 	int result = 0;
 
 	/* fastpath */
-	if (snd_power_get_state(card) == power_state)
+	if (ref)
+		snd_power_ref(card);
+	if (snd_power_get_state(card) == SNDRV_CTL_POWER_D0)
 		return 0;
 	init_waitqueue_entry(&wait, current);
 	add_wait_queue(&card->power_sleep, &wait);
@@ -1008,13 +1015,34 @@ int snd_power_wait(struct snd_card *card, unsigned int power_state)
 			result = -ENODEV;
 			break;
 		}
-		if (snd_power_get_state(card) == power_state)
+		if (snd_power_get_state(card) == SNDRV_CTL_POWER_D0)
 			break;
+		if (ref)
+			snd_power_unref(card);
 		set_current_state(TASK_UNINTERRUPTIBLE);
 		schedule_timeout(30 * HZ);
+		if (ref)
+			snd_power_ref(card);
 	}
 	remove_wait_queue(&card->power_sleep, &wait);
 	return result;
 }
+EXPORT_SYMBOL_GPL(snd_power_wait_and_ref);
+
+/**
+ * snd_power_wait - wait until the card gets powered up (old form)
+ * @card: soundcard structure
+ * @power_state: expected power state
+ *
+ * Same as snd_power_wait_and_ref() with ref=false.
+ * @power_state must be SNDRV_CTL_POWER_D0.
+ *
+ * Return: Zero if successful, or a negative error code.
+ */
+int snd_power_wait(struct snd_card *card, unsigned int power_state)
+{
+	WARN_ON(power_state != SNDRV_CTL_POWER_D0);
+	return snd_power_wait_and_ref(card, false);
+}
 EXPORT_SYMBOL(snd_power_wait);
 #endif /* CONFIG_PM */