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

[Daniel Vetter]

On Mon, Nov 29, 2021 at 03:14:35PM +0200, Joonas Lahtinen wrote:
> (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.

To make this clear and avoid confusion: drm-misc and drm-intel work
differently for bugfixes.

drm-intel has paid maintainers who take care of cherry-picking and testing
and making sure nothing is lost.

drm-misc is all volunteers, so committers need to make sure stuff ends up
in the right place.

Hence different rules.
-Daniel

> 
> 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)
> > >
> > 
> _______________________________________________
> Linaro-mm-sig mailing list
> Linaro-mm-sig at lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/linaro-mm-sig

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch