[Intel-gfx] [PATCH] dma_fence_array: Fix PENDING_ERROR leak in dma_fence_array_signaled()

[Joonas Lahtinen]

(Switching to my @linux.intel.com address)

Quoting Christian König (2021-11-29 14:55:37)
> Am 29.11.21 um 13:46 schrieb Thomas Hellström:
> > On Mon, 2021-11-29 at 13:33 +0100, Christian König wrote:
> >> Am 29.11.21 um 13:23 schrieb Thomas Hellström:
> >>> Hi, Christian,
> >>>
> >>> On Mon, 2021-11-29 at 09:21 +0100, Christian König wrote:
> >>>> Am 29.11.21 um 08:35 schrieb Thomas Hellström:
> >>>>> If a dma_fence_array is reported signaled by a call to
> >>>>> dma_fence_is_signaled(), it may leak the PENDING_ERROR status.
> >>>>>
> >>>>> Fix this by clearing the PENDING_ERROR status if we return true
> >>>>> in
> >>>>> dma_fence_array_signaled().
> >>>>>
> >>>>> Fixes: 1f70b8b812f3 ("dma-fence: Propagate errors to dma-fence-
> >>>>> array container")
> >>>>> Cc: linaro-mm-sig at lists.linaro.org
> >>>>> Cc: Christian König <ckoenig.leichtzumerken at gmail.com>
> >>>>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> >>>>> Signed-off-by: Thomas Hellström
> >>>>> <thomas.hellstrom at linux.intel.com>
> >>>> Reviewed-by: Christian König <christian.koenig at amd.com>
> >>> How are the dma-buf / dma-fence patches typically merged? If i915
> >>> is
> >>> the only fence->error user, could we take this through drm-intel to
> >>> avoid a backmerge for upcoming i915 work?
> >> Well that one here looks like a bugfix to me, so either through
> >> drm-misc-fixes ore some i915 -fixes branch sounds fine to me.
> >>
> >> If you have any new development based on that a backmerge of the -
> >> fixes
> >> into your -next branch is unavoidable anyway.
> > Ok, I'll check with Joonas if I can take it through
> > drm-intel-gt-next, since fixes are cherry-picked from that one. Patch
> > will then appear in both the -fixes and the -next branch.
> 
> Well exactly that's the stuff Daniel told me to avoid :)
> 
> But maybe your i915 workflow is somehow better handling that than the 
> AMD workflow.

If it's a bugfix to a patch that merged through drm-misc-next, I'd
always be inclined to merge the fixup using the same process (which
would be drm-next-fixes).

In i915 we do always merge the patches to -next first, and never do a
backmerge of -fixes (as it's a cherry-picked branch) so the workflows
differ there.

Here the time between the fixup and the previous patch is so long that
either way is fine with. So feel free to apply to drm-intel-gt-next.

Regards, Joonas

> Christian.
> 
> >
> > Thanks,
> > /Thomas
> >
> >
> >> Regards,
> >> Christian.
> >>
> >>> /Thomas
> >>>
> >>>
> >>>>> ---
> >>>>>     drivers/dma-buf/dma-fence-array.c | 6 +++++-
> >>>>>     1 file changed, 5 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-
> >>>>> buf/dma-fence-array.c
> >>>>> index d3fbd950be94..3e07f961e2f3 100644
> >>>>> --- a/drivers/dma-buf/dma-fence-array.c
> >>>>> +++ b/drivers/dma-buf/dma-fence-array.c
> >>>>> @@ -104,7 +104,11 @@ static bool
> >>>>> dma_fence_array_signaled(struct
> >>>>> dma_fence *fence)
> >>>>>     {
> >>>>>           struct dma_fence_array *array =
> >>>>> to_dma_fence_array(fence);
> >>>>>     
> >>>>> -       return atomic_read(&array->num_pending) <= 0;
> >>>>> +       if (atomic_read(&array->num_pending) > 0)
> >>>>> +               return false;
> >>>>> +
> >>>>> +       dma_fence_array_clear_pending_error(array);
> >>>>> +       return true;
> >>>>>     }
> >>>>>     
> >>>>>     static void dma_fence_array_release(struct dma_fence *fence)
> >
>