linux-kernel - Re: [RFC v2 2/3] dma-buf/fence-array: add fence_array

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <577249CB.6050707@amd.com>
Date:	Tue, 28 Jun 2016 11:56:27 +0200
From:	Christian König <christian.koenig@....com>
To:	Gustavo Padovan <gustavo@...ovan.org>,
	<dri-devel@...ts.freedesktop.org>
CC:	<linux-kernel@...r.kernel.org>,
	Daniel Stone <daniels@...labora.com>,
	Daniel Vetter <daniel.vetter@...ll.ch>,
	Rob Clark <robdclark@...il.com>,
	Greg Hackmann <ghackmann@...gle.com>,
	John Harrison <John.C.Harrison@...el.com>,
	<laurent.pinchart@...asonboard.com>, <seanpaul@...gle.com>,
	<marcheu@...gle.com>, <m.chehab@...sung.com>,
	Sumit Semwal <sumit.semwal@...aro.org>,
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
	Gustavo Padovan <gustavo.padovan@...labora.co.uk>,
	Chris Wilson <chris@...is-wilson.co.uk>
Subject: Re: [RFC v2 2/3] dma-buf/fence-array: add fence_array_teardown()

Am 27.06.2016 um 21:29 schrieb Gustavo Padovan:
> From: Gustavo Padovan <gustavo.padovan@...labora.co.uk>
>
> As the array of fence callbacks held by an active struct fence_array
> each has a reference to the struct fence_array, when the owner of the
> fence_array is freed it must dispose of the callback references before
> it can free the fence_array. This can not happen simply during
> fence_release() because of the extra references and so we need a new
> function to run before the final fence_put().

As I said previously as well, this is completely superfluous.

The fence array keeps a reference to itself as long as not all callbacks 
are signaled.

So you only need to unregister your callback from the array itself and 
drop your reference when you don't need it any more in the sync file.

Additional to that having a special cleanup function like this strongly 
contradicts the approach of having an implementation independent interface.

So this is a clear NAK from my side.

Regards,
Christian.

>
> Cc: Chris Wilson <chris@...is-wilson.co.uk>
> Cc: Christian König <christian.koenig@....com>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@...labora.co.uk>
> ---
>   drivers/dma-buf/fence-array.c | 25 +++++++++++++++++++++++++
>   include/linux/fence-array.h   |  1 +
>   2 files changed, 26 insertions(+)
>
> diff --git a/drivers/dma-buf/fence-array.c b/drivers/dma-buf/fence-array.c
> index a8731c8..8891357 100644
> --- a/drivers/dma-buf/fence-array.c
> +++ b/drivers/dma-buf/fence-array.c
> @@ -142,3 +142,28 @@ struct fence_array *fence_array_create(int num_fences, struct fence **fences,
>   	return array;
>   }
>   EXPORT_SYMBOL(fence_array_create);
> +
> +/**
> + * fence_array_teardown - Teardown and clean up a fence array
> + * @array:	[in]	the fence array to teardown
> + *
> + * This function removes callbacks and extra references to the base fence on
> + * the fence_array for each unsignalled fence in the array. It should be called
> + * before calling fence_put() to remove the last reference on a fence_array,
> + * otherwise the fence won't be released due to extra references holded by the
> + * the fences that still have signalling enabled.
> + */
> +void fence_array_teardown(struct fence_array *array)
> +{
> +	struct fence_array_cb *cb = (void *)(&array[1]);
> +	int i;
> +
> +	for (i = 0; i < array->num_fences; i++) {
> +		if (fence_is_signaled(array->fences[i]))
> +		    continue;
> +
> +		fence_remove_callback(array->fences[i], &cb[i].cb);
> +		fence_put(&array->base);
> +	}
> +}
> +EXPORT_SYMBOL(fence_array_teardown);
> diff --git a/include/linux/fence-array.h b/include/linux/fence-array.h
> index d2e9f40..9f1b923 100644
> --- a/include/linux/fence-array.h
> +++ b/include/linux/fence-array.h
> @@ -79,5 +79,6 @@ static inline struct fence_array *to_fence_array(struct fence *fence)
>   struct fence_array *fence_array_create(int num_fences, struct fence **fences,
>   				       u64 context, unsigned seqno,
>   				       bool signal_on_any);
> +void fence_array_teardown(struct fence_array *array);
>   
>   #endif /* __LINUX_FENCE_ARRAY_H */