linux-kernel - Re: [PATCH v2 01/11] dma-buf/sync_file: de-stage sync

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: <20160128092355.GS11240@phenom.ffwll.local>
Date:	Thu, 28 Jan 2016 10:23:55 +0100
From:	Daniel Vetter <daniel@...ll.ch>
To:	Greg Hackmann <ghackmann@...gle.com>
Cc:	Gustavo Padovan <gustavo@...ovan.org>,
	Emil Velikov <emil.l.velikov@...il.com>,
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Linux-Kernel@...r. Kernel. Org" <linux-kernel@...r.kernel.org>,
	devel@...verdev.osuosl.org,
	ML dri-devel <dri-devel@...ts.freedesktop.org>,
	Daniel Stone <daniels@...labora.com>,
	Arve Hjønnevåg <arve@...roid.com>,
	Riley Andrews <riandrews@...roid.com>,
	Daniel Vetter <daniel.vetter@...ll.ch>,
	Rob Clark <robdclark@...il.com>,
	John Harrison <John.C.Harrison@...el.com>,
	Gustavo Padovan <gustavo.padovan@...labora.co.uk>
Subject: Re: [PATCH v2 01/11] dma-buf/sync_file: de-stage sync_file

On Wed, Jan 27, 2016 at 01:41:03PM -0800, Greg Hackmann wrote:
> On 01/27/2016 12:25 PM, Gustavo Padovan wrote:
> >>>>Is there a value in keeping the abi unchanged?
> >>>>If not, then Documentation/ioctl/botching-up-ioctls.txt is worth a read.
> >>>
> >>>None from me. I'll look where we can improve the ABI.
> 
> Android has existing clients of the current ABI.  Thankfully they're all
> contained in system services like SurfaceFlinger, since end-user apps don't
> get direct access to fence fds.
> 
> As long the ABI breaks don't remove functionality we depend on, we can wrap
> around them in our userspace libsync.  I'd rather not have to do that, but
> it's a price I'm willing to pay to get this moved out of staging.
> 
> >>  - struct sync_file_info_data::fence_info is of type __u8 yet it is "a
> >>fence_info struct for every fence in the sync_file". Thus shouldn't
> >>one use "struct fence_info" as the type ?
> >
> >Agreed. But I'm currently thinking if we really should keep this ioctl.
> >
> >	Gustavo
> >
> 
> I'm not seeing any consumers of driver_data in our tree.  OTOH completely
> getting rid of the ioctl would be a problem, since SurfaceFlinger depends on
> the timestamp information for its own bookkeeping.

If we remove driver_data (and len is superflous too), then I think we
should also make the master struct use common ioctl pattern:
- Add a num_fences field or similar that the kernel fills out.
- Make pt_info an __u64 pointer instead of a variable-length array (and
  length) - ioctl payload sizes are somewhat limited.

This way the interface is future-proofed for truly patalogical number of
fences (which surface flinger won't do, but could happen in
server/opencl/media workloads I'd imagine).

And I think driver_data really shouldn't be there, it makes things
complicated with the array of variable-sized objects, and generic
userspace can't really use it - for debug output we already have
obj/driver_name per fence point, which I think is good enough.  

Would that be ok for you from the Android side if Gustavo also provides a
patch to update libsync? I don't think the ABI is fundamentally broken,
but this light cleanup would be nice.

Wrt keeping SYNC_WAIT: I think that's totally fine. Redundant since
polling is supported, but not really an issue imo either. If we're totally
lazy we could implement SYNC_WAIT internally using poll and shave off a
few lines of the implementation.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch