Re: [PATCH v8 2/3] drm/tyr: add GPU reset infrastructure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] drm/tyr: prepared clocks leaked on error in
  TyrPlatformDriver::probe()
- [Low] drm/tyr: overly restrictive &'reset self lifetime in
  ResetHandle::schedule()
--

commit 2f1b0845121531379549eca9ca27fd1ffc58562f
Author: Onur Özkan <work@onurozkan.dev>

drm/tyr: add GPU reset infrastructure

This commit adds GPU reset infrastructure using a dedicated ordered
workqueue and an SRCU-based hardware access gate, and drains pending
or running reset work on teardown.

Link: https://gitlab.freedesktop.org/panfrost/linux/-/work_items/28

> diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
> index 5b57bc9a1d3e7..863c8008a132b 100644
> --- a/drivers/gpu/drm/tyr/driver.rs
> +++ b/drivers/gpu/drm/tyr/driver.rs
[ ... ]
> @@ -171,8 +153,11 @@ fn probe<'bound>(
>                  _reg <- unsafe { drm::Registration::new(
>                      pdev.as_ref(),
>                      unreg_dev,
> -                    pin_init!(TyrDrmRegistrationData {
> +                    try_pin_init!(TyrDrmRegistrationData {
>                          pdev,
> +                        // SAFETY: The registration is owned by platform driver data,
> +                        // so reset work is drained before borrowed data expires.
> +                        reset <- reset::ResetHandle::new(pdev, iomem.as_arc_borrow())?,
>                          fw: firmware,
>                          clks <- new_mutex!(Clocks {
>                              core: core_clk,

[Severity: Medium]
Does this leak the prepared and enabled clocks if ResetHandle::new() or
its PinInit initializer fails?

Drop for Clk and OptionalClk only calls clk_put(), while
disable_unprepare() is only called in Clocks::drop():

drivers/gpu/drm/tyr/driver.rs:Clocks::drop() {
    fn drop(&mut self) {
        self.core.disable_unprepare();
        self.stacks.disable_unprepare();
        self.coregroup.disable_unprepare();
    }
}

Because TyrDrmRegistrationData now initializes reset via try_pin_init!
before constructing Clocks, a failure in Queue::build() or HwGate::new()
(as well as earlier error paths after prepare_enable() in probe()) drops
core_clk, stacks_clk, and coregroup_clk without disabling or unpreparing
them, leaving the hardware clocks enabled and triggering a warning in
clk_put().  Wrapping the clocks in Clocks immediately after
prepare_enable() would ensure they are cleaned up on all error paths.

> diff --git a/drivers/gpu/drm/tyr/reset.rs b/drivers/gpu/drm/tyr/reset.rs
> new file mode 100644
> index 0000000000000..640371b3104da
> --- /dev/null
> +++ b/drivers/gpu/drm/tyr/reset.rs
[ ... ]
> +pub(crate) struct ResetHandle<'reset> {
> +    #[pin]
> +    controller: ScopedWork<Controller<'reset>>,
> +    wq: OwnedQueue,
> +}
[ ... ]
> +    /// Schedules a GPU reset on the dedicated workqueue.
> +    ///
> +    /// If a reset is already pending or in progress the call is a no-op.
> +    #[expect(dead_code)]
> +    pub(crate) fn schedule(&'reset self) {
> +        // TODO: Similar to `panthor_device_schedule_reset()` in Panthor, add a
> +        // power management check once Tyr supports it.
> +
> +        if self
> +            .controller
> +            .try_transition(ResetState::Idle, ResetState::Pending)
> +        {
> +            let _ = self.wq.enqueue_scoped(&self.controller);
> +        }
> +    }

[Severity: Low]
Can ResetHandle::schedule() take &self instead of &'reset self?

ScopedWork wraps its payload in Opaque (UnsafeCell), making
ResetHandle<'reset> invariant over 'reset, where 'reset is the device
binding lifetime in TyrDrmRegistrationData<'drm>.  Requiring &'reset self
prevents callers holding a normal short-lived borrow of
TyrDrmRegistrationData or ResetHandle from calling schedule(), and
neither try_transition() nor enqueue_scoped() needs the reference to live
for 'reset.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-tyr-reset-impl-v8-0-2ee298ac9808@onurozkan.dev?part=2