Re: [Intel-gfx] [PATCH] snd/hda: Only get/put display_power once
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 > > > > > > Cc: Takashi Iwai > > > > > > Cc: Imre Deak > > > > > > > > > > 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 > > 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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] snd/hda: Only get/put display_power once
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 > > > > > Cc: Takashi Iwai > > > > > Cc: Imre Deak > > > > > > > > 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 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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] snd/hda: Only get/put display_power once
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
> > > > Cc: Takashi Iwai
> > > > Cc: Imre Deak
> > >
> > > 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
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
Cc: Imre Deak
Signed-off-by: Takashi Iwai
---
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(>hlink_list);
bus->idx = idx++;
- mutex_init(>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(>unsol_work, snd_hdac_bus_process_unsol_events);
spin_lock_init(>reg_lock);
mutex_init(>cmd_mutex);
+ mutex_init(>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(>lock);
if (enable)
set_bit(idx, >display_power_status);
else
clear_bit(idx, >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(>lock);
}
EXPORT_SYMBOL_GPL(snd_hdac_display_power);
--
2.16.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] snd/hda: Only get/put display_power once
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 > > > Cc: Takashi Iwai > > > Cc: Imre Deak > > > > 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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] snd/hda: Only get/put display_power once
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 > > Cc: Takashi Iwai > > Cc: Imre Deak > > 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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] snd/hda: Only get/put display_power once
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
> Cc: Takashi Iwai
> Cc: Imre Deak
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(>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(>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
Re: [Intel-gfx] [PATCH] snd/hda: Only get/put display_power once
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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] snd/hda: Only get/put display_power once
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
Cc: Takashi Iwai
Cc: Imre Deak
---
sound/hda/hdac_component.c | 23 ++-
1 file changed, 10 insertions(+), 13 deletions(-)
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(>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(>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);
--
2.20.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
