snd/hda: Only get/put display_power once

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

Details

Message ID s5hk1g2uq6s.wl-tiwai@suse.de
State New
Headers show
Series "snd/hda: Only get/put display_power once" ( rev: 2 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Takashi Iwai April 10, 2019, 10:09 a.m.
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.


thanks,

Takashi

Patch hide | download patch | download mbox

--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -367,7 +367,8 @@  struct hdac_bus {
 	/* DRM component interface */
 	struct drm_audio_component *audio_component;
 	long display_power_status;
-	unsigned long display_power_active;
+	bool display_power_active;
+	unsigned long display_cookie;
 
 	/* parameters required for enhanced capabilities */
 	int num_streams;
diff --git a/sound/hda/hdac_component.c b/sound/hda/hdac_component.c
index 13915fdc6a54..da20f439578a 100644
--- a/sound/hda/hdac_component.c
+++ b/sound/hda/hdac_component.c
@@ -78,24 +78,19 @@  void snd_hdac_display_power(struct hdac_bus *bus, unsigned int idx, bool enable)
 		return;
 
 	if (bus->display_power_status) {
-		if (!bus->display_power_active) {
-			unsigned long cookie = -1;
-
+		if (!cmpxchg(&bus->display_power_active, false, true)) {
 			if (acomp->ops->get_power)
-				cookie = acomp->ops->get_power(acomp->dev);
+				bus->display_cookie =
+					acomp->ops->get_power(acomp->dev);
 
 			snd_hdac_set_codec_wakeup(bus, true);
 			snd_hdac_set_codec_wakeup(bus, false);
-			bus->display_power_active = cookie;
 		}
 	} else {
-		if (bus->display_power_active) {
-			unsigned long cookie = bus->display_power_active;
-
+		if (xchg(&bus->display_power_active, false)) {
 			if (acomp->ops->put_power)
-				acomp->ops->put_power(acomp->dev, cookie);
-
-			bus->display_power_active = 0;
+				acomp->ops->put_power(acomp->dev,
+						      bus->display_cookie);
 		}
 	}
 }

Comments

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? :)

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.
-Chris
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.


thanks,

Takashi