snd/hda: Only get/put display_power once

Submitted by Takashi Iwai on April 10, 2019, 11:03 a.m.

Details

Message ID s5h8swiunph.wl-tiwai@suse.de
State Accepted
Commit 4bdc42094d9c4af75f90ee49b9009ea8dfe41444
Headers show
Series "snd/hda: Only get/put display_power once" ( rev: 3 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Takashi Iwai April 10, 2019, 11:03 a.m.
On Wed, 10 Apr 2019 12:44:49 +0200,
Takashi Iwai wrote:
> 
> On Wed, 10 Apr 2019 12:24:24 +0200,
> Chris Wilson wrote:
> > 
> > Quoting Takashi Iwai (2019-04-10 11:09:47)
> > > On Wed, 10 Apr 2019 10:17:33 +0200,
> > > Chris Wilson wrote:
> > > > 
> > > > While we only allow a single display power reference, the current
> > > > acquisition/release is racy and a direct call may run concurrently with
> > > > a runtime-pm worker. Prevent the double unreference by atomically
> > > > tracking the display_power_active cookie.
> > > > 
> > > > Testcase: igt/i915_pm_rpm/module-reload #glk-dsi
> > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Cc: Takashi Iwai <tiwai@suse.de>
> > > > Cc: Imre Deak <imre.deak@intel.com>
> > > 
> > > I rather prefer a more straightforward conversion, e.g. something like
> > > below.  Checking the returned cookie as the state flag is not quite
> > > intuitive, so revive the boolean state flag, and handle it
> > > atomically.
> > 
> > Access to the cookie itself is not atomic there, and theoretically
> > there could be a get/put/get running concurrently. Are you sure don't
> > want a refcount and lock here? :)
> 
> The refcount is what we want to reduce, so the suitable option would
> be a (yet another) mutex although the cmpxchg() should be enough for
> normal cases.
> 
> > Your call. For the case CI is hitting, it should do the trick (as we are
> > only seeing the race on put/put I think). CI will answer in a hour or
> > two.
> 
> OK, once when it seems working, I'll respin a patch with a mutex
> instead.  We have already a one that is used for the link state
> change, and this can be reused.

It's even simpler, so maybe this is a better way to go...

If this is confirmed to work, feel free to queue via drm tree.
I can't apply this because this is on top of your recent cookie and
sub-component changes that aren't on sound git tree (yet).


thanks,

Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] ALSA: hda: Fix racy display power access

snd_hdac_display_power() doesn't handle the concurrent calls carefully
enough, and it may lead to the doubly get_power or put_power calls,
when a runtime PM and an async work get called in racy way.

This patch addresses it by reusing the bus->lock mutex that has been
used for protecting the link state change in ext bus code, so that it
can protect against racy display state changes.  The initialization of
bus->lock was moved from snd_hdac_ext_bus_init() to
snd_hdac_bus_init() as well accordingly.

Testcase: igt/i915_pm_rpm/module-reload #glk-dsi
Reported-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/hda/ext/hdac_ext_bus.c | 1 -
 sound/hda/hdac_bus.c         | 1 +
 sound/hda/hdac_component.c   | 6 +++++-
 3 files changed, 6 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/sound/hda/ext/hdac_ext_bus.c b/sound/hda/ext/hdac_ext_bus.c
index 9c37d9af3023..ec7715c6b0c0 100644
--- a/sound/hda/ext/hdac_ext_bus.c
+++ b/sound/hda/ext/hdac_ext_bus.c
@@ -107,7 +107,6 @@  int snd_hdac_ext_bus_init(struct hdac_bus *bus, struct device *dev,
 	INIT_LIST_HEAD(&bus->hlink_list);
 	bus->idx = idx++;
 
-	mutex_init(&bus->lock);
 	bus->cmd_dma_state = true;
 
 	return 0;
diff --git a/sound/hda/hdac_bus.c b/sound/hda/hdac_bus.c
index 012305177f68..ad8eee08013f 100644
--- a/sound/hda/hdac_bus.c
+++ b/sound/hda/hdac_bus.c
@@ -38,6 +38,7 @@  int snd_hdac_bus_init(struct hdac_bus *bus, struct device *dev,
 	INIT_WORK(&bus->unsol_work, snd_hdac_bus_process_unsol_events);
 	spin_lock_init(&bus->reg_lock);
 	mutex_init(&bus->cmd_mutex);
+	mutex_init(&bus->lock);
 	bus->irq = -1;
 	return 0;
 }
diff --git a/sound/hda/hdac_component.c b/sound/hda/hdac_component.c
index 13915fdc6a54..dfe7e755f594 100644
--- a/sound/hda/hdac_component.c
+++ b/sound/hda/hdac_component.c
@@ -69,13 +69,15 @@  void snd_hdac_display_power(struct hdac_bus *bus, unsigned int idx, bool enable)
 
 	dev_dbg(bus->dev, "display power %s\n",
 		enable ? "enable" : "disable");
+
+	mutex_lock(&bus->lock);
 	if (enable)
 		set_bit(idx, &bus->display_power_status);
 	else
 		clear_bit(idx, &bus->display_power_status);
 
 	if (!acomp || !acomp->ops)
-		return;
+		goto unlock;
 
 	if (bus->display_power_status) {
 		if (!bus->display_power_active) {
@@ -98,6 +100,8 @@  void snd_hdac_display_power(struct hdac_bus *bus, unsigned int idx, bool enable)
 			bus->display_power_active = 0;
 		}
 	}
+ unlock:
+	mutex_unlock(&bus->lock);
 }
 EXPORT_SYMBOL_GPL(snd_hdac_display_power);
 

