RE: [PATCH v14 03/10] usbip: exporting devices: new connect operation

[fx IWATA NOBUO]

Hello,

> > Implementation of new connect operation. This is linked as a part of
> > usbip command. With this patch, usbip command has following
> operations.
> >
> >  bind
> >  unbind
> >  list (local/remote)
> >  attach
> >  detach
> >  port
> >  connect ... this patch
> 
> Don't include existing commands. Just list the new one.

OK.

> Why do you call this connect. Isn't it really export
> - connect isn't accurate.
> Call it export/unexport.

Considering that attach is corresponding to import request, I think
export is not good. Furthermore, export is already used in many places
of the code, ex. usbip_host_common.c: usbip_export_device(). So I don't
want to use export.

Operation | PDU | Description
--+-+-
attach|import   |invite a device
detach|NA   |quit invited
( )   |export   |dedicate a device
( )   |un-export|quit dedicated

I will change it if there's user friendly, well-known and fit to
existing operations.

> > In device side node, this binds a device internally, sends an export
> > request and receives export response. The definition of the request and
> > response are defined in original code: tools/usb/usbip/usbip_network.h
> > but was not used. They are corresponding to NEW-3 in following
> diagram.
> >
> > EXISTING) - invites devices from application(vhci)-side
> >  +--+
> +--+
> >  device--+ STUB |   | application/VHCI |
> >  +--+
> +--+
> >  (server)   (client)
> >  1) # usbipd ... start daemon
> >  = = =
> >  2) # usbip list --local
> >  3) # usbip bind
> >   <--- list bound devices ---  4) # usbip list --remote
> >   <--- import a device --  5) # usbip attach
> >  = = =
> >  X disconnected6) # usbip detach
> >  7) usbip unbind
> >
> > NEW) - dedicates devices from device(stub)-side
> >  +--+
> +--+
> >  device--+ STUB |   | application/VHCI |
> >  +--+
> +--+
> >  (client)   (server)
> 
> Make the left side server and right side client. I think you might be
> using server and client network terminology. I would like to see server
> as the system that has the device physically attached to.
> 
> I still want to see server as the one that is connected to the device.
> It is just that in this new proposed model, server is exporting devices.

I know that existing USB/IP users are familiar with word 'server'.
It was consistent to client-server model.
In new operation, to call server the device side node will cause
 confusion because it is a client as client-server model.
So I think it's better to use 'device-side' and 'application-side' to
specify nodes. These word can intuitively denote the nodes.
In this version, I marked (server) and (client) but I will remove
them because it causes existing user's confusion.

> Also does usbip run here in user mode. Can any user run uspip and initiate
> export? If so, I don't think I can take this patch series. I don't like to
> see non root being able to export and/or make attached devices public.

New commands and daemon needs root privilege same as 'bind', 'unbind'
 and 'attach'.

They uses same sysfs interface to 'bind', 'unbind' and 'attach'.

> > 1) # usbipa ... start
> daemon
> 
> Did you mean uspipd or is uspipa a new daemon?
> Okay now I see that there is new daemon introduced in patch 7/10 -
> Why do you need to add a new daemon? Why not add it to usbipd instead?

By separating usbipa, administrators can select to use or not to use
new operation. If they want to use, run usbipa.

Also to simplify dependency.
  - usbipd depends stub and vudc.
  - usbipa depends vhci.

> I would like to see all of this information in README. Including this
> in every single patch is confusing.
> btw you are also missing doc changes for these new options you are
> adding.
> When you change README, please include the both server and client end
> commands just like README does now.

README and doc is changed by 9/10 change to documentation.

> >  = = =
> >  2) # usbip list --local
> >  3) # usbip connect--- export a device -->
> >  = = =
> >  4) # usbip disconnect --- un-export a device --->
> >
> >  Bind and unbind are done in connect and disconnect internally.
> >
> > Signed-off-by: Nobuo Iwata 
> > ---
> >  tools/usb/usbip/src/Makefile.am |   3 +-
> >  tools/usb/usbip/src/usbip.c |   7 +
> >  tools/usb/usbip/src/usbip.h |   3 +
> >  tools/usb/usbip/src/usbip_connect.c | 212
> 
> >  4 files changed, 224 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/usb/usbip/src/Makefile.am
> b/tools/usb/usbip/src/Makefile.am
> > index e81a4eb..0947476 100644
> > --- 

