Hi Nobuo Iwata,
Sorry for the delay. This one got lost in my Inbox.
On 05/28/2017 08:31 PM, Nobuo Iwata wrote:
> This patch adds recovery from false busy state on concurrent attach
> operation.
>
> The procedure of attach operation is as below.
>
> 1) Find an unused port in /sys/devices/platform/vhci_hcd/status.
> (userspace)
>
> 2) Request attach found port to driver through
> /sys/devices/platform/vhci_hcd/attach. (userspace)
>
> 3) Lock table, reserve requested port and unlock table. (vhci driver)
>
> Attaching more than one remote devices concurrently, same unused port
> number will be found in step-1. Then one request will succeed and
> others will fail even though there are some unused ports.
>
> It is possible to avoid this fail by introdocing userspace exclusive
> control. But it's exaggerated for this special condition. The locking
> itself is done in driver.
Please spell check the change log.
>
> With this patch, driver returns EBUSY when requested port has already
> been used. In this case, attach command retries from step-1: finding
> another unused port. If there's no unused port, the attach operation
> will fail. Othrwise it retries automatically using new free port.
>
> Currunt userspace src/usbip_attach.c:import_device() doesn't use the
> errno.
>
> vhci-hcd's interface (only errno) is changed as following.
>
> Current errno New errno Condition
> EINVAL same as left specified port numbre is in invalid
> range
> EAGAIN same as left platform_get_drvdata() failed
> EINVAL same as left specified socket fd is not valid
> EINVAL EBUSY specified port status is not free
>
> ---
> Version information
>
> v2)
> Gathered usbip_vhci_driver_close() for errors under an exit label.
Greg KH already pointed this out. Please fix this.
>
> Signed-off-by: Nobuo Iwata <nobuo.iwata@xxxxxxxxxxxxxxx>
> ---
> drivers/usb/usbip/vhci_sysfs.c | 8 ++++++--
> tools/usb/usbip/src/usbip_attach.c | 33 +++++++++++++++++-------------
> 2 files changed, 25 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
> index b96e5b1..5d4be4b 100644
> --- a/drivers/usb/usbip/vhci_sysfs.c
> +++ b/drivers/usb/usbip/vhci_sysfs.c
> @@ -214,7 +214,7 @@ static ssize_t store_detach(struct device *dev, struct device_attribute *attr,
>
> ret = vhci_port_disconnect(hcd_to_vhci(hcd), rhport);
> if (ret < 0)
> - return -EINVAL;
> + return ret;
Why are you changing the return value here? vhci_port_disconnect()
currently only returns -EINVAL, however of that changes, userspace
will see a different value. If the correct retunr value in this failure
path is -EINVAL, let's not change that, unless there are good reasons
do so.
The rest looks good to me.
>
> usbip_dbg_vhci_sysfs("Leave\n");
>
> @@ -314,7 +314,11 @@ static ssize_t store_attach(struct device *dev, struct device_attribute *attr,
> sockfd_put(socket);
>
> dev_err(dev, "port %d already used\n", rhport);
> - return -EINVAL;
> + /*
> + * Will be retried from userspace
> + * if there's another free port.
> + */
> + return -EBUSY;
> }
>
> dev_info(dev, "pdev(%u) rhport(%u) sockfd(%d)\n",
> diff --git a/tools/usb/usbip/src/usbip_attach.c b/tools/usb/usbip/src/usbip_attach.c
> index 70a6b50..f1e7ddd 100644
> --- a/tools/usb/usbip/src/usbip_attach.c
> +++ b/tools/usb/usbip/src/usbip_attach.c
> @@ -98,27 +98,32 @@ static int import_device(int sockfd, struct usbip_usb_device *udev)
> rc = usbip_vhci_driver_open();
> if (rc < 0) {
> err("open vhci_driver");
> - return -1;
> + goto err_out;
> }
>
> - port = usbip_vhci_get_free_port();
> - if (port < 0) {
> - err("no free port");
> - usbip_vhci_driver_close();
> - return -1;
> - }
> + do {
> + port = usbip_vhci_get_free_port();
> + if (port < 0) {
> + err("no free port");
> + goto err_driver_close;
> + }
>
> - rc = usbip_vhci_attach_device(port, sockfd, udev->busnum,
> - udev->devnum, udev->speed);
> - if (rc < 0) {
> - err("import device");
> - usbip_vhci_driver_close();
> - return -1;
> - }
> + rc = usbip_vhci_attach_device(port, sockfd, udev->busnum,
> + udev->devnum, udev->speed);
> + if (rc < 0 && errno != EBUSY) {
> + err("import device");
> + goto err_driver_close;
> + }
> + } while (rc < 0);
>
> usbip_vhci_driver_close();
>
> return port;
> +
> +err_driver_close:
> + usbip_vhci_driver_close();
> +err_out:
> + return -1;
> }
>
> static int query_import_device(int sockfd, char *busid)
>
thanks,
-- Shuah
--
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
