All of lore.kernel.org
 help / color / mirror / Atom feed
From: David Lechner <dlechner@baylibre.com>
To: Maarten Lankhorst <dev@lankhorst.se>, intel-xe@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
	Ingo Molnar <mingo@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Will Deacon <will@kernel.org>, Waiman Long <longman@redhat.com>,
	Boqun Feng <boqun.feng@gmail.com>
Subject: Re: [PATCH-resent-to-correct-ml 0/8] drm/xe: Convert xe_force_wake calls to guard helpers.
Date: Tue, 4 Feb 2025 11:40:11 -0600	[thread overview]
Message-ID: <e4befaab-9622-40d3-bd4c-e12111df45e7@baylibre.com> (raw)
In-Reply-To: <20250204132238.162608-1-dev@lankhorst.se>

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/

  parent reply	other threads:[~2025-02-06 15:46 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-02-04 13:22 [PATCH-resent-to-correct-ml 0/8] drm/xe: Convert xe_force_wake calls to guard helpers Maarten Lankhorst
2025-02-04 13:22 ` [PATCH-resent-to-correct-ml 1/8] header/cleanup.h: Add _init_args to DEFINE_LOCK_GUARD_1(_COND) Maarten Lankhorst
2025-02-04 13:22 ` [PATCH-resent-to-correct-ml 2/8] drm/xe/gt: Unify xe_hw_fence_irq_finish() calls Maarten Lankhorst
2025-02-04 15:20   ` Lucas De Marchi
2025-02-04 13:22 ` [PATCH-resent-to-correct-ml 3/8] drm/xe: Add scoped guards for xe_force_wake Maarten Lankhorst
2025-02-04 15:28   ` Lucas De Marchi
2025-02-04 16:30   ` Michal Wajdeczko
2025-02-04 22:28     ` Maarten Lankhorst
2025-02-04 22:49       ` Rodrigo Vivi
2025-02-04 13:22 ` [PATCH-resent-to-correct-ml 4/8] drm/xe: Add xe_force_wake_get_all Maarten Lankhorst
2025-02-04 13:22 ` [PATCH-resent-to-correct-ml 5/8] drm/xe/coredump: Use guard helpers for xe_force_wake Maarten Lankhorst
2025-02-04 15:40   ` Lucas De Marchi
2025-02-04 13:22 ` [PATCH-resent-to-correct-ml 6/8] drm/xe/gsc: Use guard helper for xe_gsc_print_info Maarten Lankhorst
2025-02-04 13:22 ` [PATCH-resent-to-correct-ml 7/8] drm/xe/vram: Use xe_force_wake guard helper Maarten Lankhorst
2025-02-04 13:22 ` [PATCH-resent-to-correct-ml 8/8] drm/xe/gt: Convert to xe_force_wake guard helpers Maarten Lankhorst
2025-02-04 15:21 ` ✓ CI.Patch_applied: success for drm/xe: Convert xe_force_wake calls to " Patchwork
2025-02-04 15:21 ` ✗ CI.checkpatch: warning " Patchwork
2025-02-04 15:22 ` ✓ CI.KUnit: success " Patchwork
2025-02-04 15:38 ` ✓ CI.Build: " Patchwork
2025-02-04 15:41 ` ✓ CI.Hooks: " Patchwork
2025-02-04 15:42 ` ✗ CI.checksparse: warning " Patchwork
2025-02-04 17:40 ` David Lechner [this message]
2025-02-05 20:11   ` [PATCH-resent-to-correct-ml 0/8] " Maarten Lankhorst
2025-02-05  6:12 ` ✓ Xe.CI.BAT: success for " Patchwork

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=e4befaab-9622-40d3-bd4c-e12111df45e7@baylibre.com \
    --to=dlechner@baylibre.com \
    --cc=boqun.feng@gmail.com \
    --cc=dev@lankhorst.se \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=intel-xe@lists.freedesktop.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=longman@redhat.com \
    --cc=mingo@kernel.org \
    --cc=peterz@infradead.org \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.