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.
> >
> >>+ 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
--
balbi
Attachment:
signature.asc
Description: Digital signature
