RE: [PATCH v13 05/10] usbip: exporting devices: modifications to daemon
Hello,
> > -static int do_accept(int listenfd)
> > +static int do_accept(int listenfd, char *host, char *port)
> In my opinion you should pass also the size of those arrays to this
> function instead of hardcoding them like this.
OK. I will add them.
> > - if (usbip_driver_open(driver))
> > + if (usbip_open_driver())
>
> What's the purpose of this function?
> > - usbip_driver_close(driver);
> > + usbip_close_driver();
>
> As above? Why not make this a global variable and leave those calls as
> they were?
usbip_driver_open/close - host side abstraction including stub and vudc.
usbip_open/close_driver - abstraction both host(stub/vudc) and vhci.
usbip_driver_ has widely used as function names and file names
for host side abstraction.
If they were usbip_host_, I can use usbip_driver_open/close
for both host(stub/vudc) and vhci.
> I totally don't understand why those functions exist?
> Their names are very confusing and they don't implement any additional
> functionality.
The abstraction is shared with patch 7/10.
> > - driver = &device_driver;
> > + usbip_update_driver();
>
> usbip_update_driver() is t totally unrelated to what this assignment
> really does.
>
> It changes the backend from stub driver to vudc not updating the driver.
It used to make symmetric device side and application side.
In device side, it switches to device driver.
In application side, no operation.
> usbip_dev.c is not a best name for this file. It causes confusion (at least
> to me) as I don't know to what the dev part is related, usb device, device
> (stub side) or what?
Device side.
> > + memset(&req, 0, sizeof(req));
> > + memset(&reply, 0, sizeof(reply));
>
> You don't need to 0 the reply.
This is moved from usbipd.c.
I will remove.
> > + PACK_OP_DEVLIST_REPLY(1, &reply);
> Could you please make the defines for 1 and 0 in this macro?
> As far as I understand the code it means pack and unpack but it would
> be much more readable if we have a define for this.
Here's the list in the original code.
usbip_attach.c: PACK_OP_IMPORT_REQUEST(0, &request);
usbip_attach.c: PACK_OP_IMPORT_REPLY(0, &reply);
usbip_list.c: PACK_OP_DEVLIST_REPLY(0, &reply);
usbip_network.c:PACK_OP_COMMON(1, &op_common);
usbip_network.c:PACK_OP_COMMON(0, &op_common);
usbipd.c: PACK_OP_IMPORT_REQUEST(0, &req);
usbipd.c: PACK_OP_DEVLIST_REPLY(1, &reply);
Could I fix them in a refactoring patch including memset(0) and etc
in files tot touched in this set or outside of this set?
PACK_OP_PACK 1
PACK_OP_UNPACK 0
> are we waiting here for message of size 0 or I missed something?
No. It's just moved from usbipd.c.
I will remove it.
Thank you,
n.iwata
//
> -Original Message-
> From: Krzysztof Opasiak [mailto:k.opas...@samsung.com]
> Sent: Friday, December 02, 2016 6:19 AM
> To: fx IWATA NOBUO; valentina.mane...@gmail.com; shuah...@samsung.com;
> gre...@linuxfoundation.org; linux-usb@vger.kernel.org
> Cc: fx MICHIMURA TADAO
> Subject: Re: [PATCH v13 05/10] usbip: exporting devices: modifications to
> daemon
>
>
> Hi,
>
> On 11/22/2016 07:48 AM, Nobuo Iwata wrote:
> (...)
>
> >
> > -static int do_accept(int listenfd)
> > +static int do_accept(int listenfd, char *host, char *port)
> > {
> > int connfd;
> > struct sockaddr_storage ss;
> > socklen_t len = sizeof(ss);
> > - char host[NI_MAXHOST], port[NI_MAXSERV];
> > int rc;
> >
> > memset(&ss, 0, sizeof(ss));
> > @@ -319,8 +124,8 @@ static int do_accept(int listenfd)
> > return -1;
> > }
> >
> > - rc = getnameinfo((struct sockaddr *)&ss, len, host, sizeof(host),
> > -port, sizeof(port), NI_NUMERICHOST |
> NI_NUMERICSERV);
> > + rc = getnameinfo((struct sockaddr *)&ss, len, host, NI_MAXHOST,
> > +port, NI_MAXSERV, NI_NUMERICHOST |
> NI_NUMERICSERV);
>
> You have removed the host and port variables from here and placed them in
> caller of this function, but you left here sizes of that variables.
>
> It's not very good as at some point we may change the size of host and port
> arrays and can easily forget to change those sizes here.
>
> In my opinion you should pass also the size of those arrays to this function
> instead of hardcoding them like this.
>
> > if (rc)
> > err("getnameinfo: %s", gai_strerror(rc));
> >
> > @@ -334,6 +139,9 @@ static int do_accept(int listenfd) #endif
> > info("connection from %s:%s", host, port);
> >
> > + /* should set TCP_NODELAY for usbip */
> > + usbip_net_set_nodelay(connfd);
> > +
> > return connfd;
> > }
> >
> > @@ -341,14 +149,15 @@ int process_request(int listenfd) {
> > pid_t childpid;
> > int connfd;
> > + char host[NI_MAXHOST], port[NI_MAXSERV];
> >
> > - connfd = do_accept(listenfd);
> > + connfd = do_accept(listenfd, host, port);
> > if (connfd < 0)
> > return -1;
> > childpid = fork();
> > if (
XHCI controller does not detect USB key insertion
Hello everyone,
I'm trying out a SoC with a brand new USB controller, which is (supposedly)
a standard XHCI controller. In theory, I would just need to build the right
driver, and everything would auto-magically work, right?
So my defconfig contains:
CONFIG_USB=y
CONFIG_USB_ANNOUNCE_NEW_DEVICES=y
CONFIG_USB_XHCI_HCD=y
CONFIG_USB_XHCI_PLATFORM=y
CONFIG_USB_STORAGE=y
CONFIG_USB_STORAGE_DEBUG=y
And my device tree contains:
usb3@3004 {
compatible = "generic-xhci";
reg = <0x3004 0x1>;
interrupts = ;
};
The boot messages I get:
[1.618214] xhci-hcd 3004.usb3: xHCI Host Controller
[1.623611] xhci-hcd 3004.usb3: new USB bus registered, assigned bus
number 1
[1.631181] reset function is xhci_plat_setup
[1.635588] xhci_plat_setup from usb_add_hcd
[1.640109] xhci-hcd 3004.usb3: hcc params 0x30003192 hci version 0x100
quirks 0x00010010
[1.648766] xhci-hcd 3004.usb3: irq 22, io mem 0x3004
[1.654572] xhci_plat_start from usb_add_hcd
[1.659086] usb usb1: New USB device found, idVendor=1d6b, idProduct=0002
[1.665943] usb usb1: New USB device strings: Mfr=3, Product=2,
SerialNumber=1
[1.673228] usb usb1: Product: xHCI Host Controller
[1.678154] usb usb1: Manufacturer: Linux 4.7.0-rc6 xhci-hcd
[1.683865] usb usb1: SerialNumber: 3004.usb3
[1.689391] hub 1-0:1.0: USB hub found
[1.693227] hub 1-0:1.0: 1 port detected
[1.697601] xhci-hcd 3004.usb3: xHCI Host Controller
[1.702983] xhci-hcd 3004.usb3: new USB bus registered, assigned bus
number 2
[1.710545] reset function is xhci_plat_setup
[1.714950] xhci_plat_setup from usb_add_hcd
[1.719265] xhci_plat_start from usb_add_hcd
[1.723653] usb usb2: We don't know the algorithms for LPM for this host,
disabling LPM.
[1.731956] usb usb2: New USB device found, idVendor=1d6b, idProduct=0003
[1.738814] usb usb2: New USB device strings: Mfr=3, Product=2,
SerialNumber=1
[1.746100] usb usb2: Product: xHCI Host Controller
[1.751025] usb usb2: Manufacturer: Linux 4.7.0-rc6 xhci-hcd
[1.756736] usb usb2: SerialNumber: 3004.usb3
[1.762195] hub 2-0:1.0: USB hub found
[1.766027] hub 2-0:1.0: 1 port detected
[1.770661] usbcore: registered new interface driver usb-storage
[1.784584] usbcore: registered new interface driver usbhid
[1.790213] usbhid: USB HID core driver
Which looks encouraging, right?
Am I supposed to have had USB interrupts at that point?
# cat /proc/interrupts
CPU0 CPU1 CPU2 CPU3
20:609365393356 GIC-0 29 Edge twd
21:101 0 0 0 INTC 1 Level serial
22: 0 0 0 0 INTC 67 Level
xhci-hcd:usb1
IPI0: 0 0 0 0 CPU wakeup interrupts
IPI1: 0 0 0 0 Timer broadcast interrupts
IPI2:794620 1223 1045 Rescheduling interrupts
IPI3: 0 37 37 37 Function call interrupts
IPI4: 0 0 0 0 CPU stop interrupts
IPI5: 0 0 0 0 IRQ work interrupts
IPI6: 0 0 0 0 completion interrupts
Err: 0
When I insert a USB key, nothing happens :-(
# lsusb -v
Bus 001 Device 001: ID 1d6b:0002
Bus 002 Device 001: ID 1d6b:0003
I'd like to hear suggestions about what I can tweak to fix the problem.
Regards.
--
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: [BUG 4.9] New led trigger usbport gets the kernel to panic
On Thu, 1 Dec 2016 17:56:07 +0100 Rafał Miłecki wrote: > On 12/01/2016 03:28 PM, Ralph Sennhauser wrote: > > Below the oops with your debug patch applied. > > > > (...) > > > > root@wrt1900acs:/# cd sys/class/leds/pca963x\:shelby\:white\:usb2/ > > root@wrt1900acs:/sys/devices/platform/soc/soc:internal-regs/f1011000.i2c/i2c-0/0-0068/leds/pca963x:shelby:white:usb2# > > echo usbport > trigger [ 124.727665] leds > > pca963x:shelby:white:usb2: [usbport_trig_add_port] port:dd708240 > > [ 124.735266] leds pca963x:shelby:white:usb2: > > [usbport_trig_add_port] port->port_name:usb1-port1 > > port->data:dd708140 [ 124.745671] leds pca963x:shelby:white:usb2: > > [usbport_trig_add_port] port:dd7083c0 [ 124.753194] leds > > pca963x:shelby:white:usb2: [usbport_trig_add_port] > > port->port_name:usb2-port1 port->data:dd708140 [ 124.763594] leds > > pca963x:shelby:white:usb2: [usbport_trig_add_port] port:dd708300 > > [ 124.771114] leds pca963x:shelby:white:usb2: > > [usbport_trig_add_port] port->port_name:usb3-port1 > > port->data:dd708140 > > root@wrt1900acs:/sys/devices/platform/soc/soc:internal-regs/f1011000.i2c/i2c-0/0-0068/leds/pca963x:shelby:white:usb2# > > echo 1 > ports/usb2-port1[ 171.649751] leds > > pca963x:shelby:white:usb2: [usbport_trig_port_store] buf:1 > > [ 171.649751] size:2 > > > > [ 171.660160] leds pca963x:shelby:white:usb2: > > [usbport_trig_port_store] port:dd7083c0 [ 171.668103] leds > > pca963x:shelby:white:usb2: [usbport_trig_port_store] > > port->port_name:usb2-port1 port->data:dd708140 [ 171.678678] > > [usbport_trig_update_count] usbport_data->count:0 [ 171.684457] > > [usbport_trig_update_count] usbport_data->count:0 [ 171.690253] > > Unable to handle kernel NULL pointer dereference at virtual address > > > > Oh, so this happens a bit later than I expected or I could read from > the backtrace. Anyway this debugging was still helpful, I think I can > see a possible problem. > > So most likely the crash happens at the: > led_cdev->brightness_set(led_cdev, ...); > call. I'm not sure what I was thinking when writing that code. It > looks wrong. > > The thing is some LEDs (drivers) don't provide brightness_set op. > It's fine, we should just use brightness_set_blocking op when that > happens. Of course kernel has proper helpers for that, we don't have > to worry about the choice of op or scheduling the work. I have no > idea why I didn't use a proper helper in the first place. > > So we should simply replace above call with one of following ones: > 1) led_set_brightness(led_cdev, ...); > 2) led_set_brightness_nosleep(led_cdev, ...); > 3) led_set_brightness_sync(led_cdev, ...); > > I still have to check which one is correct. In theory we don't deal > blinking at this point so we shouldn't need to use led_set_brightness. > > led_set_brightness_nosleep looks like the most likely correct choice. > > led_set_brightness_sync requires brightness_set_blocking which is not > always present so most likely we don't want this one. > > > If you have some free time and you want to play with this, please > replace led_cdev->brightness_set > with > led_set_brightness_nosleep > and give it a try. This should fix crashes for you. > > I'll look at this again during next days. Hi Rafał I just gave 2) led_set_brightness_nosleep a try. The kernel no longer crashes and the led does work as expected. Thanks Ralph -- 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: XHCI controller does not detect USB key insertion
Hi,
Mason writes:
> Hello everyone,
>
> I'm trying out a SoC with a brand new USB controller, which is (supposedly)
> a standard XHCI controller. In theory, I would just need to build the right
> driver, and everything would auto-magically work, right?
perhaps, but there might be needed initialization of other resources
like PHYs and stuff like that.
> So my defconfig contains:
>
> CONFIG_USB=y
> CONFIG_USB_ANNOUNCE_NEW_DEVICES=y
> CONFIG_USB_XHCI_HCD=y
> CONFIG_USB_XHCI_PLATFORM=y
> CONFIG_USB_STORAGE=y
> CONFIG_USB_STORAGE_DEBUG=y
>
>
> And my device tree contains:
>
> usb3@3004 {
> compatible = "generic-xhci";
> reg = <0x3004 0x1>;
> interrupts = ;
> };
>
>
> The boot messages I get:
>
> [1.618214] xhci-hcd 3004.usb3: xHCI Host Controller
> [1.623611] xhci-hcd 3004.usb3: new USB bus registered, assigned bus
> number 1
> [1.631181] reset function is xhci_plat_setup
> [1.635588] xhci_plat_setup from usb_add_hcd
> [1.640109] xhci-hcd 3004.usb3: hcc params 0x30003192 hci version
> 0x100 quirks 0x00010010
> [1.648766] xhci-hcd 3004.usb3: irq 22, io mem 0x3004
> [1.654572] xhci_plat_start from usb_add_hcd
> [1.659086] usb usb1: New USB device found, idVendor=1d6b, idProduct=0002
> [1.665943] usb usb1: New USB device strings: Mfr=3, Product=2,
> SerialNumber=1
> [1.673228] usb usb1: Product: xHCI Host Controller
> [1.678154] usb usb1: Manufacturer: Linux 4.7.0-rc6 xhci-hcd
> [1.683865] usb usb1: SerialNumber: 3004.usb3
> [1.689391] hub 1-0:1.0: USB hub found
> [1.693227] hub 1-0:1.0: 1 port detected
> [1.697601] xhci-hcd 3004.usb3: xHCI Host Controller
> [1.702983] xhci-hcd 3004.usb3: new USB bus registered, assigned bus
> number 2
> [1.710545] reset function is xhci_plat_setup
> [1.714950] xhci_plat_setup from usb_add_hcd
> [1.719265] xhci_plat_start from usb_add_hcd
> [1.723653] usb usb2: We don't know the algorithms for LPM for this host,
> disabling LPM.
> [1.731956] usb usb2: New USB device found, idVendor=1d6b, idProduct=0003
> [1.738814] usb usb2: New USB device strings: Mfr=3, Product=2,
> SerialNumber=1
> [1.746100] usb usb2: Product: xHCI Host Controller
> [1.751025] usb usb2: Manufacturer: Linux 4.7.0-rc6 xhci-hcd
> [1.756736] usb usb2: SerialNumber: 3004.usb3
> [1.762195] hub 2-0:1.0: USB hub found
> [1.766027] hub 2-0:1.0: 1 port detected
> [1.770661] usbcore: registered new interface driver usb-storage
> [1.784584] usbcore: registered new interface driver usbhid
> [1.790213] usbhid: USB HID core driver
>
> Which looks encouraging, right?
yes
> Am I supposed to have had USB interrupts at that point?
nope, unless a device was already plugged in.
> # cat /proc/interrupts
>CPU0 CPU1 CPU2 CPU3
> 20:609365393356 GIC-0 29 Edge twd
> 21:101 0 0 0 INTC 1 Level
> serial
> 22: 0 0 0 0 INTC 67 Level
> xhci-hcd:usb1
> IPI0: 0 0 0 0 CPU wakeup interrupts
> IPI1: 0 0 0 0 Timer broadcast interrupts
> IPI2:794620 1223 1045 Rescheduling interrupts
> IPI3: 0 37 37 37 Function call interrupts
> IPI4: 0 0 0 0 CPU stop interrupts
> IPI5: 0 0 0 0 IRQ work interrupts
> IPI6: 0 0 0 0 completion interrupts
> Err: 0
>
>
> When I insert a USB key, nothing happens :-(
>
> # lsusb -v
> Bus 001 Device 001: ID 1d6b:0002
> Bus 002 Device 001: ID 1d6b:0003
>
>
> I'd like to hear suggestions about what I can tweak to fix the problem.
go to your documentation and see if you have initialized
everything. Which SoC is this?
--
balbi
--
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 06/12] usb: dwc3: omap: Replace the extcon API
Hi, Chanwoo Choi writes: > Hi Felipe, > > On 2016년 11월 30일 19:36, Felipe Balbi wrote: >> >> Hi, >> >> Chanwoo Choi writes: >>> This patch uses the resource-managed extcon API for >>> extcon_register_notifier() >>> and replaces the deprecated extcon API as following: >>> - extcon_get_cable_state_() -> extcon_get_state() >>> >>> Signed-off-by: Chanwoo Choi >> >> Acked-by: Felipe Balbi >> > > Thanks for your review. > > Each patch has no any dependency among patches. > So, If possible, could you pick the patch6/8/9/10/11/12 on your tree? my tree is closed for v4.10, I can pick it up for v4.11 -- balbi -- 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: RE: [PATCH] usb: chipidea: use better pattern with WARN_ON()
On 2 Dec 2016 2:17 p.m., "Atul Raj" wrote:
>
>
> >
> >Instead of using:
> >if (cond) {
> > WARN_ON(1);
> > do_stuff();
> >}
> >
> >Use a better pattern with WARN_ON() placed in if condition:
> >
> >if (WARN_ON(cond))
> > do_stuff();
> >
> >Signed-off-by: Atul Raj
> >---
> > drivers/usb/chipidea/ci_hdrc_imx.c | 8 ++--
> > 1 file changed, 2 insertions(+), 6 deletions(-)
> >
> >diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c
> >b/drivers/usb/chipidea/ci_hdrc_imx.c
> >index 0991794..2228e44 100644
> >--- a/drivers/usb/chipidea/ci_hdrc_imx.c
> >+++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> >@@ -374,10 +374,8 @@ static int imx_controller_resume(struct device *dev)
> >
> > dev_dbg(dev, "at %s\n", __func__);
> >
> >-if (!data->in_lpm) {
> >-WARN_ON(1);
> >+if (WARN_ON(!data->in_lpm))
> > return 0;
> >-}
> >
> > ret = imx_prepare_enable_clks(dev);
> > if (ret)
> >@@ -442,10 +440,8 @@ static int ci_hdrc_imx_runtime_suspend(struct device
> >*dev)
> > struct ci_hdrc_imx_data *data = dev_get_drvdata(dev);
> > int ret;
> >
> >-if (data->in_lpm) {
> >-WARN_ON(1);
> >+if (WARN_ON(data->in_lpm))
> > return 0;
> >-}
> >
> > ret = imx_usbmisc_set_wakeup(data->usbmisc_data, true);
> > if (ret) {
> >--
>
> please cc: linux-usb@vger.kernel.org
>
Dear Peter
As I am a newbie, I have been instructed by greg k-h to first try to submit
patches in staging drivers. So I am trying to submit some patches there. I had
sent you the patch to review before greg asked me to do so.
Maybe the patch I sent could be base 64-encoded as per samsung mailing policy.
From now onwards I will use gmail(atulraj.n...@gmail.com) till I get exception
from encoding.
I am sorry for all the trouble.
I am cc: ing the patch to email address as asked by you.
Thanks
RE: RE: [PATCH] usb: chipidea: use better pattern with WARN_ON()
On 2 Dec 2016 2:18 p.m., "Atul Raj" wrote:
>
>
>
> >Instead of using:
> >if (cond) {
> > WARN_ON(1);
> > do_stuff();
> >}
> >
> >Use a better pattern with WARN_ON() placed in if condition:
> >
> >if (WARN_ON(cond))
> > do_stuff();
> >
> >Signed-off-by: Atul Raj
> >---
> > drivers/usb/chipidea/core.c | 12 +++-
> > 1 file changed, 3 insertions(+), 9 deletions(-)
> >
> >diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c index
> >3dbb4a2..1a43689 100644
> >--- a/drivers/usb/chipidea/core.c
> >+++ b/drivers/usb/chipidea/core.c
> >@@ -1128,10 +1128,8 @@ static int ci_controller_resume(struct device *dev)
> >
> > dev_dbg(dev, "at %s\n", __func__);
> >
> >-if (!ci->in_lpm) {
> >-WARN_ON(1);
> >+if (WARN_ON(!ci->in_lpm))
> > return 0;
> >-}
> >
> > ci_hdrc_enter_lpm(ci, false);
> > if (ci->usb_phy) {
> >@@ -1169,10 +1167,8 @@ static int ci_suspend(struct device *dev)
> > if (ci->in_lpm)
> > pm_runtime_resume(dev);
> >
> >-if (ci->in_lpm) {
> >-WARN_ON(1);
> >+if (WARN_ON(ci->in_lpm))
> > return 0;
> >-}
> >
> > if (device_may_wakeup(dev)) {
> > if (ci_otg_is_fsm_mode(ci))
> >@@ -1215,10 +1211,8 @@ static int ci_runtime_suspend(struct device *dev)
> >
> > dev_dbg(dev, "at %s\n", __func__);
> >
> >-if (ci->in_lpm) {
> >-WARN_ON(1);
> >+if (WARN_ON(ci->in_lpm))
> > return 0;
> >-}
> >
> > if (ci_otg_is_fsm_mode(ci))
> > ci_otg_fsm_suspend_for_srp(ci);
> >--
>
> please cc: linux-usb@vger.kernel.org
>
Dear Peter
As I am a newbie, I have been instructed by greg k-h to first try to submit
patches in staging drivers. So I am trying to submit some patches there. I had
sent you the patch to review before greg asked me to do so.
Maybe the patch I sent could be base64-encoded as per samsung mailing policy.
From now onwards I will use gmail(atulraj.n...@gmail.com) till I get exception
from encoding.
I am sorry for all the trouble.
I am cc:ing the patch to email address as asked by you.
Thanks
RE: [PATCH v13 00/10] usbip: exporting devices
Hello, > I am not clear on this. Is Client and server behind different > firewalls? No. The firewall is in between internet and > So the above doesn't apply to a) and it works? Yes. > What does this mean? Why is this connection from outside? 'usbip attach -r' tries to establish connection. > Is this something that could be solved by opening ports in > the firewall? Usually, http(80) and https(443) from inside is opened for internet Access. > Can we use server and client terminology which is we use in instead of > App and Dev. In https://www.kernel.org/doc/readme/tools-usb-usbip-README, I couldn't find the definition. Do you mean 'server' is a linux machine which has devices or linux machine which runs server daemon? In existing way, they are same. In new way, they are not. The linux machine which has device doesn't have server daemon. I'd like to confirm the definition to answer rest of questions. Best Regards, n.iwata // > -Original Message- > From: Shuah Khan [mailto:shuah...@samsung.com] > Sent: Friday, December 02, 2016 9:39 AM > To: fx IWATA NOBUO; valentina.mane...@gmail.com; > gre...@linuxfoundation.org; linux-usb@vger.kernel.org > Cc: fx MICHIMURA TADAO; Shuah Khan > Subject: Re: [PATCH v13 00/10] usbip: exporting devices > > On 11/23/2016 10:01 PM, fx IWATA NOBUO wrote: > > Hello, > > > Looks like you removed the text. Let's go back to the goals. > Can we use server and client terminology which is we use in instead of App > and Dev. It makes lot easier to understand. > > https://www.kernel.org/doc/readme/tools-usb-usbip-README > > >>1. Goal > > >>1-1) To give flexibility to direction of connection Using USB/IP in > >>internet, there can be two cases. > >> a) an application is inside firewall and devices are outside. > > This is the case Client is behind firewall and server is outside the > firewall. > > >>b) devices are inside firewall and an application is inside. > > I am not clear on this. Is Client and server behind different firewalls? > > >>In case-a, import works because the connection is from inside. > > > EXISTING) > > APP# usbip attach ---(passed)> DEV# usbipd > > DEV# usbipd (blocked)|| <- APP# usbip attach > > So the above doesn't apply to a) and it works? > > >> In case-b, import doesn't works because the connection is from outside. > > What does this mean? Why is this connection from outside? > > >> Connection from device side is needed. This patch set adds the > >> direction of connection establishment. > > Is this something that could be solved by opening ports in the firewall? > > > >> Can you elaborate on the use-case a bit more? What does it mean to > >> "Connection from device side is needed"? > > > > I'd like to update ending part of use case as following. > > --- > > Firewall, proxy, or router in front of internet usually blocks > > connections from internet regarding all TCP ports. They opens some > > ports, usually HTTP(80) and HTTPS(443), for connection from inside. > > In combination with WebSocket proxy, USB/IP can establish connection > > from inside of the firewall. > > > > Enterprise/SOHO/Home Firewall/Proxy/Router Internet > > > > EXISTING) > > APP# usbip attach ---(passed)> DEV# usbipd > > DEV# usbipd (blocked)|| <- APP# usbip attach > > > > NEW) > > DEV# usbip connect --(passed)> DEV# usbipa > > APP# usbipa (blocked)|| <- APP# usbip connect > > > > Attach operation can invite devices in internet but cannot invite > > devices from internet. On the other hand, connect operation can > > dedicate devices to internet but cannot dedicate devices in internet. > > The above isn't very clear. Do you mean to say, client can find devices > exported by a server? When you say Internet how many servers are we looking > at? Random servers or known servers? > > Current USB/IP use-cases are: > > - Server could export USB devices to virtual machines running on it. > Access is confined and limited to that one system. > - Server could export USB devices and clients as long as no firewalls > in between. > > In the above two models, user has permissions to use server and client. > In this new proposed model, how does this work? Who sets up the server to > export or push exports to clients. How does server know which clients to > push exports to? > > With a feature like this, it is important to understand the use-cases in > detail. > > thanks, > -- Shuah > > > --- > > > >> I would like to see server side and client side operations clearly. > >> It would help me understand the use-case you are trying to add. > > > > It's shown in README and manual page added by patch 9/10 as below. > > > > app:# insmod usbip-core.ko > > app:# insmod vhci-hcd.ko > > > > app:# usbipa -D > > - Start usbip daemon. > > > > dev:# (Physically attach your USB device.) > > > > dev:# insmod usbip-core.ko > > dev:# insmod u
Re: [PATCH 22/25] usb: host: xhci: remove unnecessary list_for_each_entry_safe()
Hello! On 12/1/2016 4:31 PM, Felipe Balbi wrote: the _save() version of list iterators are supposed to be used when _safe(). list_entry is going to be removed from the list while iterating. Since xhci_handle_stopped_cmd_ring() is not removing anything from the list, just converting commands into No-Op TRBs, we don't need to use the safe version. Signed-off-by: Felipe Balbi --- drivers/usb/host/xhci-ring.c | 21 ++--- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index 50244dee6b43..ebb52ffab805 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c [...] @@ -1250,7 +1250,6 @@ static void xhci_handle_stopped_cmd_ring(struct xhci_hcd *xhci, mod_timer(&xhci->cmd_timer, jiffies + XHCI_CMD_DEFAULT_TIMEOUT); xhci_ring_cmd_db(xhci); } - return; Not mentioned in the patch description? } MBR, Sergei -- 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: XHCI controller does not detect USB key insertion
On 02/12/2016 10:03, Felipe Balbi wrote:
> Mason wrote:
>
>> I'm trying out a SoC with a brand new USB controller, which is (supposedly)
>> a standard XHCI controller. In theory, I would just need to build the right
>> driver, and everything would auto-magically work, right?
>
> perhaps, but there might be needed initialization of other resources
> like PHYs and stuff like that.
Let me dive into additional details...
First of all, there is a register aptly called "USB3_RESET" which
is used to release several USB3-related blocks from reset.
Of course, that's the first register I tweaked :-)
There are *3* address ranges with USB3-related registers.
1) one called host_usb30_xhcl (I believe "xhcl" is a typo for "xhci")
This is the address I passed to the Linux driver. The first register
is CAPLENGTH_VERSION. I assume these are the standard XHCI registers.
(Last register is XHCL_EXTENDED_CAP3_USB3 at offset 0xc008)
2) one called host_usb30_port
This contains "Device and Port Specific Registers".
Is it standard?
How is Linux supposed to know where to find it?
Contains registers such as
Device Transaction Status
Device UTMI command and status for USB2
Set ISOC Delay
USB3 Function Notification
Rx DMA BD Start Address for Control Endpoint
EP Burst Size
Tx DMA BD Start Address Control Endpoint
EP $N IN/OUT
Device Notification Register
EP_Isochronous Timestamp
Are registers named LTSSM_TIMER_REGISTER{1,2,3} standard?
they have fields such as reg_12_ms_timeout (and other numbers like 2, 6, 100,
300)
3) one called host_usb30
This contains lower-level stuff
0x2e800 CONFIG
0x2e804 CONTROL
0x2e808 TEST
0x2e80c STATUS
0x2e810 CLK_RST_0
0x2e814 CLK_RST_1
0x2e818 PARAM_0
0x2e81c PARAM_1
0x2e820 PARAM_2
0x2e880 SNPS_CR_ADD
0x2e884 SNPS_CR_DATA
0x2e8c0 RESET_CTRL
I haven't touched any of these so far.
>> # lsusb -v
>> Bus 001 Device 001: ID 1d6b:0002
>> Bus 002 Device 001: ID 1d6b:0003
Isn't lsusb verbose supposed to print much more than that?
>> I'd like to hear suggestions about what I can tweak to fix the problem.
>
> go to your documentation and see if you have initialized
> everything. Which SoC is this?
(Sad face) All the documentation I have is in front of me, and nothing
is ringing a bell. This is a Sigma Designs SoC, with a Pravega XHCI
controller + Synopsys PHY.
The documentation I have:
Pravega_Dual_Mode_Datasheet_v10c.pdf (documents IP signals)
Pravega_Dual_Mode_Controller_Programmers_Reference_manual_v1.pdf (documents IP
registers)
PHY databook (very low-level stuff)
SoC register mapping (for how the SoC maps the IP signals to registers)
So far, I'm stumped :-(
Regards.
--
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] usb: gadget: uvc: fix UVC_ATTR macro for UVCG_OPTS_ATTR
Hello!
On 12/2/2016 9:22 AM, Jassi Brar wrote:
From: Jassi Brar
Typo in commit 76e0da34c7cec5a7d introduced a bug that prevents
scripts/checkpatch.pl didn't complain about the commit citing style?
You forgot to specify the summary line enclosed in ("").
creation of streaming_{interval,maxpacket,maxburst} nodes for
invalid 'aname' node.
Signed-off-by: Jassi Brar
[...]
MBR, Sergei
--
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: XHCI controller does not detect USB key insertion
On Fri, Dec 02, 2016 at 11:24:05AM +0100, Mason wrote: > >> # lsusb -v > >> Bus 001 Device 001: ID 1d6b:0002 > >> Bus 002 Device 001: ID 1d6b:0003 > > Isn't lsusb verbose supposed to print much more than that? Yes, if you are using the usbutils version of 'lsusb', odds are this is busybox, right? And these are just the root hubs, that the USB controller driver creates as "virtual" USB devices, they are not "real" USB devices on your bus. hope this helps, 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
Re: [PATCH 01/25] usb: host: xhci: dynamically allocate devs array
On 01.12.2016 15:30, Felipe Balbi wrote: Instead of always defaulting to a 256-entry array, we can dynamically allocate devs based on what HW tells us it supports. Note that we can't, yet, purge MAX_HC_SLOTS completely because of struct xhci_device_context_array reliance on it. Signed-off-by: Felipe Balbi --- drivers/usb/host/xhci-hub.c | 2 +- drivers/usb/host/xhci-mem.c | 4 ++-- drivers/usb/host/xhci-ring.c | 2 +- drivers/usb/host/xhci.c | 19 +++ drivers/usb/host/xhci.h | 2 +- 5 files changed, 20 insertions(+), 9 deletions(-) ... diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 1cd56417cbec..1113b5fea7b4 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -766,6 +766,8 @@ void xhci_shutdown(struct usb_hcd *hcd) /* Yet another workaround for spurious wakeups at shutdown with HSW */ if (xhci->quirks & XHCI_SPURIOUS_WAKEUP) pci_set_power_state(to_pci_dev(hcd->self.controller), PCI_D3hot); + + kfree(xhci->devs); I don't think freeing xhci->devs at PCI .shutdown callback will work for driver unload, or for the PCI hotplugged Alpine Ridge xhci solution. xhci->devs would leak at next driver load. xhci_mem_clenup() would be better } #ifdef CONFIG_PM @@ -4896,6 +4898,11 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks) xhci->quirks |= quirks; + xhci->devs = kcalloc(HCS_MAX_SLOTS(xhci->hcs_params1) + 1, + sizeof(struct xhci_virt_device *), GFP_KERNEL); maybe allocate the memory in xhci_mem_init(), it's done a bit later in xhci_gen_setup() from xhci_init(). we halt, reset and set dma mask in between, but those should not depend on xhci->devs in any way. -Mathias -- 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: XHCI controller does not detect USB key insertion
On 02/12/2016 11:42, Greg KH wrote: > On Fri, Dec 02, 2016 at 11:24:05AM +0100, Mason wrote: > >> # lsusb -v >> Bus 001 Device 001: ID 1d6b:0002 >> Bus 002 Device 001: ID 1d6b:0003 >> >> Isn't lsusb verbose supposed to print much more than that? > > Yes, if you are using the usbutils version of 'lsusb', odds are this is > busybox, right? Right. (You win a digital cookie.) cd buildroot && make menuconfig Drop BR2_PACKAGE_USBMOUNT (maybe it causes unexpected issues) Add BR2_PACKAGE_USBUTILS (I want the real deal) > And these are just the root hubs, that the USB controller driver creates > as "virtual" USB devices, they are not "real" USB devices on your bus. # lsusb --version lsusb (usbutils) 007 I see there's a 008 version. Am I missing out on important diagnostics? # lsusb -v Bus 002 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub Device Descriptor: bLength18 bDescriptorType 1 bcdUSB 3.00 bDeviceClass9 Hub bDeviceSubClass 0 Unused bDeviceProtocol 3 bMaxPacketSize0 9 idVendor 0x1d6b Linux Foundation idProduct 0x0003 3.0 root hub bcdDevice4.07 iManufacturer 3 Linux 4.7.0-rc6 xhci-hcd iProduct2 xHCI Host Controller iSerial 1 3004.usb3 bNumConfigurations 1 Configuration Descriptor: bLength 9 bDescriptorType 2 wTotalLength 31 bNumInterfaces 1 bConfigurationValue 1 iConfiguration 0 bmAttributes 0xe0 Self Powered Remote Wakeup MaxPower0mA Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber0 bAlternateSetting 0 bNumEndpoints 1 bInterfaceClass 9 Hub bInterfaceSubClass 0 Unused bInterfaceProtocol 0 Full speed (or root) hub iInterface 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x81 EP 1 IN bmAttributes3 Transfer TypeInterrupt Synch Type None Usage Type Data wMaxPacketSize 0x0004 1x 4 bytes bInterval 12 bMaxBurst 0 Hub Descriptor: bLength 12 bDescriptorType 42 nNbrPorts 1 wHubCharacteristic 0x000a No power switching (usb 1.0) Per-port overcurrent protection bPwrOn2PwrGood 10 * 2 milli seconds bHubContrCurrent 0 milli Ampere bHubDecLat 0.0 micro seconds wHubDelay 0 nano seconds DeviceRemovable0x00 Hub Port Status: Port 1: .02a0 5Gbps power Rx.Detect Binary Object Store Descriptor: bLength 5 bDescriptorType15 wTotalLength 15 bNumDeviceCaps 1 SuperSpeed USB Device Capability: bLength10 bDescriptorType16 bDevCapabilityType 3 bmAttributes 0x00 wSpeedsSupported 0x0008 Device can operate at SuperSpeed (5Gbps) bFunctionalitySupport 3 Lowest fully-functional device speed is SuperSpeed (5Gbps) bU1DevExitLat 0 micro seconds bU2DevExitLat 0 micro seconds Device Status: 0x0001 Self Powered Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub Device Descriptor: bLength18 bDescriptorType 1 bcdUSB 2.00 bDeviceClass9 Hub bDeviceSubClass 0 Unused bDeviceProtocol 1 Single TT bMaxPacketSize064 idVendor 0x1d6b Linux Foundation idProduct 0x0002 2.0 root hub bcdDevice4.07 iManufacturer 3 Linux 4.7.0-rc6 xhci-hcd iProduct2 xHCI Host Controller iSerial 1 3004.usb3 bNumConfigurations 1 Configuration Descriptor: bLength 9 bDescriptorType 2 wTotalLength 25 bNumInterfaces 1 bConfigurationValue 1 iConfiguration 0 bmAttributes 0xe0 Self Powered Remote Wakeup MaxPower0mA Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber0 bAlternateSetting 0 bNumEndpoints 1 bInterfaceClass 9 Hub bInterfaceSubClass 0 Unused bInterfaceProtocol 0 Full speed (or root) hub iInterface 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x81 EP 1 IN bmAttributes3 Transfer TypeInterrupt Synch Type None
Re: XHCI controller does not detect USB key insertion
On Fri, Dec 02, 2016 at 12:08:21PM +0100, Mason wrote:
> On 02/12/2016 11:42, Greg KH wrote:
>
> > On Fri, Dec 02, 2016 at 11:24:05AM +0100, Mason wrote:
> >
> >> # lsusb -v
> >> Bus 001 Device 001: ID 1d6b:0002
> >> Bus 002 Device 001: ID 1d6b:0003
> >>
> >> Isn't lsusb verbose supposed to print much more than that?
> >
> > Yes, if you are using the usbutils version of 'lsusb', odds are this is
> > busybox, right?
>
> Right. (You win a digital cookie.)
Yeah!
{munch munch}
> cd buildroot && make menuconfig
> Drop BR2_PACKAGE_USBMOUNT (maybe it causes unexpected issues)
> Add BR2_PACKAGE_USBUTILS (I want the real deal)
>
> > And these are just the root hubs, that the USB controller driver creates
> > as "virtual" USB devices, they are not "real" USB devices on your bus.
>
> # lsusb --version
> lsusb (usbutils) 007
> I see there's a 008 version.
Wow, 008 was released in 2014, what type of old repo are you using that
has 007 as the "latest"?
And I really should go do a new update, lots of bug fixes have happened
since 2014...
> Am I missing out on important diagnostics?
Not really, if you are not doing a lot of USB 3-only device specific
work, which is where the majority of the changes have happened in the
past 2 years in the tool.
>
> # lsusb -v
>
> Bus 002 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
> Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> Does everything look normal?
Yes, two internal "virtual" hubs.
> Or are there any investigation-worthy nuggets?
You need to figure out why the driver isn't getting interrupts, that's
the main problem for your hardware at the moment, lsusb isn't going to
help you out at all with that...
good luck!
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] usb: host: xhci: Handle the right timeout command
If a command event is found on the event ring during an interrupt,
we need to stop the command timer with del_timer(). Since del_timer()
can fail if the timer is running and waiting on the xHCI lock, then
it maybe get the wrong timeout command in xhci_handle_command_timeout()
if host fetched a new command and updated the xhci->current_cmd in
handle_cmd_completion(). For this situation, we need a way to signal
to the command timer that everything is fine and it should exit.
We should introduce a counter (xhci->current_cmd_pending) for the number
of pending commands. If we need to cancel the command timer and del_timer()
succeeds, we decrement the number of pending commands. If del_timer() fails,
we leave the number of pending commands alone.
For handling timeout command, in xhci_handle_command_timeout() we will check
the counter after decrementing it, if the counter (xhci->current_cmd_pending)
is 0, which means xhci->current_cmd is the right timeout command. If the
counter (xhci->current_cmd_pending) is greater than 0, which means current
timeout command has been handled by host and host has fetched new command
as xhci->current_cmd, then just return and wait for new current command.
Signed-off-by: Baolin Wang
---
This patch is based on Lu Baolu's new fix patch:
usb: xhci: fix possible wild pointer
---
drivers/usb/host/xhci-ring.c | 26 +-
drivers/usb/host/xhci.h |1 +
2 files changed, 26 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 62dd1c7..a62904e 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1253,6 +1253,7 @@ static void xhci_handle_stopped_cmd_ring(struct xhci_hcd
*xhci,
if ((xhci->cmd_ring->dequeue != xhci->cmd_ring->enqueue) &&
!(xhci->xhc_state & XHCI_STATE_DYING)) {
xhci->current_cmd = cur_cmd;
+ xhci->current_cmd_pending++;
mod_timer(&xhci->cmd_timer, jiffies + XHCI_CMD_DEFAULT_TIMEOUT);
xhci_ring_cmd_db(xhci);
}
@@ -1274,6 +1275,21 @@ void xhci_handle_command_timeout(unsigned long data)
return;
}
+ xhci->current_cmd_pending--;
+ /*
+* If the current_cmd_pending is 0, which means current command is
+* timeout.
+*
+* If the current_cmd_pending is greater than 0, which means current
+* timeout command has been handled by host and host has fetched new
+* command as xhci->current_cmd, then just return and wait for new
+* current command.
+*/
+ if (xhci->current_cmd_pending > 0) {
+ spin_unlock_irqrestore(&xhci->lock, flags);
+ return;
+ }
+
if (xhci->current_cmd->status == COMP_CMD_ABORT)
second_timeout = true;
xhci->current_cmd->status = COMP_CMD_ABORT;
@@ -1336,7 +1352,13 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
cmd = list_entry(xhci->cmd_list.next, struct xhci_command, cmd_list);
- del_timer(&xhci->cmd_timer);
+ /*
+* If the command timer is running on another CPU, we don't decrement
+* current_cmd_pending, since we didn't successfully stop the command
+* timer.
+*/
+ if (del_timer(&xhci->cmd_timer))
+ xhci->current_cmd_pending--;
trace_xhci_cmd_completion(cmd_trb, (struct xhci_generic_trb *) event);
@@ -1424,6 +1446,7 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
if (cmd->cmd_list.next != &xhci->cmd_list) {
xhci->current_cmd = list_entry(cmd->cmd_list.next,
struct xhci_command, cmd_list);
+ xhci->current_cmd_pending++;
mod_timer(&xhci->cmd_timer, jiffies + XHCI_CMD_DEFAULT_TIMEOUT);
} else if (xhci->current_cmd == cmd) {
xhci->current_cmd = NULL;
@@ -3924,6 +3947,7 @@ static int queue_command(struct xhci_hcd *xhci, struct
xhci_command *cmd,
if (xhci->cmd_list.next == &cmd->cmd_list &&
!timer_pending(&xhci->cmd_timer)) {
xhci->current_cmd = cmd;
+ xhci->current_cmd_pending++;
mod_timer(&xhci->cmd_timer, jiffies + XHCI_CMD_DEFAULT_TIMEOUT);
}
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 9dbaacf..5d81257 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1567,6 +1567,7 @@ struct xhci_hcd {
unsigned intcmd_ring_reserved_trbs;
struct timer_list cmd_timer;
struct xhci_command *current_cmd;
+ u32 current_cmd_pending;
struct xhci_ring*event_ring;
struct xhci_ersterst;
/* Scratchpad */
--
1.7.9.5
--
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
Re: [PATCH 22/25] usb: host: xhci: remove unnecessary list_for_each_entry_safe()
Hi Felipe, On 2 December 2016 at 18:15, Sergei Shtylyov wrote: > Hello! > > On 12/1/2016 4:31 PM, Felipe Balbi wrote: > >> the _save() version of list iterators are supposed to be used when > > >_safe(). > >> list_entry is going to be removed from the list while iterating. Since >> xhci_handle_stopped_cmd_ring() is not removing anything from the list, >> just converting commands into No-Op TRBs, we don't need to use the safe >> version. >> >> Signed-off-by: Felipe Balbi >> --- >> drivers/usb/host/xhci-ring.c | 21 ++--- >> 1 file changed, 10 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c >> index 50244dee6b43..ebb52ffab805 100644 >> --- a/drivers/usb/host/xhci-ring.c >> +++ b/drivers/usb/host/xhci-ring.c > > [...] >> >> @@ -1250,7 +1250,6 @@ static void xhci_handle_stopped_cmd_ring(struct >> xhci_hcd *xhci, >> mod_timer(&xhci->cmd_timer, jiffies + >> XHCI_CMD_DEFAULT_TIMEOUT); >> xhci_ring_cmd_db(xhci); >> } >> - return; > > >Not mentioned in the patch description? Just a note, this has been fixed by my previous patch: https://lkml.org/lkml/2016/11/24/219 -- Baolin.wang Best Regards -- 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 24/25] usb: host: xhci: use trb_to_noop() from xhci_handle_stopped_cmd_ring()
On 01.12.2016 15:31, Felipe Balbi wrote:
instead of open coding how to convert a TRB to no-op, let's use our
newly introduced helper.
Signed-off-by: Felipe Balbi
---
drivers/usb/host/xhci-ring.c | 12 ++--
1 file changed, 2 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 80fa3dbdbdd8..973182ee6954 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1218,7 +1218,6 @@ static void xhci_handle_stopped_cmd_ring(struct xhci_hcd
*xhci,
struct xhci_command *cur_cmd)
{
struct xhci_command *cmd;
- u32 cycle_state;
/* Turn all aborted commands in list to no-ops, then restart */
list_for_each_entry(cmd, &xhci->cmd_list,
@@ -1231,15 +1230,8 @@ static void xhci_handle_stopped_cmd_ring(struct xhci_hcd
*xhci,
xhci_dbg(xhci, "Turn aborted command %p to no-op\n",
cmd->command_trb);
- /* get cycle state from the original cmd trb */
- cycle_state = le32_to_cpu(
- cmd->command_trb->generic.field[3]) & TRB_CYCLE;
- /* modify the command trb to no-op command */
- cmd->command_trb->generic.field[0] = 0;
- cmd->command_trb->generic.field[1] = 0;
- cmd->command_trb->generic.field[2] = 0;
- cmd->command_trb->generic.field[3] = cpu_to_le32(
- TRB_TYPE(TRB_CMD_NOOP) | cycle_state);
+
+ trb_to_noop(cmd->command_trb);
This won't work, notice that no ops for transfer and command rings are
different.
The trb_to_noop() helper you added in 23/25 set trb type to TRB_TR_NOOP
/* Transfer Ring No-op (not for the command ring) */
#define TRB_TR_NOOP 8
/* No-op Command - not for transfer rings */
#define TRB_CMD_NOOP23
-Mathias
--
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] usb: gadget: udc: core: fix return code of usb_gadget_probe_driver()
On 12/01/2016 10:44 PM, Felix Hädicke wrote:
> Hi,
>
>> Hi,
>>
>> Good catch but I have a few comments to commit message:
>>
>> On 12/01/2016 12:24 AM, Felix Hädicke wrote:
>>> This fixes a regression which was introduced by commit f1bddbb, by
>>> reverting a small fragment of commit 855ed04.
>>>
>> Not exactly. The regression has been introduced by commit 855ed04
>> itself.This commit replaced previous construction similar what you sent
>> now with simple if():
>>
>> @@ -535,11 +556,7 @@ int usb_gadget_probe_driver(struct
>> usb_gadget_driver *driver)
>> if (!ret)
>> break;
>> }
>> - if (ret)
>> - ret = -ENODEV;
>> - else if (udc->driver)
>> - ret = -EBUSY;
>> - else
>> + if (!ret && !udc->driver)
>> goto found;
>> } else {
>> list_for_each_entry(udc, &udc_list, list) {
>
> The regression bug I want to fix with this patch was introduced with
> commit f1bddbb, not 855ed04. With 855ed04, the this if/else construct
> was changed. But concerning the usb_gadget_probe_driver() return code,
> this was ok.
Nope. Return code was 0 and gadget has not been bound to any udc but
added to pending list. Consider the following sequence:
echo "udc" > g1/UDC
echo "udc" > g2/UDC
echo "" > g1/UDC
The expected result is that second line should fail with EBUSY. And
without f1bddbb the returned value will be 0 and g2 will be pending on a
list. After line 3 udc will be free but g2 will still be hanging on a
pending list.
> There was no code path were a value which was not meant to
> be the function return code was accidentally returned. With commit
> f1bddbb, and the introduction of a new code path for the flag
> match_existing_only, this changed.
>
This flag changed the error from "leave it on the list forever when udc
is busy" to "NULL ptr dereference" which is obviously much worse.
>> After that commit, if UDC is busy, gadget is added to the list of
>> pending gadgets. Unfortunately this list is not rescanned in
>> usb_gadget_unregister_driver(). This means that such gadget is going to
>> stay on this list forever so it's a bug.
>
> Ok, I can confirm this bug. But it is not the issue which I am
> addressing with this patch. This is just about the
> usb_gadget_probe_driver() return code (on which other functions,
> paritcularly gadget configfs's|gadget_dev_desc_UDC_store|() rely).
>
This commit fixes both f1bddbb and 855ed04 just the bug is a little bit
different but it's still a bug.
>> Commit f1bddbb make this bug more visible (as it causes NULL pointer
>> dereference) as gadget has not been added to the list of pending gadgets
>> and we try to remove it from there.
>>
>> Summing up, in my personal opinion I think that you should add:
>>
>> Fixes: 855ed04 ("usb: gadget: udc-core: independent registration of
>> gadgets and gadget drivers")
>
> As explained above, I think this would be wrong.
>
I understand that your target was to fix NULL ptr dereference but you
can kill two birds with the same stone as you fix also the previous bug;).
I suggested to place commit 855ed04 instead of f1bddbb because 855ed04
appears earlier in a tree and even if someone doesn't have f1bddbb your
patch is useful for him because it fix a bug introduced in that commit
(gadget pending forever).
Best regards,
--
Krzysztof Opasiak
Samsung R&D 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 06/12] usb: dwc3: omap: Replace the extcon API
Hi Felipe, 2016-12-02 18:03 GMT+09:00 Felipe Balbi : > > Hi, > > Chanwoo Choi writes: >> Hi Felipe, >> >> On 2016년 11월 30일 19:36, Felipe Balbi wrote: >>> >>> Hi, >>> >>> Chanwoo Choi writes: This patch uses the resource-managed extcon API for extcon_register_notifier() and replaces the deprecated extcon API as following: - extcon_get_cable_state_() -> extcon_get_state() Signed-off-by: Chanwoo Choi >>> >>> Acked-by: Felipe Balbi >>> >> >> Thanks for your review. >> >> Each patch has no any dependency among patches. >> So, If possible, could you pick the patch6/8/9/10/11/12 on your tree? > > my tree is closed for v4.10, I can pick it up for v4.11 Thanks for your pickup to 4.11. -- Best Regards, Chanwoo Choi -- 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 v13 03/10] usbip: exporting devices: new connect operation
On 12/02/2016 08:05 AM, fx IWATA NOBUO wrote: > Hello, > >> In my humble opinion connect is not a best name for this operation. > > Yes. I think connect is not perfect. > >> it not only starts the connection to a remote server but also> exports >> a device. I think that a name like export or attach_to_remote would be >> more suitable. (...) > > I think one word name is better. > > 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 think connect/disconnect is not bad. > I will change it if there's user friendly, well-known and suit with > existing name. > Ehh, as usually the non technical problems are the toughest one;) I would be happy to suggest you a good name for this but I also cannot figure out any;) connect was not perfect from my point of view as it suggest that we are only establishing a tcp connection not connecting particular device but maybe it's only mine problem and misunderstanding;) Best regards, -- Krzysztof Opasiak Samsung R&D 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 1/1] usb: xhci: fix possible wild pointer
On 02.12.2016 04:29, Lu Baolu wrote:
handle_cmd_completion() frees a command structure which might
be still referenced by xhci->current_cmd. This might cause
problem when xhci->current_cmd is accessed after that.
A real-life case could be like this. The host takes a very long
time to respond to a command, and the command timer is fired at
the same time when the command completion event arrives. The
command completion handler frees xhci->current_cmd before the
timer function can grab xhci->lock. Afterward, timer function
grabs the lock and go ahead with checking and setting members
of xhci->current_cmd.
Cc: # v3.16+
Signed-off-by: Lu Baolu
---
drivers/usb/host/xhci-ring.c | 16 +++-
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index bdf6b13..13e05f6 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1267,14 +1267,16 @@ void xhci_handle_command_timeout(unsigned long data)
bool second_timeout = false;
xhci = (struct xhci_hcd *) data;
- /* mark this command to be cancelled */
spin_lock_irqsave(&xhci->lock, flags);
- if (xhci->current_cmd) {
- if (xhci->current_cmd->status == COMP_CMD_ABORT)
- second_timeout = true;
- xhci->current_cmd->status = COMP_CMD_ABORT;
+ if (!xhci->current_cmd) {
+ spin_unlock_irqrestore(&xhci->lock, flags);
+ return;
}
+ if (xhci->current_cmd->status == COMP_CMD_ABORT)
+ second_timeout = true;
+ xhci->current_cmd->status = COMP_CMD_ABORT;
+
/* Make sure command ring is running before aborting it */
hw_ring_state = xhci_read_64(xhci, &xhci->op_regs->cmd_ring);
if ((xhci->cmd_ring_state & CMD_RING_STATE_RUNNING) &&
@@ -1422,6 +1424,10 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
xhci->current_cmd = list_entry(cmd->cmd_list.next,
struct xhci_command, cmd_list);
mod_timer(&xhci->cmd_timer, jiffies + XHCI_CMD_DEFAULT_TIMEOUT);
+ } else if (xhci->current_cmd == cmd) {
+ xhci->current_cmd = NULL;
+ } else {
+ xhci_warn(xhci, "WARN current_cmd doesn't match command\n");
}
event_handled:
Thanks,
I might do some tweaking to (or remove) the warn message when applying if
that is ok
-Mathias
--
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 v13 05/10] usbip: exporting devices: modifications to daemon
On 12/02/2016 09:36 AM, fx IWATA NOBUO wrote: > Hello, > >>> -static int do_accept(int listenfd) >>> +static int do_accept(int listenfd, char *host, char *port) > >> In my opinion you should pass also the size of those arrays to this >> function instead of hardcoding them like this. > > OK. I will add them. > >>> - if (usbip_driver_open(driver)) >>> + if (usbip_open_driver()) >> >> What's the purpose of this function? > >>> - usbip_driver_close(driver); >>> + usbip_close_driver(); >> >> As above? Why not make this a global variable and leave those calls as >> they were? > > usbip_driver_open/close - host side abstraction including stub and vudc. > > usbip_open/close_driver - abstraction both host(stub/vudc) and vhci. > > usbip_driver_ has widely used as function names and file names > for host side abstraction. > If they were usbip_host_, I can use usbip_driver_open/close > for both host(stub/vudc) and vhci. usbip_host() is not correct name to use for both stub and vudc as from USB point of view stub is on a host but vudc is on a device side maybe a kind of usbipd_backed_init() would be more suitable? > >> I totally don't understand why those functions exist? >> Their names are very confusing and they don't implement any additional >> functionality. > > The abstraction is shared with patch 7/10. > >>> - driver = &device_driver; >>> + usbip_update_driver(); >> >> usbip_update_driver() is t totally unrelated to what this assignment >> really does. >> >> It changes the backend from stub driver to vudc not updating the driver. > > It used to make symmetric device side and application side. > In device side, it switches to device driver. > In application side, no operation. > So as above. I suggest to call it usbipd_backend() instead of driver. >> usbip_dev.c is not a best name for this file. It causes confusion (at least >> to me) as I don't know to what the dev part is related, usb device, device >> (stub side) or what? > > Device side. It's misleading as in USB we have host and device and this file is used on both (stub and vudc). > >>> + memset(&req, 0, sizeof(req)); >>> + memset(&reply, 0, sizeof(reply)); >> >> You don't need to 0 the reply. > > This is moved from usbipd.c. > I will remove. > >>> + PACK_OP_DEVLIST_REPLY(1, &reply); >> Could you please make the defines for 1 and 0 in this macro? >> As far as I understand the code it means pack and unpack but it would >> be much more readable if we have a define for this. > > Here's the list in the original code. > > usbip_attach.c: PACK_OP_IMPORT_REQUEST(0, &request); > usbip_attach.c: PACK_OP_IMPORT_REPLY(0, &reply); > usbip_list.c: PACK_OP_DEVLIST_REPLY(0, &reply); > usbip_network.c: PACK_OP_COMMON(1, &op_common); > usbip_network.c: PACK_OP_COMMON(0, &op_common); > usbipd.c: PACK_OP_IMPORT_REQUEST(0, &req); > usbipd.c: PACK_OP_DEVLIST_REPLY(1, &reply); > > Could I fix them in a refactoring patch including memset(0) and etc > in files tot touched in this set or outside of this set? > > PACK_OP_PACK 1 > PACK_OP_UNPACK 0 > Please do this outside this set as it's totally unrelated and just mark this series as depending on that change. It's a good idea to change this in all files not only in those you touch. Best regards, -- Krzysztof Opasiak Samsung R&D 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 2/3] usb: musb: Add a quirk to preserve MUSB_DEVCTL during suspend
On 11/29/2016 06:56 PM, Bin Liu wrote:
> On Tue, Nov 29, 2016 at 06:41:04PM +0100, Alexandre Bailon wrote:
>> On 11/29/2016 05:16 PM, Bin Liu wrote:
>>> Hi,
>>>
>>> On Mon, Nov 28, 2016 at 05:26:20PM +0100, Alexandre Bailon wrote:
On da8xx, VBUS is not maintained during suspend when musb is in host mode.
On resume, all the connected devices will be disconnected and then will
be enumerated again.
This happens because MUSB_DEVCTL is cleared during suspend.
Add a quirk to preserve MUSB_DEVCTL during a suspend.
>>>
>>> Will preserving MUSB_DEVCTL prevent the device getting disconnected?
>> Yes. Preserving MUSB_DEVCTL keeps VBUS on though the PHY is off during
>> suspend.
>
> VBUS will be on, but does it prevent disconnecting the usb device on
> resume? I don't have a DA8xx to test, but I doubt it, since the PHY is
> off.
Actually, the PHY is not really off.
The name is probably confusing but the PHY off only put the PHY in the
lowest power state (chapter 10.11.1 of omapl138 TRM).
I have tried with the TI kernel and it was able to suspend and resume
the omapl138-lcdk without without getting a disconnect.
That why I wrote this series.
Regards,
Alexandre
>
>> Note that in device mode, the disconnect still happen but I think it's
>> the expected behavior.
>
> Right, the PHY off disables the pullup.
>
>>> This doesn't happen on am335x. The PHY is off during suspend, during
>>> resume, PHY gets on, MUSB generates MUSB_CONNECT interrupt, then
>>> re-enumeration happens.
>> I haven't been able to try on the BBB. I don't know if my config is
>> incorrect or if some patches are missing but I was not able to
>> suspend it.
>
> I heard the mainline kernel currently is broken on am335x PM, so
> suspend/resume doesn't work. I had to test it on TI kernel.
>
> Regards,
> -Bin.
>
>>
>> Regards,
>> Alexandre
>>>
>>> Regards,
>>> -Bin.
>>>
Signed-off-by: Alexandre Bailon
---
drivers/usb/musb/musb_core.c | 13 +++--
drivers/usb/musb/musb_core.h | 1 +
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index 9e22646..7e2cd98 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -1040,14 +1040,15 @@ static void musb_enable_interrupts(struct musb
*musb)
}
-static void musb_generic_disable(struct musb *musb)
+static void musb_generic_disable(struct musb *musb, bool suspend)
{
void __iomem*mbase = musb->mregs;
musb_disable_interrupts(musb);
/* off */
- musb_writeb(mbase, MUSB_DEVCTL, 0);
+ if (!suspend || !(musb->io.quirks & MUSB_PRESERVE_DEVCTL))
+ musb_writeb(mbase, MUSB_DEVCTL, 0);
}
/*
@@ -1106,7 +1107,7 @@ void musb_stop(struct musb *musb)
{
/* stop IRQs, timers, ... */
musb_platform_disable(musb);
- musb_generic_disable(musb);
+ musb_generic_disable(musb, false);
musb_dbg(musb, "HDRC disabled");
/* FIXME
@@ -2310,7 +2311,7 @@ musb_init_controller(struct device *dev, int nIrq,
void __iomem *ctrl)
/* be sure interrupts are disabled before connecting ISR */
musb_platform_disable(musb);
- musb_generic_disable(musb);
+ musb_generic_disable(musb, false);
/* Init IRQ workqueue before request_irq */
INIT_DELAYED_WORK(&musb->irq_work, musb_irq_work);
@@ -2486,7 +2487,7 @@ static int musb_remove(struct platform_device *pdev)
musb_gadget_cleanup(musb);
spin_lock_irqsave(&musb->lock, flags);
musb_platform_disable(musb);
- musb_generic_disable(musb);
+ musb_generic_disable(musb, false);
spin_unlock_irqrestore(&musb->lock, flags);
musb_writeb(musb->mregs, MUSB_DEVCTL, 0);
pm_runtime_dont_use_autosuspend(musb->controller);
@@ -2663,7 +2664,7 @@ static int musb_suspend(struct device *dev)
unsigned long flags;
musb_platform_disable(musb);
- musb_generic_disable(musb);
+ musb_generic_disable(musb, true);
WARN_ON(!list_empty(&musb->pending_list));
spin_lock_irqsave(&musb->lock, flags);
diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
index a611e2f..22961ef 100644
--- a/drivers/usb/musb/musb_core.h
+++ b/drivers/usb/musb/musb_core.h
@@ -172,6 +172,7 @@ struct musb_io;
*/
struct musb_platform_ops {
+#define MUSB_PRESERVE_DEVCTL BIT(7)
#define MUSB_DMA_UX500BIT(6)
#define MUSB_DMA_CPPI41 BIT(5)
#define MUSB_DMA_CPPI BIT(4)
--
2.7.3
>>
--
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
usb: warning in vhci_hcd_probe/lockdep_init_map
Hi! I've got the following error report while booting the kernel with various usb configs enabled. On commit 2caceb3294a78c389b462e7e236a4e744a53a474 (Dec 1). gadgetfs: USB Gadget filesystem, version 24 Aug 2004 usbip_core: USB/IP Core v1.0.0 vhci_hcd vhci_hcd: USB/IP Virtual Host Controller vhci_hcd vhci_hcd: new USB bus registered, assigned bus number 2 BUG: key 88006a7e8d18 not in .data! [ cut here ] WARNING: CPU: 0 PID: 1 at kernel/locking/lockdep.c:3131 lockdep_init_map+0x60c/0x770 DEBUG_LOCKS_WARN_ON(1)[1.567044] Modules linked in: CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.9.0-rc7+ #58 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 88006bce6eb8 81f96c8a 0a02 11000d79cd6a ed000d79cd62 00046bce6ed8 41b58ab3 8598af40 81f969f8 41b58ab3 0200 Call Trace: [< inline >] __dump_stack lib/dump_stack.c:15 [] dump_stack+0x292/0x398 lib/dump_stack.c:51 [] __warn+0x19f/0x1e0 kernel/panic.c:550 [] warn_slowpath_fmt+0xc5/0x110 kernel/panic.c:565 [] lockdep_init_map+0x60c/0x770 kernel/locking/lockdep.c:3131 [] __kernfs_create_file+0x114/0x2a0 fs/kernfs/file.c:954 [] sysfs_add_file_mode_ns+0x225/0x520 fs/sysfs/file.c:305 [< inline >] create_files fs/sysfs/group.c:64 [] internal_create_group+0x239/0x8f0 fs/sysfs/group.c:134 [] sysfs_create_group+0x1f/0x30 fs/sysfs/group.c:156 [] vhci_start+0x5b4/0x7a0 drivers/usb/usbip/vhci_hcd.c:978 [] usb_add_hcd+0x8da/0x1c60 drivers/usb/core/hcd.c:2867 [] vhci_hcd_probe+0x97/0x130 drivers/usb/usbip/vhci_hcd.c:1103 [] platform_drv_probe+0x8a/0x180 drivers/base/platform.c:568 [< inline >] really_probe drivers/base/dd.c:375 [] driver_probe_device+0x514/0x980 drivers/base/dd.c:515 [] __device_attach_driver+0x22b/0x290 drivers/base/dd.c:610 [] bus_for_each_drv+0x15c/0x200 drivers/base/bus.c:463 [] __device_attach+0x266/0x3c0 drivers/base/dd.c:667 [] device_initial_probe+0x1a/0x20 drivers/base/dd.c:714 [] bus_probe_device+0x1e6/0x290 drivers/base/bus.c:557 [] device_add+0xd06/0x1660 drivers/base/core.c:1136 [] platform_device_add+0x315/0x660 drivers/base/platform.c:408 [] platform_device_register_full+0x396/0x4b0 drivers/base/platform.c:540 [< inline >] platform_device_register_resndata ./include/linux/platform_device.h:111 [< inline >] platform_device_register_simple ./include/linux/platform_device.h:140 [< inline >] add_platform_device drivers/usb/usbip/vhci_hcd.c:1213 [] vhci_hcd_init+0x215/0x305 drivers/usb/usbip/vhci_hcd.c:1254 [] do_one_initcall+0xf6/0x360 init/main.c:778 [< inline >] do_initcall_level init/main.c:844 [< inline >] do_initcalls init/main.c:852 [< inline >] do_basic_setup init/main.c:870 [] kernel_init_freeable+0x5c7/0x6a1 init/main.c:1017 [] kernel_init+0x13/0x180 init/main.c:943 [] ret_from_fork+0x2a/0x40 arch/x86/entry/entry_64.S:433 ---[ end trace c33c7b202cf3aac8 ]--- -- 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: USB gadget: configfs hid mouse device
Krzysztof Opasiak wrote: > You may also try to use your own C program instead of echo to check if > hex value is correctly written. This was the crucial hint. Seem like echo is doing something fishy here. "mouse" Works fine with my Linux host now, but unfortunately does not using Windows 10 as a host! Here is the HID I'm using: 0x05, 0x01, //Usage Page(Generic Desktop Controls) 0x09, 0x02, //Usage (Mouse) 0xa1, 0x01, //Collection (Application) 0x09, 0x01, //Usage (pointer) 0xa1, 0x00, //Collection (Physical) 0x05, 0x09, //Usage Page (Button) 0x19, 0x01, //Usage Minimum(1) 0x29, 0x05, //Usage Maximum(5) 0x15, 0x00, //Logical Minimum(1) 0x25, 0x01, //Logical Maximum(1) 0x95, 0x05, //Report Count(5) 0x75, 0x01, //Report Size(1) 0x81, 0x02, //Input(Data,Variable,Absolute,BitField) 0x95, 0x01, //Report Count(1) 0x75, 0x03, //Report Size(3) 0x81, 0x01, //Input(Constant,Array,Absolute,BitField) 0x05, 0x01, //Usage Page(Generic Desktop Controls) 0x09, 0x30, //Usage(x) 0x09, 0x31, //Usage(y) 0x09, 0x38, //Usage(Wheel) 0x15, 0x81, //Logical Minimum(-127) 0x25, 0x7F, //Logical Maximum(127) 0x75, 0x08, //Report Size(8) 0x95, 0x03, //Report Count(3) 0x81, 0x06, //Input(Data,Variable,Relative,BitField) 0xc0, //End Collection 0xc0 //End Collection Windows properly detects a HID mouse pointer, but I'm unable to make the Windows mouse actually move when sending movement events. Sven -- -- 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 v13 06/10] usbip: exporting devices: modifications to attach and detach
On 11/22/2016 07:48 AM, Nobuo Iwata wrote:
> Refactoring to attach and detach operation to reuse common portion to
> application(vhci)-side daemon.
>
> The new application(vhci)-side daemon executes same procedures as
> attach and detach. Most of common code to access vhci has been
> implemented in VHCI userspace wrapper(libsrc/vhci_driver.c) but left in
> attach and detach. With this patch, an accessor of vhci driver and
> tracking of vhci connections in attach and detach are moved to the VHCI
> userspace wrapper.
>
> Here, attach, detach and application(vhci)-side daemon is EXISTING-5,
> EXISTING-6 and NEW-1 respectively 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 --->
>
> Signed-off-by: Nobuo Iwata
> ---
> tools/usb/usbip/libsrc/vhci_driver.c | 99
> tools/usb/usbip/libsrc/vhci_driver.h | 6 +-
> tools/usb/usbip/src/usbip_attach.c | 50 ++
> tools/usb/usbip/src/usbip_detach.c | 13 ++--
> 4 files changed, 100 insertions(+), 68 deletions(-)
>
> diff --git a/tools/usb/usbip/libsrc/vhci_driver.c
> b/tools/usb/usbip/libsrc/vhci_driver.c
> index ad92047..d2221c5 100644
> --- a/tools/usb/usbip/libsrc/vhci_driver.c
> +++ b/tools/usb/usbip/libsrc/vhci_driver.c
> @@ -1,5 +1,6 @@
> /*
> - * Copyright (C) 2005-2007 Takahiro Hirofuchi
> + * Copyright (C) 2015 Nobuo Iwata
> + * 2005-2007 Takahiro Hirofuchi
> */
>
> #include "usbip_common.h"
> @@ -7,6 +8,8 @@
> #include
> #include
> #include
> +#include
> +#include
> #include "sysfs_utils.h"
>
> #undef PROGNAME
> @@ -215,6 +218,25 @@ static int read_record(int rhport, char *host, unsigned
> long host_len,
> return 0;
> }
>
> +#define OPEN_HC_MODE_FIRST 0
> +#define OPEN_HC_MODE_REOPEN 1
> +
> +static int open_hc_device(int mode)
> +{
> + if (mode == OPEN_HC_MODE_REOPEN)
> + udev_device_unref(vhci_driver->hc_device);
> +
> + vhci_driver->hc_device =
> + udev_device_new_from_subsystem_sysname(udev_context,
> +USBIP_VHCI_BUS_TYPE,
> +USBIP_VHCI_DRV_NAME);
> + if (!vhci_driver->hc_device) {
> + err("udev_device_new_from_subsystem_sysname failed");
> + return -1;
> + }
> + return 0;
> +}
> +
Could you please elaborate why this function takes an argument and why
you are reopening vhci device while refreshing device list? there is
nothing about this in commit msg.
> /* -- */
>
> int usbip_vhci_driver_open(void)
> @@ -227,28 +249,21 @@ int usbip_vhci_driver_open(void)
>
> vhci_driver = calloc(1, sizeof(struct usbip_vhci_driver));
sizeof(*vhci_driver);
>
> - /* will be freed in usbip_driver_close() */
> - vhci_driver->hc_device =
> - udev_device_new_from_subsystem_sysname(udev_context,
> -USBIP_VHCI_BUS_TYPE,
> -USBIP_VHCI_DRV_NAME);
> - if (!vhci_driver->hc_device) {
> - err("udev_device_new_from_subsystem_sysname failed");
> - goto err;
> - }
> + if (open_hc_device(OPEN_HC_MODE_FIRST))
> + goto err_free_driver;
>
> vhci_driver->nports = get_nports();
>
> dbg("available ports: %d", vhci_driver->nports);
>
> if (refresh_imported_device_list())
> - goto err;
> + goto err_unref_device;
>
> return 0;
>
> -err:
> +err_unref_device:
> udev_device_unref(vhci_driver->hc_device);
> -
> +err_free_driver:
> if (vhci_driver)
> free(vhci_driver);
>
> @@ -277,7 +292,8 @@ void usbip_vhci_driver_close(void)
>
> int usbip_vhci_refresh_device_list(void)
> {
> -
> + if (open_hc_device(OPEN_HC_MODE_REOPEN))
> + goto err;
usb/gadget: warning in dev_config/memdup_user
Hi! I've got the following error report while running the syzkaller fuzzer. The length passed to memdup_user() directly without limitations. On commit 2caceb3294a78c389b462e7e236a4e744a53a474 (Dec 1). WARNING: CPU: 3 PID: 14477 at mm/page_alloc.c:3511 __alloc_pages_nodemask+0x159c/0x1e20 Kernel panic - not syncing: panic_on_warn set ... CPU: 3 PID: 14477 Comm: syz-executor0 Not tainted 4.9.0-rc7+ #61 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 880039b67298 81f96bba 0200 11000736cde6 ed000736cdde 0a06 41b58ab3 8598af00 81f96928 41b58ab3 859423c8 81432790 Call Trace: [< inline >] __dump_stack lib/dump_stack.c:15 [] dump_stack+0x292/0x398 lib/dump_stack.c:51 [] panic+0x1cb/0x3a9 kernel/panic.c:179 [] __warn+0x1c4/0x1e0 kernel/panic.c:542 [] warn_slowpath_null+0x2c/0x40 kernel/panic.c:585 [< inline >] __alloc_pages_slowpath mm/page_alloc.c:3511 [] __alloc_pages_nodemask+0x159c/0x1e20 mm/page_alloc.c:3781 [] alloc_pages_current+0x1c7/0x6b0 mm/mempolicy.c:2072 [< inline >] alloc_pages include/linux/gfp.h:469 [] kmalloc_order+0x1f/0x70 mm/slab_common.c:1015 [] kmalloc_order_trace+0x1f/0x160 mm/slab_common.c:1026 [< inline >] kmalloc_large include/linux/slab.h:422 [] __kmalloc_track_caller+0x1df/0x290 mm/slub.c:4233 [] memdup_user+0x2c/0xa0 mm/util.c:137 [] dev_config+0x2e2/0x1180 drivers/usb/gadget/legacy/inode.c:1776 [] __vfs_write+0x5d5/0x760 fs/read_write.c:510 [] vfs_write+0x170/0x4e0 fs/read_write.c:560 [< inline >] SYSC_write fs/read_write.c:607 [] SyS_write+0xfb/0x230 fs/read_write.c:599 [] entry_SYSCALL_64_fastpath+0x1f/0xc2 Dumping ftrace buffer: (ftrace buffer empty) Kernel Offset: disabled -- 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] usbip: vudc: fix: Clear already_seen flag also for ep0
On 12/01/2016 11:14 AM, Krzysztof Opasiak wrote:
> ep_list inside gadget structure doesn't contain ep0.
> It is stored separately in ep0 field.
>
> This causes an urb hang if gadget driver decides to
> delay setup handling. On host side this is visible as
> timeout error when setting configuration.
>
> This bug can be reproduced using for example any gadget
> with mass storage function.
>
> Fixes: abdb29574322 ("usbip: vudc: Add vudc_transfer")
> Signed-off-by: Krzysztof Opasiak
Greg,
Assuming you want to continue in the same mode as you pulling
in the USB/IP patches with Ack. This patch looks good. Please
pick this up. If you want me to maintain USB/IP tree and send you
pull request, I can do that as well. Whatever works for you.
Acked-by: Shuah Khan
thanks,
-- Shuah
> ---
> Changes since v1:
> - Use first 12 digits of commit sha in fixes tag
>
> ---
> drivers/usb/usbip/vudc_transfer.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/usb/usbip/vudc_transfer.c
> b/drivers/usb/usbip/vudc_transfer.c
> index aba6bd4..bc0296d 100644
> --- a/drivers/usb/usbip/vudc_transfer.c
> +++ b/drivers/usb/usbip/vudc_transfer.c
> @@ -339,6 +339,8 @@ static void v_timer(unsigned long _vudc)
> total = timer->frame_limit;
> }
>
> + /* We have to clear ep0 flags separately as it's not on the list */
> + udc->ep[0].already_seen = 0;
> list_for_each_entry(_ep, &udc->gadget.ep_list, ep_list) {
> ep = to_vep(_ep);
> ep->already_seen = 0;
>
--
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] usbip: vudc: Refactor init_vudc_hw() to be more obvious
On 12/02/2016 04:15 PM, Shuah Khan wrote:
> Hi Krzysztof,
>
> Thanks for the patch.
>
> On 12/01/2016 10:02 AM, Krzysztof Opasiak wrote:
>> Current implementation of init_vudc_hw() adds ep0 to ep_list
>> and then after looping through all endpoints removes it from
>> that list.
>>
>> As this may be misleading let's refactor this function
>> and avoid adding and removing ep0 to eplist and place it
>> immediately in correct place.
>
>>
>> Signed-off-by: Krzysztof Opasiak
>> ---
>> drivers/usb/usbip/vudc_dev.c | 38 +-
>> 1 file changed, 21 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c
>> index 7091848..a5ca29b 100644
>> --- a/drivers/usb/usbip/vudc_dev.c
>> +++ b/drivers/usb/usbip/vudc_dev.c
>> @@ -549,30 +549,37 @@ static int init_vudc_hw(struct vudc *udc)
>> sprintf(ep->name, "ep%d%s", num,
>> i ? (is_out ? "out" : "in") : "");
>> ep->ep.name = ep->name;
>> +
>> +ep->ep.ops = &vep_ops;
>> +
>> +ep->halted = ep->wedged = ep->already_seen =
>> +ep->setup_stage = 0;
>
> Do you need to clear these explicitly. kcalloc() should do it for you.
Well, that's true. I may remove this if you like.
>
>> +usb_ep_set_maxpacket_limit(&ep->ep, ~0);
>> +ep->ep.max_streams = 16;
>> +ep->gadget = &udc->gadget;
>> +ep->desc = NULL;
Probably the same here.
>> +INIT_LIST_HEAD(&ep->req_queue);
>> +
>> if (i == 0) {
>> +/* ep0 */
>> ep->ep.caps.type_control = true;
>> ep->ep.caps.dir_out = true;
>> ep->ep.caps.dir_in = true;
>> +
>> +udc->gadget.ep0 = &ep->ep;
>> } else {
>> +/* All other eps */
>> ep->ep.caps.type_iso = true;
>> ep->ep.caps.type_int = true;
>> ep->ep.caps.type_bulk = true;
>> -}
>>
>> -if (is_out)
>> -ep->ep.caps.dir_out = true;
>> -else
>> -ep->ep.caps.dir_in = true;
>> +if (is_out)
>> +ep->ep.caps.dir_out = true;
>> +else
>> +ep->ep.caps.dir_in = true;
>
> You are moving the is_out handling which was common for all eps
> including ep0 under non-eop0 - is that right?
Yes it's right. take a look at ep0 inside if(). We set there both
directions to true so setting one of them once again to true doesn't
make any sense.
>
>>
>> -ep->ep.ops = &vep_ops;
>> -list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list);
>> -ep->halted = ep->wedged = ep->already_seen =
>> -ep->setup_stage = 0;
>> -usb_ep_set_maxpacket_limit(&ep->ep, ~0);
>> -ep->ep.max_streams = 16;
>> -ep->gadget = &udc->gadget;
>> -ep->desc = NULL;
>> -INIT_LIST_HEAD(&ep->req_queue);
>> +list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list);
>> +}
>> }
>>
>> spin_lock_init(&udc->lock);
>> @@ -589,9 +596,6 @@ static int init_vudc_hw(struct vudc *udc)
>> ud->eh_ops.reset= vudc_device_reset;
>> ud->eh_ops.unusable = vudc_device_unusable;
Best regards,
--
Krzysztof Opasiak
Samsung R&D 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] usbip: vudc: Refactor init_vudc_hw() to be more obvious
On 12/02/2016 08:27 AM, Krzysztof Opasiak wrote:
>
>
> On 12/02/2016 04:15 PM, Shuah Khan wrote:
>> Hi Krzysztof,
>>
>> Thanks for the patch.
>>
>> On 12/01/2016 10:02 AM, Krzysztof Opasiak wrote:
>>> Current implementation of init_vudc_hw() adds ep0 to ep_list
>>> and then after looping through all endpoints removes it from
>>> that list.
>>>
>>> As this may be misleading let's refactor this function
>>> and avoid adding and removing ep0 to eplist and place it
>>> immediately in correct place.
>>
>>>
>>> Signed-off-by: Krzysztof Opasiak
>>> ---
>>> drivers/usb/usbip/vudc_dev.c | 38 +-
>>> 1 file changed, 21 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c
>>> index 7091848..a5ca29b 100644
>>> --- a/drivers/usb/usbip/vudc_dev.c
>>> +++ b/drivers/usb/usbip/vudc_dev.c
>>> @@ -549,30 +549,37 @@ static int init_vudc_hw(struct vudc *udc)
>>> sprintf(ep->name, "ep%d%s", num,
>>> i ? (is_out ? "out" : "in") : "");
>>> ep->ep.name = ep->name;
>>> +
>>> + ep->ep.ops = &vep_ops;
>>> +
>>> + ep->halted = ep->wedged = ep->already_seen =
>>> + ep->setup_stage = 0;
>>
>> Do you need to clear these explicitly. kcalloc() should do it for you.
>
> Well, that's true. I may remove this if you like.
Please do. It is redundant.
>
>>
>>> + usb_ep_set_maxpacket_limit(&ep->ep, ~0);
>>> + ep->ep.max_streams = 16;
>>> + ep->gadget = &udc->gadget;
>>> + ep->desc = NULL;
>
> Probably the same here.
>
>>> + INIT_LIST_HEAD(&ep->req_queue);
>>> +
>>> if (i == 0) {
>>> + /* ep0 */
>>> ep->ep.caps.type_control = true;
>>> ep->ep.caps.dir_out = true;
>>> ep->ep.caps.dir_in = true;
>>> +
>>> + udc->gadget.ep0 = &ep->ep;
>>> } else {
>>> + /* All other eps */
>>> ep->ep.caps.type_iso = true;
>>> ep->ep.caps.type_int = true;
>>> ep->ep.caps.type_bulk = true;
>>> - }
>>>
>>> - if (is_out)
>>> - ep->ep.caps.dir_out = true;
>>> - else
>>> - ep->ep.caps.dir_in = true;
>>> + if (is_out)
>>> + ep->ep.caps.dir_out = true;
>>> + else
>>> + ep->ep.caps.dir_in = true;
>>
>> You are moving the is_out handling which was common for all eps
>> including ep0 under non-eop0 - is that right?
>
> Yes it's right. take a look at ep0 inside if(). We set there both
> directions to true so setting one of them once again to true doesn't
> make any sense.
>
Yeah. I missed that.
thanks,
-- Shuah
>>
>>>
>>> - ep->ep.ops = &vep_ops;
>>> - list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list);
>>> - ep->halted = ep->wedged = ep->already_seen =
>>> - ep->setup_stage = 0;
>>> - usb_ep_set_maxpacket_limit(&ep->ep, ~0);
>>> - ep->ep.max_streams = 16;
>>> - ep->gadget = &udc->gadget;
>>> - ep->desc = NULL;
>>> - INIT_LIST_HEAD(&ep->req_queue);
>>> + list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list);
>>> + }
>>> }
>>>
>>> spin_lock_init(&udc->lock);
>>> @@ -589,9 +596,6 @@ static int init_vudc_hw(struct vudc *udc)
>>> ud->eh_ops.reset= vudc_device_reset;
>>> ud->eh_ops.unusable = vudc_device_unusable;
>
> Best regards,
>
--
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] usbip: vudc: Refactor init_vudc_hw() to be more obvious
Hi Krzysztof,
Thanks for the patch.
On 12/01/2016 10:02 AM, Krzysztof Opasiak wrote:
> Current implementation of init_vudc_hw() adds ep0 to ep_list
> and then after looping through all endpoints removes it from
> that list.
>
> As this may be misleading let's refactor this function
> and avoid adding and removing ep0 to eplist and place it
> immediately in correct place.
>
> Signed-off-by: Krzysztof Opasiak
> ---
> drivers/usb/usbip/vudc_dev.c | 38 +-
> 1 file changed, 21 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c
> index 7091848..a5ca29b 100644
> --- a/drivers/usb/usbip/vudc_dev.c
> +++ b/drivers/usb/usbip/vudc_dev.c
> @@ -549,30 +549,37 @@ static int init_vudc_hw(struct vudc *udc)
> sprintf(ep->name, "ep%d%s", num,
> i ? (is_out ? "out" : "in") : "");
> ep->ep.name = ep->name;
> +
> + ep->ep.ops = &vep_ops;
> +
> + ep->halted = ep->wedged = ep->already_seen =
> + ep->setup_stage = 0;
Do you need to clear these explicitly. kcalloc() should do it for you.
> + usb_ep_set_maxpacket_limit(&ep->ep, ~0);
> + ep->ep.max_streams = 16;
> + ep->gadget = &udc->gadget;
> + ep->desc = NULL;
> + INIT_LIST_HEAD(&ep->req_queue);
> +
> if (i == 0) {
> + /* ep0 */
> ep->ep.caps.type_control = true;
> ep->ep.caps.dir_out = true;
> ep->ep.caps.dir_in = true;
> +
> + udc->gadget.ep0 = &ep->ep;
> } else {
> + /* All other eps */
> ep->ep.caps.type_iso = true;
> ep->ep.caps.type_int = true;
> ep->ep.caps.type_bulk = true;
> - }
>
> - if (is_out)
> - ep->ep.caps.dir_out = true;
> - else
> - ep->ep.caps.dir_in = true;
> + if (is_out)
> + ep->ep.caps.dir_out = true;
> + else
> + ep->ep.caps.dir_in = true;
You are moving the is_out handling which was common for all eps
including ep0 under non-eop0 - is that right?
>
> - ep->ep.ops = &vep_ops;
> - list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list);
> - ep->halted = ep->wedged = ep->already_seen =
> - ep->setup_stage = 0;
> - usb_ep_set_maxpacket_limit(&ep->ep, ~0);
> - ep->ep.max_streams = 16;
> - ep->gadget = &udc->gadget;
> - ep->desc = NULL;
> - INIT_LIST_HEAD(&ep->req_queue);
> + list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list);
> + }
> }
>
> spin_lock_init(&udc->lock);
> @@ -589,9 +596,6 @@ static int init_vudc_hw(struct vudc *udc)
> ud->eh_ops.reset= vudc_device_reset;
> ud->eh_ops.unusable = vudc_device_unusable;
>
> - udc->gadget.ep0 = &udc->ep[0].ep;
> - list_del_init(&udc->ep[0].ep.ep_list);
> -
> v_init_timer(udc);
> return 0;
>
>
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] usbip: vudc: Refactor init_vudc_hw() to be more obvious
On 12/02/2016 04:37 PM, Shuah Khan wrote: > On 12/02/2016 08:27 AM, Krzysztof Opasiak wrote: >> >> >> On 12/02/2016 04:15 PM, Shuah Khan wrote: >>> Hi Krzysztof, >>> >>> Thanks for the patch. >>> >>> On 12/01/2016 10:02 AM, Krzysztof Opasiak wrote: Current implementation of init_vudc_hw() adds ep0 to ep_list and then after looping through all endpoints removes it from that list. As this may be misleading let's refactor this function and avoid adding and removing ep0 to eplist and place it immediately in correct place. >>> Signed-off-by: Krzysztof Opasiak --- drivers/usb/usbip/vudc_dev.c | 38 +- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c index 7091848..a5ca29b 100644 --- a/drivers/usb/usbip/vudc_dev.c +++ b/drivers/usb/usbip/vudc_dev.c @@ -549,30 +549,37 @@ static int init_vudc_hw(struct vudc *udc) sprintf(ep->name, "ep%d%s", num, i ? (is_out ? "out" : "in") : ""); ep->ep.name = ep->name; + + ep->ep.ops = &vep_ops; + + ep->halted = ep->wedged = ep->already_seen = + ep->setup_stage = 0; >>> >>> Do you need to clear these explicitly. kcalloc() should do it for you. >> >> Well, that's true. I may remove this if you like. > > Please do. It is redundant. > I will do this and send v2 shortly. Thanks, -- Krzysztof Opasiak Samsung R&D 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 v13 07/10] usbip: exporting devices: new application-side daemon
On 11/22/2016 07:48 AM, Nobuo Iwata wrote:
> This patch is the new application(vhci)-side daemon specific code.
>
> The daemons are consisting three files.
> usbip.c : common code.
> usbip_dev.c: device(stub)-side specific code.
> usbip_app.c: application(vhci)-side specific code - this patch.
>
> Here, device-side daemon is EXISTING-1 and application-side daemon is
> NEW-1 in figure 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 --->
>
> Signed-off-by: Nobuo Iwata
> ---
> tools/usb/usbip/libsrc/vhci_driver.c | 19 +++
> tools/usb/usbip/libsrc/vhci_driver.h | 1 +
> tools/usb/usbip/src/Makefile.am | 7 +-
> tools/usb/usbip/src/usbipd.c | 12 +-
> tools/usb/usbip/src/usbipd_app.c | 242 +++
> 5 files changed, 279 insertions(+), 2 deletions(-)
>
> diff --git a/tools/usb/usbip/libsrc/vhci_driver.c
> b/tools/usb/usbip/libsrc/vhci_driver.c
> index d2221c5..3fe92ff 100644
> --- a/tools/usb/usbip/libsrc/vhci_driver.c
> +++ b/tools/usb/usbip/libsrc/vhci_driver.c
> @@ -314,6 +314,25 @@ int usbip_vhci_get_free_port(void)
> return -1;
> }
>
> +struct usbip_imported_device *usbip_vhci_find_device(char *host, char *busid)
> +{
> + int ret;
> + char rhost[NI_MAXHOST] = "unknown host";
> + char rserv[NI_MAXSERV] = "unknown port";
> + char rbusid[SYSFS_BUS_ID_SIZE];
> +
> + for (int i = 0; i < vhci_driver->nports; i++) {
> + ret = read_record(vhci_driver->idev[i].port, rhost, NI_MAXHOST,
> + rserv, NI_MAXSERV, rbusid);
sizeof(rhist), sizeof(rserv)
BTW.
The read_record() function is really weird and probably it should be
also refactored. We pass 3 arrays but pass size only for 2 of them and
size of third argument is hard coded inside this function:(
> + if (!ret &&
> + !strncmp(host, rhost, NI_MAXHOST) &&
> + !strncmp(busid, rbusid, SYSFS_BUS_ID_SIZE)) {
> + return vhci_driver->idev + i;
> + }
Is there any reason why you don't check port here?
I also thing that it's better to ensure that strings are \0 terminated
rather than using strncmp all the time.
> + }
> + return NULL;
> +}
> +
(...)
> +
> +static int import_device(int sockfd, struct usbip_usb_device *udev)
> +{
> + int rc;
> + int port;
> +
> + dbg("Sockfd:%d", sockfd);
> + port = usbip_vhci_get_free_port();
> + if (port < 0) {
> + err("no free port");
> + return -1;
> + }
> +
> + dump_usb_device(udev);
> + rc = usbip_vhci_attach_device(port, sockfd, udev->busnum,
> + udev->devnum, udev->speed);
> + if (rc < 0) {
> + err("import device");
> + return -1;
> + }
> +
> + return port;
> +}
> +
> +static int recv_request_export(int sockfd, char *host, char *port)
> +{
> + struct op_export_request req;
> + struct op_export_reply reply;
> + int rhport = 0;
> + int error = 0;
> + int rc;
> +
> + memset(&req, 0, sizeof(req));
> + memset(&reply, 0, sizeof(reply));
As before. You don't need to memset() reply.
> +
> + rc = usbip_net_recv(sockfd, &req, sizeof(req));
> + if (rc < 0) {
> + dbg("usbip_net_recv failed: export request");
> + return -1;
> + }
> + PACK_OP_EXPORT_REQUEST(0, &req);
> +
> + rhport = import_device(sockfd, &req.udev);
> + if (rhport < 0) {
> + dbg("export request busid %s: failed", req.udev.busid);
> + error = 1;
> + }
> +
> + rc = usbip_net_send_op_common(sockfd, OP_REP_EXPORT,
> + (!error ? ST_OK : ST_NA));
> + if (rc < 0) {
> + dbg("usbip_net_send_op_common failed: %#0x", OP_REP_EXPORT);
> + return -1;
> + }
> +
> + if (!error)
> + reply.returncode = 0;
> + else
> + reply.retu
Re: [PATCH v13 00/10] usbip: exporting devices
On 12/02/2016 02:38 AM, fx IWATA NOBUO wrote: > Hello, > >> I am not clear on this. Is Client and server behind different >> firewalls? > > No. > The firewall is in between internet and > >> So the above doesn't apply to a) and it works? > > Yes. > >> What does this mean? Why is this connection from outside? > > 'usbip attach -r' tries to establish connection. That doesn't tell me much. I am looking to see in the above where are the server and client located. > >> Is this something that could be solved by opening ports in >> the firewall? > Usually, http(80) and https(443) from inside is opened for internet > Access. You can open other ports if need be and if you have access to firewall. > > >> Can we use server and client terminology which is we use in instead of >> App and Dev. > > In https://www.kernel.org/doc/readme/tools-usb-usbip-README, I couldn't > find the definition. > > Do you mean 'server' is a linux machine which has devices > or linux machine which runs server daemon? > > In existing way, they are same. Yes. Server is the system with the USB device physically attached to. > > In new way, they are not. > The linux machine which has device doesn't have server daemon. Okay this is a big change and that isn't very clear in any of the change logs. This is one of my concerns with the patch series. I want to understand how a different server (system that doesn't have the USB device physically attached) can be authorized to export devices that don't belong to it. Something about this model doesn't sound right. If I have system that sits behind a firewall, I don't want USB devices visible to anybody and everybody. I don't see anything in this series that disallows that, short of saying don't enable USB/IP. I would like to know why there is a need to change the existing server-client model. I still would like to see a concrete answer why the current model doesn't work. Doesn't making sure port 3240 isn't blocked on the firewall help? btw: could you please re-run get_maintainers - my email address in the MAINTAINERS file changed a while ago. I think your email might be outdated. thanks, -- Shuah > > I'd like to confirm the definition to answer rest of questions. > > Best Regards, > > n.iwata > // >> -Original Message- >> From: Shuah Khan [mailto:shuah...@samsung.com] >> Sent: Friday, December 02, 2016 9:39 AM >> To: fx IWATA NOBUO; valentina.mane...@gmail.com; >> gre...@linuxfoundation.org; linux-usb@vger.kernel.org >> Cc: fx MICHIMURA TADAO; Shuah Khan >> Subject: Re: [PATCH v13 00/10] usbip: exporting devices >> >> On 11/23/2016 10:01 PM, fx IWATA NOBUO wrote: >>> Hello, >>> >> Looks like you removed the text. Let's go back to the goals. >> Can we use server and client terminology which is we use in instead of App >> and Dev. It makes lot easier to understand. >> >> https://www.kernel.org/doc/readme/tools-usb-usbip-README >> 1. Goal >> 1-1) To give flexibility to direction of connection Using USB/IP in internet, there can be two cases. a) an application is inside firewall and devices are outside. >> >> This is the case Client is behind firewall and server is outside the >> firewall. >> b) devices are inside firewall and an application is inside. >> >> I am not clear on this. Is Client and server behind different firewalls? >> In case-a, import works because the connection is from inside. >> >>> EXISTING) >>> APP# usbip attach ---(passed)> DEV# usbipd >>> DEV# usbipd (blocked)|| <- APP# usbip attach >> >> So the above doesn't apply to a) and it works? >> In case-b, import doesn't works because the connection is from outside. >> >> What does this mean? Why is this connection from outside? >> Connection from device side is needed. This patch set adds the direction of connection establishment. >> >> Is this something that could be solved by opening ports in the firewall? >> >> Can you elaborate on the use-case a bit more? What does it mean to "Connection from device side is needed"? >>> >>> I'd like to update ending part of use case as following. >>> --- >>> Firewall, proxy, or router in front of internet usually blocks >>> connections from internet regarding all TCP ports. They opens some >>> ports, usually HTTP(80) and HTTPS(443), for connection from inside. >>> In combination with WebSocket proxy, USB/IP can establish connection >>> from inside of the firewall. >>> >>> Enterprise/SOHO/Home Firewall/Proxy/Router Internet >>> >>> EXISTING) >>> APP# usbip attach ---(passed)> DEV# usbipd >>> DEV# usbipd (blocked)|| <- APP# usbip attach >>> >>> NEW) >>> DEV# usbip connect --(passed)> DEV# usbipa >>> APP# usbipa (blocked)|| <- APP# usbip connect >>> >>> Attach operation can invite devices in internet but cannot invite >>> devices from internet. On the other hand, connect operation can >>> dedicate devices
Re: [PATCH v13 08/10] usbip: exporting devices: change to usbip_list.c
On 11/22/2016 07:48 AM, Nobuo Iwata wrote:
> Correction to wording inconsistency around import and export in
> usbip_list.c regarding output title, help and function names.
>
> 'exported' was used for devices bound in remote and to be attached with
> 'import' request. This patch set uses pre-defined 'export' request to
> connect device.
>
> To avoid mixed usage of 'export', 'importable' is used for devices to
> be attached with 'import' request.
>
> The word 'imported' has already been used in output of port operation.
> It is consistent to this patch.
>
After fixing suggestion about placement of close() function you may add my:
Reviewed-by: Krzysztof Opasiak
In addition I think that this patch should be send separately before
this whole series.
> Signed-off-by: Nobuo Iwata
> ---
> tools/usb/usbip/src/usbip_list.c | 17 +
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/tools/usb/usbip/src/usbip_list.c
> b/tools/usb/usbip/src/usbip_list.c
> index f1b38e8..cf69f31 100644
> --- a/tools/usb/usbip/src/usbip_list.c
> +++ b/tools/usb/usbip/src/usbip_list.c
> @@ -44,7 +44,7 @@
> static const char usbip_list_usage_string[] =
> "usbip list [-p|--parsable] \n"
> "-p, --parsable Parsable list format\n"
> - "-r, --remote=List the exportable USB devices on \n"
> + "-r, --remote=List the importable USB devices on \n"
> "-l, --localList the local USB devices\n";
>
> void usbip_list_usage(void)
> @@ -52,7 +52,7 @@ void usbip_list_usage(void)
> printf("usage: %s", usbip_list_usage_string);
> }
>
> -static int get_exported_devices(char *host, int sockfd)
> +static int get_importable_devices(char *host, int sockfd)
> {
> char product_name[100];
> char class_name[100];
> @@ -82,14 +82,14 @@ static int get_exported_devices(char *host, int sockfd)
> return -1;
> }
> PACK_OP_DEVLIST_REPLY(0, &reply);
> - dbg("exportable devices: %d\n", reply.ndev);
> + dbg("importable devices: %d\n", reply.ndev);
>
> if (reply.ndev == 0) {
> - info("no exportable devices found on %s", host);
> + info("no importable devices found on %s", host);
> return 0;
> }
>
> - printf("Exportable USB devices\n");
> + printf("Importable USB devices\n");
> printf("==\n");
> printf(" - %s\n", host);
>
> @@ -134,7 +134,7 @@ static int get_exported_devices(char *host, int sockfd)
> return 0;
> }
>
> -static int list_exported_devices(char *host)
> +static int list_importable_devices(char *host)
> {
> int rc;
> int sockfd;
> @@ -147,9 +147,10 @@ static int list_exported_devices(char *host)
> }
> dbg("connected to %s:%s", host, usbip_port_string);
>
> - rc = get_exported_devices(host, sockfd);
> + rc = get_importable_devices(host, sockfd);
> if (rc < 0) {
> err("failed to get device list from %s", host);
> + close(sockfd);
> return -1;
> }
Why not just close before if?
Best regards,
--
Krzysztof Opasiak
Samsung R&D 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: usb/gadget: warning in dev_config/memdup_user
On Fri, Dec 02, 2016 at 04:07:38PM +0100, Andrey Konovalov wrote: > Hi! > > I've got the following error report while running the syzkaller fuzzer. > > The length passed to memdup_user() directly without limitations. > > On commit 2caceb3294a78c389b462e7e236a4e744a53a474 (Dec 1). > > WARNING: CPU: 3 PID: 14477 at mm/page_alloc.c:3511 > __alloc_pages_nodemask+0x159c/0x1e20 > Kernel panic - not syncing: panic_on_warn set ... > > CPU: 3 PID: 14477 Comm: syz-executor0 Not tainted 4.9.0-rc7+ #61 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 > 880039b67298 81f96bba 0200 11000736cde6 > ed000736cdde 0a06 41b58ab3 8598af00 > 81f96928 41b58ab3 859423c8 81432790 > Call Trace: > [< inline >] __dump_stack lib/dump_stack.c:15 > [] dump_stack+0x292/0x398 lib/dump_stack.c:51 > [] panic+0x1cb/0x3a9 kernel/panic.c:179 > [] __warn+0x1c4/0x1e0 kernel/panic.c:542 > [] warn_slowpath_null+0x2c/0x40 kernel/panic.c:585 > [< inline >] __alloc_pages_slowpath mm/page_alloc.c:3511 > [] __alloc_pages_nodemask+0x159c/0x1e20 > mm/page_alloc.c:3781 > [] alloc_pages_current+0x1c7/0x6b0 mm/mempolicy.c:2072 > [< inline >] alloc_pages include/linux/gfp.h:469 > [] kmalloc_order+0x1f/0x70 mm/slab_common.c:1015 > [] kmalloc_order_trace+0x1f/0x160 mm/slab_common.c:1026 > [< inline >] kmalloc_large include/linux/slab.h:422 > [] __kmalloc_track_caller+0x1df/0x290 mm/slub.c:4233 > [] memdup_user+0x2c/0xa0 mm/util.c:137 > [] dev_config+0x2e2/0x1180 > drivers/usb/gadget/legacy/inode.c:1776 > [] __vfs_write+0x5d5/0x760 fs/read_write.c:510 > [] vfs_write+0x170/0x4e0 fs/read_write.c:560 > [< inline >] SYSC_write fs/read_write.c:607 > [] SyS_write+0xfb/0x230 fs/read_write.c:599 > [] entry_SYSCALL_64_fastpath+0x1f/0xc2 > Dumping ftrace buffer: >(ftrace buffer empty) > Kernel Offset: disabled Oh how nice, we check to ensure that the buffer is not too small, but no one checks if it is too big :( Felipe, would a patch like the one below solve this? If so, I'll resend it in a "proper" format... thanks, greg k-h - diff --git a/drivers/usb/gadget/legacy/inode.c b/drivers/usb/gadget/legacy/inode.c index bd82dd12deff..42f9003b1621 100644 --- a/drivers/usb/gadget/legacy/inode.c +++ b/drivers/usb/gadget/legacy/inode.c @@ -1765,6 +1765,10 @@ dev_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr) if (len < (USB_DT_CONFIG_SIZE + USB_DT_DEVICE_SIZE + 4)) return -EINVAL; + /* No one needs more than 64k... */ + if (len > PAGE_SIZE * 8) + return -EINVAL; + /* we might need to change message format someday */ if (copy_from_user (&tag, buf, 4)) return -EFAULT; -- 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] usbip: vudc: fix: Clear already_seen flag also for ep0
On Fri, Dec 02, 2016 at 08:23:49AM -0700, Shuah Khan wrote:
> On 12/01/2016 11:14 AM, Krzysztof Opasiak wrote:
> > ep_list inside gadget structure doesn't contain ep0.
> > It is stored separately in ep0 field.
> >
> > This causes an urb hang if gadget driver decides to
> > delay setup handling. On host side this is visible as
> > timeout error when setting configuration.
> >
> > This bug can be reproduced using for example any gadget
> > with mass storage function.
> >
> > Fixes: abdb29574322 ("usbip: vudc: Add vudc_transfer")
> > Signed-off-by: Krzysztof Opasiak
>
> Greg,
>
> Assuming you want to continue in the same mode as you pulling
> in the USB/IP patches with Ack. This patch looks good. Please
> pick this up. If you want me to maintain USB/IP tree and send you
> pull request, I can do that as well. Whatever works for you.
>
> Acked-by: Shuah Khan
A simple "acked-by" like this is all I need, no need to spin up a whole
tree just for a single driver. I can take it from here.
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
RE: [PATCH] usb: gadget: udc: atmel: used managed kasprintf
From: Alexandre Belloni > Sent: 01 December 2016 10:27 > Use devm_kasprintf instead of simple kasprintf to free the allocated memory > when needed. s/when needed/when the device is freed/ > Suggested-by: Peter Rosin > Signed-off-by: Alexandre Belloni > --- > drivers/usb/gadget/udc/atmel_usba_udc.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c > b/drivers/usb/gadget/udc/atmel_usba_udc.c > index 45bc997d0711..aec72fe8273c 100644 > --- a/drivers/usb/gadget/udc/atmel_usba_udc.c > +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c > @@ -1978,7 +1978,8 @@ static struct usba_ep * atmel_udc_of_init(struct > platform_device *pdev, > dev_err(&pdev->dev, "of_probe: name error(%d)\n", ret); > goto err; > } > - ep->ep.name = kasprintf(GFP_KERNEL, "ep%d", ep->index); > + ep->ep.name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "ep%d", > + ep->index); Acually why bother mallocing such a small string at all. The maximum length is 12 bytes even if 'index' are unrestricted. David -- 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: usb/gadget: warning in dev_config/memdup_user
On Fri, Dec 02, 2016 at 04:56:15PM +0100, Greg Kroah-Hartman wrote: > On Fri, Dec 02, 2016 at 04:07:38PM +0100, Andrey Konovalov wrote: > > Hi! > > > > I've got the following error report while running the syzkaller fuzzer. > > > > The length passed to memdup_user() directly without limitations. > > > > On commit 2caceb3294a78c389b462e7e236a4e744a53a474 (Dec 1). > > > > WARNING: CPU: 3 PID: 14477 at mm/page_alloc.c:3511 > > __alloc_pages_nodemask+0x159c/0x1e20 > > Kernel panic - not syncing: panic_on_warn set ... > > > > CPU: 3 PID: 14477 Comm: syz-executor0 Not tainted 4.9.0-rc7+ #61 > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 > > 880039b67298 81f96bba 0200 11000736cde6 > > ed000736cdde 0a06 41b58ab3 8598af00 > > 81f96928 41b58ab3 859423c8 81432790 > > Call Trace: > > [< inline >] __dump_stack lib/dump_stack.c:15 > > [] dump_stack+0x292/0x398 lib/dump_stack.c:51 > > [] panic+0x1cb/0x3a9 kernel/panic.c:179 > > [] __warn+0x1c4/0x1e0 kernel/panic.c:542 > > [] warn_slowpath_null+0x2c/0x40 kernel/panic.c:585 > > [< inline >] __alloc_pages_slowpath mm/page_alloc.c:3511 > > [] __alloc_pages_nodemask+0x159c/0x1e20 > > mm/page_alloc.c:3781 > > [] alloc_pages_current+0x1c7/0x6b0 mm/mempolicy.c:2072 > > [< inline >] alloc_pages include/linux/gfp.h:469 > > [] kmalloc_order+0x1f/0x70 mm/slab_common.c:1015 > > [] kmalloc_order_trace+0x1f/0x160 mm/slab_common.c:1026 > > [< inline >] kmalloc_large include/linux/slab.h:422 > > [] __kmalloc_track_caller+0x1df/0x290 mm/slub.c:4233 > > [] memdup_user+0x2c/0xa0 mm/util.c:137 > > [] dev_config+0x2e2/0x1180 > > drivers/usb/gadget/legacy/inode.c:1776 > > [] __vfs_write+0x5d5/0x760 fs/read_write.c:510 > > [] vfs_write+0x170/0x4e0 fs/read_write.c:560 > > [< inline >] SYSC_write fs/read_write.c:607 > > [] SyS_write+0xfb/0x230 fs/read_write.c:599 > > [] entry_SYSCALL_64_fastpath+0x1f/0xc2 > > Dumping ftrace buffer: > >(ftrace buffer empty) > > Kernel Offset: disabled > > Oh how nice, we check to ensure that the buffer is not too small, but no > one checks if it is too big :( > > Felipe, would a patch like the one below solve this? If so, I'll resend > it in a "proper" format... > > thanks, > > greg k-h > > - > > diff --git a/drivers/usb/gadget/legacy/inode.c > b/drivers/usb/gadget/legacy/inode.c > index bd82dd12deff..42f9003b1621 100644 > --- a/drivers/usb/gadget/legacy/inode.c > +++ b/drivers/usb/gadget/legacy/inode.c > @@ -1765,6 +1765,10 @@ dev_config (struct file *fd, const char __user *buf, > size_t len, loff_t *ptr) > if (len < (USB_DT_CONFIG_SIZE + USB_DT_DEVICE_SIZE + 4)) > return -EINVAL; > > + /* No one needs more than 64k... */ > + if (len > PAGE_SIZE * 8) > + return -EINVAL; That's what I get for trying to be cute with comments right after a long lunch, the math is wrong... But the main concept should be sane. -- 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: usb: warning in vhci_hcd_probe/lockdep_init_map
On Fri, Dec 02, 2016 at 03:35:44PM +0100, Andrey Konovalov wrote: > Hi! > > I've got the following error report while booting the kernel with > various usb configs enabled. Any hint as to what these "various usb configs" are? > On commit 2caceb3294a78c389b462e7e236a4e744a53a474 (Dec 1). > > gadgetfs: USB Gadget filesystem, version 24 Aug 2004 > usbip_core: USB/IP Core v1.0.0 > vhci_hcd vhci_hcd: USB/IP Virtual Host Controller > vhci_hcd vhci_hcd: new USB bus registered, assigned bus number 2 > BUG: key 88006a7e8d18 not in .data! > [ cut here ] > WARNING: CPU: 0 PID: 1 at kernel/locking/lockdep.c:3131 > lockdep_init_map+0x60c/0x770 > DEBUG_LOCKS_WARN_ON(1)[1.567044] Modules linked in: > CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.9.0-rc7+ #58 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 > 88006bce6eb8 81f96c8a 0a02 11000d79cd6a > ed000d79cd62 00046bce6ed8 41b58ab3 8598af40 > 81f969f8 41b58ab3 0200 > Call Trace: > [< inline >] __dump_stack lib/dump_stack.c:15 > [] dump_stack+0x292/0x398 lib/dump_stack.c:51 > [] __warn+0x19f/0x1e0 kernel/panic.c:550 > [] warn_slowpath_fmt+0xc5/0x110 kernel/panic.c:565 > [] lockdep_init_map+0x60c/0x770 > kernel/locking/lockdep.c:3131 > [] __kernfs_create_file+0x114/0x2a0 fs/kernfs/file.c:954 > [] sysfs_add_file_mode_ns+0x225/0x520 fs/sysfs/file.c:305 > [< inline >] create_files fs/sysfs/group.c:64 > [] internal_create_group+0x239/0x8f0 fs/sysfs/group.c:134 > [] sysfs_create_group+0x1f/0x30 fs/sysfs/group.c:156 > [] vhci_start+0x5b4/0x7a0 drivers/usb/usbip/vhci_hcd.c:978 > [] usb_add_hcd+0x8da/0x1c60 drivers/usb/core/hcd.c:2867 > [] vhci_hcd_probe+0x97/0x130 > drivers/usb/usbip/vhci_hcd.c:1103 > [] platform_drv_probe+0x8a/0x180 > drivers/base/platform.c:568 > [< inline >] really_probe drivers/base/dd.c:375 > [] driver_probe_device+0x514/0x980 drivers/base/dd.c:515 > [] __device_attach_driver+0x22b/0x290 drivers/base/dd.c:610 > [] bus_for_each_drv+0x15c/0x200 drivers/base/bus.c:463 > [] __device_attach+0x266/0x3c0 drivers/base/dd.c:667 > [] device_initial_probe+0x1a/0x20 drivers/base/dd.c:714 > [] bus_probe_device+0x1e6/0x290 drivers/base/bus.c:557 > [] device_add+0xd06/0x1660 drivers/base/core.c:1136 > [] platform_device_add+0x315/0x660 > drivers/base/platform.c:408 > [] platform_device_register_full+0x396/0x4b0 > drivers/base/platform.c:540 > [< inline >] platform_device_register_resndata > ./include/linux/platform_device.h:111 > [< inline >] platform_device_register_simple > ./include/linux/platform_device.h:140 > [< inline >] add_platform_device drivers/usb/usbip/vhci_hcd.c:1213 > [] vhci_hcd_init+0x215/0x305 > drivers/usb/usbip/vhci_hcd.c:1254 > [] do_one_initcall+0xf6/0x360 init/main.c:778 > [< inline >] do_initcall_level init/main.c:844 > [< inline >] do_initcalls init/main.c:852 > [< inline >] do_basic_setup init/main.c:870 > [] kernel_init_freeable+0x5c7/0x6a1 init/main.c:1017 > [] kernel_init+0x13/0x180 init/main.c:943 > [] ret_from_fork+0x2a/0x40 arch/x86/entry/entry_64.S:433 > ---[ end trace c33c7b202cf3aac8 ]--- Odd, I don't see anything strange here. Shuah, any ideas what is going wrong here? 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
Re: usb: warning in vhci_hcd_probe/lockdep_init_map
On Fri, Dec 2, 2016 at 4:58 PM, Greg Kroah-Hartman wrote: > On Fri, Dec 02, 2016 at 03:35:44PM +0100, Andrey Konovalov wrote: >> Hi! >> >> I've got the following error report while booting the kernel with >> various usb configs enabled. > > Any hint as to what these "various usb configs" are? Hi Greg, Here's my .config: https://gist.github.com/xairy/46bc7495dfa7d78701d937af81db0175 Thanks for looking at this! > >> On commit 2caceb3294a78c389b462e7e236a4e744a53a474 (Dec 1). >> >> gadgetfs: USB Gadget filesystem, version 24 Aug 2004 >> usbip_core: USB/IP Core v1.0.0 >> vhci_hcd vhci_hcd: USB/IP Virtual Host Controller >> vhci_hcd vhci_hcd: new USB bus registered, assigned bus number 2 >> BUG: key 88006a7e8d18 not in .data! >> [ cut here ] >> WARNING: CPU: 0 PID: 1 at kernel/locking/lockdep.c:3131 >> lockdep_init_map+0x60c/0x770 >> DEBUG_LOCKS_WARN_ON(1)[1.567044] Modules linked in: >> CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.9.0-rc7+ #58 >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 >> 88006bce6eb8 81f96c8a 0a02 11000d79cd6a >> ed000d79cd62 00046bce6ed8 41b58ab3 8598af40 >> 81f969f8 41b58ab3 0200 >> Call Trace: >> [< inline >] __dump_stack lib/dump_stack.c:15 >> [] dump_stack+0x292/0x398 lib/dump_stack.c:51 >> [] __warn+0x19f/0x1e0 kernel/panic.c:550 >> [] warn_slowpath_fmt+0xc5/0x110 kernel/panic.c:565 >> [] lockdep_init_map+0x60c/0x770 >> kernel/locking/lockdep.c:3131 >> [] __kernfs_create_file+0x114/0x2a0 fs/kernfs/file.c:954 >> [] sysfs_add_file_mode_ns+0x225/0x520 fs/sysfs/file.c:305 >> [< inline >] create_files fs/sysfs/group.c:64 >> [] internal_create_group+0x239/0x8f0 fs/sysfs/group.c:134 >> [] sysfs_create_group+0x1f/0x30 fs/sysfs/group.c:156 >> [] vhci_start+0x5b4/0x7a0 drivers/usb/usbip/vhci_hcd.c:978 >> [] usb_add_hcd+0x8da/0x1c60 drivers/usb/core/hcd.c:2867 >> [] vhci_hcd_probe+0x97/0x130 >> drivers/usb/usbip/vhci_hcd.c:1103 >> [] platform_drv_probe+0x8a/0x180 >> drivers/base/platform.c:568 >> [< inline >] really_probe drivers/base/dd.c:375 >> [] driver_probe_device+0x514/0x980 drivers/base/dd.c:515 >> [] __device_attach_driver+0x22b/0x290 >> drivers/base/dd.c:610 >> [] bus_for_each_drv+0x15c/0x200 drivers/base/bus.c:463 >> [] __device_attach+0x266/0x3c0 drivers/base/dd.c:667 >> [] device_initial_probe+0x1a/0x20 drivers/base/dd.c:714 >> [] bus_probe_device+0x1e6/0x290 drivers/base/bus.c:557 >> [] device_add+0xd06/0x1660 drivers/base/core.c:1136 >> [] platform_device_add+0x315/0x660 >> drivers/base/platform.c:408 >> [] platform_device_register_full+0x396/0x4b0 >> drivers/base/platform.c:540 >> [< inline >] platform_device_register_resndata >> ./include/linux/platform_device.h:111 >> [< inline >] platform_device_register_simple >> ./include/linux/platform_device.h:140 >> [< inline >] add_platform_device drivers/usb/usbip/vhci_hcd.c:1213 >> [] vhci_hcd_init+0x215/0x305 >> drivers/usb/usbip/vhci_hcd.c:1254 >> [] do_one_initcall+0xf6/0x360 init/main.c:778 >> [< inline >] do_initcall_level init/main.c:844 >> [< inline >] do_initcalls init/main.c:852 >> [< inline >] do_basic_setup init/main.c:870 >> [] kernel_init_freeable+0x5c7/0x6a1 init/main.c:1017 >> [] kernel_init+0x13/0x180 init/main.c:943 >> [] ret_from_fork+0x2a/0x40 arch/x86/entry/entry_64.S:433 >> ---[ end trace c33c7b202cf3aac8 ]--- > > Odd, I don't see anything strange here. Shuah, any ideas what is going > wrong here? > > 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
Re: [PATCH] usb: gadget: udc: atmel: used managed kasprintf
On 02/12/2016 at 15:59:57 +, David Laight wrote : > From: Alexandre Belloni > > Sent: 01 December 2016 10:27 > > Use devm_kasprintf instead of simple kasprintf to free the allocated memory > > when needed. > > s/when needed/when the device is freed/ > > > Suggested-by: Peter Rosin > > Signed-off-by: Alexandre Belloni > > --- > > drivers/usb/gadget/udc/atmel_usba_udc.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c > > b/drivers/usb/gadget/udc/atmel_usba_udc.c > > index 45bc997d0711..aec72fe8273c 100644 > > --- a/drivers/usb/gadget/udc/atmel_usba_udc.c > > +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c > > @@ -1978,7 +1978,8 @@ static struct usba_ep * atmel_udc_of_init(struct > > platform_device *pdev, > > dev_err(&pdev->dev, "of_probe: name error(%d)\n", ret); > > goto err; > > } > > - ep->ep.name = kasprintf(GFP_KERNEL, "ep%d", ep->index); > > + ep->ep.name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "ep%d", > > +ep->index); > > Acually why bother mallocing such a small string at all. > The maximum length is 12 bytes even if 'index' are unrestricted. > IIRC, using statically allocated string is failing somewhere is the USB core but I don't remember all the details. -- Alexandre Belloni, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- 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: usb: warning in vhci_hcd_probe/lockdep_init_map
On 12/02/2016 09:09 AM, Andrey Konovalov wrote: > On Fri, Dec 2, 2016 at 4:58 PM, Greg Kroah-Hartman > wrote: >> On Fri, Dec 02, 2016 at 03:35:44PM +0100, Andrey Konovalov wrote: >>> Hi! >>> >>> I've got the following error report while booting the kernel with >>> various usb configs enabled. >> >> Any hint as to what these "various usb configs" are? > > Hi Greg, > > Here's my .config: > https://gist.github.com/xairy/46bc7495dfa7d78701d937af81db0175 > > Thanks for looking at this! > >> >>> On commit 2caceb3294a78c389b462e7e236a4e744a53a474 (Dec 1). Looks like the above commit at the latest on Linus's master that is do with ovl: fix d_real() for stacked fs Nothing jumped out at me from the log. What's your ability to bisect the problem? >>> >>> gadgetfs: USB Gadget filesystem, version 24 Aug 2004 >>> usbip_core: USB/IP Core v1.0.0 >>> vhci_hcd vhci_hcd: USB/IP Virtual Host Controller >>> vhci_hcd vhci_hcd: new USB bus registered, assigned bus number 2 >>> BUG: key 88006a7e8d18 not in .data! >>> [ cut here ] >>> WARNING: CPU: 0 PID: 1 at kernel/locking/lockdep.c:3131 >>> lockdep_init_map+0x60c/0x770 >>> DEBUG_LOCKS_WARN_ON(1)[1.567044] Modules linked in: >>> CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.9.0-rc7+ #58 >>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 >>> 88006bce6eb8 81f96c8a 0a02 11000d79cd6a >>> ed000d79cd62 00046bce6ed8 41b58ab3 8598af40 >>> 81f969f8 41b58ab3 0200 >>> Call Trace: >>> [< inline >] __dump_stack lib/dump_stack.c:15 >>> [] dump_stack+0x292/0x398 lib/dump_stack.c:51 >>> [] __warn+0x19f/0x1e0 kernel/panic.c:550 >>> [] warn_slowpath_fmt+0xc5/0x110 kernel/panic.c:565 >>> [] lockdep_init_map+0x60c/0x770 >>> kernel/locking/lockdep.c:3131 >>> [] __kernfs_create_file+0x114/0x2a0 fs/kernfs/file.c:954 >>> [] sysfs_add_file_mode_ns+0x225/0x520 fs/sysfs/file.c:305 >>> [< inline >] create_files fs/sysfs/group.c:64 >>> [] internal_create_group+0x239/0x8f0 fs/sysfs/group.c:134 >>> [] sysfs_create_group+0x1f/0x30 fs/sysfs/group.c:156 >>> [] vhci_start+0x5b4/0x7a0 >>> drivers/usb/usbip/vhci_hcd.c:978 >>> [] usb_add_hcd+0x8da/0x1c60 drivers/usb/core/hcd.c:2867 >>> [] vhci_hcd_probe+0x97/0x130 >>> drivers/usb/usbip/vhci_hcd.c:1103 >>> [] platform_drv_probe+0x8a/0x180 >>> drivers/base/platform.c:568 >>> [< inline >] really_probe drivers/base/dd.c:375 >>> [] driver_probe_device+0x514/0x980 drivers/base/dd.c:515 >>> [] __device_attach_driver+0x22b/0x290 >>> drivers/base/dd.c:610 >>> [] bus_for_each_drv+0x15c/0x200 drivers/base/bus.c:463 >>> [] __device_attach+0x266/0x3c0 drivers/base/dd.c:667 >>> [] device_initial_probe+0x1a/0x20 drivers/base/dd.c:714 >>> [] bus_probe_device+0x1e6/0x290 drivers/base/bus.c:557 >>> [] device_add+0xd06/0x1660 drivers/base/core.c:1136 >>> [] platform_device_add+0x315/0x660 >>> drivers/base/platform.c:408 >>> [] platform_device_register_full+0x396/0x4b0 >>> drivers/base/platform.c:540 >>> [< inline >] platform_device_register_resndata >>> ./include/linux/platform_device.h:111 >>> [< inline >] platform_device_register_simple >>> ./include/linux/platform_device.h:140 >>> [< inline >] add_platform_device drivers/usb/usbip/vhci_hcd.c:1213 >>> [] vhci_hcd_init+0x215/0x305 >>> drivers/usb/usbip/vhci_hcd.c:1254 >>> [] do_one_initcall+0xf6/0x360 init/main.c:778 >>> [< inline >] do_initcall_level init/main.c:844 >>> [< inline >] do_initcalls init/main.c:852 >>> [< inline >] do_basic_setup init/main.c:870 >>> [] kernel_init_freeable+0x5c7/0x6a1 init/main.c:1017 >>> [] kernel_init+0x13/0x180 init/main.c:943 >>> [] ret_from_fork+0x2a/0x40 arch/x86/entry/entry_64.S:433 >>> ---[ end trace c33c7b202cf3aac8 ]--- >> >> Odd, I don't see anything strange here. Shuah, any ideas what is going >> wrong here? I will take a look at this. Nothing jumped out at me. >> >> thanks, >> >> greg k-h > > 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: usb: warning in vhci_hcd_probe/lockdep_init_map
On 12/02/2016 09:23 AM, Shuah Khan wrote: > On 12/02/2016 09:09 AM, Andrey Konovalov wrote: >> On Fri, Dec 2, 2016 at 4:58 PM, Greg Kroah-Hartman >> wrote: >>> On Fri, Dec 02, 2016 at 03:35:44PM +0100, Andrey Konovalov wrote: Hi! I've got the following error report while booting the kernel with various usb configs enabled. >>> >>> Any hint as to what these "various usb configs" are? >> >> Hi Greg, >> >> Here's my .config: >> https://gist.github.com/xairy/46bc7495dfa7d78701d937af81db0175 >> >> Thanks for looking at this! >> >>> On commit 2caceb3294a78c389b462e7e236a4e744a53a474 (Dec 1). > > Looks like the above commit at the latest on Linus's master > that is do with > > ovl: fix d_real() for stacked fs > > Nothing jumped out at me from the log. What's your ability to > bisect the problem? Greg/Andrey, Okay looks like the problem is due to vhci using dynamically allocated memory for the lock_key. This problem is introduced in commit: 0775a9cbc694e8c7276688be3bbd2f386167ab54 >From 0775a9cbc694e8c7276688be3bbd2f386167ab54 Mon Sep 17 00:00:00 2001 From: Nobuo Iwata Date: Mon, 13 Jun 2016 11:33:40 +0900 Subject: [PATCH] usbip: vhci extension: modifications to vhci driver which changed the attribute_group from static to dynamically allocated. I will see if I can fix this without undoing the commit itself thanks, -- Shuah > gadgetfs: USB Gadget filesystem, version 24 Aug 2004 usbip_core: USB/IP Core v1.0.0 vhci_hcd vhci_hcd: USB/IP Virtual Host Controller vhci_hcd vhci_hcd: new USB bus registered, assigned bus number 2 BUG: key 88006a7e8d18 not in .data! [ cut here ] WARNING: CPU: 0 PID: 1 at kernel/locking/lockdep.c:3131 lockdep_init_map+0x60c/0x770 DEBUG_LOCKS_WARN_ON(1)[1.567044] Modules linked in: CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.9.0-rc7+ #58 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 88006bce6eb8 81f96c8a 0a02 11000d79cd6a ed000d79cd62 00046bce6ed8 41b58ab3 8598af40 81f969f8 41b58ab3 0200 Call Trace: [< inline >] __dump_stack lib/dump_stack.c:15 [] dump_stack+0x292/0x398 lib/dump_stack.c:51 [] __warn+0x19f/0x1e0 kernel/panic.c:550 [] warn_slowpath_fmt+0xc5/0x110 kernel/panic.c:565 [] lockdep_init_map+0x60c/0x770 kernel/locking/lockdep.c:3131 [] __kernfs_create_file+0x114/0x2a0 fs/kernfs/file.c:954 [] sysfs_add_file_mode_ns+0x225/0x520 fs/sysfs/file.c:305 [< inline >] create_files fs/sysfs/group.c:64 [] internal_create_group+0x239/0x8f0 fs/sysfs/group.c:134 [] sysfs_create_group+0x1f/0x30 fs/sysfs/group.c:156 [] vhci_start+0x5b4/0x7a0 drivers/usb/usbip/vhci_hcd.c:978 [] usb_add_hcd+0x8da/0x1c60 drivers/usb/core/hcd.c:2867 [] vhci_hcd_probe+0x97/0x130 drivers/usb/usbip/vhci_hcd.c:1103 [] platform_drv_probe+0x8a/0x180 drivers/base/platform.c:568 [< inline >] really_probe drivers/base/dd.c:375 [] driver_probe_device+0x514/0x980 drivers/base/dd.c:515 [] __device_attach_driver+0x22b/0x290 drivers/base/dd.c:610 [] bus_for_each_drv+0x15c/0x200 drivers/base/bus.c:463 [] __device_attach+0x266/0x3c0 drivers/base/dd.c:667 [] device_initial_probe+0x1a/0x20 drivers/base/dd.c:714 [] bus_probe_device+0x1e6/0x290 drivers/base/bus.c:557 [] device_add+0xd06/0x1660 drivers/base/core.c:1136 [] platform_device_add+0x315/0x660 drivers/base/platform.c:408 [] platform_device_register_full+0x396/0x4b0 drivers/base/platform.c:540 [< inline >] platform_device_register_resndata ./include/linux/platform_device.h:111 [< inline >] platform_device_register_simple ./include/linux/platform_device.h:140 [< inline >] add_platform_device drivers/usb/usbip/vhci_hcd.c:1213 [] vhci_hcd_init+0x215/0x305 drivers/usb/usbip/vhci_hcd.c:1254 [] do_one_initcall+0xf6/0x360 init/main.c:778 [< inline >] do_initcall_level init/main.c:844 [< inline >] do_initcalls init/main.c:852 [< inline >] do_basic_setup init/main.c:870 [] kernel_init_freeable+0x5c7/0x6a1 init/main.c:1017 [] kernel_init+0x13/0x180 init/main.c:943 [] ret_from_fork+0x2a/0x40 arch/x86/entry/entry_64.S:433 ---[ end trace c33c7b202cf3aac8 ]--- >>> >>> Odd, I don't see anything strange here. Shuah, any ideas what is going >>> wrong here? > > I will take a look at this. Nothing jumped out at me. > >>> >>> thanks, >>> >>> greg k-h >> >> > > 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/majordom
Re: [PATCH v13 09/10] usbip: exporting devices: chage to documenattion
On 11/22/2016 07:48 AM, Nobuo Iwata wrote: > This patch adds function and usage of new connect operation, disconnect > operation and application(vhci)-side daemon to README and manuals. > > At this point, the wording, 'server' and 'client' are ambiguous in > several place. > > For existing attach command, the daemon runs device side machine and > attach command is executed in application side machine. Then 'server' > is used for device side and 'client' is for application side. > > For the new connect command, the daemon runs applications side machine > and connect command is executed in device side machine. Now, 'server' > and 'client' run in different machine than before. > > To avoid confusion, to represent things to be done in device side node > by both command and daemon, words 'device-side' is used instead of > 'server'. To represent things to be done is application side node by > both command and daemon, 'applicationr-side' are used instead of > 'client'. > In my humble opinion device side is not a good term. Meaning of Device Side means USB gadget so using it here may cause misunderstanding. Application side I think is also not a perfect choice. Current terminology (server/client) is also imperfect after this series. Maybe using a terms from semiconductors world like donor (machine which provides the device) and acceptor (machine which accepts the device) would be better? Best regards, -- Krzysztof Opasiak Samsung R&D 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 v13 10/10] usbip: exporting devices: modifications to protocol text
On 11/22/2016 07:48 AM, Nobuo Iwata wrote: > This patch adds export and un-export request/response to > Documentation/usb/usbip_protocol.txt. > > The definition of the structs has been defined in original code of > tools/usb/usbip/usbip_network.h but not described in the document. > > Adding the request and response, words 'server' and 'client' are > ambiguous in several place. To avoid confusion, 'device-side' and > 'application-side' are written together with 'server' and 'client'. > > 'export' was used in the counter side of 'import' request. This patch > organizes usage of 'import' and 'export'. > I would be very happy if we could avoid device and application side terms like I described in previous email. > Signed-off-by: Nobuo Iwata > --- > Documentation/usb/usbip_protocol.txt | 204 --- > 1 file changed, 181 insertions(+), 23 deletions(-) > > diff --git a/Documentation/usb/usbip_protocol.txt > b/Documentation/usb/usbip_protocol.txt > index 16b6fe2..d4be5b6 100644 > --- a/Documentation/usb/usbip_protocol.txt > +++ b/Documentation/usb/usbip_protocol.txt > @@ -1,20 +1,26 @@ > PRELIMINARY DRAFT, MAY CONTAIN MISTAKES! > 28 Jun 2011 > +MODIFIED FOR CONNECT AND DISCONNECT OPERARION. > +07 March 2016 > > -The USB/IP protocol follows a server/client architecture. The server exports > the > -USB devices and the clients imports them. The device driver for the exported > -USB device runs on the client machine. > +The USB/IP protocol follows a server/client architecture between two > computers > +one has USB devices and the other runs application using the devices. There > are > +two ways for initiation. > > -The client may ask for the list of the exported USB devices. To get the list > the > -client opens a TCP/IP connection towards the server, and sends an > OP_REQ_DEVLIST > -packet on top of the TCP/IP connection (so the actual OP_REQ_DEVLIST may be > sent > -in one or more pieces at the low level transport layer). The server sends > back > -the OP_REP_DEVLIST packet which lists the exported USB devices. Finally the > -TCP/IP connection is closed. > +The first way is to import devices from application-side. In this way, the > +server runs in device-side and the client runs in application-side. In device > +side user makes devices importable with 'bind' operation. > > +The client may ask for the list of the importable USB devices. To get the > list > +the client opens a TCP/IP connection towards the server, and sends an > +OP_REQ_DEVLIST packet on top of the TCP/IP connection (so the actual > +OP_REQ_DEVLIST may be sent in one or more pieces at the low level transport > +layer). The server sends back the OP_REP_DEVLIST packet which lists the > +importable USB devices. Finally the TCP/IP connection is closed. > + > + application-sidedevice-side > virtual host controller usb host > - "client" "server" > - (imports USB devices) (exports USB devices) > + "client" (lists importable devices)"server" >| | >| OP_REQ_DEVLIST | >| --> | > @@ -23,18 +29,13 @@ TCP/IP connection is closed. >| <-- | >| | > > -Once the client knows the list of exported USB devices it may decide to use > one > -of them. First the client opens a TCP/IP connection towards the server and > -sends an OP_REQ_IMPORT packet. The server replies with OP_REP_IMPORT. If the > -import was successful the TCP/IP connection remains open and will be used > -to transfer the URB traffic between the client and the server. The client may > -send two types of packets: the USBIP_CMD_SUBMIT to submit an URB, and > -USBIP_CMD_UNLINK to unlink a previously submitted URB. The answers of the > -server may be USBIP_RET_SUBMIT and USBIP_RET_UNLINK respectively. > +Once the client knows the list of importable USB devices it may decide to use > +one of them. First the client opens a TCP/IP connection towards the server > and > +sends an OP_REQ_IMPORT packet. The server replies with OP_REP_IMPORT. > > + application-sidedevice-side > virtual host controller usb host > - "client" "server" > - (imports USB devices) (exports USB devices) > + "client" (imports a USB device) "server" >| | >| OP_REQ_IMPORT | >| --> | > @@ -42,6 +43,32 @@ server may be USB
Re: [PATCH v13 09/10] usbip: exporting devices: chage to documenattion
On 12/02/2016 06:21 PM, Shuah Khan wrote: > On 12/02/2016 10:16 AM, Krzysztof Opasiak wrote: >> >> >> On 11/22/2016 07:48 AM, Nobuo Iwata wrote: >>> This patch adds function and usage of new connect operation, disconnect >>> operation and application(vhci)-side daemon to README and manuals. >>> >>> At this point, the wording, 'server' and 'client' are ambiguous in >>> several place. >>> >>> For existing attach command, the daemon runs device side machine and >>> attach command is executed in application side machine. Then 'server' >>> is used for device side and 'client' is for application side. >>> >>> For the new connect command, the daemon runs applications side machine >>> and connect command is executed in device side machine. Now, 'server' >>> and 'client' run in different machine than before. >>> >>> To avoid confusion, to represent things to be done in device side node >>> by both command and daemon, words 'device-side' is used instead of >>> 'server'. To represent things to be done is application side node by >>> both command and daemon, 'applicationr-side' are used instead of >>> 'client'. >>> >> >> In my humble opinion device side is not a good term. >> >> Meaning of Device Side means USB gadget so using it here may cause >> misunderstanding. Application side I think is also not a perfect choice. >> >> Current terminology (server/client) is also imperfect after this series. > > Introduce a new word for the intermediate server - In any case I have yet > to see a good argument for why we need this patch series. > I will post my summary after going through all patches in this series as a reply to cover letter. >> >> Maybe using a terms from semiconductors world like donor (machine which >> provides the device) and acceptor (machine which accepts the device) >> would be better? >> >> Best regards, >> > > Please don't introduce new terminology. The current terminology is > correct and makes sense: > > Server - system with device physically attached that exports the devices > Client - system that imports the device. Totally agree that it makes sens unless this series is applied. If we apply this series then server and client is a little bit ambiguous. Best regards, -- Krzysztof Opasiak Samsung R&D 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 v13 09/10] usbip: exporting devices: chage to documenattion
On 12/02/2016 10:16 AM, Krzysztof Opasiak wrote: > > > On 11/22/2016 07:48 AM, Nobuo Iwata wrote: >> This patch adds function and usage of new connect operation, disconnect >> operation and application(vhci)-side daemon to README and manuals. >> >> At this point, the wording, 'server' and 'client' are ambiguous in >> several place. >> >> For existing attach command, the daemon runs device side machine and >> attach command is executed in application side machine. Then 'server' >> is used for device side and 'client' is for application side. >> >> For the new connect command, the daemon runs applications side machine >> and connect command is executed in device side machine. Now, 'server' >> and 'client' run in different machine than before. >> >> To avoid confusion, to represent things to be done in device side node >> by both command and daemon, words 'device-side' is used instead of >> 'server'. To represent things to be done is application side node by >> both command and daemon, 'applicationr-side' are used instead of >> 'client'. >> > > In my humble opinion device side is not a good term. > > Meaning of Device Side means USB gadget so using it here may cause > misunderstanding. Application side I think is also not a perfect choice. > > Current terminology (server/client) is also imperfect after this series. Introduce a new word for the intermediate server - In any case I have yet to see a good argument for why we need this patch series. > > Maybe using a terms from semiconductors world like donor (machine which > provides the device) and acceptor (machine which accepts the device) > would be better? > > Best regards, > Please don't introduce new terminology. The current terminology is correct and makes sense: Server - system with device physically attached that exports the devices Client - system that imports the device. thanks, -- Shuah -- Shuah Khan Sr. Linux Kernel Developer Open Source Innovation Group Samsung Research America(Silicon Valley) shuah...@samsung.com -- 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 v2] usbip: vudc: Refactor init_vudc_hw() to be more obvious
Current implementation of init_vudc_hw() adds ep0 to ep_list
and then after looping through all endpoints removes it from
that list.
As this may be misleading let's refactor this function
and avoid adding and removing ep0 to eplist and place it
immediately in correct place.
In addition let's remove redundant 0 assignments as ep
array is zeroed during allocation.
Signed-off-by: Krzysztof Opasiak
---
Changes since v1:
- remove redundant assignments
---
drivers/usb/usbip/vudc_dev.c | 35 ++-
1 file changed, 18 insertions(+), 17 deletions(-)
diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c
index 7091848df6c8..70b3540ece2a 100644
--- a/drivers/usb/usbip/vudc_dev.c
+++ b/drivers/usb/usbip/vudc_dev.c
@@ -549,30 +549,34 @@ static int init_vudc_hw(struct vudc *udc)
sprintf(ep->name, "ep%d%s", num,
i ? (is_out ? "out" : "in") : "");
ep->ep.name = ep->name;
+
+ ep->ep.ops = &vep_ops;
+
+ usb_ep_set_maxpacket_limit(&ep->ep, ~0);
+ ep->ep.max_streams = 16;
+ ep->gadget = &udc->gadget;
+ INIT_LIST_HEAD(&ep->req_queue);
+
if (i == 0) {
+ /* ep0 */
ep->ep.caps.type_control = true;
ep->ep.caps.dir_out = true;
ep->ep.caps.dir_in = true;
+
+ udc->gadget.ep0 = &ep->ep;
} else {
+ /* All other eps */
ep->ep.caps.type_iso = true;
ep->ep.caps.type_int = true;
ep->ep.caps.type_bulk = true;
- }
- if (is_out)
- ep->ep.caps.dir_out = true;
- else
- ep->ep.caps.dir_in = true;
+ if (is_out)
+ ep->ep.caps.dir_out = true;
+ else
+ ep->ep.caps.dir_in = true;
- ep->ep.ops = &vep_ops;
- list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list);
- ep->halted = ep->wedged = ep->already_seen =
- ep->setup_stage = 0;
- usb_ep_set_maxpacket_limit(&ep->ep, ~0);
- ep->ep.max_streams = 16;
- ep->gadget = &udc->gadget;
- ep->desc = NULL;
- INIT_LIST_HEAD(&ep->req_queue);
+ list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list);
+ }
}
spin_lock_init(&udc->lock);
@@ -589,9 +593,6 @@ static int init_vudc_hw(struct vudc *udc)
ud->eh_ops.reset= vudc_device_reset;
ud->eh_ops.unusable = vudc_device_unusable;
- udc->gadget.ep0 = &udc->ep[0].ep;
- list_del_init(&udc->ep[0].ep.ep_list);
-
v_init_timer(udc);
return 0;
--
2.9.3
--
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: XHCI controller does not detect USB key insertion
On 02/12/2016 14:46, Neil Armstrong wrote: > On 12/02/2016 11:24 AM, Mason wrote: > >> (Sad face) All the documentation I have is in front of me, and nothing >> is ringing a bell. This is a Sigma Designs SoC, with a Pravega XHCI >> controller + Synopsys PHY. >> >> The documentation I have: >> >> Pravega_Dual_Mode_Datasheet_v10c.pdf (documents IP signals) >> Pravega_Dual_Mode_Controller_Programmers_Reference_manual_v1.pdf (documents >> IP registers) >> PHY databook (very low-level stuff) >> SoC register mapping (for how the SoC maps the IP signals to registers) > > You should have all the necessary bits to enable and configure the Embedded > Synopsys PHY ! > > You should have some register mapping of the PHY signals, or at least a way > to write those registers. > > You should have a reset, clock gate and eventually a power regulator to > enable in order to have the PHY running. I'll dump all the non-0 non-standard registers. Maybe someone more experienced than me will spot an obvious mistake. host_usb30_0_config: 0x2e800 - host_usb30_0_fladj 0x20 - host_usb30_0_usb30_controller_cg_disable 0x0 - host_usb30_0_mode_select 0x1 - host_usb30_0_device_reset_mode 0x0 host_usb30_0_control: 0x2e804 - host_usb30_0_app_lfps_u3_wp0x0 - host_usb30_0_link_up 0x1 - host_usb30_0_msi_msg_sent 0x0 - host_usb30_0_usb3_p0_over_current 0x0 - host_usb30_0_usb2_p0_over_current 0x0 host_usb30_0_test: 0x2e808 - host_usb30_0_test_powerdown_hsp0x0 - host_usb30_0_test_powerdown_ssp0x0 - host_usb30_0_test_burnin 0x0 - host_usb30_0_acjt_level0x14 - host_usb30_0_lane0_tx2rx_loopbk0x0 - host_usb30_0_rtune_req 0x0 host_usb30_0_status: 0x2e80c - host_usb30_0_phystatus 0x0 - host_usb30_0_usb2_p0_pp0x1 - host_usb30_0_usb3_p0_pp0x1 - host_usb30_0_usb3_sleep0x0 - host_usb30_0_rtune_ack 0x0 host_usb30_0_clk_rst_0: 0x2e810 - host_usb30_0_commononn 0x1 - host_usb30_0_portreset 0x0 - host_usb30_0_refclksel 0x2 - host_usb30_0_teneable 0x1 - host_usb30_0_fsel 0x27 - host_usb30_0_mpll_multiplier 0x19 - host_usb30_0_ref_clkdiv2 0x0 - host_usb30_0_ref_ssp_en0x1 - host_usb30_0_ref_use_pad 0x0 - host_usb30_0_ssc_en0x1 - host_usb30_0_ssc_range 0x0 host_usb30_0_clk_rst_1: 0x2e814 - host_usb30_0_ssc_ref_clk_sel 0x88 - host_usb30_0_sleepm0x1 - host_usb30_0_vbusvldext0x1 host_usb30_0_param_0: 0x2e818 - host_usb30_0_compdistune 0x4 - host_usb30_0_otgtune 0x4 - host_usb30_0_sqrxtune 0x3 - host_usb30_0_txfsltune 0x3 - host_usb30_0_txhsxvtune0x3 - host_usb30_0_txpreempltune 0x0 - host_usb30_0_txpreemppulsetune 0x0 - host_usb30_0_txrestune 0x1 - host_usb30_0_txrisetune0x2 - host_usb30_0_txvreftune0x4 host_usb30_0_param_1: 0x2e81c - host_usb30_0_los_bias 0x5 - host_usb30_0_los_level 0xc - host_usb30_0_pcs_rx_los_mask_val 0xf0 - host_usb30_0_pcs_tx_deemph_3p5db 0x18 - host_usb30_0_pcs_tx_deemph_6db 0x21 host_usb30_0_param_2: 0x2e820 - host_usb30_0_pcs_tx_swing_full 0x73 - host_usb30_0_lane0_tx_term_offset 0x0 - host_usb30_0_tx_vboost_lvl 0x4 host_usb30_0_SNPS_CR_ADD: 0x2e880 - host_usb30_0_snps_cr_add 0xe03c DEVICE_AND_PORT_000: 0x7005 - sw_reset 0x0 - gen_resume 0x0 - ss_support
Re: XHCI controller does not detect USB key insertion
[ Fix incorrect address for Felipe ] On 02/12/2016 14:46, Neil Armstrong wrote: > On 12/02/2016 11:24 AM, Mason wrote: > >> (Sad face) All the documentation I have is in front of me, and nothing >> is ringing a bell. This is a Sigma Designs SoC, with a Pravega XHCI >> controller + Synopsys PHY. >> >> The documentation I have: >> >> Pravega_Dual_Mode_Datasheet_v10c.pdf (documents IP signals) >> Pravega_Dual_Mode_Controller_Programmers_Reference_manual_v1.pdf (documents >> IP registers) >> PHY databook (very low-level stuff) >> SoC register mapping (for how the SoC maps the IP signals to registers) > > You should have all the necessary bits to enable and configure the Embedded > Synopsys PHY ! > > You should have some register mapping of the PHY signals, or at least a way > to write those registers. > > You should have a reset, clock gate and eventually a power regulator to > enable in order to have the PHY running. I'll dump all the non-0 non-standard registers. Maybe someone more experienced than me will spot an obvious mistake. host_usb30_0_config: 0x2e800 - host_usb30_0_fladj 0x20 - host_usb30_0_usb30_controller_cg_disable 0x0 - host_usb30_0_mode_select 0x1 - host_usb30_0_device_reset_mode 0x0 host_usb30_0_control: 0x2e804 - host_usb30_0_app_lfps_u3_wp0x0 - host_usb30_0_link_up 0x1 - host_usb30_0_msi_msg_sent 0x0 - host_usb30_0_usb3_p0_over_current 0x0 - host_usb30_0_usb2_p0_over_current 0x0 host_usb30_0_test: 0x2e808 - host_usb30_0_test_powerdown_hsp0x0 - host_usb30_0_test_powerdown_ssp0x0 - host_usb30_0_test_burnin 0x0 - host_usb30_0_acjt_level0x14 - host_usb30_0_lane0_tx2rx_loopbk0x0 - host_usb30_0_rtune_req 0x0 host_usb30_0_status: 0x2e80c - host_usb30_0_phystatus 0x0 - host_usb30_0_usb2_p0_pp0x1 - host_usb30_0_usb3_p0_pp0x1 - host_usb30_0_usb3_sleep0x0 - host_usb30_0_rtune_ack 0x0 host_usb30_0_clk_rst_0: 0x2e810 - host_usb30_0_commononn 0x1 - host_usb30_0_portreset 0x0 - host_usb30_0_refclksel 0x2 - host_usb30_0_teneable 0x1 - host_usb30_0_fsel 0x27 - host_usb30_0_mpll_multiplier 0x19 - host_usb30_0_ref_clkdiv2 0x0 - host_usb30_0_ref_ssp_en0x1 - host_usb30_0_ref_use_pad 0x0 - host_usb30_0_ssc_en0x1 - host_usb30_0_ssc_range 0x0 host_usb30_0_clk_rst_1: 0x2e814 - host_usb30_0_ssc_ref_clk_sel 0x88 - host_usb30_0_sleepm0x1 - host_usb30_0_vbusvldext0x1 host_usb30_0_param_0: 0x2e818 - host_usb30_0_compdistune 0x4 - host_usb30_0_otgtune 0x4 - host_usb30_0_sqrxtune 0x3 - host_usb30_0_txfsltune 0x3 - host_usb30_0_txhsxvtune0x3 - host_usb30_0_txpreempltune 0x0 - host_usb30_0_txpreemppulsetune 0x0 - host_usb30_0_txrestune 0x1 - host_usb30_0_txrisetune0x2 - host_usb30_0_txvreftune0x4 host_usb30_0_param_1: 0x2e81c - host_usb30_0_los_bias 0x5 - host_usb30_0_los_level 0xc - host_usb30_0_pcs_rx_los_mask_val 0xf0 - host_usb30_0_pcs_tx_deemph_3p5db 0x18 - host_usb30_0_pcs_tx_deemph_6db 0x21 host_usb30_0_param_2: 0x2e820 - host_usb30_0_pcs_tx_swing_full 0x73 - host_usb30_0_lane0_tx_term_offset 0x0 - host_usb30_0_tx_vboost_lvl 0x4 host_usb30_0_SNPS_CR_ADD: 0x2e880 - host_usb30_0_snps_cr_add 0xe03c DEVICE_AND_PORT_000: 0x7005 - sw_reset 0x0 - gen_resume 0x0 - s
Re: [PATCHv13 2/3] usb: USB Type-C connector class
On Wed, Nov 30, 2016 at 11:19:10AM +0200, Heikki Krogerus wrote:
> Hi Greg,
>
> On Tue, Nov 29, 2016 at 05:27:44PM +0100, Greg KH wrote:
> > > +struct typec_cable {
> > > + struct device dev;
> > > + enum typec_plug_typetype;
> > > + u32 vdo;
> > > + unsigned intusb_pd:1;
> > > + unsigned intactive:1;
> > > + unsigned intsop_pp_controller:1;
> > > +
> > > + struct typec_plug plug[2];
> >
> > WTF???
> >
> > Think about what this structure now represents. You have 3 different
> > reference counted objects, all embedded in the same structure. Who
> > "owns" the lifecycle of it? What happens if plug[1]'s reference count
> > is grabbed a bunch by someone reading a lot of files for it, and then
> > the "larger" typec_cable.dev reference count is decremented to 0 because
> > the core is done with it. oops, boom, ick, splat, and if you are lucky
> > the device reboots itself, if not, someone just got root and read your
> > bank account information...
>
> I have to ask. How could that happen since the cable is the parent?
>
> > I'm being harsh here because this is really really really badly designed
>
> Don't worry about it, I can handle it. In fact, one could argue that I
> like getting slapped by you based on the comments I keep getting, but
> I assure you that is not the case ;-)
>
> > code. Go back and think about your data structures, what they are
> > trying to represent, and _WHO_ owns and controls them. The typec core
> > should be the one that allocates and manages the lifecycle of them, not
> > some random external entity where you just hope and pray that they got
> > it right (hint, they can not as they do not know what the core did with
> > the reference counts.)
> >
> > Right now you are almost there, the typec core registers and tries to
> > manage the structures, but it doesn't allocate/free them, and that's the
> > big problem, because really, with a structure that has 3 different
> > reference counts, no one can. That's an impossibility.
> >
> > This needs a lot more work, sorry.
>
> I was trying to cut corners, which clearly was wrong. I know what I
> need to do. All the parts simply need to be registered normally. No
> shortcuts.
>
> > I'm now going to require that you get other internal Intel developers to
> > sign off on this code before I review it again. You have resources at
> > your disposal that others do not with your internal mailing lists
> > containing senior kernel developers. Use it and don't waste the
> > community's time to do basic code review that they should be doing
> > instead.
>
> Fair enough.
>
> > How did we get to a v13 of this patch series without anyone else even
> > seeing this? That's worrysome as well...
>
> I guess for somebody writing the port drivers my awesome shortcut felt
> kinda nice. All they would have to do is announce connection when it
> happens, and the class then tried figured out everything for them,
> what needs to be created and so on. But yes, that is wrong!
>
At least for my part I very much concentrated on making sure that
the user space ABI as well as the port driver API are sane and usable.
The driver interface is not my area of expertise. As such, my testing
and understanding of that part was limited to "it appears to work,
it must be ok". I very much relied on you to get this part right.
That makes me feel really bad. It isn't fun to have my "Reviewed-by"
on a patch that gets (and apparently deserves) a WTF from a senior
kernel maintainer. This hurts both your and my reputation, and obviously
will make me quite hesitant to add a "Reviewed-by:" to the next version
of the series.
No more shortcuts, please.
Thanks,
Guenter
--
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 v13 00/10] usbip: exporting devices
Dear all, On 11/22/2016 07:48 AM, Nobuo Iwata wrote: > Dear all, > > This series of patches adds exporting device operation to USB/IP. > > NOTE: > This patch set modifies only userspace codes in tools/usb/usbip. > > 1. Goal > > 1-1) To give flexibility to direction of connection > Using USB/IP in internet, there can be two cases. > a) an application is inside firewall and devices are outside. > b) devices are inside firewall and an application is inside. > In case-a, import works because the connection is from inside. > In case-b, import doesn't works because the connection is from outside. > Connection from device side is needed. This patch set adds the > direction of connection establishment. > This feature looks for me like a knife. It may be useful in some cases but you may also hurt your self or some one else quite easily. > NOTE: > Directions of URBs requests and responses are not changed. Only > direction of connection establishment initiated with usbip command is > added to exsiting one. > Generally what this series does is just changing the initiator role. Classically Client connects to a server to get some device which is available. This patch allows to leave your server waiting for new devices to be connected to it from a remote machine. I see some points when it may be useful. For example you can start some virtual machine and connect different devices to it and check what's the system reaction. This may be useful is you would like to test for example some malware if it's playing badly with some USB devices. Of course you could do the same thing without this series but then you need to interact with the VM each time when you would like to connect and disconnect the device. > 4. Combination with vUDC > > New operations work with vUDC. --device option specifies vUDC mode as > well as list operaion. With stub, connect and disconnect execute bind > and unbind internally. With vUDC, they do not execute bind and unbind. > They are done by UDC interface. > If you combine what I wrote above with vUDC you can get quite nice automated testing laboratory for USB malware investigation as you can emulate any USB device quite easily and then simply attach it to VM. > 5. Security consideration > > When application side daemon is not started, this patch set doesn't > affect exsiting security. > > Daemons accept following requests form network : > EXISTING) 'list --remote' and 'attach' > NEW) 'connect' and 'desconnect' > > TCP wrappers allows and/or denies network access. It is enabled when > the daemons are compiled with ./configure --with-tcp-wrappers. > > When the daemons are running with SSL or Secure WebSocket tunneling > proxy, the proxy can use client authentication with certificate files. > There are also other security implications. With this patch it's quite easy to leak some USB outside restricted company network. Let's say that malicious employee has server and you run usbipa on it on port 9418 which is git (just example). Then he goes to his company connect his restricted USB license key or anything else to the computer and simply pass it using USB/IP to his remote server. > 6. Mixed usage > > Both existing and new way work in same machines simultaneously. Status > of devices and ports are controlled in stub and vhci driver. > After reading the whole series I realized that USB/IP tools lacks support for multiple instances of vhci which we have now available and configurable via Kconfig. > 7. Wording > > Adding the new operation, some inconsistnecies in wording are appeared > in documentation, function name, etc. If needed, they are fixed. > > 'export' is used for bind and 'exported' is used for bound. They are > changed to 'make importable' and 'imported' respectively. The words are > not new. For example, in the output of port operation, 'imported > devices' is already used. > > 'client' and 'server' are switched between existing and new operation. > Sometimes they implies device-side and application-side. So, words > 'device-side' and 'application-side' are used in documentations as > needed for clarity. > I wrote my opinion about wording in patch 9. All in all. In my humble opinion the idea in this series is worth discussion but: 1) I would like to see some real numbers of performance difference not just "smoother motion" like described in [1]. Then we can see what's the real performance difference between this and simple web sockets/tcp tunnel 2) This series contains some fixes/improvements which in my opinion should be split into separate series on which this one may depend. 3) I don't like the fact that we are sending so much information about the device during connect/disconnect operation as it's never used. is there any RFC or other document which standardize this message? If not then maybe we can just drop redundant information? 4) In most patches there are some mistakes which should be fixed before applying. Footnotes:
Re: [PATCHv13 2/3] usb: USB Type-C connector class
On Fri, Dec 02, 2016 at 10:04:39AM -0800, Guenter Roeck wrote: > The driver interface is not my area of expertise. As such, my testing > and understanding of that part was limited to "it appears to work, > it must be ok". I very much relied on you to get this part right. > > That makes me feel really bad. It isn't fun to have my "Reviewed-by" > on a patch that gets (and apparently deserves) a WTF from a senior > kernel maintainer. This hurts both your and my reputation, and obviously > will make me quite hesitant to add a "Reviewed-by:" to the next version > of the series. Hey, it doesn't bother me at all, I want and need your reviews of those portions that I don't know as well (i.e. the userspace api and functionality.) So don't take it personally, the driver model isn't that easy of a topic to mess with in places, loads of people get it wrong. > No more shortcuts, please. I totally agree :) Also, I want some Intel reviews on this, that should help make a lot of these types of issues go away... 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
Re: [PATH net v2] cdc_ether: Fix handling connection notification
From: Kristian Evensen
Date: Thu, 1 Dec 2016 14:23:17 +0100
> Commit bfe9b9d2df66 ("cdc_ether: Improve ZTE MF823/831/910 handling")
> introduced a work-around in usbnet_cdc_status() for devices that exported
> cdc carrier on twice on connect. Before the commit, this behavior caused
> the link state to be incorrect. It was assumed that all CDC Ethernet
> devices would either export this behavior, or send one off and then one on
> notification (which seems to be the default behavior).
>
> Unfortunately, it turns out multiple devices sends a connection
> notification multiple times per second (via an interrupt), even when
> connection state does not change. This has been observed with several
> different USB LAN dongles (at least), for example 13b1:0041 (Linksys).
> After bfe9b9d2df66, the link state has been set as down and then up for
> each notification. This has caused a flood of Netlink NEWLINK messages and
> syslog to be flooded with messages similar to:
>
> cdc_ether 2-1:2.0 eth1: kevent 12 may have been dropped
>
> This commit fixes the behavior by reverting usbnet_cdc_status() to how it
> was before bfe9b9d2df66. The work-around has been moved to a separate
> status-function which is only called when a known, affect device is
> detected.
>
> v1->v2:
>
> * Do not open-code netif_carrier_ok() (thanks Henning Schild).
> * Call netif_carrier_off() instead of usb_link_change(). This prevents
> calling schedule_work() twice without giving the work queue a chance to be
> processed (thanks Bjørn Mork).
>
> Fixes: bfe9b9d2df66 ("cdc_ether: Improve ZTE MF823/831/910 handling")
> Reported-by: Henning Schild
> Signed-off-by: Kristian Evensen
Applied and queued up for -stable, thanks.
--
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] usb: hub: Wait for connection to be reestablished after port reset
On Thu, 1 Dec 2016, Guenter Roeck wrote: > On a system with a defective USB device connected to an USB hub, > an endless sequence of port connect events was observed. The sequence > of events as observed is as follows: > > - Port reports connected event (port status=USB_PORT_STAT_CONNECTION). > - Event handler debounces port and resets it by calling hub_port_reset(). > - hub_port_reset() calls hub_port_wait_reset() to wait for the reset > to complete. > - The reset completes, but USB_PORT_STAT_CONNECTION is not immediately > set in the port status register. > - hub_port_wait_reset() returns -ENOTCONN. > - Port initialization sequence is aborted. > - A few milliseconds later, the port again reports a connected event, > and the sequence repeats. > > This continues either forever or, randomly, stops if the connection > is already re-established when the port status is read. It results in > a high rate of udev events. This in turn destabilizes userspace since > the above sequence holds the device mutex pretty much continuously > and prevents userspace from actually reading the device status. > > To prevent the problem from happening, let's wait for the connection > to be re-established after a port reset. If the device was actually > disconnected, the code will still return an error, but it will do so > only after the long reset timeout. > > Cc: Douglas Anderson > Signed-off-by: Guenter Roeck Acked-by: Alan Stern > --- > drivers/usb/core/hub.c | 11 +-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > index cbb146736f57..9bcb649e0e8c 100644 > --- a/drivers/usb/core/hub.c > +++ b/drivers/usb/core/hub.c > @@ -2731,8 +2731,15 @@ static int hub_port_wait_reset(struct usb_hub *hub, > int port1, > if (ret < 0) > return ret; > > - /* The port state is unknown until the reset completes. */ > - if (!(portstatus & USB_PORT_STAT_RESET)) > + /* > + * The port state is unknown until the reset completes. > + * > + * On top of that, some chips may require additional time > + * to re-establish a connection after the reset is complete, > + * so also wait for the connection to be re-established. > + */ > + if (!(portstatus & USB_PORT_STAT_RESET) && > + (portstatus & USB_PORT_STAT_CONNECTION)) > break; > > /* switch to the long delay after two short delay failures */ > -- 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] USB: OHCI: ohci-omap: remove useless functions
On Fri, 2 Dec 2016 csmanjuvi...@gmail.com wrote:
> From: Manjunath Goudar
>
> The ohci_hcd_omap_drv_probe and ohci_hcd_omap_drv_remove
> functions are removed as these are useless functions except calling
> usb_hcd_omap_probe and usb_hcd_omap_remove functions.
>
> The usb_hcd_omap_probe function renamed to ohci_hcd_omap_probe
> and usb_hcd_omap_remove function renamed to ohci_hcd_omap_remove
> for proper naming.
>
> Signed-off-by: Manjunath Goudar
> Cc: Alan Stern
> Cc: Greg Kroah-Hartman
> Cc: linux-usb@vger.kernel.org
> Cc: linux-o...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> ---
Acked-by: Alan Stern
> drivers/usb/host/ohci-omap.c | 36 +++-
> 1 file changed, 11 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/usb/host/ohci-omap.c b/drivers/usb/host/ohci-omap.c
> index 495c145..1e809d7 100644
> --- a/drivers/usb/host/ohci-omap.c
> +++ b/drivers/usb/host/ohci-omap.c
> @@ -296,15 +296,14 @@ static int ohci_omap_reset(struct usb_hcd *hcd)
> /*-*/
>
> /**
> - * usb_hcd_omap_probe - initialize OMAP-based HCDs
> + * ohci_hcd_omap_probe - initialize OMAP-based HCDs
> * Context: !in_interrupt()
> *
> * Allocates basic resources for this USB host controller, and
> * then invokes the start() method for the HCD associated with it
> * through the hotplug entry's driver_data.
> */
> -static int usb_hcd_omap_probe (const struct hc_driver *driver,
> - struct platform_device *pdev)
> +static int ohci_hcd_omap_probe(struct platform_device *pdev)
> {
> int retval, irq;
> struct usb_hcd *hcd = 0;
> @@ -336,7 +335,8 @@ static int usb_hcd_omap_probe (const struct hc_driver
> *driver,
> }
>
>
> - hcd = usb_create_hcd (driver, &pdev->dev, dev_name(&pdev->dev));
> + hcd = usb_create_hcd(&ohci_omap_hc_driver, &pdev->dev,
> + dev_name(&pdev->dev));
> if (!hcd) {
> retval = -ENOMEM;
> goto err0;
> @@ -384,17 +384,18 @@ static int usb_hcd_omap_probe (const struct hc_driver
> *driver,
> /* may be called with controller, bus, and devices active */
>
> /**
> - * usb_hcd_omap_remove - shutdown processing for OMAP-based HCDs
> + * ohci_hcd_omap_remove - shutdown processing for OMAP-based HCDs
> * @dev: USB Host Controller being removed
> * Context: !in_interrupt()
> *
> - * Reverses the effect of usb_hcd_omap_probe(), first invoking
> + * Reverses the effect of ohci_hcd_omap_probe(), first invoking
> * the HCD's stop() method. It is always called from a thread
> * context, normally "rmmod", "apmd", or something similar.
> */
> -static inline void
> -usb_hcd_omap_remove (struct usb_hcd *hcd, struct platform_device *pdev)
> +static int ohci_hcd_omap_remove(struct platform_device *pdev)
> {
> + struct usb_hcd *hcd = platform_get_drvdata(pdev);
> +
> dev_dbg(hcd->self.controller, "stopping USB Controller\n");
> usb_remove_hcd(hcd);
> omap_ohci_clock_power(0);
> @@ -409,21 +410,6 @@ usb_hcd_omap_remove (struct usb_hcd *hcd, struct
> platform_device *pdev)
> usb_put_hcd(hcd);
> clk_put(usb_dc_ck);
> clk_put(usb_host_ck);
> -}
> -
> -/*-*/
> -
> -static int ohci_hcd_omap_drv_probe(struct platform_device *dev)
> -{
> - return usb_hcd_omap_probe(&ohci_omap_hc_driver, dev);
> -}
> -
> -static int ohci_hcd_omap_drv_remove(struct platform_device *dev)
> -{
> - struct usb_hcd *hcd = platform_get_drvdata(dev);
> -
> - usb_hcd_omap_remove(hcd, dev);
> -
> return 0;
> }
>
> @@ -472,8 +458,8 @@ static int ohci_omap_resume(struct platform_device *dev)
> * Driver definition to register with the OMAP bus
> */
> static struct platform_driver ohci_hcd_omap_driver = {
> - .probe = ohci_hcd_omap_drv_probe,
> - .remove = ohci_hcd_omap_drv_remove,
> + .probe = ohci_hcd_omap_probe,
> + .remove = ohci_hcd_omap_remove,
> .shutdown = usb_hcd_platform_shutdown,
> #ifdef CONFIG_PM
> .suspend= ohci_omap_suspend,
>
--
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] usb: gadget: uvc: fix UVC_ATTR macro for UVCG_OPTS_ATTR
On Friday 02 Dec 2016 07:56:32 Greg KH wrote:
> On Fri, Dec 02, 2016 at 11:52:02AM +0530, Jassi Brar wrote:
> > From: Jassi Brar
> >
> > Typo in commit 76e0da34c7cec5a7d introduced a bug that prevents
> > creation of streaming_{interval,maxpacket,maxburst} nodes for
> > invalid 'aname' node.
> >
> > Signed-off-by: Jassi Brar
>
> Fixes: tag?
> cc: stable tag?
>
> Come on, you know better than this...
Apart from that,
Reviewed-by: Laurent Pinchart
--
Regards,
Laurent Pinchart
--
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] usbip: vudc: Refactor init_vudc_hw() to be more obvious
On 12/02/2016 10:42 AM, Krzysztof Opasiak wrote:
> Current implementation of init_vudc_hw() adds ep0 to ep_list
> and then after looping through all endpoints removes it from
> that list.
>
> As this may be misleading let's refactor this function
> and avoid adding and removing ep0 to eplist and place it
> immediately in correct place.
>
> In addition let's remove redundant 0 assignments as ep
> array is zeroed during allocation.
>
> Signed-off-by: Krzysztof Opasiak
> ---
> Changes since v1:
> - remove redundant assignments
Thanks.
Greg,
Here is my ack. Could you please pick this up.
Acked-by: Shuah Khan
thanks,
-- Shuah
> ---
> drivers/usb/usbip/vudc_dev.c | 35 ++-
> 1 file changed, 18 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c
> index 7091848df6c8..70b3540ece2a 100644
> --- a/drivers/usb/usbip/vudc_dev.c
> +++ b/drivers/usb/usbip/vudc_dev.c
> @@ -549,30 +549,34 @@ static int init_vudc_hw(struct vudc *udc)
> sprintf(ep->name, "ep%d%s", num,
> i ? (is_out ? "out" : "in") : "");
> ep->ep.name = ep->name;
> +
> + ep->ep.ops = &vep_ops;
> +
> + usb_ep_set_maxpacket_limit(&ep->ep, ~0);
> + ep->ep.max_streams = 16;
> + ep->gadget = &udc->gadget;
> + INIT_LIST_HEAD(&ep->req_queue);
> +
> if (i == 0) {
> + /* ep0 */
> ep->ep.caps.type_control = true;
> ep->ep.caps.dir_out = true;
> ep->ep.caps.dir_in = true;
> +
> + udc->gadget.ep0 = &ep->ep;
> } else {
> + /* All other eps */
> ep->ep.caps.type_iso = true;
> ep->ep.caps.type_int = true;
> ep->ep.caps.type_bulk = true;
> - }
>
> - if (is_out)
> - ep->ep.caps.dir_out = true;
> - else
> - ep->ep.caps.dir_in = true;
> + if (is_out)
> + ep->ep.caps.dir_out = true;
> + else
> + ep->ep.caps.dir_in = true;
>
> - ep->ep.ops = &vep_ops;
> - list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list);
> - ep->halted = ep->wedged = ep->already_seen =
> - ep->setup_stage = 0;
> - usb_ep_set_maxpacket_limit(&ep->ep, ~0);
> - ep->ep.max_streams = 16;
> - ep->gadget = &udc->gadget;
> - ep->desc = NULL;
> - INIT_LIST_HEAD(&ep->req_queue);
> + list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list);
> + }
> }
>
> spin_lock_init(&udc->lock);
> @@ -589,9 +593,6 @@ static int init_vudc_hw(struct vudc *udc)
> ud->eh_ops.reset= vudc_device_reset;
> ud->eh_ops.unusable = vudc_device_unusable;
>
> - udc->gadget.ep0 = &udc->ep[0].ep;
> - list_del_init(&udc->ep[0].ep.ep_list);
> -
> v_init_timer(udc);
> return 0;
>
>
--
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 v5 1/6] usb: separate out sysdev pointer from usb_bus
Hi all, On Thu, Nov 17, 2016 at 05:13:43PM +0530, Sriram Dash wrote: > From: Arnd Bergmann > > For xhci-hcd platform device, all the DMA parameters are not > configured properly, notably dma ops for dwc3 devices. > > The idea here is that you pass in the parent of_node along with > the child device pointer, so it would behave exactly like the > parent already does. The difference is that it also handles all > the other attributes besides the mask. > > sysdev will represent the physical device, as seen from firmware > or bus.Splitting the usb_bus->controller field into the > Linux-internal device (used for the sysfs hierarchy, for printks > and for power management) and a new pointer (used for DMA, > DT enumeration and phy lookup) probably covers all that we really > need. > > Signed-off-by: Arnd Bergmann > Signed-off-by: Sriram Dash > Tested-by: Baolin Wang > Cc: Felipe Balbi > Cc: Grygorii Strashko > Cc: Sinjan Kumar > Cc: David Fisher > Cc: Catalin Marinas > Cc: "Thang Q. Nguyen" > Cc: Yoshihiro Shimoda > Cc: Stephen Boyd > Cc: Bjorn Andersson > Cc: Ming Lei > Cc: Jon Masters > Cc: Dann Frazier > Cc: Peter Chen > Cc: Leo Li > --- > Changes in v5: > - No update > > Changes in v4: > - No update > > Changes in v3: > - usb is_device_dma_capable instead of directly accessing > dma props. > > Changes in v2: > - Split the patch wrt driver I didn't notice this series had been reposted a few times. For some reason, this wasn't that easy to find in search engines... Anyway, when the whole series is applied, this fixes my XHCI probe issues for DWC3 host mode. Thanks! Tested-by: Brian Norris But I noticed that Felipe has applied patches 5 and 6 in -next, while the rest are still outstanding. That means I hit the dma_mask WARN_ON() in xhci-plat.c, and it eventually fails to probe with -EIO still: [2.060272] [ cut here ] [2.064908] WARNING: CPU: 5 PID: 1 at drivers/usb/host/xhci-plat.c:159 xhci_plat_probe+0x5c/0x444 ... [2.25] [] xhci_plat_probe+0x5c/0x444 [2.294456] [] platform_drv_probe+0x60/0xac [2.300200] [] driver_probe_device+0x12c/0x2a0 [2.306204] [] __driver_attach+0x84/0xb0 [2.311687] [] bus_for_each_dev+0x9c/0xcc [2.317256] [] driver_attach+0x2c/0x34 [2.322566] [] bus_add_driver+0xf0/0x1f4 [2.328049] [] driver_register+0x9c/0xe8 [2.333530] [] __platform_driver_register+0x60/0x6c [2.339968] [] xhci_plat_init+0x2c/0x34 [2.345366] [] do_one_initcall+0xa4/0x13c [2.350936] [] kernel_init_freeable+0x1bc/0x274 [2.357026] [] kernel_init+0x18/0x104 [2.362247] [] ret_from_fork+0x10/0x50 [2.374615] xhci-hcd: probe of xhci-hcd.1.auto failed with error -5 [2.380962] [ cut here ] [2.385588] WARNING: CPU: 4 PID: 1 at drivers/usb/host/xhci-plat.c:159 xhci_plat_probe+0x5c/0x444 ... [2.637372] [] xhci_plat_probe+0x5c/0x444 [2.642941] [] platform_drv_probe+0x60/0xac [2.648685] [] driver_probe_device+0x12c/0x2a0 [2.654688] [] __driver_attach+0x84/0xb0 [2.660170] [] bus_for_each_dev+0x9c/0xcc [2.665739] [] driver_attach+0x2c/0x34 [2.671048] [] bus_add_driver+0xf0/0x1f4 [2.676532] [] driver_register+0x9c/0xe8 [2.682012] [] __platform_driver_register+0x60/0x6c [2.688450] [] xhci_plat_init+0x2c/0x34 [2.693845] [] do_one_initcall+0xa4/0x13c [2.699415] [] kernel_init_freeable+0x1bc/0x274 [2.705505] [] kernel_init+0x18/0x104 [2.710726] [] ret_from_fork+0x10/0x50 [2.716075] xhci-hcd: probe of xhci-hcd.2.auto failed with error -5 What's happening with patches 1-4? Regards, Brian -- 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: [RFC][PATCH 1/3] phy: phy-hi6220-usb: Wire up extconn support to hikey's phy driver
On 12/1/2016 12:12 PM, John Stultz wrote:
> On Thu, Dec 1, 2016 at 12:23 AM, Kishon Vijay Abraham I wrote:
>> Hi,
>>
>> On Wednesday 23 November 2016 09:16 AM, John Stultz wrote:
>>> This wires extconn support to hikey's phy driver, and
>>> connects it to the usb UDC layer via a usb_phy structure.
>>>
>>> Not sure if this is the right way to connect phy -> UDC,
>>> but I'm lacking a clear example.
>>>
>>> Cc: Wei Xu
>>> Cc: Guodong Xu
>>> Cc: Amit Pundir
>>> Cc: Rob Herring
>>> Cc: John Youn
>>> Cc: Douglas Anderson
>>> Cc: Chen Yu
>>> Cc: Kishon Vijay Abraham I
>>> Cc: Felipe Balbi
>>> Cc: Greg Kroah-Hartman
>>> Cc: linux-usb@vger.kernel.org
>>> Signed-off-by: John Stultz
>>> ---
>>> arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 11 +++
>>> drivers/phy/Kconfig | 2 +
>>> drivers/phy/phy-hi6220-usb.c | 139
>>> ++
>>> 3 files changed, 152 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
>>> b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
>>> index 17839db..171fbb2 100644
>>> --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
>>> +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
>>> @@ -732,10 +732,21 @@
>>> regulator-always-on;
>>> };
>>>
>>> + usb_vbus: usb-vbus {
>>> + compatible = "linux,extcon-usb-gpio";
>>> + id-gpio = <&gpio2 6 1>;
>>> + };
>>> +
>>> + usb_id: usb-id {
>>> + compatible = "linux,extcon-usb-gpio";
>>> + id-gpio = <&gpio2 5 1>;
>>> + };
>>> +
>>> usb_phy: usbphy {
>>> compatible = "hisilicon,hi6220-usb-phy";
>>> #phy-cells = <0>;
>>> phy-supply = <&fixed_5v_hub>;
>>> + extcon = <&usb_vbus>, <&usb_id>;
>>> hisilicon,peripheral-syscon = <&sys_ctrl>;
>>> };
>>>
>>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>>> index fe00f91..76f4f17 100644
>>> --- a/drivers/phy/Kconfig
>>> +++ b/drivers/phy/Kconfig
>>> @@ -254,8 +254,10 @@ config PHY_MT65XX_USB3
>>> config PHY_HI6220_USB
>>> tristate "hi6220 USB PHY support"
>>> depends on (ARCH_HISI && ARM64) || COMPILE_TEST
>>> + depends on EXTCON
>>> select GENERIC_PHY
>>> select MFD_SYSCON
>>> + select USB_PHY
>>> help
>>> Enable this to support the HISILICON HI6220 USB PHY.
>>>
>>> diff --git a/drivers/phy/phy-hi6220-usb.c b/drivers/phy/phy-hi6220-usb.c
>>> index b2141cb..89d8475 100644
>>> --- a/drivers/phy/phy-hi6220-usb.c
>>> +++ b/drivers/phy/phy-hi6220-usb.c
>>> @@ -12,7 +12,12 @@
>>> #include
>>> #include
>>> #include
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> #include
>>> +#include
>>>
>>> #define SC_PERIPH_CTRL4 0x00c
>>>
>>> @@ -44,9 +49,21 @@
>>>
>>> #define EYE_PATTERN_PARA 0x7053348c
>>>
>>> +
>>> +struct hi6220_usb_cable {
>>> + struct notifier_block nb;
>>> + struct extcon_dev *extcon;
>>> + int state;
>>> +};
>>> +
>>> struct hi6220_priv {
>>> struct regmap *reg;
>>> struct device *dev;
>>> + struct usb_phy phy;
>>> +
>>> + struct delayed_work work;
>>> + struct hi6220_usb_cable vbus;
>>> + struct hi6220_usb_cable id;
>>> };
>>>
>>> static void hi6220_phy_init(struct hi6220_priv *priv)
>>> @@ -112,23 +129,85 @@ static int hi6220_phy_exit(struct phy *phy)
>>> return hi6220_phy_setup(priv, false);
>>> }
>>>
>>> +
>>> static struct phy_ops hi6220_phy_ops = {
>>> .init = hi6220_phy_start,
>>> .exit = hi6220_phy_exit,
>>> .owner = THIS_MODULE,
>>> };
>>>
>>> +static void hi6220_detect_work(struct work_struct *work)
>>> +{
>>> + struct hi6220_priv *priv =
>>> + container_of(to_delayed_work(work), struct hi6220_priv, work);
>>> + struct usb_otg *otg = priv->phy.otg;
>>> +
>>> + if (!IS_ERR(priv->vbus.extcon))
>>> + priv->vbus.state = extcon_get_cable_state_(priv->vbus.extcon,
>>> + EXTCON_USB);
>>> + if (!IS_ERR(priv->id.extcon))
>>> + priv->id.state = extcon_get_cable_state_(priv->id.extcon,
>>> + EXTCON_USB_HOST);
>>> + if (otg->gadget) {
>>> + if (priv->id.state)
>>> + usb_gadget_vbus_connect(otg->gadget);
>>> + else
>>> + usb_gadget_vbus_disconnect(otg->gadget);
>>> + }
>>> +}
>>> +
>>> +static int hi6220_otg_vbus_notifier(struct notifier_block *nb,
>>> + unsigned long event, void *ptr)
>>> +{
>>> + struct hi6220_usb_cable *vbus = container_of(nb,
>>> + struct hi6220_usb_cable, nb);
>>> + s
Re: [PATCH] usb: dwc2: fix flags for DMA descriptor allocation in dwc2_hsotg_ep_enable
On 12/1/2016 1:02 AM, Marek Szyprowski wrote:
> dwc2_hsotg_ep_enable can be called from interrupt context, so all
> allocations should be done with GFP_ATOMIC flags. This fixes following
> issue on ARM architecture:
>
> [] (unwind_backtrace) from [] (show_stack+0x10/0x14)
> [] (show_stack) from [] (dump_stack+0x74/0x94)
> [] (dump_stack) from [] (__warn+0xd4/0x100)
> [] (__warn) from [] (warn_slowpath_null+0x20/0x28)
> [] (warn_slowpath_null) from []
> (smp_call_function_many+0xcc/0x2a4)
> [] (smp_call_function_many) from []
> (on_each_cpu_mask+0x38/0xa8)
> [] (on_each_cpu_mask) from []
> (start_isolate_page_range+0x134/0x1b8)
> [] (start_isolate_page_range) from []
> (alloc_contig_range+0xac/0x2f8)
> [] (alloc_contig_range) from [] (cma_alloc+0xe0/0x1a8)
> [] (cma_alloc) from [] (__alloc_from_contiguous+0x38/0xe0)
> [] (__alloc_from_contiguous) from []
> (cma_allocator_alloc+0x30/0x38)
> [] (cma_allocator_alloc) from [] (__dma_alloc+0x1c0/0x2c8)
> [] (__dma_alloc) from [] (arm_dma_alloc+0x3c/0x48)
> [] (arm_dma_alloc) from []
> (dwc2_hsotg_ep_enable+0xec/0x46c)
> [] (dwc2_hsotg_ep_enable) from []
> (usb_ep_enable+0x2c/0x3c)
> [] (usb_ep_enable) from [] (ecm_set_alt+0xa8/0x154)
> [] (ecm_set_alt) from [] (composite_setup+0xd74/0x1540)
> [] (composite_setup) from []
> (dwc2_hsotg_complete_setup+0xb8/0x370)
> [] (dwc2_hsotg_complete_setup) from []
> (usb_gadget_giveback_request+0xc/0x10)
> [] (usb_gadget_giveback_request) from []
> (dwc2_hsotg_complete_request+0x78/0x128)
> [] (dwc2_hsotg_complete_request) from []
> (dwc2_hsotg_epint+0x69c/0x81c)
> [] (dwc2_hsotg_epint) from [] (dwc2_hsotg_irq+0xfc/0x748)
> [] (dwc2_hsotg_irq) from []
> (__handle_irq_event_percpu+0x58/0x140)
> [] (__handle_irq_event_percpu) from []
> (handle_irq_event_percpu+0x1c/0x58)
> [] (handle_irq_event_percpu) from []
> (handle_irq_event+0x38/0x5c)
> [] (handle_irq_event) from []
> (handle_fasteoi_irq+0xc4/0x19c)
> [] (handle_fasteoi_irq) from []
> (generic_handle_irq+0x18/0x28)
> [] (generic_handle_irq) from []
> (__handle_domain_irq+0x6c/0xe4)
> [] (__handle_domain_irq) from []
> (gic_handle_irq+0x50/0x9c)
> [] (gic_handle_irq) from [] (__irq_svc+0x6c/0xa8)
>
> Fixes: 5f54c54b0ba83 ("usb: dwc2: gadget: Add DDMA chain pointers to
> dwc2_hsotg_ep structure")
> Signed-off-by: Marek Szyprowski
> ---
> drivers/usb/dwc2/gadget.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index b95930f20d90..c55db4aa54d6 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -3753,7 +3753,7 @@ static int dwc2_hsotg_ep_enable(struct usb_ep *ep,
> hs_ep->desc_list = dma_alloc_coherent(hsotg->dev,
> MAX_DMA_DESC_NUM_GENERIC *
> sizeof(struct dwc2_dma_desc),
> - &hs_ep->desc_list_dma, GFP_KERNEL);
> + &hs_ep->desc_list_dma, GFP_ATOMIC);
> if (!hs_ep->desc_list) {
> ret = -ENOMEM;
> goto error2;
>
Acked-by: John Youn
Regards,
John
--
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 0/2] USB: resume time optimization by using spec minimums
The USB resume code in the kernel currently uses a set of hard coded delay values that are defined in the USB 2.0 spec. Specifically these three have the most effect on resume time: - tdrsmdn: resume signal time (20ms - infinity) usb 2.0 spec 7.1.7.7 - trsmrcy: resume recovery time (10ms) usb 2.0 spec 7.1.7.7 - trstrcy: reset recovery time (0ms - infinity) usb 2.0 spec 7.1.7.5 These values have been padded considerably in order to accomodate non-compliant devices. - tdrsmdn: resume signal time = 40ms - trsmrcy: resume recovery time = 10ms - trstrcy: reset recovery time = 50ms I propose that we remove the padding and use the spec minumums instead. The following patches do this by creating a struct which contains these three values, and allowing a module parameter to select between the current defaults and the spec minimums. The patch could be simplified to just use 3 defines, but I'm wary of making changes to core USB code without providing a backup and alot of community testing. The first part is the core functionality. The second part provides a debug interface through sysfs to tweak the values directly at runtime. I've created a blog entry on 01.org with some analyze_suspend test cases illustrating the benefits: - https://01.org/suspendresume/blogs/tebrandt/2016/usb-resume-optimization-using-spec-minimum-delays Todd Brandt (2): USB: turn off padding of resume time delays USB: resume timing debug Documentation/kernel-parameters.txt | 3 + drivers/usb/core/hub.c | 127 ++-- drivers/usb/dwc2/hcd.c | 2 +- drivers/usb/host/ehci-hcd.c | 4 +- drivers/usb/host/ehci-hub.c | 6 +- drivers/usb/host/fotg210-hcd.c | 2 +- drivers/usb/host/isp116x-hcd.c | 2 +- drivers/usb/host/oxu210hp-hcd.c | 4 +- drivers/usb/host/r8a66597-hcd.c | 2 +- drivers/usb/host/sl811-hcd.c| 2 +- drivers/usb/host/uhci-hub.c | 4 +- drivers/usb/host/xhci-hub.c | 6 +- drivers/usb/host/xhci-ring.c| 2 +- drivers/usb/isp1760/isp1760-hcd.c | 2 +- drivers/usb/musb/musb_core.c| 6 +- drivers/usb/musb/musb_virthub.c | 2 +- include/linux/usb.h | 24 ++- 17 files changed, 171 insertions(+), 29 deletions(-) -- 2.1.4 -- 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 1/2] USB: turn off padding of resume time delays
Add a module parameter that replaces the USB_RESUME_TIMEOUT
and other hardcoded delay numbers with the USB spec minimums.
By default the patch retains the current values.
The USB subsystem currently uses heavily padded values for TDRSMDN
and TRSTRCY. This patch keeps the current values by default, but if
the kernel is booted with usbcore.padding=0 they are set to the
spec minimums with no padding. The result is significant performance
improvement in usb device resume.
Signed-off-by: Todd Brandt
---
Documentation/kernel-parameters.txt | 3 +++
drivers/usb/core/hub.c | 27 ++-
drivers/usb/dwc2/hcd.c | 2 +-
drivers/usb/host/ehci-hcd.c | 4 ++--
drivers/usb/host/ehci-hub.c | 6 +++---
drivers/usb/host/fotg210-hcd.c | 2 +-
drivers/usb/host/isp116x-hcd.c | 2 +-
drivers/usb/host/oxu210hp-hcd.c | 4 ++--
drivers/usb/host/r8a66597-hcd.c | 2 +-
drivers/usb/host/sl811-hcd.c| 2 +-
drivers/usb/host/uhci-hub.c | 4 ++--
drivers/usb/host/xhci-hub.c | 6 +++---
drivers/usb/host/xhci-ring.c| 2 +-
drivers/usb/isp1760/isp1760-hcd.c | 2 +-
drivers/usb/musb/musb_core.c| 6 +++---
drivers/usb/musb/musb_virthub.c | 2 +-
include/linux/usb.h | 24 +++-
17 files changed, 71 insertions(+), 29 deletions(-)
diff --git a/Documentation/kernel-parameters.txt
b/Documentation/kernel-parameters.txt
index 37babf9..19d9e62 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -4237,6 +4237,9 @@ bytes respectively. Such letter suffixes can also be
entirely omitted.
usbcore.blinkenlights=
[USB] Set to cycle leds on hubs (default 0 = off).
+ usbcore.padding=
+ [USB] Pad the USB 2.0 spec timing values (default 1 =
on).
+
usbcore.old_scheme_first=
[USB] Start with the old device initialization
scheme (default 0 = off).
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index cbb1467..43242e3 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -99,6 +99,17 @@ EXPORT_SYMBOL_GPL(ehci_cf_port_reset_rwsem);
#define HUB_DEBOUNCE_STEP25
#define HUB_DEBOUNCE_STABLE 100
+static bool padding = true;
+module_param(padding, bool, 0444);
+MODULE_PARM_DESC(padding, "USB timing value padding");
+
+struct _usb_timing_config usb_timing = {
+ .tdrsmdn = USB_TIMING_TDRSMDN_DEF,
+ .trsmrcy = USB_TIMING_TRSMRCY_DEF,
+ .trstrcy = USB_TIMING_TRSTRCY_DEF
+};
+EXPORT_SYMBOL_GPL(usb_timing);
+
static void hub_release(struct kref *kref);
static int usb_reset_and_verify_device(struct usb_device *udev);
@@ -2884,7 +2895,7 @@ static int hub_port_reset(struct usb_hub *hub, int port1,
done:
if (status == 0) {
/* TRSTRCY = 10 ms; plus some extra */
- msleep(10 + 40);
+ msleep(usb_timing.trstrcy);
if (udev) {
struct usb_hcd *hcd = bus_to_hcd(udev->bus);
@@ -3476,10 +3487,10 @@ int usb_port_resume(struct usb_device *udev,
pm_message_t msg)
if (status) {
dev_dbg(&port_dev->dev, "can't resume, status %d\n", status);
} else {
- /* drive resume for USB_RESUME_TIMEOUT msec */
+ /* drive resume for TDRSMDN msec */
dev_dbg(&udev->dev, "usb %sresume\n",
(PMSG_IS_AUTO(msg) ? "auto-" : ""));
- msleep(USB_RESUME_TIMEOUT);
+ msleep(usb_timing.tdrsmdn);
/* Virtual root hubs can trigger on GET_PORT_STATUS to
* stop resume signaling. Then finish the resume
@@ -3488,7 +3499,7 @@ int usb_port_resume(struct usb_device *udev, pm_message_t
msg)
status = hub_port_status(hub, port1, &portstatus, &portchange);
/* TRSMRCY = 10 msec */
- msleep(10);
+ msleep(usb_timing.trsmrcy);
}
SuspendCleared:
@@ -3574,7 +3585,7 @@ static int hub_handle_remote_wakeup(struct usb_hub *hub,
unsigned int port,
if (udev) {
/* TRSMRCY = 10 msec */
- msleep(10);
+ msleep(usb_timing.trsmrcy);
usb_unlock_port(port_dev);
ret = usb_remote_wakeup(udev);
@@ -5262,6 +5273,12 @@ int usb_hub_init(void)
return -1;
}
+ if (!padding) {
+ usb_timing.tdrsmdn = USB_TIMING_TDRSMDN_MIN;
+ usb_timing.trsmrcy = USB_TIMING_TRSMRCY_MIN;
+ usb_timing.trstrcy = USB_TIMING_TRSTRCY_MIN;
+ }
+
/*
* The workqueue needs to be freezable to avoid interfering with
* USB-PERSIST port handover. Otherwise it might see that a full-speed
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index df5a065..39a2c5d 100
[PATCH 2/2] USB: resume timing debug
Add debug support for experimenting with USB timing delay
values on the fly. This provides a debug interface through
/sys/kernel/usb where a user can tweak the values. The code
enforces the spec minimums so that a user can't set them
too low.
Signed-off-by: Todd Brandt
---
drivers/usb/core/hub.c | 100 +
1 file changed, 100 insertions(+)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 43242e3..6dea6a7 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -5265,8 +5265,98 @@ static struct usb_driver hub_driver = {
.supports_autosuspend = 1,
};
+static ssize_t tdrsmdn_show(struct kobject *kobj,
+ struct kobj_attribute *attr, char *buf)
+{
+ return sprintf(buf, "%d\n", usb_timing.tdrsmdn);
+}
+
+static ssize_t tdrsmdn_store(struct kobject *kobj,
+ struct kobj_attribute *attr, const char *buf, size_t count)
+{
+ int ret, val;
+
+ ret = kstrtoint(buf, 10, &val);
+ if (ret < 0)
+ return ret;
+
+ if (val < 20)
+ usb_timing.tdrsmdn = 20;
+ else
+ usb_timing.tdrsmdn = val;
+
+ return count;
+}
+
+static ssize_t trsmrcy_show(struct kobject *kobj,
+ struct kobj_attribute *attr, char *buf)
+{
+ return sprintf(buf, "%d\n", usb_timing.trsmrcy);
+}
+
+static ssize_t trsmrcy_store(struct kobject *kobj,
+ struct kobj_attribute *attr, const char *buf, size_t count)
+{
+ int ret, val;
+
+ ret = kstrtoint(buf, 10, &val);
+ if (ret < 0)
+ return ret;
+
+ if (val < 10)
+ usb_timing.trsmrcy = 10;
+ else
+ usb_timing.trsmrcy = val;
+
+ return count;
+}
+
+static ssize_t trstrcy_show(struct kobject *kobj,
+ struct kobj_attribute *attr, char *buf)
+{
+ return sprintf(buf, "%d\n", usb_timing.trstrcy);
+}
+
+static ssize_t trstrcy_store(struct kobject *kobj,
+ struct kobj_attribute *attr, const char *buf, size_t count)
+{
+ int ret, val;
+
+ ret = kstrtoint(buf, 10, &val);
+ if (ret < 0)
+ return ret;
+
+ if (val < 0)
+ usb_timing.trstrcy = 0;
+ else
+ usb_timing.trstrcy = val;
+
+ return count;
+}
+
+static struct kobj_attribute tdrsmdn_attr =
+ __ATTR(tdrsmdn, 0664, tdrsmdn_show, tdrsmdn_store);
+static struct kobj_attribute trsmrcy_attr =
+ __ATTR(trsmrcy, 0664, trsmrcy_show, trsmrcy_store);
+static struct kobj_attribute trstrcy_attr =
+ __ATTR(trstrcy, 0664, trstrcy_show, trstrcy_store);
+
+static struct attribute *attrs[] = {
+ &tdrsmdn_attr.attr,
+ &trsmrcy_attr.attr,
+ &trstrcy_attr.attr,
+ NULL
+};
+
+static struct attribute_group attr_group = {
+ .attrs = attrs,
+};
+
int usb_hub_init(void)
{
+ int retval;
+ struct kobject *usb_kobj;
+
if (usb_register(&hub_driver) < 0) {
printk(KERN_ERR "%s: can't register hub driver\n",
usbcore_name);
@@ -5279,6 +5369,16 @@ int usb_hub_init(void)
usb_timing.trstrcy = USB_TIMING_TRSTRCY_MIN;
}
+ usb_kobj = kobject_create_and_add("usb", kernel_kobj);
+ if (!usb_kobj)
+ return -ENOMEM;
+
+ retval = sysfs_create_group(usb_kobj, &attr_group);
+ if (retval) {
+ kobject_put(usb_kobj);
+ return retval;
+ }
+
/*
* The workqueue needs to be freezable to avoid interfering with
* USB-PERSIST port handover. Otherwise it might see that a full-speed
--
2.1.4
--
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
