From: "Christian König" <christian.koenig@amd.com>
To: renpanpan <renpanpan@kylinos.cn>, linux-kernel@vger.kernel.org
Cc: alexander.deucher@amd.com, airlied@gmail.com
Subject: Re: [PATCH 2/2] drm/amdgpu: Use guard() for spin locks
Date: Wed, 11 Mar 2026 10:18:51 +0100 [thread overview]
Message-ID: <440dad58-1397-44d7-a69f-9e6908cf5806@amd.com> (raw)
In-Reply-To: <20260311091317.3519855-1-renpanpan@kylinos.cn>
On 3/11/26 10:13, renpanpan wrote:
>
> Clean up the code using guard() for spin locks.
>
> Merely code refactoring, and no behavior change.
Well you clearly have unintended behavior changes in this patch. So absolutely clear NAK to that!
>
> Signed-off-by: renpanpan <renpanpan@kylinos.cn>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 71 ++++++++--------------
> drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 8 +--
> 2 files changed, 28 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 3e19b51a2763..f9ba1fb36762 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -708,7 +708,6 @@ bool amdgpu_device_supports_smart_shift(struct amdgpu_device *adev)
> void amdgpu_device_mm_access(struct amdgpu_device *adev, loff_t pos,
> void *buf, size_t size, bool write)
> {
> - unsigned long flags;
> uint32_t hi = ~0, tmp = 0;
> uint32_t *data = buf;
> uint64_t last;
> @@ -719,7 +718,7 @@ void amdgpu_device_mm_access(struct amdgpu_device *adev, loff_t pos,
>
> BUG_ON(!IS_ALIGNED(pos, 4) || !IS_ALIGNED(size, 4));
>
> - spin_lock_irqsave(&adev->mmio_idx_lock, flags);
> + guard(spinlock_irqsave)(&adev->mmio_idx_lock);
> for (last = pos + size; pos < last; pos += 4) {
> tmp = pos >> 31;
>
> @@ -734,7 +733,6 @@ void amdgpu_device_mm_access(struct amdgpu_device *adev, loff_t pos,
> *data++ = RREG32_NO_KIQ(mmMM_DATA);
> }
>
> - spin_unlock_irqrestore(&adev->mmio_idx_lock, flags);
> drm_dev_exit(idx);
This suddenly holds the spinlock while calling drm_dev_exit() because the goard is still in scope.
Same thing on a bunch of other places in the rest of the patch.
Regards,
Christian
> }
>
> @@ -1087,7 +1085,7 @@ void amdgpu_device_xcc_wreg(struct amdgpu_device *adev,
> u32 amdgpu_device_indirect_rreg(struct amdgpu_device *adev,
> u32 reg_addr)
> {
> - unsigned long flags, pcie_index, pcie_data;
> + unsigned long pcie_index, pcie_data;
> void __iomem *pcie_index_offset;
> void __iomem *pcie_data_offset;
> u32 r;
> @@ -1095,14 +1093,13 @@ u32 amdgpu_device_indirect_rreg(struct amdgpu_device *adev,
> pcie_index = adev->nbio.funcs->get_pcie_index_offset(adev);
> pcie_data = adev->nbio.funcs->get_pcie_data_offset(adev);
>
> - spin_lock_irqsave(&adev->pcie_idx_lock, flags);
> + guard(spinlock_irqsave)(&adev->pcie_idx_lock);
> pcie_index_offset = (void __iomem *)adev->rmmio + pcie_index * 4;
> pcie_data_offset = (void __iomem *)adev->rmmio + pcie_data * 4;
>
> writel(reg_addr, pcie_index_offset);
> readl(pcie_index_offset);
> r = readl(pcie_data_offset);
> - spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
>
> return r;
> }
> @@ -1110,7 +1107,7 @@ u32 amdgpu_device_indirect_rreg(struct amdgpu_device *adev,
> u32 amdgpu_device_indirect_rreg_ext(struct amdgpu_device *adev,
> u64 reg_addr)
> {
> - unsigned long flags, pcie_index, pcie_index_hi, pcie_data;
> + unsigned long pcie_index, pcie_index_hi, pcie_data;
> u32 r;
> void __iomem *pcie_index_offset;
> void __iomem *pcie_index_hi_offset;
> @@ -1133,7 +1130,7 @@ u32 amdgpu_device_indirect_rreg_ext(struct amdgpu_device *adev,
> pcie_index_hi = 0;
> }
>
> - spin_lock_irqsave(&adev->pcie_idx_lock, flags);
> + guard(spinlock_irqsave)(&adev->pcie_idx_lock);
> pcie_index_offset = (void __iomem *)adev->rmmio + pcie_index * 4;
> pcie_data_offset = (void __iomem *)adev->rmmio + pcie_data * 4;
> if (pcie_index_hi != 0)
> @@ -1154,8 +1151,6 @@ u32 amdgpu_device_indirect_rreg_ext(struct amdgpu_device *adev,
> readl(pcie_index_hi_offset);
> }
>
> - spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
> -
> return r;
> }
>
> @@ -1170,7 +1165,7 @@ u32 amdgpu_device_indirect_rreg_ext(struct amdgpu_device *adev,
> u64 amdgpu_device_indirect_rreg64(struct amdgpu_device *adev,
> u32 reg_addr)
> {
> - unsigned long flags, pcie_index, pcie_data;
> + unsigned long pcie_index, pcie_data;
> void __iomem *pcie_index_offset;
> void __iomem *pcie_data_offset;
> u64 r;
> @@ -1178,7 +1173,7 @@ u64 amdgpu_device_indirect_rreg64(struct amdgpu_device *adev,
> pcie_index = adev->nbio.funcs->get_pcie_index_offset(adev);
> pcie_data = adev->nbio.funcs->get_pcie_data_offset(adev);
>
> - spin_lock_irqsave(&adev->pcie_idx_lock, flags);
> + guard(spinlock_irqsave)(&adev->pcie_idx_lock);
> pcie_index_offset = (void __iomem *)adev->rmmio + pcie_index * 4;
> pcie_data_offset = (void __iomem *)adev->rmmio + pcie_data * 4;
>
> @@ -1190,7 +1185,6 @@ u64 amdgpu_device_indirect_rreg64(struct amdgpu_device *adev,
> writel(reg_addr + 4, pcie_index_offset);
> readl(pcie_index_offset);
> r |= ((u64)readl(pcie_data_offset) << 32);
> - spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
>
> return r;
> }
> @@ -1198,7 +1192,7 @@ u64 amdgpu_device_indirect_rreg64(struct amdgpu_device *adev,
> u64 amdgpu_device_indirect_rreg64_ext(struct amdgpu_device *adev,
> u64 reg_addr)
> {
> - unsigned long flags, pcie_index, pcie_data;
> + unsigned long pcie_index, pcie_data;
> unsigned long pcie_index_hi = 0;
> void __iomem *pcie_index_offset;
> void __iomem *pcie_index_hi_offset;
> @@ -1210,7 +1204,7 @@ u64 amdgpu_device_indirect_rreg64_ext(struct amdgpu_device *adev,
> if ((reg_addr >> 32) && (adev->nbio.funcs->get_pcie_index_hi_offset))
> pcie_index_hi = adev->nbio.funcs->get_pcie_index_hi_offset(adev);
>
> - spin_lock_irqsave(&adev->pcie_idx_lock, flags);
> + guard(spinlock_irqsave)(&adev->pcie_idx_lock);
> pcie_index_offset = (void __iomem *)adev->rmmio + pcie_index * 4;
> pcie_data_offset = (void __iomem *)adev->rmmio + pcie_data * 4;
> if (pcie_index_hi != 0)
> @@ -1240,8 +1234,6 @@ u64 amdgpu_device_indirect_rreg64_ext(struct amdgpu_device *adev,
> readl(pcie_index_hi_offset);
> }
>
> - spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
> -
> return r;
> }
>
> @@ -1256,14 +1248,14 @@ u64 amdgpu_device_indirect_rreg64_ext(struct amdgpu_device *adev,
> void amdgpu_device_indirect_wreg(struct amdgpu_device *adev,
> u32 reg_addr, u32 reg_data)
> {
> - unsigned long flags, pcie_index, pcie_data;
> + unsigned long pcie_index, pcie_data;
> void __iomem *pcie_index_offset;
> void __iomem *pcie_data_offset;
>
> pcie_index = adev->nbio.funcs->get_pcie_index_offset(adev);
> pcie_data = adev->nbio.funcs->get_pcie_data_offset(adev);
>
> - spin_lock_irqsave(&adev->pcie_idx_lock, flags);
> + guard(spinlock_irqsave)(&adev->pcie_idx_lock);
> pcie_index_offset = (void __iomem *)adev->rmmio + pcie_index * 4;
> pcie_data_offset = (void __iomem *)adev->rmmio + pcie_data * 4;
>
> @@ -1271,13 +1263,12 @@ void amdgpu_device_indirect_wreg(struct amdgpu_device *adev,
> readl(pcie_index_offset);
> writel(reg_data, pcie_data_offset);
> readl(pcie_data_offset);
> - spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
> }
>
> void amdgpu_device_indirect_wreg_ext(struct amdgpu_device *adev,
> u64 reg_addr, u32 reg_data)
> {
> - unsigned long flags, pcie_index, pcie_index_hi, pcie_data;
> + unsigned long pcie_index, pcie_index_hi, pcie_data;
> void __iomem *pcie_index_offset;
> void __iomem *pcie_index_hi_offset;
> void __iomem *pcie_data_offset;
> @@ -1289,7 +1280,7 @@ void amdgpu_device_indirect_wreg_ext(struct amdgpu_device *adev,
> else
> pcie_index_hi = 0;
>
> - spin_lock_irqsave(&adev->pcie_idx_lock, flags);
> + guard(spinlock_irqsave)(&adev->pcie_idx_lock);
> pcie_index_offset = (void __iomem *)adev->rmmio + pcie_index * 4;
> pcie_data_offset = (void __iomem *)adev->rmmio + pcie_data * 4;
> if (pcie_index_hi != 0)
> @@ -1311,7 +1302,6 @@ void amdgpu_device_indirect_wreg_ext(struct amdgpu_device *adev,
> readl(pcie_index_hi_offset);
> }
>
> - spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
> }
>
> /**
> @@ -1325,14 +1315,14 @@ void amdgpu_device_indirect_wreg_ext(struct amdgpu_device *adev,
> void amdgpu_device_indirect_wreg64(struct amdgpu_device *adev,
> u32 reg_addr, u64 reg_data)
> {
> - unsigned long flags, pcie_index, pcie_data;
> + unsigned long pcie_index, pcie_data;
> void __iomem *pcie_index_offset;
> void __iomem *pcie_data_offset;
>
> pcie_index = adev->nbio.funcs->get_pcie_index_offset(adev);
> pcie_data = adev->nbio.funcs->get_pcie_data_offset(adev);
>
> - spin_lock_irqsave(&adev->pcie_idx_lock, flags);
> + guard(spinlock_irqsave)(&adev->pcie_idx_lock);
> pcie_index_offset = (void __iomem *)adev->rmmio + pcie_index * 4;
> pcie_data_offset = (void __iomem *)adev->rmmio + pcie_data * 4;
>
> @@ -1346,13 +1336,12 @@ void amdgpu_device_indirect_wreg64(struct amdgpu_device *adev,
> readl(pcie_index_offset);
> writel((u32)(reg_data >> 32), pcie_data_offset);
> readl(pcie_data_offset);
> - spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
> }
>
> void amdgpu_device_indirect_wreg64_ext(struct amdgpu_device *adev,
> u64 reg_addr, u64 reg_data)
> {
> - unsigned long flags, pcie_index, pcie_data;
> + unsigned long pcie_index, pcie_data;
> unsigned long pcie_index_hi = 0;
> void __iomem *pcie_index_offset;
> void __iomem *pcie_index_hi_offset;
> @@ -1363,7 +1352,7 @@ void amdgpu_device_indirect_wreg64_ext(struct amdgpu_device *adev,
> if ((reg_addr >> 32) && (adev->nbio.funcs->get_pcie_index_hi_offset))
> pcie_index_hi = adev->nbio.funcs->get_pcie_index_hi_offset(adev);
>
> - spin_lock_irqsave(&adev->pcie_idx_lock, flags);
> + guard(spinlock_irqsave)(&adev->pcie_idx_lock);
> pcie_index_offset = (void __iomem *)adev->rmmio + pcie_index * 4;
> pcie_data_offset = (void __iomem *)adev->rmmio + pcie_data * 4;
> if (pcie_index_hi != 0)
> @@ -1395,7 +1384,6 @@ void amdgpu_device_indirect_wreg64_ext(struct amdgpu_device *adev,
> readl(pcie_index_hi_offset);
> }
>
> - spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
> }
>
> /**
> @@ -1764,17 +1752,15 @@ static int amdgpu_device_wb_init(struct amdgpu_device *adev)
> */
> int amdgpu_device_wb_get(struct amdgpu_device *adev, u32 *wb)
> {
> - unsigned long flags, offset;
> + unsigned long offset;
>
> - spin_lock_irqsave(&adev->wb.lock, flags);
> + guard(spinlock_irqsave)(&adev->wb.lock);
> offset = find_first_zero_bit(adev->wb.used, adev->wb.num_wb);
> if (offset < adev->wb.num_wb) {
> __set_bit(offset, adev->wb.used);
> - spin_unlock_irqrestore(&adev->wb.lock, flags);
> *wb = offset << 3; /* convert to dw offset */
> return 0;
> } else {
> - spin_unlock_irqrestore(&adev->wb.lock, flags);
> return -EINVAL;
> }
> }
> @@ -1789,13 +1775,11 @@ int amdgpu_device_wb_get(struct amdgpu_device *adev, u32 *wb)
> */
> void amdgpu_device_wb_free(struct amdgpu_device *adev, u32 wb)
> {
> - unsigned long flags;
>
> wb >>= 3;
> - spin_lock_irqsave(&adev->wb.lock, flags);
> + guard(spinlock_irqsave)(&adev->wb.lock);
> if (wb < adev->wb.num_wb)
> __clear_bit(wb, adev->wb.used);
> - spin_unlock_irqrestore(&adev->wb.lock, flags);
> }
>
> /**
> @@ -7379,34 +7363,31 @@ void amdgpu_device_halt(struct amdgpu_device *adev)
> u32 amdgpu_device_pcie_port_rreg(struct amdgpu_device *adev,
> u32 reg)
> {
> - unsigned long flags, address, data;
> - u32 r;
> + unsigned long address, data;
>
> address = adev->nbio.funcs->get_pcie_port_index_offset(adev);
> data = adev->nbio.funcs->get_pcie_port_data_offset(adev);
>
> - spin_lock_irqsave(&adev->pcie_idx_lock, flags);
> + guard(spinlock_irqsave)(&adev->pcie_idx_lock);
> WREG32(address, reg * 4);
> (void)RREG32(address);
> - r = RREG32(data);
> - spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
> - return r;
> +
> + return RREG32(data);
> }
>
> void amdgpu_device_pcie_port_wreg(struct amdgpu_device *adev,
> u32 reg, u32 v)
> {
> - unsigned long flags, address, data;
> + unsigned long address, data;
>
> address = adev->nbio.funcs->get_pcie_port_index_offset(adev);
> data = adev->nbio.funcs->get_pcie_port_data_offset(adev);
>
> - spin_lock_irqsave(&adev->pcie_idx_lock, flags);
> + guard(spinlock_irqsave)(&adev->pcie_idx_lock);
> WREG32(address, reg * 4);
> (void)RREG32(address);
> WREG32(data, v);
> (void)RREG32(data);
> - spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
> }
>
> /**
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> index 1054d66c54fa..8eb2c917d622 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> @@ -664,9 +664,8 @@ void amdgpu_fence_driver_hw_init(struct amdgpu_device *adev)
> void amdgpu_fence_driver_set_error(struct amdgpu_ring *ring, int error)
> {
> struct amdgpu_fence_driver *drv = &ring->fence_drv;
> - unsigned long flags;
>
> - spin_lock_irqsave(&drv->lock, flags);
> + guard(spinlock_irqsave)(&drv->lock);
> for (unsigned int i = 0; i <= drv->num_fences_mask; ++i) {
> struct dma_fence *fence;
>
> @@ -675,7 +674,6 @@ void amdgpu_fence_driver_set_error(struct amdgpu_ring *ring, int error)
> if (fence && !dma_fence_is_signaled_locked(fence))
> dma_fence_set_error(fence, error);
> }
> - spin_unlock_irqrestore(&drv->lock, flags);
> }
>
> /**
> @@ -719,7 +717,6 @@ void amdgpu_fence_driver_update_timedout_fence_state(struct amdgpu_fence *af)
> struct dma_fence __rcu **ptr;
> struct amdgpu_fence *fence;
> struct amdgpu_ring *ring = af->ring;
> - unsigned long flags;
> u32 seq, last_seq;
> bool reemitted = false;
>
> @@ -727,7 +724,7 @@ void amdgpu_fence_driver_update_timedout_fence_state(struct amdgpu_fence *af)
> seq = ring->fence_drv.sync_seq & ring->fence_drv.num_fences_mask;
>
> /* mark all fences from the guilty context with an error */
> - spin_lock_irqsave(&ring->fence_drv.lock, flags);
> + guard(spinlock_irqsave)(&ring->fence_drv.lock);
> do {
> last_seq++;
> last_seq &= ring->fence_drv.num_fences_mask;
> @@ -748,7 +745,6 @@ void amdgpu_fence_driver_update_timedout_fence_state(struct amdgpu_fence *af)
> }
> rcu_read_unlock();
> } while (last_seq != seq);
> - spin_unlock_irqrestore(&ring->fence_drv.lock, flags);
>
> if (reemitted) {
> /* if we've already reemitted once then just cancel everything */
> --
> 2.25.1
>
prev parent reply other threads:[~2026-03-11 9:19 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-03-11 9:13 renpanpan
2026-03-11 9:18 ` Christian König [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=440dad58-1397-44d7-a69f-9e6908cf5806@amd.com \
--to=christian.koenig@amd.com \
--cc=airlied@gmail.com \
--cc=alexander.deucher@amd.com \
--cc=linux-kernel@vger.kernel.org \
--cc=renpanpan@kylinos.cn \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®