staging/android: add TODO to de-stage android sync framework

Commit Message

Gustavo Padovan Nov. 23, 2015, 7:41 p.m. UTC

From: Gustavo Padovan <[e-mail]>

 - remove sw_sync, it is used only for testing/debugging and should not
be upstreamed.
 - port sw_sync testcases to use debugfs somehow
 - clean up and ABI check for security issues
 - move the sync framework to drivers/base/dma-buf

Cc: Arve Hjønnevåg <[e-mail]>
Cc: Riley Andrews <[e-mail]>
Cc: Daniel Vetter <[e-mail]>
Cc: Rob Clark <[e-mail]>
Cc: Greg Hackmann <[e-mail]>
Cc: John Harrison <[e-mail]>
Signed-off-by: Gustavo Padovan <[e-mail]>
---
 drivers/staging/android/TODO | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Daniel Vetter Nov. 24, 2015, 8:51 a.m. UTC | #1

On Mon, Nov 23, 2015 at 05:41:53PM -0200, Gustavo Padovan wrote:
> From: Gustavo Padovan <[e-mail]>
> 
>  - remove sw_sync, it is used only for testing/debugging and should not
> be upstreamed.
>  - port sw_sync testcases to use debugfs somehow
>  - clean up and ABI check for security issues
>  - move the sync framework to drivers/base/dma-buf
> 
> Cc: Arve Hjønnevåg <[e-mail]>
> Cc: Riley Andrews <[e-mail]>
> Cc: Daniel Vetter <[e-mail]>
> Cc: Rob Clark <[e-mail]>
> Cc: Greg Hackmann <[e-mail]>
> Cc: John Harrison <[e-mail]>
> Signed-off-by: Gustavo Padovan <[e-mail]>

This reflects my recollection of various discussions at conferences and on
irc. Acked-by: Daniel Vetter <[e-mail]>

> ---
>  drivers/staging/android/TODO | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO
> index 8f3ac37..2375dae 100644
> --- a/drivers/staging/android/TODO
> +++ b/drivers/staging/android/TODO
> @@ -25,5 +25,12 @@ ion/
>     exposes existing cma regions and doesn't reserve unecessarily memory when
>     booting a system which doesn't use ion.
>  
> +sync framework:
> + - remove sw_sync, it is used only for testing/debugging and should not be
> +upstreamed.
> + - port sw_sync testcases to use debugfs somehow
> + - clean up and ABI check for security issues
> + - move it to drivers/base/dma-buf
> +
>  Please send patches to Greg Kroah-Hartman <[e-mail]> and Cc:
>  Arve Hjønnevåg <[e-mail]> and Riley Andrews <[e-mail]>
> -- 
> 2.1.0
>

Daniel Vetter Nov. 24, 2015, 8:53 a.m. UTC | #2

On Tue, Nov 24, 2015 at 09:51:12AM +0100, Daniel Vetter wrote:
> On Mon, Nov 23, 2015 at 05:41:53PM -0200, Gustavo Padovan wrote:
> > From: Gustavo Padovan <[e-mail]>
> > 
> >  - remove sw_sync, it is used only for testing/debugging and should not
> > be upstreamed.
> >  - port sw_sync testcases to use debugfs somehow
> >  - clean up and ABI check for security issues
> >  - move the sync framework to drivers/base/dma-buf
> > 
> > Cc: Arve Hjønnevåg <[e-mail]>
> > Cc: Riley Andrews <[e-mail]>
> > Cc: Daniel Vetter <[e-mail]>
> > Cc: Rob Clark <[e-mail]>
> > Cc: Greg Hackmann <[e-mail]>
> > Cc: John Harrison <[e-mail]>
> > Signed-off-by: Gustavo Padovan <[e-mail]>
> 
> This reflects my recollection of various discussions at conferences and on
> irc. Acked-by: Daniel Vetter <[e-mail]>

Coffee just kicked in ;-)