RE: [PATCH v14 03/10] usbip: exporting devices: new connect operation

[fx IWATA NOBUO]

Hello,

> >  Bind and unbind are done in connect and disconnect internally.
> 
> Please be more precise here and mention that those operation are done
> for a stub driver only and for vudc you need to bind gadget manually
> using usb gadget subsystem before using connect function.

> > +   int bind = 1;
> 
> Shouldn't this parameter be exposed to cmd line among remote, busid and
> device?

I decided to separate bind from connect: not to bind in connect 
operation.
Regarding both stub and vudc, I think it will be simpler and easier to 
understand for users.

To do so, there's no need to export bind/unbind in previous patch.

Thank you for your advice,

Nobuo Iwata
//
> -Original Message-
> From: Krzysztof Opasiak [mailto:k.opas...@samsung.com]
> Sent: Thursday, January 12, 2017 9:44 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 v14 03/10] usbip: exporting devices: new connect
> operation
> 
> 
> Hi,
> 
> 
> On 12/26/2016 08:08 AM, Nobuo Iwata wrote:
> > Implementation of new connect operation. This is linked as a part of
> > usbip command. With this patch, usbip command has following
> operations.
> >
> >  bind
> >  unbind
> >  list (local/remote)
> >  attach
> >  detach
> >  port
> >  connect ... this patch
> >
> > In device side node, this binds a device internally, sends an export
> > request and receives export response. The definition of the request and
> > response are defined in original code: tools/usb/usbip/usbip_network.h
> > but was not used. They are corresponding to NEW-3 in following
> diagram.
> >
> > EXISTING) - invites devices from application(vhci)-side
> >  +--+
> +--+
> >  device--+ STUB |   | application/VHCI |
> >  +--+
> +--+
> >  (server)   (client)
> >  1) # usbipd ... start daemon
> >  = = =
> >  2) # usbip list --local
> >  3) # usbip bind
> >   <--- list bound devices ---  4) # usbip list --remote
> >   <--- import a device --  5) # usbip attach
> >  = = =
> >  X disconnected6) # usbip detach
> >  7) usbip unbind
> >
> > NEW) - dedicates devices from device(stub)-side
> >  +--+
> +--+
> >  device--+ STUB |   | application/VHCI |
> >  +--+
> +--+
> >  (client)   (server)
> > 1) # usbipa ... start
> daemon
> >  = = =
> >  2) # usbip list --local
> >  3) # usbip connect--- export a device -->
> >  = = =
> >  4) # usbip disconnect --- un-export a device --->
> >
> >  Bind and unbind are done in connect and disconnect internally.
> 
> Please be more precise here and mention that those operation are done
> for a stub driver only and for vudc you need to bind gadget manually
> using usb gadget subsystem before using connect function.
> 
> >
> > Signed-off-by: Nobuo Iwata <nobuo.iw...@fujixerox.co.jp>
> > ---
> >  tools/usb/usbip/src/Makefile.am |   3 +-
> >  tools/usb/usbip/src/usbip.c |   7 +
> >  tools/usb/usbip/src/usbip.h |   3 +
> >  tools/usb/usbip/src/usbip_connect.c | 212
> 
> >  4 files changed, 224 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/usb/usbip/src/Makefile.am
> b/tools/usb/usbip/src/Makefile.am
> > index e81a4eb..0947476 100644
> > --- a/tools/usb/usbip/src/Makefile.am
> > +++ b/tools/usb/usbip/src/Makefile.am
> > @@ -6,6 +6,7 @@ sbin_PROGRAMS := usbip usbipd
> >
> >  usbip_SOURCES := usbip.h utils.h usbip.c utils.c usbip_network.c \
> >  usbip_attach.c usbip_detach.c usbip_list.c \
> > -usbip_bind.c usbip_unbind.c usbip_port.c
> > +usbip_bind.c usbip_unbind.c usbip_port.c \
> > +usbip_connect.c
> >
> >  usbipd_SOURCES := usbip_network.h usbipd.c usbip_network.c
> > diff --git a/tools/usb/usbip/src/usbip.c
> b/tools/usb/usbip/src/usbip.c
> > index d7599d9..ad2a259 100644
> > --- a/tools/usb/usbip/src/usbip.c
> > +++ b/tools/usb/usbip/src/usbip.c
> > @@ -4,6 +4,7 @@
> >   *
> >   * Copyright (C) 2011 matt mooney <m...@muteddi

Re: [PATCH v14 03/10] usbip: exporting devices: new connect operation

[Krzysztof Opasiak]

On 01/13/2017 01:06 AM, Shuah Khan wrote:
> On 12/26/2016 12:08 AM, Nobuo Iwata wrote:
>> Implementation of new connect operation. This is linked as a part of 
>> usbip command. With this patch, usbip command has following operations.
>>
>>  bind
>>  unbind
>>  list (local/remote)
>>  attach
>>  detach
>>  port
>>  connect ... this patch
> 
> Don't include existing commands. Just list the new one. Why do you call
> this connect. Isn't it really export - connect isn't accurate. Call it
> export/unexport.
> 
>>
>> In device side node, this binds a device internally, sends an export 
>> request and receives export response. The definition of the request and 
>> response are defined in original code: tools/usb/usbip/usbip_network.h 
>> but was not used. They are corresponding to NEW-3 in following diagram. 
>>
>> EXISTING) - invites devices from application(vhci)-side
>>  +--+   +--+
>>  device--+ STUB |   | application/VHCI |
>>  +--+   +--+
>>  (server)   (client)
>>  1) # usbipd ... start daemon
>>  = = =
>>  2) # usbip list --local
>>  3) # usbip bind
>>   <--- list bound devices ---  4) # usbip list --remote
>>   <--- import a device --  5) # usbip attach
>>  = = =
>>  X disconnected6) # usbip detach
>>  7) usbip unbind
>>
>> NEW) - dedicates devices from device(stub)-side
>>  +--+   +--+
>>  device--+ STUB |   | application/VHCI |
>>  +--+   +--+
>>  (client)   (server)
> 
> Make the left side server and right side client. I think you might be
> using server and client network terminology. I would like to see server
> as the system that has the device physically attached to.
> 
> 
> I still want to see server as the one that is connected to the device.
> It is just that in this new proposed model, server is exporting devices.
> 
> Also does usbip run here in user mode. Can any user run uspip and initiate
> export? If so, I don't think I can take this patch series. I don't like to
> see non root being able to export and/or make attached devices public.
> 

By default it's not possible. See my response to patch 9.

Best regards,
-- 
Krzysztof Opasiak
Samsung R 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

Re: [PATCH v14 03/10] usbip: exporting devices: new connect operation

[Shuah Khan]

On 12/26/2016 12:08 AM, Nobuo Iwata wrote:
> Implementation of new connect operation. This is linked as a part of 
> usbip command. With this patch, usbip command has following operations.
> 
>  bind
>  unbind
>  list (local/remote)
>  attach
>  detach
>  port
>  connect ... this patch

Don't include existing commands. Just list the new one. Why do you call
this connect. Isn't it really export - connect isn't accurate. Call it
export/unexport.

> 
> In device side node, this binds a device internally, sends an export 
> request and receives export response. The definition of the request and 
> response are defined in original code: tools/usb/usbip/usbip_network.h 
> but was not used. They are corresponding to NEW-3 in following diagram. 
> 
> EXISTING) - invites devices from application(vhci)-side
>  +--+   +--+
>  device--+ STUB |   | application/VHCI |
>  +--+   +--+
>  (server)   (client)
>  1) # usbipd ... start daemon
>  = = =
>  2) # usbip list --local
>  3) # usbip bind
>   <--- list bound devices ---  4) # usbip list --remote
>   <--- import a device --  5) # usbip attach
>  = = =
>  X disconnected6) # usbip detach
>  7) usbip unbind
> 
> NEW) - dedicates devices from device(stub)-side
>  +--+   +--+
>  device--+ STUB |   | application/VHCI |
>  +--+   +--+
>  (client)   (server)

