usb: usbtmc: Add vendor specific/asynchronous read/write ioctls

Zitat von Greg KH <[email protected]>:

Quoted 32 lines · On Mon, May 21, 2018 at 08:41:22PM +0000, [email protected] wrote:

Quoted 31 lines

Zitat von Greg KH <[email protected]>:

Quoted 14 lines · On Fri, May 18, 2018 at 02:48:11PM +0000, [email protected] wrote:

Quoted 13 lines

Zitat von Greg KH <[email protected]>:

Quoted 10 lines · Guido Kiener

Quoted 9 lines

(quoted diff) +9 -0

+/*
+ * 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 */

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+	__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

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

structure directly with something like:

	__u8 message[0];

?

Quoted 8 lines

Quoted 5 lines

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?

Quoted 9 lines

Quoted 8 lines

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));

Quoted 3 lines

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.

Quoted 3 lines

Quoted 1 line

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?

Quoted 9 lines

Quoted 2 lines

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.

Quoted 4 lines

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.