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..

Cc: Ingo Molnar <mingo@kernel.org>
Cc: David Lechner <dlechner@baylibre.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Will Deacon <will@kernel.org>
Cc: Waiman Long <longman@redhat.com>
Cc: Boqun Feng <boqun.feng@gmail.com>

Maarten Lankhorst (8):
  header/cleanup.h: Add _init_args to DEFINE_LOCK_GUARD_1(_COND)
  drm/xe/gt: Unify xe_hw_fence_irq_finish() calls.
  drm/xe: Add scoped guards for xe_force_wake
  drm/xe: Add xe_force_wake_get_all
  drm/xe/coredump: Use guard helpers for xe_force_wake.
  drm/xe/gsc: Use guard helper for xe_gsc_print_info.
  drm/xe/vram: Use xe_force_wake guard helper
  drm/xe/gt: Convert to xe_force_wake guard helpers

 drivers/gpu/drm/xe/xe_devcoredump.c |  36 ++---
 drivers/gpu/drm/xe/xe_force_wake.c  | 161 ++++++++++++++----
 drivers/gpu/drm/xe/xe_force_wake.h  |  17 ++
 drivers/gpu/drm/xe/xe_gsc.c         |  22 +--
 drivers/gpu/drm/xe/xe_gt.c          | 243 ++++++++++------------------
 drivers/gpu/drm/xe/xe_vram.c        |  45 +++---
 include/linux/cleanup.h             |  30 ++--
 7 files changed, 293 insertions(+), 261 deletions(-)

-- 
2.47.1

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/

Hey,

Thanks for your feedback. I think in this case you are right, and it 
will become unreadable.

I'll try to make sparse annotations work, and then see if we can enable 
it in CI for all.

Cheers,
~Maarten

On 2025-02-04 18:40, David Lechner wrote:
> 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/