> > ---
> >  drivers/staging/android/TODO | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO
> > index 8f3ac37..2375dae 100644
> > --- a/drivers/staging/android/TODO
> > +++ b/drivers/staging/android/TODO
> > @@ -25,5 +25,12 @@ ion/
> >     exposes existing cma regions and doesn't reserve unecessarily memory when
> >     booting a system which doesn't use ion.
> >  
> > +sync framework:
> > + - remove sw_sync, it is used only for testing/debugging and should not be
> > +upstreamed.
> > + - port sw_sync testcases to use debugfs somehow

With all the effort going on around kselftest it'd be good to integrate
the existing testsuite google has into upstream too. Should probably be
listed here too.
-Daniel

> > + - clean up and ABI check for security issues
> > + - move it to drivers/base/dma-buf
> > +
> >  Please send patches to Greg Kroah-Hartman <[e-mail]> and Cc:
> >  Arve Hjønnevåg <[e-mail]> and Riley Andrews <[e-mail]>
> > -- 
> > 2.1.0
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Greg Hackmann Nov. 24, 2015, 4:51 p.m. UTC | #3

On 11/23/15 11:41 AM, Gustavo Padovan wrote:
> + - remove sw_sync, it is used only for testing/debugging and should not be
> +upstreamed.
> + - port sw_sync testcases to use debugfs somehow

A quick but important nitpick:

sw_sync itself is just an in-kernel helper for creating fences, when you 
don't have something like sync timeline primitives baked into your hardware.

CONFIG_SW_SYNC_USER adds the interface for creating and signaling 
sw_sync objects from userspace.  This is the part that's dangerous and 
only intended for testing, etc.

AFAIK CONFIG_SW_SYNC_USER is the only part people have been objecting 
to.  I'm fine with removing it.  Removing the kernel-facing side of 
sw_sync would be a problem for us, since many drivers use it to create 
their fences.

Greg Hackmann Nov. 24, 2015, 5:28 p.m. UTC | #4

On 11/24/2015 12:53 AM, Daniel Vetter wrote:
> With all the effort going on around kselftest it'd be good to integrate
> the existing testsuite google has into upstream too. Should probably be
> listed here too.
> -Daniel

The test code's available in AOSP:

https://android.googlesource.com/platform/system/core/+/master/libsync/tests/

Be warned that it sits on top of a small helper library, uses C++ 
heavily, and depends on googletest.  So it's going to need reworking 
before it's suitable for the kernel tree.  But you can at least see the 
kinds of things it's testing (and where the SW_SYNC_USER parts fit into 
the picture).

Gustavo Padovan Nov. 24, 2015, 5:32 p.m. UTC | #5

2015-11-24 Greg Hackmann <[e-mail]>:

> On 11/23/15 11:41 AM, Gustavo Padovan wrote:
> >+ - remove sw_sync, it is used only for testing/debugging and should not be
> >+upstreamed.
> >+ - port sw_sync testcases to use debugfs somehow
> 
> A quick but important nitpick:
> 
> sw_sync itself is just an in-kernel helper for creating fences, when you
> don't have something like sync timeline primitives baked into your hardware.
> 
> CONFIG_SW_SYNC_USER adds the interface for creating and signaling sw_sync
> objects from userspace.  This is the part that's dangerous and only intended
> for testing, etc.
> 
> AFAIK CONFIG_SW_SYNC_USER is the only part people have been objecting to.
> I'm fine with removing it.  Removing the kernel-facing side of sw_sync would
> be a problem for us, since many drivers use it to create their fences.

Right, I probably misundertood things, I'm okay with removing only
CONFIG_SW_SYNC_USER and if others are okay too I'll just send an updated
patch for the TODO.

	Gustavo

Patch

diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO
index 8f3ac37..2375dae 100644
--- a/drivers/staging/android/TODO
+++ b/drivers/staging/android/TODO
@@ -25,5 +25,12 @@  ion/
    exposes existing cma regions and doesn't reserve unecessarily memory when
    booting a system which doesn't use ion.
 
+sync framework:
+ - remove sw_sync, it is used only for testing/debugging and should not be
+upstreamed.
+ - port sw_sync testcases to use debugfs somehow
+ - clean up and ABI check for security issues
+ - move it to drivers/base/dma-buf
+
 Please send patches to Greg Kroah-Hartman <[e-mail]> and Cc:
 Arve Hjønnevåg <[e-mail]> and Riley Andrews <[e-mail]>