Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Inverted logic for checking kernel queues status breaks privileged instruction fault handling. --
commit 7d87e331de93eb7e4e60fea7e6c8746a5d76ff9d Author: Ulisses Paixao <[email protected]> drm/amd/amdgpu: remove duplicated code in gfx_v11 and gfx_v12 This commit deduplicates the handling of privileged instruction faults for gfx v11 and v12. It introduces a common function, amdgpu_gfx_handle_priv_fault, to identify the faulty ring and trigger a scheduler fault. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c > index 0506b90f318e4..836dad8fe06bb 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c [ ... ] > @@ -855,6 +855,46 @@ int amdgpu_gfx_enable_kgq(struct amdgpu_device *adev, > int xcc_id) > return r; > } > > +/** > + * amdgpu_gfx_handle_priv_fault - Handle privileged instruction fault > + * > + * @adev: amdgpu_device pointer > + * @me_id: micro-engine ID of the faulty ring > + * @pipe_id: pipe ID of the faulty ring > + * @queue_id: queue ID of the faulty ring > + * > + * This function handles privileged instruction faults by identifying > + * the faulty ring (gfx or compute) and triggering a scheduler fault > + */ > +void amdgpu_gfx_handle_priv_fault(struct amdgpu_device *adev, > + u8 me_id, u8 pipe_id, u8 queue_id) > +{ > + struct amdgpu_ring *ring; > + int i; > + > + /* The scheduler only handles kernel queues so this is a no-op > + without them. */ > + if (!adev->gfx.disable_kq) { > + return; > + } [Severity: High] Is this early return condition inverted? The original code executed the fault handling logic inside an if (!adev->gfx.disable_kq) block, but here it returns and bypasses the logic when disable_kq is false (meaning kernel queues are enabled). Could this cause the scheduler to miss privileged instruction faults during normal operation, potentially leading to unrecovered GPU hangs since it fails to trigger a timeout or recovery reset? > + > + for (i = 0; i < adev->gfx.num_gfx_rings; i++) { > + ring = &adev->gfx.gfx_ring[i]; [ ... ] -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
