On Tue, May 22, 2018 at 11:36:13AM +0000, guido@xxxxxxxxxxxxxxxxxx wrote:
>
> Zitat von Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>:
>
> > On Mon, May 21, 2018 at 08:41:22PM +0000, guido@xxxxxxxxxxxxxxxxxx wrote:
> > >
> > > Zitat von Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>:
> > >
> > > > On Fri, May 18, 2018 at 02:48:11PM +0000, guido@xxxxxxxxxxxxxxxxxx wrote:
> > > > >
> > > > > Zitat von Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>:
> > > > >
> > > > > > 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
