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 >
