Hello, > This doesn't look like a simple change to rename and reuse an unused > function. This patch does lot more and is changing the user interface. > Looks like instead of taking an integer value for device lookup, you > are changing it to char *. > > Any reason why you have to change the user interface to go to char > *busid? > > I would like to see a good explanation why this user interface change > is necessary. I can't figure out why the second argument was int because it was unused. usbip_get_device() is a stub-side (usb-host) function. As you know, only port number in vhci-side can be int. Others are bus-id. Furthermore, the unused code searches list node position. It doesn't make sense. Here, this patch set needs to find bound device with bus-id in stub-side. I may add explanation above to change log. Or I can change new function name like usbip_find_device() and leave the unused function once. Please, let me know how should I do. Best Regards, nobuo.iwata // > -----Original Message----- > From: Shuah Khan [mailto:shuah.kh@xxxxxxxxxxx] > Sent: Thursday, November 24, 2016 6:02 AM > To: fx IWATA NOBUO; valentina.manea.m@xxxxxxxxx; > gregkh@xxxxxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx > Cc: fx MICHIMURA TADAO; Shuah Khan > Subject: Re: [PATCH v13 02/10] usbip: exporting devices: modifications to > host side libraries > > On 11/21/2016 11:48 PM, Nobuo Iwata wrote: > > usbip_get_device() method in usbip_host_driver_ops was not used. It is > > modified as a function to find an exported device for new operations > > 'connect' and 'disconnect'. > > > > bind and unbind function are exported to reuse by new connect and > > disconnect operation. > > This doesn't look like a simple change to rename and reuse an unused function. > This patch does lot more and is changing the user interface. > Looks like instead of taking an integer value for device lookup, you are > changing it to char *. > > Any reason why you have to change the user interface to go to char *busid? > > I would like to see a good explanation why this user interface change is > necessary. > > thanks, > -- Shuah > > > > > Here, connect and disconnect is NEW-3 and NEW-4 respactively in > > diagram below. > > > > EXISTING) - invites devices from application(vhci)-side > > +------+ > +------------------+ > > device--+ STUB | | application/VHCI | > > +------+ > +------------------+ > > 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 disconnected 6) usbip detach > > 7) usbip unbind > > > > NEW) - dedicates devices from device(stb)-side > > +------+ > +------------------+ > > device--+ STUB | | application/VHCI | > > +------+ > +------------------+ > > 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. > > > > Signed-off-by: Nobuo Iwata <nobuo.iwata@xxxxxxxxxxxxxxx> > > --- > > tools/usb/usbip/libsrc/usbip_host_common.c | 6 ++---- > > tools/usb/usbip/libsrc/usbip_host_common.h | 8 ++++---- > > tools/usb/usbip/src/usbip.h | 3 +++ > > tools/usb/usbip/src/usbip_bind.c | 4 ++-- > > tools/usb/usbip/src/usbip_unbind.c | 4 ++-- > > 5 files changed, 13 insertions(+), 12 deletions(-) > > > > diff --git a/tools/usb/usbip/libsrc/usbip_host_common.c > > b/tools/usb/usbip/libsrc/usbip_host_common.c > > index 9d41522..6a98d6c 100644 > > --- a/tools/usb/usbip/libsrc/usbip_host_common.c > > +++ b/tools/usb/usbip/libsrc/usbip_host_common.c > > @@ -256,17 +256,15 @@ int usbip_export_device(struct > > usbip_exported_device *edev, int sockfd) } > > > > struct usbip_exported_device *usbip_generic_get_device( > > - struct usbip_host_driver *hdriver, int num) > > + struct usbip_host_driver *hdriver, char *busid) > > { > > struct list_head *i; > > struct usbip_exported_device *edev; > > - int cnt = 0; > > > > list_for_each(i, &hdriver->edev_list) { > > edev = list_entry(i, struct usbip_exported_device, node); > > - if (num == cnt) > > + if (!strncmp(busid, edev->udev.busid, > SYSFS_BUS_ID_SIZE)) > > return edev; > > - cnt++; > > } > > > > return NULL; > > diff --git a/tools/usb/usbip/libsrc/usbip_host_common.h > > b/tools/usb/usbip/libsrc/usbip_host_common.h > > index a64b803..f9a9def 100644 > > --- a/tools/usb/usbip/libsrc/usbip_host_common.h > > +++ b/tools/usb/usbip/libsrc/usbip_host_common.h > > @@ -38,7 +38,7 @@ struct usbip_host_driver_ops { > > void (*close)(struct usbip_host_driver *hdriver); > > int (*refresh_device_list)(struct usbip_host_driver *hdriver); > > struct usbip_exported_device * (*get_device)( > > - struct usbip_host_driver *hdriver, int num); > > + struct usbip_host_driver *hdriver, char *busid); > > > > int (*read_device)(struct udev_device *sdev, > > struct usbip_usb_device *dev); > > @@ -86,11 +86,11 @@ static inline int usbip_refresh_device_list(struct > > usbip_host_driver *hdriver) } > > > > static inline struct usbip_exported_device * -usbip_get_device(struct > > usbip_host_driver *hdriver, int num) > > +usbip_get_device(struct usbip_host_driver *hdriver, char *busid) > > { > > if (!hdriver->ops.get_device) > > return NULL; > > - return hdriver->ops.get_device(hdriver, num); > > + return hdriver->ops.get_device(hdriver, busid); > > } > > > > /* Helper functions for implementing driver backend */ @@ -99,6 +99,6 > > @@ void usbip_generic_driver_close(struct usbip_host_driver *hdriver); > > int usbip_generic_refresh_device_list(struct usbip_host_driver > > *hdriver); int usbip_export_device(struct usbip_exported_device > > *edev, int sockfd); struct usbip_exported_device > *usbip_generic_get_device( > > - struct usbip_host_driver *hdriver, int num); > > + struct usbip_host_driver *hdriver, char *busid); > > > > #endif /* __USBIP_HOST_COMMON_H */ > > diff --git a/tools/usb/usbip/src/usbip.h b/tools/usb/usbip/src/usbip.h > > index 84fe66a..c296910 100644 > > --- a/tools/usb/usbip/src/usbip.h > > +++ b/tools/usb/usbip/src/usbip.h > > @@ -37,4 +37,7 @@ void usbip_list_usage(void); void > > usbip_bind_usage(void); void usbip_unbind_usage(void); > > > > +int usbip_bind_device(char *busid); > > +int usbip_unbind_device(char *busid); > > + > > #endif /* __USBIP_H */ > > diff --git a/tools/usb/usbip/src/usbip_bind.c > > b/tools/usb/usbip/src/usbip_bind.c > > index fa46141..2401745 100644 > > --- a/tools/usb/usbip/src/usbip_bind.c > > +++ b/tools/usb/usbip/src/usbip_bind.c > > @@ -139,7 +139,7 @@ static int unbind_other(char *busid) > > return status; > > } > > > > -static int bind_device(char *busid) > > +int usbip_bind_device(char *busid) > > { > > int rc; > > struct udev *udev; > > @@ -200,7 +200,7 @@ int usbip_bind(int argc, char *argv[]) > > > > switch (opt) { > > case 'b': > > - ret = bind_device(optarg); > > + ret = usbip_bind_device(optarg); > > goto out; > > default: > > goto err_out; > > diff --git a/tools/usb/usbip/src/usbip_unbind.c > > b/tools/usb/usbip/src/usbip_unbind.c > > index a4a496c..324d986 100644 > > --- a/tools/usb/usbip/src/usbip_unbind.c > > +++ b/tools/usb/usbip/src/usbip_unbind.c > > @@ -39,7 +39,7 @@ void usbip_unbind_usage(void) > > printf("usage: %s", usbip_unbind_usage_string); } > > > > -static int unbind_device(char *busid) > > +int usbip_unbind_device(char *busid) > > { > > char bus_type[] = "usb"; > > int rc, ret = -1; > > @@ -127,7 +127,7 @@ int usbip_unbind(int argc, char *argv[]) > > > > switch (opt) { > > case 'b': > > - ret = unbind_device(optarg); > > + ret = usbip_unbind_device(optarg); > > goto out; > > default: > > goto err_out; > > > > > -- > Shuah Khan > Sr. Linux Kernel Developer > Open Source Innovation Group > Samsung Research America(Silicon Valley) shuah.kh@xxxxxxxxxxx -- 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
