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] [day] [month] [year] [list]

Message-ID: <Z6KZglMsB-4EI8YC@intel.com>
Date: Tue, 4 Feb 2025 17:49:38 -0500
From: Rodrigo Vivi <rodrigo.vivi@...el.com>
To: Maarten Lankhorst <dev@...khorst.se>
CC: Michal Wajdeczko <michal.wajdeczko@...el.com>,
	<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 11:28:03PM +0100, Maarten Lankhorst wrote:
> Hey,
> 
> 
> On 2025-02-04 17:30, Michal Wajdeczko wrote:
> > Hi Maarten,
> > 
> > On 04.02.2025 14:22, 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.
> > 
> > You may want to take a look at [1], which was based on [2], that
> > introduce fw guard class (and it was already acked and reviewed).
> > Merging was postponed only due to a request to prepare larger series
> > that would convert all existing usages to the new model.
> > 
> > And similar guard approach for our RPM was proposed in [3]
> > 
> > Michal
> > 
> > [1] https://patchwork.freedesktop.org/series/141516/
> > [2] https://patchwork.freedesktop.org/series/134958/
> > [3] https://patchwork.freedesktop.org/series/134955/
> 
> Excellent. I'm glad we're in agreement that doing forcewake handling in
> guard handlers is a good thing. :-)

Just for the record. I had a similar feeling back there and
also now with the new series: I believe the code itself keeps harder
to read and follow.

But if that's really a big advantage on the protection like you are
all advocating for, let's go ahead.

> 
> I have taken a look at the patch series. I think the approach I've taken is
> a refinement of your series. Yours is already nearly there, but it still
> keeps the rough edges of the original API.
> 
> To smooth them, I have created 2 constructors, xe_force_wake, and
> xe_force_wake_get. The former is used if you want to run code regardless
> whether it succeeds, the latter is when you do.
> 
> This allows code like:
> scoped_cond_guard(xe_force_wake_get, return -ETIMEDOUT, fw,
> XE_FORCE_WAKE_ALL) {}
> to work flawlessly as intended, without having to check
> xe_force_wake_ref_has_domain(XE_FORCE_WAKE_ALL);
> 
> I think this cleanup removes a nasty source of errors.
> 
> When you don't care about failure:
> scoped_guard(xe_force_wake, fw, XE_FORCE_WAKE_ALL) {
> 	if (!xe_force_wake_scope_has_domain(XE_FORCE_WAKE_ALL))
> 		printk("Oh noez, anyway..\n");
> 
> 	/* Continue and pretend nothing happened */
> }
> 
> And for optional code, same as scoped_cond_guard, but as scoped_guard:
> 
> scoped_guard(xe_force_wake_get, fw, XE_FORCE_WAKE_ALL) {
> 	/* Only runs this block if acquire completely succeeded, otherwise use
> xe_force_wake */
> }
> 
> All in all, I'm open for bikesheds, but I think this has the potential to
> improve xe_force_wake handling even further!
> 
> I wasn't aware of your previous attempt when I wrote this and fought
> linux/cleanup.h, otherwise I would have taken that as a base and credit your
> work.
> 
> Cheers,
> ~Maarten
>