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?
Here is a sample algorithm with a minimum (4kB) of copy operation:
See function: tmc_raw_write_common(..) in
https://github.com/GuidoKiener/linux-usbtmc/blob/master/test-raw.c
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.
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?
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.
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
