Re: [PATCH v7 3/5] dma-buf: Add ioctls to allow userspace to flush

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: David Herrmann <dh.herrmann@gmail.com>
Cc: "Daniel Thompson" <daniel.thompson@linaro.org>,
	"Stéphane Marchesin" <marcheu@google.com>,
	"Thomas Hellstrom" <thellstrom@vmware.com>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	"Jerome Glisse" <jglisse@redhat.com>,
	"David Reveman" <reveman@google.com>,
	"Daniel Vetter" <daniel.vetter@ffwll.ch>,
	"Daniel Vetter" <daniel.vetter@intel.com>
Subject: Re: [PATCH v7 3/5] dma-buf: Add ioctls to allow userspace to flush
Date: Thu, 11 Feb 2016 21:10:55 +0200	[thread overview]
Message-ID: <20160211191054.GG23290@intel.com> (raw)
In-Reply-To: <CANq1E4ThVv1-nuqJCkTqTNt1xqtGbLgtw4YLVwtp=mkhB6jBNg@mail.gmail.com>

On Thu, Feb 11, 2016 at 07:19:45PM +0100, David Herrmann wrote:
> Hi
> 
> On Thu, Feb 11, 2016 at 7:08 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > On Thu, Feb 11, 2016 at 03:54:38PM -0200, Tiago Vignatti wrote:
> >>
> >> Thanks for reviewing, David. Please take a look in my comments in-line.
> >>
> >>
> >> On 02/09/2016 07:26 AM, David Herrmann wrote:
> >> >
> >> > On Tue, Dec 22, 2015 at 10:36 PM, Tiago Vignatti
> >> > <tiago.vignatti@intel.com> wrote:
> >> >> From: Daniel Vetter <daniel.vetter@ffwll.ch>
> > <snip>
> >> >> +
> >> >> +#define DMA_BUF_SYNC_READ      (1 << 0)
> >> >> +#define DMA_BUF_SYNC_WRITE     (2 << 0)
> >> >> +#define DMA_BUF_SYNC_RW        (DMA_BUF_SYNC_READ | DMA_BUF_SYNC_WRITE)
> >> >> +#define DMA_BUF_SYNC_START     (0 << 2)
> >> >> +#define DMA_BUF_SYNC_END       (1 << 2)
> >> >> +#define DMA_BUF_SYNC_VALID_FLAGS_MASK \
> >> >> +       (DMA_BUF_SYNC_RW | DMA_BUF_SYNC_END)
> >> >> +
> >> >> +#define DMA_BUF_BASE           'b'
> >> >> +#define DMA_BUF_IOCTL_SYNC     _IOW(DMA_BUF_BASE, 0, struct dma_buf_sync)
> >> >
> >> > Why _IOW? A read-only ioctl should be able to call DMA_BUF_SYNC_READ, right?
> >>
> >> yup. I've changed it to _IOWR now.
> >
> > AFAICS the ioctl only does copy_from_user() so _IOW seemed perfectly
> > correct to me.
> 
> AFAIK, the _IOC_DIR() arguments in ioctls specify the mode of the
> operation, not the way the arguments are used. So if read(2) was an
> ioctl, it would be annotated as _IOC_READ (even though it _writes_
> into the passed buffer). write(2) would be annotated as _IOC_WRITE
> (even though it _reads_ the buffer). As such, they correspond to the
> file-access mode, whether you opened it readable and/or writable.
> 
> Anyway, turns out VFS does not verify those. As such, you can specify
> whatever you want. I just checked with different existing ioctls
> throughout the kernel (`git grep _IOC_DIR`), and they seem to match
> what I describe.

Not sure which ones you checked because I don't think I've ever seen
that interpretation in the kernel. Though I suppose often it sort of
matches, eg. when the ioctl just gets/sets some value. Any relationship
to some hardware operation is mostly coincidental (well, except when
the hardware really just does some kind of read/write operation).
And for lost ioctls there is no hardware interaction whatsoever.

As far as checking the file access mode goes, well lots of ioctls
totally ignore it.

About the direction you're right. _IOW means userspace writes to
the kernel, and _IOR means userspace reads from the kernel. Which
is exactly what the code did.

Anyway ioctl-numbers.txt says this:
_IO    an ioctl with no parameters
_IOW   an ioctl with write parameters (copy_from_user)
_IOR   an ioctl with read parameters  (copy_to_user)
_IOWR  an ioctl with both write and read parameters.

so I win ;)

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel