Re: [PATCH v2 1/1] usbip: auto retry for concurrent attach

[Shuah Khan]

On 08/08/2017 03:11 AM, fx IWATA NOBUO wrote:
> Hello,
> 
>> -Original Message-
>> From: Shuah Khan [mailto:sh...@kernel.org]
>> Sent: Thursday, August 03, 2017 2:27 AM
>> To: fx IWATA NOBUO <nobuo.iw...@fujixerox.co.jp>;
>> valentina.mane...@gmail.com; gre...@linuxfoundation.org;
>> linux-usb@vger.kernel.org
>> Cc: fx MICHIMURA TADAO <michimura.ta...@fujixerox.co.jp>; Shuah
>> Khan <sh...@kernel.org>; Shuah Khan <shua...@osg.samsung.com>
>> Subject: Re: [PATCH v2 1/1] usbip: auto retry for concurrent attach
>>
>> 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.
> 
> I will.
> 
>>> 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 leftspecified port numbre is in
>> invalid
>>> range
>>> EAGAIN  same as leftplatform_get_drvdata() failed
>>> EINVAL  same as leftspecified 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.
> 
> OK.
> 
>>> Signed-off-by: Nobuo Iwata <nobuo.iw...@fujixerox.co.jp>
>>> ---
>>>  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.
> 
> Attach operation is done by searching a free port via sysfs and attach
> specifying found free port with the port number.
> 
> If 2 processes try to attach concurrently, they will find same port
> number and one will fail. In this failed case, it can restart from
> searching a free port and it will be successfully attached if there's
> another free port.
> 
> But other cases, specified port number is in invalid range (EINVAL),
> platform_get_drvdata() failed (EAGAIN) or specified socket fd is not
> valid (EINVAL), should not be retried.
> 
> To distinguish the cases, the different errno (EBUSY) is need for the
> case if port number is valid range but used.

Sounds reasonable.

thanks,
-- Shuah
--
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

RE: [PATCH v2 1/1] usbip: auto retry for concurrent attach

[fx IWATA NOBUO]

Hello,

> -Original Message-
> From: Shuah Khan [mailto:sh...@kernel.org]
> Sent: Thursday, August 03, 2017 2:27 AM
> To: fx IWATA NOBUO <nobuo.iw...@fujixerox.co.jp>;
> valentina.mane...@gmail.com; gre...@linuxfoundation.org;
> linux-usb@vger.kernel.org
> Cc: fx MICHIMURA TADAO <michimura.ta...@fujixerox.co.jp>; Shuah
> Khan <sh...@kernel.org>; Shuah Khan <shua...@osg.samsung.com>
> Subject: Re: [PATCH v2 1/1] usbip: auto retry for concurrent attach
> 
> 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.

I will.

> > 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 leftspecified port numbre is in
> invalid
> > range
> > EAGAIN  same as leftplatform_get_drvdata() failed
> > EINVAL  same as leftspecified 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.

OK.

> > Signed-off-by: Nobuo Iwata <nobuo.iw...@fujixerox.co.jp>
> > ---
> >  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.

Attach operation is done by searching a free port via sysfs and attach
specifying found free port with the port number.

If 2 processes try to attach concurrently, they will find same port
number and one will fail. In this failed case, it can restart from
searching a free port and it will be successfully attached if there's
another free port.

But other cases, specified port number is in invalid range (EINVAL),
platform_get_drvdata() failed (EAGAIN) or specified socket fd is not
valid (EINVAL), should not be retried.

To distinguish the cases, the different errno (EBUSY) is need for the
case if port number is valid range but used.

There may be other solutions:
Assigning port number in kernel module and return it to userspace.
Introducing exclusive control to userspace.
I think they need much difference in interface and functionality than this
patch.

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

Re: [PATCH v2 1/1] usbip: auto retry for concurrent attach

[Shuah Khan]

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
> EINVALsame as leftspecified port numbre is in invalid 
> range
> EAGAINsame as leftplatform_get_drvdata() failed
> EINVALsame as leftspecified socket fd is not valid
> EINVALEBUSY   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 
> ---
>  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:
> + 

Re: [PATCH v2 1/1] usbip: auto retry for concurrent attach

[Greg KH]

On Mon, May 29, 2017 at 11:31:07AM +0900, 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
> EINVALsame as leftspecified port numbre is in invalid 
> range
> EAGAINsame as leftplatform_get_drvdata() failed
> EINVALsame as leftspecified socket fd is not valid
> EINVALEBUSY   specified port status is not free
> 
> ---
> Version information
> 
> v2)
> Gathered usbip_vhci_driver_close() for errors under an exit label.
> 
> Signed-off-by: Nobuo Iwata 

Version info should go below this line:

> ---
>  drivers/usb/usbip/vhci_sysfs.c |  8 ++--
>  tools/usb/usbip/src/usbip_attach.c | 33 +-
>  2 files changed, 25 insertions(+), 16 deletions(-)

Otherwise you just cut off the signed-off-by line in the changelog :(

Also, I need an ack from a usbip maintainer...

thanks,

greg k-h
--
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