Dear Bjørn, I might not fully understand your comments but I try my best. > usbip-host.ko and vhci-hcd.ko are running on different hosts, likely > on different hardware and kernel versions, so this picture is > impossible. If this discusses USB level interoperability, the issue is came from USB/IP itself before this patch. I started from 3.14.2. I tested several combinations and almost good except USB storage error in 3.14.2 which fixed later kernel. I tested a host side porting with libusb using Windows 7 and 8.1. WebCam failed in setup phase but others work well. I think integrators who utilize this infrastructure arrange certain conditions including application delivery, update mechanism, manual operation rules and etc. There may be problems but they can be managed. > two TCP sessions. Sorry for my poor picture. TCP session is one as well as original USB/IP. After preparing phase, the URBs are transferred in same session. > You can implement a TCP<->websocket proxy in userspace on each side Yes. But assuming small device (like raspberry Pi) with WebCam isochronous transfer, this patch helps to get smoother motion. I think it's not bad to put small hooks to usbip-host.ko and vhci-hcd.ko , introduce a separate module for userspace transfer if it clears errors pointed out below. > a) old and new client/server versions, and > b) clients/servers with and without usbip-ux.ko > Is a new userspace required if usbip-ux.ko is loaded, or will the old > ABI continue to work as before? Yes. New userspace is required if usbip-ux.ko is loaded. Old ABI works as before if usbip-ux.ko is not loaded. It's possible to change that old user space binary works as before even if usbip-ux.ko exists. Is it needed? usbip-ux.ko is written in usbip/websocket/README and not in usbip/README. It is assumed to be loaded when usbws and usbwsd/usbwsa are used. > So you didn't run checkpatch on this? I guess not: > total: 52 errors, 51 warnings, 1183 lines checked No. I'm very sorry that I didn't understand SubmittingPatches. I will clear them. > Ouch. What happened to usbip_trx_ops on errors? I will fix. > Testing for mode before calling a mode dependent callback looks weird. > Why don't you do all the USBIP_TRX_MODE_KERNEL work in > usbip_kernel_sendmsg? I will check the logic. > Please don't break string constants. I will fix. > Is this really compat-safe? I used these pointers for debug messages in userspace to identify connection instead of sockfd. I will change the interface. Thank you spending time, nobuo iwata // > -----Original Message----- > From: Bjørn Mork [mailto:bjorn@xxxxxxx] > Sent: Tuesday, January 05, 2016 8:35 PM > To: fx IWATA NOBUO > Cc: valentina.manea.m@xxxxxxxxx; shuah.kh@xxxxxxxxxxx; > gregkh@xxxxxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; fx MICHIMURA > TADAO > Subject: Re: [PATCH v6 04/11] usbip: kernel module for userspace URBs > transmission > > Nobuo Iwata <nobuo.iwata@xxxxxxxxxxxxxxx> writes: > > > Originally, USB/IP transmits requests and response PDUs for > > preparation to transfer URBs in user space, after the preparation, > > URBs are transmitted in kernel space. > > > > To make easy to introduce application network protocols like WebSocket > > and so on, the driver, usbip_ux.ko, forwards URBs to USB/IP user space > > utilities. It's just like fuse driver for user space file system. > > Then, utilities transfer URBs in user space. > > > > To do so, usbip_trx_ops makes send/receive functions pluggable. > > kernel_sendmsg() and kernel_recvmsg() for kernel mode transfer can be > > substituted by read/write methods to user space utilities. > > > > In the absence of usbip_ux.ko, original kernel space transferring is > > valid. usbip_ux.ko replaces usbip_trx_ops in its init routine. > > > > A) Original - kernel space URBs transfer > > > > User +-------+ 1) import/export +-------+ > > space |uspipd,|<-------------------->|usbip, | > > |usbip | |usbipa | > > +---+---+ +---+---+ > > | | > > 2)Set sockfd| |2)Set sockfd > > thru sysfs| | thru sysfs > > V V > > Kernel +-------+ 4)URBs +-------+ > > space |usbip |<-------------------->|vhci | > > |host.ko| |hcd.ko | > > +-------+ +-------+ > > 3)link to kernel trx_ops 3)link to kernel trx_ops > > > AFAICS, usbip-host.ko and vhci-hcd.ko are running on different hosts, > likely on different hardware and kernel versions, so this picture is > impossible. > > But I'm not going to be more difficult than necessary :) I can see what > you are getting at: There is a kernel TCP socket on each side of the "4)URBs" > channel, and you want to be able to replace it with something else. > > But I am still not convinced about this strategy. There is nothing in the > old protocol preventing you from terminating a TCP session locally on each > host, and do whatever you like between these two TCP sessions. > You can implement a TCP<->websocket proxy in userspace on each side of the > link without hooking into the kernel at all, passing one end of each session > to the existing usbip kernel implementation. > > Or am I missing something? > > > > B) New - user space URBs transfer > > > > User +-------+ 1)import/export +-------+ > > space |uspipd,|<-------------------->|usbip, | > > +---|usbip |<-------------------->|usbipa |---+ > > 2)Set sockfd|+--+-------+ 6)URBs +-------+--+|2)Set > sockfd > > thru ioctl|| ^ ^ || thru > ioctl > > (right) || |5)read/write 5)read/write| || (left) > > 3)Set sockfd|| +-------+ +------+ ||3)Set > Sockfd > > thru sysfs|+-----------+ | /dev/usbip-ux| +-----------+| thru > sysfs > > (left) V V V V V V > (right) > > Kernel +-------+ +-------+ +-------+ +-------+ > > space |usbip |<----->|usbip | |usbip |<----->|vhci | > > |host.ko|5)send |ux.ko | |ux.ko |5)send |hcd.ko | > > +-------+ recv +-------+ +-------+ recv +-------+ > > 4)link to user trx_ops 4)link to user trx_ops > > > Thanks for trying to draw it up. That really helps understaning. But this > got a little complicated.... I assume you keep the protocol (the > "1) import/export" arrow) compatible, so that you can mix and match > (assuming fallback to TCP transport): > > a) old and new client/server versions, and > b) clients/servers with and without usbip-ux.ko > > Is this correct? > > How about userspace<->kernel compatibility in each side: Is a new > userspace required if usbip-ux.ko is loaded, or will the old ABI continue > to work as before? > > > @@ -335,26 +336,28 @@ int usbip_recv(struct socket *sock, void *buf, > > int size) > > > > usbip_dbg_xmit("enter\n"); > > > > - if (!sock || !buf || !size) { > > - pr_err("invalid arg, sock %p buff %p size %d\n", sock, buf, > > - size); > > + if ((!ud->tcp_socket && !ud->ux) || !buf || !size) { > > + pr_err("invalid arg, sock %p ux %p buff %p size %d\n", > > + ud->tcp_socket, ud->ux, buf, size); > > return -EINVAL; > > } > > > > do { > > - sock->sk->sk_allocation = GFP_NOIO; > > iov.iov_base = buf; > > iov.iov_len = size; > > - msg.msg_name = NULL; > > - msg.msg_namelen = 0; > > - msg.msg_control = NULL; > > - msg.msg_controllen = 0; > > - msg.msg_flags = MSG_NOSIGNAL; > > + if (usbip_trx_ops->mode == USBIP_TRX_MODE_KERNEL) { > > Testing for mode before calling a mode dependent callback looks weird. > Why don't you do all the USBIP_TRX_MODE_KERNEL work in > usbip_kernel_sendmsg? > > > > + if (ux->ud) { > > + return 1; > > + } > > So you didn't run checkpatch on this? I guess not: > > total: 52 errors, 51 warnings, 1183 lines checked > > > > +static int usbip_ux_getkaddr(usbip_ux_t *ux, void __user *ubuf) { > > + struct usbip_ux_kaddr kaddr = {(void*)ux, > (void*)(ux->tcp_socket)}; > > + if (copy_to_user(ubuf, &kaddr, sizeof(kaddr))) { > > + return -EFAULT; > > + } > > + return 0; > > +} > > Huh? > > > +static int __init usbip_ux_init(void) { > > + int ret; > > + > > + usbip_trx_ops_bak = usbip_trx_ops; > > + usbip_trx_ops = &usbip_trx_user_ops; > > + > > + INIT_LIST_HEAD(&usbip_ux_list); > > + ret = alloc_chrdev_region(&usbip_ux_devno, USBIP_UX_MINOR, 1, > USBIP_UX_DEV_NAME); > > + if (ret < 0) { > > + pr_err("Fail to alloc chrdev for %s\n", > USBIP_UX_DEV_NAME); > > + goto err_out; > > + } > > + cdev_init(&usbip_ux_cdev, &usbip_ux_fops); > > + usbip_ux_cdev.owner = usbip_ux_fops.owner; > > + ret = cdev_add(&usbip_ux_cdev, usbip_ux_devno, 1); > > + if (ret < 0) { > > + pr_err("Fail to add cdev: %s\n", USBIP_UX_DEV_NAME); > > + kobject_put(&usbip_ux_cdev.kobj); > > + goto err_unregister_chrdev; > > + } > > + usbip_ux_class = class_create(THIS_MODULE, USBIP_UX_CLASS_NAME); > > + if (IS_ERR(usbip_ux_class)) { > > + pr_err("Fail to create class: %s\n", > USBIP_UX_CLASS_NAME); > > + ret = PTR_ERR(usbip_ux_class); > > + goto err_cdev_del; > > + } > > + usbip_ux_dev = device_create(usbip_ux_class, NULL, > usbip_ux_devno, NULL, USBIP_UX_DEV_NAME); > > + if (IS_ERR(usbip_ux_dev)) { > > + pr_err("Fail to create sysfs entry for %s\n", > USBIP_UX_DEV_NAME); > > + ret = PTR_ERR(usbip_ux_class); > > + goto err_cdev_del; > > + } > > + return 0; > > +err_cdev_del: > > + cdev_del(&usbip_ux_cdev); > > +err_unregister_chrdev: > > + unregister_chrdev_region(usbip_ux_devno, 1); > > +err_out: > > + return ret; > > Ouch. What happened to usbip_trx_ops on errors? > > In any case: The whole usbib-ux.ko looks like a special purpose > chardev<->sock proxy. Why do that in the kernel? > > > > --- a/drivers/usb/usbip/vhci_sysfs.c > > +++ b/drivers/usb/usbip/vhci_sysfs.c > > @@ -1,5 +1,6 @@ > > /* > > * Copyright (C) 2003-2008 Takahiro Hirofuchi > > + * Copyright (C) 2015 Nobuo Iwata > > * > > * This is free software; you can redistribute it and/or modify > > * it under the terms of the GNU General Public License as published > > by @@ -39,16 +40,17 @@ static ssize_t status_show(struct device *dev, > > struct device_attribute *attr, > > > > /* > > * output example: > > - * prt sta spd dev socket local_busid > > - * 000 004 000 000 c5a7bb80 1-2.3 > > - * 001 004 000 000 d8cee980 2-3.4 > > + * prt sta spd dev socket ux > local_busid > > + * 000 004 000 00010002 c5a7bb80 0 1-2.3 > > + * 001 004 000 00020003 d8cee980 0 2-3.4 > > * > > * IP address can be retrieved from a socket pointer address by > looking > > * up /proc/net/{tcp,tcp6}. Also, a userland program may remember > a > > * port number and its peer IP address. > > */ > > out += sprintf(out, > > - "prt sta spd bus dev socket > local_busid\n"); > > + "prt sta spd dev socket " > > + "ux local_busid\n"); > > > > for (i = 0; i < VHCI_NPORTS; i++) { > > struct vhci_device *vdev = port_to_vdev(i); @@ -59,11 > +61,19 @@ > > static ssize_t status_show(struct device *dev, struct device_attribute > *attr, > > if (vdev->ud.status == VDEV_ST_USED) { > > out += sprintf(out, "%03u %08x ", > > vdev->speed, vdev->devid); > > - out += sprintf(out, "%16p ", > vdev->ud.tcp_socket); > > + if (vdev->ud.tcp_socket) > > + out += sprintf(out, "%16p ", > vdev->ud.tcp_socket); > > + else > > + out += sprintf(out, "%016x ", 0); > > + if (vdev->ud.ux) > > + out += sprintf(out, "%16p ", > vdev->ud.ux); > > + else > > + out += sprintf(out, "%016x ", 0); > > out += sprintf(out, "%s", > dev_name(&vdev->udev->dev)); > > > > } else { > > - out += sprintf(out, "000 000 000 0000000000000000 > 0-0"); > > + out += sprintf(out, "000 00000000 > 0000000000000000 " > > + "0000000000000000 0-0"); > > } > > > > out += sprintf(out, "\n"); > > > I don't think you can change the format. And if you are allowed to do > that: Please don't break string constants. Ref CodingStyle Chapter 2: > "never break user-visible strings". > > > > +struct usbip_ux_kaddr { > > + void *ux; > > + void *sock; > > +}; > > Is this really compat-safe? > > Note that I haven't really gone through the details in your immplementation. > Just pointing out a few random details I find unlcear. > > > > Bjørn ��.n��������+%������w��{.n�����{���)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
