Re: [PATCH] snd/hda: Only get/put display_power once

[Takashi Iwai <tiwai@suse.de>]

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

--- 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);
 		}
 	}
 }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx