| Message ID | 20190410081733.22271-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | snd/hda: Only get/put display_power once | expand |
Quoting Chris Wilson (2019-04-10 09:17:33) > 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. I just get the feeling this is elaborate paper and the problem is that we shouldn't be doing a double-free in the first place. -Chris
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); } } }
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
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(-) 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);
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
diff --git a/sound/hda/hdac_component.c b/sound/hda/hdac_component.c index 13915fdc6a54..f0fd0d83c90e 100644 --- a/sound/hda/hdac_component.c +++ b/sound/hda/hdac_component.c @@ -66,6 +66,7 @@ EXPORT_SYMBOL_GPL(snd_hdac_set_codec_wakeup); void snd_hdac_display_power(struct hdac_bus *bus, unsigned int idx, bool enable) { struct drm_audio_component *acomp = bus->audio_component; + unsigned long cookie; dev_dbg(bus->dev, "display power %s\n", enable ? "enable" : "disable"); @@ -78,26 +79,22 @@ 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 (acomp->ops->get_power) - cookie = acomp->ops->get_power(acomp->dev); + cookie = -1; + if (acomp->ops->get_power) + cookie = acomp->ops->get_power(acomp->dev); + if (!cmpxchg(&bus->display_power_active, 0, cookie)) { snd_hdac_set_codec_wakeup(bus, true); snd_hdac_set_codec_wakeup(bus, false); - bus->display_power_active = cookie; + cookie = 0; } } else { - if (bus->display_power_active) { - unsigned long cookie = bus->display_power_active; + cookie = xchg(&bus->display_power_active, 0); + } - if (acomp->ops->put_power) - acomp->ops->put_power(acomp->dev, cookie); + if (cookie && acomp->ops->put_power) + acomp->ops->put_power(acomp->dev, cookie); - bus->display_power_active = 0; - } - } } EXPORT_SYMBOL_GPL(snd_hdac_display_power);
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> --- sound/hda/hdac_component.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-)