Make the left side server and right side client. I think you might be
using server and client network terminology. I would like to see server
as the system that has the device physically attached to.


I still want to see server as the one that is connected to the device.
It is just that in this new proposed model, server is exporting devices.

Also does usbip run here in user mode. Can any user run uspip and initiate
export? If so, I don't think I can take this patch series. I don't like to
see non root being able to export and/or make attached devices public.

> 1) # usbipa ... start daemon

Did you mean uspipd or is uspipa a new daemon? Okay now I see that
there is new daemon introduced in patch 7/10 - Why do you need to
add a new daemon? Why not add it to usbipd instead?

I would like to see all of this information in README. Including this
in every single patch is confusing. btw you are also missing doc changes
for these new options you are adding.

When you change README, please include the both server and client end
commands just like README does now.

>  = = =
>  2) # usbip list --local
>  3) # usbip connect--- export a device -->
>  = = =
>  4) # usbip disconnect --- un-export a device --->
> 
>  Bind and unbind are done in connect and disconnect internally.
> 
> Signed-off-by: Nobuo Iwata 
> ---
>  tools/usb/usbip/src/Makefile.am |   3 +-
>  tools/usb/usbip/src/usbip.c |   7 +
>  tools/usb/usbip/src/usbip.h |   3 +
>  tools/usb/usbip/src/usbip_connect.c | 212 
>  4 files changed, 224 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/usb/usbip/src/Makefile.am b/tools/usb/usbip/src/Makefile.am
> index e81a4eb..0947476 100644
> --- a/tools/usb/usbip/src/Makefile.am
> +++ b/tools/usb/usbip/src/Makefile.am
> @@ -6,6 +6,7 @@ sbin_PROGRAMS := usbip usbipd
>  
>  usbip_SOURCES := usbip.h utils.h usbip.c utils.c usbip_network.c \
>usbip_attach.c usbip_detach.c usbip_list.c \
> -  usbip_bind.c usbip_unbind.c usbip_port.c
> +  usbip_bind.c usbip_unbind.c usbip_port.c \
> +  usbip_connect.c
>  
>  usbipd_SOURCES := usbip_network.h usbipd.c usbip_network.c
> diff --git a/tools/usb/usbip/src/usbip.c b/tools/usb/usbip/src/usbip.c
> index d7599d9..ad2a259 100644
> --- a/tools/usb/usbip/src/usbip.c
> +++ b/tools/usb/usbip/src/usbip.c
> @@ -4,6 +4,7 @@
>   *
>   * Copyright (C) 2011 matt mooney 
>   *   2005-2007 Takahiro Hirofuchi
> + * Copyright (C) 2015-2016 Nobuo Iwata 
>   *
>   * This program is free software: you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License as published by
> @@ -76,6 +77,12 @@ static const struct command cmds[] = {
>   .usage = usbip_detach_usage
>   },
>   {
> + .name  = "connect",
> + .fn= usbip_connect,
> + .help  = "Connect a USB device to a remote computer",
> + .usage = usbip_connect_usage
> + },
> + {
>   .name  = "list",
>   .fn= usbip_list,
>   .help  = "List exportable or local 

Re: [PATCH v14 03/10] usbip: exporting devices: new connect operation

[Krzysztof Opasiak]

Hi,


On 12/26/2016 08:08 AM, Nobuo Iwata wrote:
> Implementation of new connect operation. This is linked as a part of 
> usbip command. With this patch, usbip command has following operations.
> 
>  bind
>  unbind
>  list (local/remote)
>  attach
>  detach
>  port
>  connect ... this patch
> 
> In device side node, this binds a device internally, sends an export 
> request and receives export response. The definition of the request and 
> response are defined in original code: tools/usb/usbip/usbip_network.h 
> but was not used. They are corresponding to NEW-3 in following diagram. 
> 
> EXISTING) - invites devices from application(vhci)-side
>  +--+   +--+
>  device--+ STUB |   | application/VHCI |
>  +--+   +--+
>  (server)   (client)
>  1) # usbipd ... start daemon
>  = = =
>  2) # usbip list --local
>  3) # usbip bind
>   <--- list bound devices ---  4) # usbip list --remote
>   <--- import a device --  5) # usbip attach
>  = = =
>  X disconnected6) # usbip detach
>  7) usbip unbind
> 
> NEW) - dedicates devices from device(stub)-side
>  +--+   +--+
>  device--+ STUB |   | application/VHCI |
>  +--+   +--+
>  (client)   (server)
> 1) # usbipa ... start daemon
>  = = =
>  2) # usbip list --local
>  3) # usbip connect--- export a device -->
>  = = =
>  4) # usbip disconnect --- un-export a device --->
> 
>  Bind and unbind are done in connect and disconnect internally.

