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

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

On Tue, May 22, 2018 at 11:36:13AM +0000, guido@kiener-muenchen.de wrote:
> 
> Zitat von Greg KH <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.
> 
> A typically user API is something like this:
> ssize_t write(int fd, const void *buf, size_t count);
> ssize_t send(int sockfd, const void *buf, size_t len, int flags);
> 
> We use a similar function:
> ViStatus viWrite(ViSession vi, ViBuf buf, ViUInt32 count, ViPUInt32 retCount)
> 
> When a user calls viWrite(vi, "*IDN? + 4 MB ...\n", 40000000, &retcont) how
> can we pass the big buf to the kernel without any copy operation?

Your function looks _just_ like a normal write() call, why are you even
trying to wrap it in an ioctl?  Just use write()!

> > > 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.
> > 
> 
> The compat structure is:
> 
> struct compat_usbtmc_message {
> 	u64 transfer_size;
> 	u64 transferred;
> 	compat_uptr_t message;
> 	u32 flags;
> } __packed;
> 
> For AMD64 it works.

Show me the size and layout of both structures for a 32bit and 64bit
userspace please.  There are tools that do this.

Again, this is not how to do it!  You do not put a variable sized
variable in the middle of a structure.  void * changes size!  Either
align everything properly or throw it at the end or even better yet,
DON'T DO THIS AT ALL, JUST USE write()! :)

> > > BTW the driver has no .compat_ioctl entry.
> > 
> > Because you didn't need it until now.
> 
> ioctl(fd,USBTMC_IOCTL_CLEAR) returns errno = 25 (=ENOTTY) in Linux
> Kernel 4.15 when running test-raw32 created with
> gcc -m32 test-raw.c -o test-raw32
> Does it work with other kernel versions?

I have no idea what you are testing here, sorry.

> > > 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.
> 
> I just do not like to be slower than libusb or other operating systems.

If you do this correctly, your bottleneck will be the USB connection.

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