Comments

Quoting Takashi Iwai (2019-04-10 12:03:22)
> On Wed, 10 Apr 2019 12:44:49 +0200,
> Takashi Iwai wrote:
> > 
> > On Wed, 10 Apr 2019 12:24:24 +0200,
> > Chris Wilson wrote:
> > > 
> > > Quoting Takashi Iwai (2019-04-10 11:09:47)
> > > > On Wed, 10 Apr 2019 10:17:33 +0200,
> > > > Chris Wilson wrote:
> > > > > 
> > > > > While we only allow a single display power reference, the current
> > > > > acquisition/release is racy and a direct call may run concurrently with
> > > > > a runtime-pm worker. Prevent the double unreference by atomically
> > > > > tracking the display_power_active cookie.
> > > > > 
> > > > > Testcase: igt/i915_pm_rpm/module-reload #glk-dsi
> > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > Cc: Takashi Iwai <tiwai@suse.de>
> > > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > 
> > > > I rather prefer a more straightforward conversion, e.g. something like
> > > > below.  Checking the returned cookie as the state flag is not quite
> > > > intuitive, so revive the boolean state flag, and handle it
> > > > atomically.
> > > 
> > > Access to the cookie itself is not atomic there, and theoretically
> > > there could be a get/put/get running concurrently. Are you sure don't
> > > want a refcount and lock here? :)
> > 
> > The refcount is what we want to reduce, so the suitable option would
> > be a (yet another) mutex although the cmpxchg() should be enough for
> > normal cases.
> > 
> > > Your call. For the case CI is hitting, it should do the trick (as we are
> > > only seeing the race on put/put I think). CI will answer in a hour or
> > > two.
> > 
> > OK, once when it seems working, I'll respin a patch with a mutex
> > instead.  We have already a one that is used for the link state
> > change, and this can be reused.
> 
> It's even simpler, so maybe this is a better way to go...
> 
> If this is confirmed to work, feel free to queue via drm tree.
> I can't apply this because this is on top of your recent cookie and
> sub-component changes that aren't on sound git tree (yet).

Success \o/
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Ok, we'll plonk it in dinq, but I think it should apply to sound.git?
Looks fairly separate.

Anyway that can all be resolved in a later merge if required.

Thanks,
-Chris
On Wed, 10 Apr 2019 15:07:28 +0200,
Chris Wilson wrote:
> 
> Quoting Takashi Iwai (2019-04-10 12:03:22)
> > On Wed, 10 Apr 2019 12:44:49 +0200,
> > Takashi Iwai wrote:
> > > 
> > > On Wed, 10 Apr 2019 12:24:24 +0200,
> > > Chris Wilson wrote:
> > > > 
> > > > Quoting Takashi Iwai (2019-04-10 11:09:47)
> > > > > On Wed, 10 Apr 2019 10:17:33 +0200,
> > > > > Chris Wilson wrote:
> > > > > > 
> > > > > > While we only allow a single display power reference, the current
> > > > > > acquisition/release is racy and a direct call may run concurrently with
> > > > > > a runtime-pm worker. Prevent the double unreference by atomically
> > > > > > tracking the display_power_active cookie.
> > > > > > 
> > > > > > Testcase: igt/i915_pm_rpm/module-reload #glk-dsi
> > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > > Cc: Takashi Iwai <tiwai@suse.de>
> > > > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > > 
> > > > > I rather prefer a more straightforward conversion, e.g. something like
> > > > > below.  Checking the returned cookie as the state flag is not quite
> > > > > intuitive, so revive the boolean state flag, and handle it
> > > > > atomically.
> > > > 
> > > > Access to the cookie itself is not atomic there, and theoretically
> > > > there could be a get/put/get running concurrently. Are you sure don't
> > > > want a refcount and lock here? :)
> > > 
> > > The refcount is what we want to reduce, so the suitable option would
> > > be a (yet another) mutex although the cmpxchg() should be enough for
> > > normal cases.
> > > 
> > > > Your call. For the case CI is hitting, it should do the trick (as we are
> > > > only seeing the race on put/put I think). CI will answer in a hour or
> > > > two.
> > > 
> > > OK, once when it seems working, I'll respin a patch with a mutex
> > > instead.  We have already a one that is used for the link state
> > > change, and this can be reused.
> > 
> > It's even simpler, so maybe this is a better way to go...
> > 
> > If this is confirmed to work, feel free to queue via drm tree.
> > I can't apply this because this is on top of your recent cookie and
> > sub-component changes that aren't on sound git tree (yet).
> 
> Success \o/
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Ok, we'll plonk it in dinq, but I think it should apply to sound.git?
> Looks fairly separate.

Indeed, it's applicable, so I'm going to queue via sound tree.
I thought it would conflict but that was about the previous version
fiddling with cmpxchg().  This one is simpler, yeah.

> Anyway that can all be resolved in a later merge if required.

Sure, I guess it must be trivial, if any.


Thanks!

Takashi