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.mal...@gmail.com>
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.
> @@ -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 ?
> +#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.
--
balbi
signature.asc
Description: Digital signature
