[06/12] usb: usbtmc: Add vendor specific/asynchronous read/write ioctls

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Mon, May 21, 2018 at 08:41:22PM +0000, guido@kiener-muenchen.de wrote:
> 
> Zitat von Greg KH <gregkh@linuxfoundation.org>:
> 
> > On Fri, May 18, 2018 at 02:48:11PM +0000, guido@kiener-muenchen.de wrote:
> > > 
> > > Zitat von Greg KH <gregkh@linuxfoundation.org>:
> > > 
> > > > On Thu, May 17, 2018 at 07:03:30PM +0200, Guido Kiener wrote:
> > > > > +/*
> > > > > + * usbtmc_message->flags:
> > > > > + */
> > > > > +#define USBTMC_FLAG_ASYNC		0x0001
> > > > > +#define USBTMC_FLAG_APPEND		0x0002
> > > > > +#define USBTMC_FLAG_IGNORE_TRAILER	0x0004
> > > > > +
> > > > > +struct usbtmc_message {
> > > > > +	void __user *message; /* pointer to header and data */
> > > > > +	__u64 transfer_size; /* size of bytes to transfer */
> > > > > +	__u64 transferred; /* size of received/written bytes */
> > > > > +	__u32 flags; /* bit 0: 0 = synchronous; 1 = asynchronous */
> > > > > +} __attribute__ ((packed));
> > > >
> > > > Very odd structure.  Your userspace pointer is going to be totally out
> > > > of alignment on 32bit systems running on a 64bit kernel.  Why have a
> > > > separate pointer at all?  Why not just put the mesage at the end of this
> > > > structure directly with something like:
> > > > 	__u8 message[0];
> > > > ?
> > > >
> 
> Using a __u8 message[0] ends up with an extra malloc, memcpy, and free
> operation in userland, since we always have to append the data of a given
> user pointer to this struct. E.g. when I send 4 MByte on my (old) laptop
> this takes about plus 6 ms, and decreases bandwidth from 31,0 MByte/s
> to 29,5 MByte/s with USB 2.0.

Really?  That feels like you are doing something really odd.  I guess
you need to figure out how you get the data to/from userspace.

> I hope this struct looks better (in compat mode):
> 
> struct usbtmc_message {
>     __u64 transfer_size; /* size of bytes to transfer */
>     __u64 transferred; /* size of received/written bytes */
>     void __user *message; /* pointer to header and data */
>     __u32 flags; /* bit 0: 0 = synchronous; 1 = asynchronous */
> } __attribute__ ((packed));

No, that will not work at all.  Again, think about the size of that
pointer.

> BTW the driver has no .compat_ioctl entry.

Because you didn't need it until now.

> So I wonder how it could work with 32 bit applications on 64 bit
> systems before submission of our patches.  Do I miss something?

You are creating structures that have different sizes on those two
different userspaces.  Because of that, you would be forced to have a
compat layer.  The smart thing to do is to design the interface to not
have that type of problem at all, don't create new apis that are not
just 64-bit sane to start with.

Copying memory is fast, really fast, much much faster than sending the
data across the USB connection.  If you are seeing major problems here,
then I would first look at your userspace code, and then see what the
kernel code does.

How about you wait for this new api until you get all of the other stuff
implemented/merged?  It looks like you all need to really think about
how this will all work out.

thanks,

greg k-h
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html