Re: [PATCH-resent-to-correct-ml 0/8] drm/xe: Convert xe_force_wake calls to guard helpers.

David Lechner
2025-02-04T17:40:13Z

On 2/4/25 7:22 AM, Maarten Lankhorst wrote:
> Ignore my previous series please, it should have been sent to intel-xe, was sent to intel-gfx.
>
> Instead of all this repetition of
>
> {
> unsigned fw_ref;
>
> fw_ref = xe_force_wake_get(fw, domain);
> if (!xe_force_wake_ref_has_domain(..))
> return -ETIMEDOUT;
>
> ...
>
> out:
> xe_force_wake_put(fw_ref);
> return ret;
> }
>
> I thought I would look at how to replace it with the guard helpers.
> It is easy, but it required some minor fixes to make DEFINE_LOCK_GUARD_1
> work with extra init arguments.
>
> So I changed the function signature slightly to make the first optional argument
> a struct member (current behavior), and any additional argument goes to the init
> call.
>
> This replaces the previous code with
> {
> scoped_cond_guard(xe_force_wake_get, return -ETIMEDOUT, fw, domain) {
> ....
>
> return ret;
> }
> }
>
> I' ve thought also of playing with this:
> {
> CLASS(xe_force_wake_get, fw_ref)(fw, domain);
> if (!fw_ref.lock))
> return -ETIMEDOUT;
>
> ...
> return ret;
> }
>
> I'm just fearing that the scoped_cond_guard makes it imposssible to get this
> wrong, while in the second example it's not clear that it can fail, and that
> you have to check.
>
> Let me know what you think!
> Feedback welcome for the header change as well, should probably go into the locking tree..
>
When I tried to make a similar improvement, Linus said to please stop trying
with the conditional guard stuff [1]. So my advice is don't do it.

Just replace the:

> ...
>
> out:

with a helper function if you want to get rid of the gotos.

That is what we are doing in the iio subsystem [2][3].

[1]: https://lore.kernel.org/all/CAHk-=whn07tnDosPfn+UcAtWHBcLg=KqA16SHVv0GV4t8P1fHw@mail.gmail.com/
[2]: https://lore.kernel.org/all/20250105172613.1204781-1-jic23@kernel.org/
[3]: https://lore.kernel.org/all/20250202210045.1a9e85d7@jic23-huawei/