RE: [PATCH v3 1/2] usbip: vhci number of ports extension

[fx IWATA NOBUO]

> > ex) # insmod vhci_hcd.ko num_controllers=4
> 
> I hate module parameters, please don't do that, create these dynamically
> as needed some other way.

OK.

> > Also, ports per controller is changed from 8 to USB_MAXCHILDREN (31).
> 
> Why?  All in one patch?  Not good.

I will remove the modification.

> > It can be modified with VHCI_NPORTS flag at module compilation.
> 
> No one does that, make this dynamic as well please, in a series of patches,
> not just one huge one.

OK.

By the way, how about 2/2.
If it's acceptable, I will post it excluding 1/2.

Thank you,

n.iwata
//
--
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 v3 1/2] usbip: vhci number of ports extension

[Greg KH]

On Wed, Feb 10, 2016 at 10:55:12AM +0900, Nobuo Iwata wrote:
> This patch extends number of ports limitation in application (vhci) 
> side.
> 
> To do so, vhci driver supports multiple host controllers. The number of 
> controllers can be specified as a module parameter 'num_controllers'. 
> The default is 1.
> 
> ex) # insmod vhci_hcd.ko num_controllers=4

I hate module parameters, please don't do that, create these dynamically
as needed some other way.


> Also, ports per controller is changed from 8 to USB_MAXCHILDREN (31). 

Why?  All in one patch?  Not good.

> It can be modified with VHCI_NPORTS flag at module compilation.

No one does that, make this dynamic as well please, in a series of
patches, not just one huge one.

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

[PATCH v3 1/2] usbip: vhci number of ports extension

[Nobuo Iwata]

This patch extends number of ports limitation in application (vhci) 
side.

To do so, vhci driver supports multiple host controllers. The number of 
controllers can be specified as a module parameter 'num_controllers'. 
The default is 1.

ex) # insmod vhci_hcd.ko num_controllers=4

Also, ports per controller is changed from 8 to USB_MAXCHILDREN (31). 
It can be modified with VHCI_NPORTS flag at module compilation.

So number of ports supported by vhci is 'num_controllers' * 31.

Sysfs structure is changes as following.
BEFORE:
/sys/devices/platform
+-- vhci
+-- status
+-- attach
+-- detach
+-- usbip_debug
AFTER: example for num_controllers=4
/sys/devices/platform
+-- vhci
|   +-- nports
|   +-- status
|   +-- status.1
|   +-- status.2
|   +-- status.3
|   +-- attach
|   +-- detach
|   +-- usbip_debug
+-- vhci.1
+-- vhci.2
+-- vhci.3

vhci[.N] is shown for each host controller kobj. vhch.1, vhci.2, ... 
are shown only when num_controllers is more than 1. Only vhci has user 
space interfaces. 'nports' is newly added to give ports-per-controller 
and number of controlles. Before that, number of ports is acquired by 
counting status lines. Status is divided for each controller to avoid 
page size (4KB) limitation.

Variable wording relating port has been corrected. 'port' represents id 
across multiple controllers. 'rhport (root hub port)' represents id 
within a controller.

Some unimportant info level messages are changed to debug level because 
they are too busy when using many ports.

Following bugs are fixed with this patch

1) The last port is ignored
tools/usb/usbip/libsrc/vhci_device.s: get_nports() ignores the last 
status line because udev_device_get_sysattr_value() drops last new 
line. New version uses nports attribute so it's doesn't have this 
problem. 

2) Status header and content inconsistency
4th and 5th column are
header:  "dev bus"
content(unused): "000 000"
content(used):   "%08x", devid
Only 1st and 2nd column are used by program. In old version, sscanf() 
in parse_status expect no bus column. And bus_id string is shown in the 
last column. Then bus in header is removed and unused content is 
replaced with 8 zeros. The sscanf() expects more than 5 columns and new 
has 6 columns. These no compatibility issue for this.

NOTE: Syslog error messages "systemd-udevd[390]: error opening USB 
device 'descriptors' file" may be shown. They are not caused by this 
patch. It seems to be a systemd problem.

Signed-off-by: Nobuo Iwata 
---
 drivers/usb/usbip/README |   3 +
 drivers/usb/usbip/vhci.h |  42 ++-
 drivers/usb/usbip/vhci_hcd.c | 268 -
 drivers/usb/usbip/vhci_rx.c  |  21 +-
 drivers/usb/usbip/vhci_sysfs.c   | 294 +++
 tools/usb/usbip/libsrc/vhci_driver.c | 541 ++-
 tools/usb/usbip/libsrc/vhci_driver.h |  38 +-
 tools/usb/usbip/src/usbip_attach.c   |   8 +-
 tools/usb/usbip/src/usbip_port.c |  13 +-
 tools/usb/usbip/src/usbipd_app.c |  49 ++-
 10 files changed, 775 insertions(+), 502 deletions(-)

diff --git a/drivers/usb/usbip/README b/drivers/usb/usbip/README
index 41a2cf2..fce3d7f 100644
--- a/drivers/usb/usbip/README
+++ b/drivers/usb/usbip/README
@@ -1,3 +1,6 @@
+MODULE PARAMS:
+   - num_controllers : number of controllers. Default is 1.
+
 TODO:
- more discussion about the protocol
- testing
diff --git a/drivers/usb/usbip/vhci.h b/drivers/usb/usbip/vhci.h
index a863a98..894c2da 100644
--- a/drivers/usb/usbip/vhci.h
+++ b/drivers/usb/usbip/vhci.h
@@ -1,5 +1,6 @@
 /*
  * Copyright (C) 2003-2008 Takahiro Hirofuchi
+ * Copyright (C) 2015 Nobuo Iwata
  *
  * This is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -72,7 +73,11 @@ struct vhci_unlink {
 };
 
 /* Number of supported ports. Value has an upperbound of USB_MAXCHILDREN */
-#define VHCI_NPORTS 8
+#ifndef VHCI_NPORTS
+#define VHCI_NPORTS USB_MAXCHILDREN
+#endif
+
+#define MAX_STATUS_NAME 16
 
 /* for usb_bus.hcpriv */
 struct vhci_hcd {
@@ -93,11 +98,16 @@ struct vhci_hcd {
struct vhci_device vdev[VHCI_NPORTS];
 };
 
-extern struct vhci_hcd *the_controller;
-extern const struct attribute_group dev_attr_group;
+extern int num_controllers;
+extern struct platform_device **the_pdevs;
+extern struct attribute_group dev_attr_group;
 
 /* vhci_hcd.c */
-void rh_port_connect(int rhport, enum usb_device_speed speed);
+void rh_port_connect(struct vhci_device *vdev, enum usb_device_speed speed);
+
+/* vhci_sysfs.c */
+int vhci_init_attr_group(void);
+void vhci_finish_attr_group(void);
 
 /* vhci_rx.c */
 struct urb *pickup_urb_and_free_priv(struct vhci_device *vdev, __u32 seqnum);
@@ -106,9 +116,14 @@ int vhci_rx_loop(void *data);
 /*