Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [Medium] Spurious WARN_ON triggered during system suspend when GPU jobs are active. Pre-existing issues: - [High] Unconditional hardware access after `pm_runtime_get_sync()` failure causes kernel panic. -- commit a7ee5ddc6b752f83ff2b60cb2bbc09648f803bce Author: Adrián Larumbe <[email protected]> drm/panfrost: Add warning messages to fatal error conditions This commit adds drm_WARN_ON macros to panfrost_device_fini and panfrost_device_runtime_suspend to warn users when PM reference acquisition fails or when inflight jobs are reported during the PM suspend path. > diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c > b/drivers/gpu/drm/panfrost/panfrost_device.c > index 70c8109ea6987..d8acae9b8cfac 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_device.c > +++ b/drivers/gpu/drm/panfrost/panfrost_device.c > @@ -367,7 +368,7 @@ int panfrost_device_init(struct panfrost_device *pfdev) > > void panfrost_device_fini(struct panfrost_device *pfdev) > { > - pm_runtime_get_sync(pfdev->base.dev); > + drm_WARN_ON(&pfdev->base, pm_runtime_get_sync(pfdev->base.dev) < 0); > pm_runtime_dont_use_autosuspend(pfdev->base.dev); > pm_runtime_disable(pfdev->base.dev); > pm_runtime_put_noidle(pfdev->base.dev); [Severity: High] This isn't a bug introduced by this patch, but while we are adding a warning here, does it make sense to continue execution if pm_runtime_get_sync fails? If the device fails to power on due to a regulator or clock failure, the warning is logged but the function continues and calls: panfrost_device_fini() panfrost_jm_stop_sched_jobs() panfrost_device_disable_hw() Both of these subsequent functions write to GPU registers. Accessing hardware registers of an unclocked or unpowered device on ARM SoCs triggers a Data Abort exception, leading to a hard kernel panic. Should we abort the teardown or skip the hardware disable steps if the PM resume fails? [ ... ] > @@ -517,7 +518,7 @@ static int panfrost_device_runtime_suspend(struct device > *dev) > { > struct panfrost_device *pfdev = dev_get_drvdata(dev); > > - if (!panfrost_jm_is_idle(pfdev)) > + if (drm_WARN_ON(&pfdev->base, !panfrost_jm_is_idle(pfdev))) > return -EBUSY; > > panfrost_device_disable_hw(pfdev); [Severity: Medium] Will this trigger a spurious warning stack trace if the system is suspended while a GPU workload is still executing? When a user suspends the system with an active GPU workload, the kernel freezes userspace tasks but jobs already submitted to the hardware can continue running. During the system suspend flow, pm_runtime_force_suspend() intentionally bypasses usage counters and forces a call directly to panfrost_device_runtime_suspend(). Since the job might still be executing on the hardware, panfrost_jm_is_idle() can legitimately return false here. Returning -EBUSY to abort the suspend is the expected behavior, but the new warning assumes this state is always a bug and will print a stack trace. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10