Please be more precise here and mention that those operation are done
for a stub driver only and for vudc you need to bind gadget manually
using usb gadget subsystem before using connect function.

> 
> Signed-off-by: Nobuo Iwata 
> ---
>  tools/usb/usbip/src/Makefile.am |   3 +-
>  tools/usb/usbip/src/usbip.c |   7 +
>  tools/usb/usbip/src/usbip.h |   3 +
>  tools/usb/usbip/src/usbip_connect.c | 212 
>  4 files changed, 224 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/usb/usbip/src/Makefile.am b/tools/usb/usbip/src/Makefile.am
> index e81a4eb..0947476 100644
> --- a/tools/usb/usbip/src/Makefile.am
> +++ b/tools/usb/usbip/src/Makefile.am
> @@ -6,6 +6,7 @@ sbin_PROGRAMS := usbip usbipd
>  
>  usbip_SOURCES := usbip.h utils.h usbip.c utils.c usbip_network.c \
>usbip_attach.c usbip_detach.c usbip_list.c \
> -  usbip_bind.c usbip_unbind.c usbip_port.c
> +  usbip_bind.c usbip_unbind.c usbip_port.c \
> +  usbip_connect.c
>  
>  usbipd_SOURCES := usbip_network.h usbipd.c usbip_network.c
> diff --git a/tools/usb/usbip/src/usbip.c b/tools/usb/usbip/src/usbip.c
> index d7599d9..ad2a259 100644
> --- a/tools/usb/usbip/src/usbip.c
> +++ b/tools/usb/usbip/src/usbip.c
> @@ -4,6 +4,7 @@
>   *
>   * Copyright (C) 2011 matt mooney 
>   *   2005-2007 Takahiro Hirofuchi
> + * Copyright (C) 2015-2016 Nobuo Iwata 
>   *
>   * This program is free software: you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License as published by
> @@ -76,6 +77,12 @@ static const struct command cmds[] = {
>   .usage = usbip_detach_usage
>   },
>   {
> + .name  = "connect",
> + .fn= usbip_connect,
> + .help  = "Connect a USB device to a remote computer",
> + .usage = usbip_connect_usage
> + },
> + {
>   .name  = "list",
>   .fn= usbip_list,
>   .help  = "List exportable or local USB devices",
> diff --git a/tools/usb/usbip/src/usbip.h b/tools/usb/usbip/src/usbip.h
> index c296910..3c22c27 100644
> --- a/tools/usb/usbip/src/usbip.h
> +++ b/tools/usb/usbip/src/usbip.h
> @@ -1,6 +1,7 @@
>  /*
>   * Copyright (C) 2011 matt mooney 
>   *   2005-2007 Takahiro Hirofuchi
> + * Copyright (C) 2015-2016 Nobuo Iwata 
>   *
>   * This program is free software: you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License as published by
> @@ -30,12 +31,14 @@ int usbip_list(int argc, char *argv[]);
>  int usbip_bind(int argc, char *argv[]);
>  int usbip_unbind(int argc, char *argv[]);
>  int usbip_port_show(int argc, char *argv[]);
> +int usbip_connect(int argc, char *argv[]);
>  
>  void usbip_attach_usage(void);
>  void usbip_detach_usage(void);
>  void usbip_list_usage(void);
>  void usbip_bind_usage(void);
>  void usbip_unbind_usage(void);
> +void usbip_connect_usage(void);
>