All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Danilo Krummrich" <dakr@kernel.org>
To: "Onur Özkan" <work@onurozkan.dev>
Cc: <linux-kernel@vger.kernel.org>, <rust-for-linux@vger.kernel.org>,
	<dri-devel@lists.freedesktop.org>, <aliceryhl@google.com>,
	<daniel.almeida@collabora.com>, <airlied@gmail.com>,
	<simona@ffwll.ch>, <ojeda@kernel.org>, <boqun@kernel.org>,
	<gary@garyguo.net>, <bjorn3_gh@protonmail.com>,
	<lossin@kernel.org>, <a.hindborg@kernel.org>, <tmgross@umich.edu>
Subject: Re: [PATCH v4 3/4] drm/tyr: add GPU reset infrastructure
Date: Sat, 15 Aug 2026 13:33:01 +0200	[thread overview]
Message-ID: <DKPHGNOPN7PT.2HF3KUPO3VM79@kernel.org> (raw)
In-Reply-To: <20260815-tyr-reset-impl-v4-3-578df9a5e576@onurozkan.dev>

On Sat Aug 15, 2026 at 12:23 PM CEST, Onur Özkan wrote:
> +#[pin_data]
> +struct Controller<'bound> {

Please only use the lifetime name 'bound for bus device private data (which
defines the 'bound lifetime). Everything within is shorter lived and should
carry a different name (see also [1]).

I'd suggest 'ctrl or just 'a, once you have self-referencial fields 'ctrl (or
similar) is preferred.

[1] https://lore.kernel.org/all/DKAINVQDNE79.3JRKZDQJCSX9@kernel.org/

> +    fn reset_work(self: &Arc<Self>) {
> +        if !self.try_transition(ResetState::Pending, ResetState::InProgress) {
> +            return;
> +        }
> +
> +        dev_info!(self.pdev, "Starting GPU reset.\n");

Please use dev_dbg!().

> +        // Wait for current hardware accesses to finish before resetting.
> +        let reset_guard = self.hw.close();
> +        let reset_result = run_reset(self.pdev.as_ref(), &self.iomem);
> +        drop(reset_guard);
> +
> +        if let Err(e) = reset_result {
> +            dev_err!(self.pdev, "GPU reset failed: {:?}\n", e);
> +
> +            // TODO: Unplug the GPU.
> +            // There is no API for unplugging the GPU and this is unreachable
> +            // for now since there are no hardware users for reset API.
> +        } else {
> +            dev_info!(self.pdev, "GPU reset completed.\n");

Same here.

> +        }
> +
> +        let _ = self.try_transition(ResetState::InProgress, ResetState::Idle);
> +    }
> +}
> +
> +/// User-facing handle for scheduling resets.
> +///
> +/// Dropping the handle drains any queued or in-flight reset work before the
> +/// [`ScopedQueue`] and the clock and regulator resources are released.
> +pub(crate) struct ResetHandle<'bound> {

Same as above 'reset or just 'a.

> +    controller: Arc<Controller<'bound>>,

This can just be ScopedWork<Controller<'a>>; no extra reference count and
allocation needed, as the constructor gives you an impl PinInit.

It also gets you rid of the open-coded cancel_sync() call in the destructor of
ResetHandle.

Also note that [2] already has Send + Sync for OwnedQueue when OwnedQueue is
introduced, so you can drop patch 1 of this series.

Thanks,
Danilo

[2] https://lore.kernel.org/all/20260807165252.3849875-1-dakr@kernel.org/

  reply	other threads:[~2026-08-15 11:33 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-08-15 10:23 [PATCH v4 0/4] drm/tyr: GPU reset infrastructure Onur Özkan
2026-08-15 10:23 ` [PATCH v4 1/4] rust: workqueue: impl Send and Sync for OwnedQueue Onur Özkan
2026-08-15 10:23 ` [PATCH v4 2/4] drm/tyr: clear stale IRQ state before soft reset Onur Özkan
2026-08-15 10:23 ` [PATCH v4 3/4] drm/tyr: add GPU reset infrastructure Onur Özkan
2026-08-15 11:33   ` Danilo Krummrich [this message]
2026-08-15 10:24 ` [PATCH v4 4/4] drm/tyr: put iomem behind the hardware gate Onur Özkan
2026-08-15 10:34 ` [PATCH v4 0/4] drm/tyr: GPU reset infrastructure Onur Özkan

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=DKPHGNOPN7PT.2HF3KUPO3VM79@kernel.org \
    --to=dakr@kernel.org \
    --cc=a.hindborg@kernel.org \
    --cc=airlied@gmail.com \
    --cc=aliceryhl@google.com \
    --cc=bjorn3_gh@protonmail.com \
    --cc=boqun@kernel.org \
    --cc=daniel.almeida@collabora.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=gary@garyguo.net \
    --cc=linux-kernel@vger.kernel.org \
    --cc=lossin@kernel.org \
    --cc=ojeda@kernel.org \
    --cc=rust-for-linux@vger.kernel.org \
    --cc=simona@ffwll.ch \
    --cc=tmgross@umich.edu \
    --cc=work@onurozkan.dev \
    /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.