Hello Shuah, > Way too many changes in this one patch. > This is not an accurate description. You are really changing look ups using > bus-id instead bus-num and whether usbip_get_device() is used or not is > irrelevant. > Please don't include exporting bind_device() and unbind_device() and > changing their names in this patch. Send a separate patch for that. I will divide this patch into following 3 patches. 1) removing un used old usbip_get_device() as out of the series. 2) exporting bind/unbind() for new operations. 3) adding new find device by busid for new operations. > It makes sense to move the functions you are exporting bind_device(), > unbind_device(), and usbip_get_device() to libsrc - usbip_common.c > might be a good choice. usbip_common.c does not depend stub nor vhci. bind/unbind depends stub. And bind_device() and unbind_device() use local functions in usbip_bind.c and usbip_unbind.c. in this series So I'd like to keep them in usbip_bind.c and usbip_unbind.c. > > + if (!strncmp(busid, edev->udev.busid, > SYSFS_BUS_ID_SIZE)) > > Use strlen(edev->udev.busid) instead of SYSFS_BUS_ID_SIZE I will use strcmp() because busid in refreshed edev list must be valid. > The following isn't part of this patch, hence shouldn't be included in > the change log. I will remove. Best Regards, Nobuo Iwata // > -----Original Message----- > From: Shuah Khan [mailto:shuahkh@xxxxxxxxxxxxxxx] > Sent: Friday, January 13, 2017 6:15 AM > To: fx IWATA NOBUO <Nobuo.Iwata@xxxxxxxxxxxxxxx>; > valentina.manea.m@xxxxxxxxx; shuah@xxxxxxxxxx; > gregkh@xxxxxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx > Cc: fx MICHIMURA TADAO <MICHIMURA.Tadao@xxxxxxxxxxxxxxx>; Shuah > Khan <shuahkh@xxxxxxxxxxxxxxx>; Shuah Khan <shuah@xxxxxxxxxx> > Subject: Re: [PATCH v14 02/10] usbip: exporting devices: modifications to > host side libraries > > On 12/26/2016 12:08 AM, Nobuo Iwata wrote: > > Modifications to host driver wrapper and its related operations (i.e. > > bind/unbind). > > Way too many changes in this one patch. > > > > > usbip_get_device() method was not used. The implementation of the > > method searches a bound devices list by list index. The position in the > > list is meaningless for any operations so it is assumed not to be used > > in future. > > > > For this patch set, a method to find a bound device in the bound > > devices list by bus-id is needed. usbip_get_device() is re-implemented > > as a function to find a bound device by bus-id. > > This is not an accurate description. You are really changing look ups using > bus-id instead bus-num and whether usbip_get_device() is used or not is > irrelevant. > > Please make changes to find bound device by bus-id in a separate patch. > You will have to change usbip_get_device() anyway because you are > changing > .get_device() interface. Include exporting usbip_get_debice() in a separate > patch. > > > > > bind and unbind function are exported to reuse by new connect and > > disconnect operation. Here, connect and disconnect is NEW-3 and > NEW-4 > > respactively in diagram below. > > Please don't include exporting bind_device() and unbind_device() and > changing their names in this patch. Send a separate patch for that. > > It makes sense to move the functions you are exporting bind_device(), > unbind_device(), and usbip_get_device() to libsrc - usbip_common.c > might be a good choice. > > The following isn't part of this patch, hence shouldn't be included in > the change log. > > > > > 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 disconnected 6) # 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. > > > > 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)) > > Use strlen(edev->udev.busid) instead of 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; > > -- 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
