Re: [PATCH v4 3/4] drm/tyr: add GPU reset infrastructure

On Sat, 15 Aug 2026 13:33:01 +0200
Danilo Krummrich <[email protected]> wrote:

> 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/[email protected]/
> 

Noted, thanks.

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

Yeah I am aware that (you already said that couple times in weekly calls),
I am just being a bit lazy on this, sorry :/. I will cover this in v6.

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

Okay, I will pull your series and drop this patch along with ScopedQueue and
OwnedQueue as they are already included in your series.

Thanks,
Onur

> 
> Thanks,
> Danilo
> 
> [2] https://lore.kernel.org/all/[email protected]/