Re: [PATCH v8 1/3] drm: Add drm_work_fence helper

[SRINIVASAN SHANMUGAM <srinivasan.shanmugam@amd.com>]

On 9/9/2026 8:30 AM, SRINIVASAN SHANMUGAM wrote:
>
> On 9/9/2026 1:46 AM, Matthew Brost wrote:
>> On Tue, Sep 08, 2026 at 08:58:39AM +0000, sashiko-bot@kernel.org wrote:
>>
>> All of this looks right. So I believe the solution is:
>>
>> - s/cancel_work_/disable_work_
>> - In drm_work_fence_queue if queue_work returns false, call 
>> drm_work_fence_put()
>>
>> Matt
>
> Agreed — the race is real. Will fix in Patch 1
>
>    as you suggested:
>    - In drm_work_fence_queue: drop ref if queue_work returns false
>
>    - In drm_work_fence_cancel_sync: switch to disable_work_sync()

Hi Matt,

Sashiko flagged two more issues on v9 Patch 1 [1][2]:

In v9 we applied suggested fix:
- drm_work_fence_queue: drop ref if queue_work() returns false
- drm_work_fence_cancel_sync: switched to disable_work_sync()

However, Sashiko found two remaining problems:

1. ops->destroy() called in IRQ context:

    When the GPU fence signals, the callback runs. If queue_work()
    returns false (work already done or disabled), we call
    drm_work_fence_put() immediately — but we are inside an interrupt
    with IRQs disabled. If that drops the last ref, ops->destroy()
    runs in that interrupt context, which is not safe for drivers that
    sleep in destroy.

    Fix: instead of calling drm_work_fence_put() directly, post a
    small cleanup task via schedule_work() on system_wq. This ensures
    destroy() always runs in process context where sleeping is allowed.

2. disable_work_sync() still has a timing gap:

    The callback does two things in sequence:
      a. Removes itself from the fence list
      b. Calls queue_work()

    If driver teardown starts between (a) and (b), disable_work_sync()
    sees no pending work and returns immediately — teardown proceeds.
    The callback then calls queue_work() on an already-destroyed
    workqueue, causing a use-after-free.

    Fix: add an atomic flag. The callback sets the flag ON before (b)
    and OFF after. cancel_sync() waits until the flag is OFF before
    returning. This closes the gap between (a) and (b).

Are these approaches acceptable, or may I kno pls if do you have a 
preferred solution?

Also for Patch 3 — xe_sync_ufence_cancel() is now unused after v9
removed its only caller. Will remove it in v10.

[1] v9 Patch 1:
https://patchwork.freedesktop.org/patch/751979/?series=173582&rev=2

[2] Sashiko v9 review:
https://sashiko.dev/#/patchset/20260909044454.399340-1-srinivasan.shanmugam%40amd.com

Thanks,
Srini