linux-kernel - Re: [PATCH-resent-to-correct-ml 3/8] drm/xe: Add scoped guards for xe_force

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <smlsfjxrukptsnkhb4wkoeb6wd4ze7p77lkakzktun2qsa5c56@7bxye7oqmawc>
Date: Tue, 4 Feb 2025 09:28:05 -0600
From: Lucas De Marchi <lucas.demarchi@...el.com>
To: Maarten Lankhorst <dev@...khorst.se>
CC: <intel-xe@...ts.freedesktop.org>, <dri-devel@...ts.freedesktop.org>,
	<linux-kernel@...r.kernel.org>, Ingo Molnar <mingo@...nel.org>, David Lechner
	<dlechner@...libre.com>, Peter Zijlstra <peterz@...radead.org>, Will Deacon
	<will@...nel.org>, Waiman Long <longman@...hat.com>, Boqun Feng
	<boqun.feng@...il.com>
Subject: Re: [PATCH-resent-to-correct-ml 3/8] drm/xe: Add scoped guards for
 xe_force_wake

On Tue, Feb 04, 2025 at 02:22:32PM +0100, Maarten Lankhorst wrote:
>Instead of finding bugs where we may or may not release force_wake, I've
>decided to be inspired by the spinlock guards, and use the same ones to
>do xe_force_wake handling.
>
>Examples are added as documentation in xe_force_wake.c
>
>Signed-off-by: Maarten Lankhorst <dev@...khorst.se>
>---
> drivers/gpu/drm/xe/xe_force_wake.c | 51 ++++++++++++++++++++++++++++++
> drivers/gpu/drm/xe/xe_force_wake.h | 15 +++++++++
> 2 files changed, 66 insertions(+)
>
>diff --git a/drivers/gpu/drm/xe/xe_force_wake.c b/drivers/gpu/drm/xe/xe_force_wake.c
>index 4f6784e5abf88..805c19f6de9e7 100644
>--- a/drivers/gpu/drm/xe/xe_force_wake.c
>+++ b/drivers/gpu/drm/xe/xe_force_wake.c
>@@ -16,6 +16,57 @@
>
> #define XE_FORCE_WAKE_ACK_TIMEOUT_MS	50
>
>+/**
>+ * DOC: Force wake handling
>+ *

doc here should start explaining what is force wake, what it does, the
flags to wake specific parts of the gpu.

>+ * Traditionally, the force wake handling has been done using the error prone
>+ * set of calls:

I'd start with the new/recommended way rather than to digress on
non-recommended ways - this style doesn't age well in a few years.

>+ *
>+ * int func(struct xe_force_wake *fw)
>+ * {
>+ * 	unsigned int fw_ref = xe_force_wake_get(fw, XE_FORCEWAKE_ALL);
>+ * 	if (!fw_ref)
>+ * 		return -ETIMEDOUT;
>+ *
>+ * 	err = do_something();
>+ *
>+ * 	xe_force_wake_put(fw, fw_ref);
>+ * 	return err;
>+ * }
>+ *
>+ * A new, failure-safe approach is by using the scoped helpers,
>+ * which changes the function to this:
>+ *
>+ * int func(struct xe_force_wake *fw)
>+ * {
>+ * 	scoped_cond_guard(xe_force_wake_get, return -ETIMEDOUT, fw, XE_FORCEWAKE_ALL) {
>+ * 		return do_something();
>+ * 	}
>+ * }
>+ *
>+ * For completeness, the following options also work:
>+ * void func(struct xe_force_wake *fw)
>+ * {
>+ * 	scoped_guard(xe_force_wake_get, fw, XE_FORCEWAKE_ALL) {
>+ * 		do_something_only_if_fw_acquired();
>+ * 	}
>+ * }
>+ *
>+ * You can use xe_force_wake instead of force_wake_get, if the code
>+ * must run but errors acquiring ignored:
>+ * void func(struct xe_force_wake *fw)
>+ * {
>+ * 	scoped_guard(xe_force_wake, fw, XE_FORCEWAKE_ALL) {
>+ * 		always_do_something_maybe_fw();
>+ * 	}
>+ *
>+ * 	do_something_no_fw();
>+ *
>+ * 	guard(xe_force_wake)(fw, XE_FORCEWAKE_ALL);
>+ * 	always_do_something_maybe_fw();
>+ * }
>+ */
>+
> static const char *str_wake_sleep(bool wake)
> {
> 	return wake ? "wake" : "sleep";
>diff --git a/drivers/gpu/drm/xe/xe_force_wake.h b/drivers/gpu/drm/xe/xe_force_wake.h
>index 0e3e84bfa51c3..0fb1baae0a3a3 100644
>--- a/drivers/gpu/drm/xe/xe_force_wake.h
>+++ b/drivers/gpu/drm/xe/xe_force_wake.h
>@@ -9,6 +9,8 @@
> #include "xe_assert.h"
> #include "xe_force_wake_types.h"
>
>+#include <linux/cleanup.h>
>+
> struct xe_gt;
>
> void xe_force_wake_init_gt(struct xe_gt *gt,
>@@ -61,4 +63,17 @@ xe_force_wake_ref_has_domain(unsigned int fw_ref, enum xe_force_wake_domains dom
> 	return fw_ref & domain;
> }
>
>+DEFINE_LOCK_GUARD_1(xe_force_wake, struct xe_force_wake,
>+		    _T->fw_ref = xe_force_wake_get(_T->lock, domain),
>+		    xe_force_wake_put(_T->lock, _T->fw_ref),
>+		    unsigned int fw_ref, enum xe_force_wake_domains domain);
>+
>+DEFINE_LOCK_GUARD_1_COND(xe_force_wake, _get,
>+			 _T->fw_ref = xe_force_wake_get_all(_T->lock, domain),
>+			 enum xe_force_wake_domains domain);
>+
>+/* Only useful for guard xe_force_wake, guard xe_force_wake_get gets all or nothing */

please add an usage example here is it's where people trying to use the
interface will look at rather than the kernel-doc. But this seems not
used

>+#define xe_force_wake_scope_has_domain(domain) \
>+	(xe_force_wake_ref_has_domain(scope.fw_ref, domain))

extra paranthesis here?

Lucas De Marchi

>+
> #endif
>-- 
>2.47.1
>