Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Missing read memory barrier (rmb) in interrupt timestamp capture --
commit 68fac45f9a2ee3c91fdd1807b14f88f10a5d382e Author: Honglei Huang <[email protected]> drm/amdgpu: add SVM notifier invalidate callback and checkpoint This commit introduces the invalidate_ranges callback and checkpoint timestamp capture for AMDGPU SVM. It adds amdgpu_svm_capture_checkpoint_ts to capture the interrupt handler write pointer timestamp, and amdgpu_svm_range_invalidate to dispatch the callbacks with TLB flush batching. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_svm_range.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_svm_range.c > index 57bcbaf0fdd2b..6ac785a3b5cba 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_svm_range.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_svm_range.c [ ... ] > +void amdgpu_svm_capture_checkpoint_ts(struct amdgpu_svm *svm) > +{ > + struct amdgpu_device *adev = svm->adev; > + struct amdgpu_ih_ring *ih; > + uint32_t checkpoint_wptr; > + > + if (!adev->irq.retry_cam_enabled && adev->irq.ih1.ring_size) { > + ih = &adev->irq.ih1; > + checkpoint_wptr = amdgpu_ih_get_wptr(adev, ih); [Severity: High] Does this code need an rmb() here before reading the timestamp from the ring buffer? Since the CPU reads the write pointer (which is updated by the GPU) and then immediately decodes the timestamp from the ring buffer payload in memory via amdgpu_ih_decode_iv_ts(), couldn't the CPU or compiler reorder these reads? If reordered, the CPU might speculatively fetch the stale ring buffer payload before fetching the new write pointer, capturing a stale or garbage timestamp. > + if (ih->rptr != checkpoint_wptr) { > + WRITE_ONCE(svm->checkpoint_ts, > + amdgpu_ih_decode_iv_ts(adev, ih, > + checkpoint_wptr, -1)); > + return; > + } > + } > + > + ih = &adev->irq.ih_soft; > + checkpoint_wptr = amdgpu_ih_get_wptr(adev, ih); [Severity: High] Is an rmb() also missing here for the ih_soft ring? Similarly, without a read memory barrier, the CPU might read stale data from the ring buffer payload before observing the write pointer update. > + if (ih->rptr != checkpoint_wptr) > + WRITE_ONCE(svm->checkpoint_ts, > + amdgpu_ih_decode_iv_ts(adev, ih, > + checkpoint_wptr, -1)); > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=9
