> copy-paste in error path. Could you please fix this to use goto as only > for this single function usbip_vhci_driver_close() is called in 3 > different places.
OK. I will fix in next version. Sorry for making you to wite same kind of comment, Nobuo Iwata // > -----Original Message----- > From: Krzysztof Opasiak [mailto:k.opas...@samsung.com] > Sent: Thursday, January 12, 2017 8:02 PM > To: fx IWATA NOBUO <nobuo.iw...@fujixerox.co.jp>; > valentina.mane...@gmail.com; sh...@kernel.org; > gre...@linuxfoundation.org; linux-usb@vger.kernel.org > Cc: fx MICHIMURA TADAO <michimura.ta...@fujixerox.co.jp> > Subject: Re: [PATCH v1 1/1] usbip: auto retry for concurrent attach > > Hi, > > On 12/26/2016 05:53 AM, 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. > > > > 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 > > > > Signed-off-by: Nobuo Iwata <nobuo.iw...@fujixerox.co.jp> > > --- > > drivers/usb/usbip/vhci_sysfs.c | 8 ++++++-- > > tools/usb/usbip/src/usbip_attach.c | 28 > +++++++++++++++------------- > > 2 files changed, 21 insertions(+), 15 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; > > > > 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..695b2e5 100644 > > --- a/tools/usb/usbip/src/usbip_attach.c > > +++ b/tools/usb/usbip/src/usbip_attach.c > > @@ -101,20 +101,22 @@ static int import_device(int sockfd, struct > usbip_usb_device *udev) > > return -1; > > } > > > > - 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"); > > + usbip_vhci_driver_close(); > > + return -1; > > + } > > > > - 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"); > > + usbip_vhci_driver_close(); > > + return -1; > > + } > > copy-paste in error path. Could you please fix this to use goto as only > for this single function usbip_vhci_driver_close() is called in 3 > different places. > > > + } while (rc < 0); > > > > usbip_vhci_driver_close(); > > > > > > Best regards, > -- > Krzysztof Opasiak > Samsung R&D Institute Poland > Samsung Electronics -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
