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