On 10/27/2014 05:22 PM, Felipe Balbi wrote:
On Mon, Oct 27, 2014 at 05:15:16PM +0200, Kyösti Mälkki wrote:
On 10/27/2014 03:35 PM, Felipe Balbi wrote:
Hi,
On Sun, Oct 26, 2014 at 08:01:29PM +0200, Kyösti Mälkki wrote:
SET_FEATURE request for DEBUG_MODE only worked the first time after module
initialisation. On USB host reset or cable disconnect gserial_disconnect()
clears the associations dbgp.serial->in->desc and dbgp_serial->out->desc.
Per the USB 2.0 debug device specification, SET_FEATURE with DEBUG_MODE
is to be treated as if it were a SET_CONFIGURATION request. Redo endpoint
configuration on this request.
Code has assumption that endpoint mapping remains unchanged from what was
previously returned as a GET_DESCRIPTOR DT_DEBUG response.
Signed-off-by: Kyösti Mälkki <kyosti.malkki@xxxxxxxxx>
you're doing many things in one patch and I can't accept this.
---
drivers/usb/gadget/legacy/dbgp.c | 39 ++++++++++++++++++++++++++-------------
1 file changed, 26 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/gadget/legacy/dbgp.c b/drivers/usb/gadget/legacy/dbgp.c
index 1b07513..afb49ef 100644
--- a/drivers/usb/gadget/legacy/dbgp.c
+++ b/drivers/usb/gadget/legacy/dbgp.c
@@ -214,6 +214,7 @@ static void dbgp_disconnect(struct usb_gadget *gadget)
#ifdef CONFIG_USB_G_DBGP_PRINTK
dbgp_disable_ep();
#else
+ dev_dbg(&dbgp.gadget->dev, "disconnected\n");
first you add this debugging message which can definitely wait for
v3.19, not a fix.
gserial_disconnect(dbgp.serial);
#endif
}
@@ -237,7 +238,7 @@ static void dbgp_unbind(struct usb_gadget *gadget)
static unsigned char tty_line;
#endif
-static int __init dbgp_configure_endpoints(struct usb_gadget *gadget)
+static int dbgp_configure_endpoints(struct usb_gadget *gadget)
then you remove __init here without ever mentioning why. Separate patch.
This is the handler for an equivalent of a SET_CONFIGURATION request the
gadget will receive after every bus reset or connect. Change is relevant
once we actually do call the handler with the reception of said request,
instead of configuring the endpoints only once at module init.
@@ -273,19 +274,10 @@ static int __init dbgp_configure_endpoints(struct usb_gadget *gadget)
dbgp.serial->in->desc = &i_desc;
dbgp.serial->out->desc = &o_desc;
-
- if (gserial_alloc_line(&tty_line)) {
- stp = 3;
- goto fail_3;
- }
why are you removing this ?
To allocate the TTY only once. Change is relevant once
dbgp_configure_endpoints() is called more than once.
+#endif
return 0;
-fail_3:
- dbgp.o_ep->driver_data = NULL;
-#else
- return 0;
-#endif
fail_2:
dbgp.i_ep->driver_data = NULL;
fail_1:
@@ -324,10 +316,17 @@ static int __init dbgp_bind(struct usb_gadget *gadget,
err = -ENOMEM;
goto fail;
}
+
+ if (gserial_alloc_line(&tty_line)) {
+ stp = 4;
+ err = -ENODEV;
+ goto fail;
+ }
why do you need this here ?
#endif
+
err = dbgp_configure_endpoints(gadget);
if (err < 0) {
- stp = 4;
+ stp = 5;
goto fail;
}
@@ -379,12 +378,26 @@ static int dbgp_setup(struct usb_gadget *gadget,
err = 0;
} else if (request == USB_REQ_SET_FEATURE &&
value == USB_DEVICE_DEBUG_MODE) {
- dev_dbg(&dbgp.gadget->dev, "setup: feat debug\n");
#ifdef CONFIG_USB_G_DBGP_PRINTK
err = dbgp_enable_ep();
#else
+ if (!(dbgp.serial->in && dbgp.serial->in->desc &&
+ dbgp.serial->out && dbgp.serial->out->desc)) {
+ dev_dbg(&dbgp.gadget->dev, "needs reconfiguration\n");
+ err = dbgp_configure_endpoints(gadget);
+ if (err < 0) {
+ goto fail;
+ }
+ }
now this is the only thing mentioned in your commit log and this is only
thing that should be in $subject.
Giving this another look. Endpoint data toggle / PID parity must be
reset on SET_CONFIGURE request, right? So we should call the handler
unconditionally instead?
+ if (dbgp.serial->in && dbgp.serial->in->desc &&
+ dbgp.serial->out && dbgp.serial->out->desc) {
+ dev_dbg(&dbgp.gadget->dev, "epIn %02x, epOut %02x\n",
+ dbgp.serial->in->desc->bEndpointAddress,
+ dbgp.serial->out->desc->bEndpointAddress);
+ }
why do you need this entire branch here just for a debugging message ?
Why didn't you add this debugging message right after
dbgp_configure_endpoints() above ?
err = gserial_connect(dbgp.serial, tty_line);
#endif
+ dev_dbg(&dbgp.gadget->dev, "setup: feat debug (%d)\n", err);
another debugging message which an wait until v3.19.
Sure, I'll remove the noise from extra debugging there and improve the
commit message. I can split the move of gserial_alloc_line() too, although I
think that part is tightly coupled with the subject matter.
Sure, after you made it clear that should be fine.
Thanks
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
