On Monday 27 July 2009, Felipe Balbi wrote:
> this will prevent a possible problem where
> we will never connect to usb bus.
What problem is that?
NAK due to the obvious locking bug ...
> Cc: David Brownell <dbrownell@xxxxxxxxxxxxxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxx>
> Cc: Bryan Wu <cooloney@xxxxxxxxxx>
> Cc: Robert Jarzmik <robert.jarzmik@xxxxxxx>
> Signed-off-by: Felipe Balbi <felipe.balbi@xxxxxxxxx>
> ---
> drivers/usb/gadget/f_obex.c | 1 +
> drivers/usb/gadget/u_serial.c | 48 ++++++++++++++++++++++++++++++----------
> drivers/usb/gadget/u_serial.h | 1 +
> 3 files changed, 38 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/usb/gadget/f_obex.c b/drivers/usb/gadget/f_obex.c
> index 46d6266..1738e36 100644
> --- a/drivers/usb/gadget/f_obex.c
> +++ b/drivers/usb/gadget/f_obex.c
> @@ -387,6 +387,7 @@ obex_bind(struct usb_configuration *c, struct usb_function *f)
> gadget_is_dualspeed(c->cdev->gadget) ? "dual" : "full",
> obex->port.in->name, obex->port.out->name);
>
> + gserial_start(&obex->port, obex->port_num);
> return 0;
>
> fail:
> diff --git a/drivers/usb/gadget/u_serial.c b/drivers/usb/gadget/u_serial.c
> index fc6e709..ab01440 100644
> --- a/drivers/usb/gadget/u_serial.c
> +++ b/drivers/usb/gadget/u_serial.c
> @@ -1201,6 +1201,42 @@ void gserial_cleanup(void)
> }
>
> /**
> + * gserial_start - starts gserial
> + * @gser: the function to be started
> + * @port_num: which port to start
> + * Context: any (usually called from ->bind())
> + *
> + * This should be called in early stage (usualy bind())
> + * in order to let the gaget driver connect to the bus
> + * and let the gser->connect() be called.
I don't follow. What's the problem with just
having the function call gser_connect() when it's
activated? Why should there be any bind-time hook
beyond a single call to gserial_setup()?
> + */
> +void gserial_start(struct gserial *gser, u8 port_num)
> +{
> + struct gs_port *port;
> + unsigned long flags;
> + int status;
> +
> + if (!gs_tty_driver || port_num >= n_ports)
> + return;
> +
> + /* we "know" gserial_cleanup() hasn't been called */
> + port = ports[port_num].port;
> +
> + /* then tell the tty glue that I/O can work */
> + spin_lock_irqsave(&port->port_lock, flags);
> + gser->ioport = port;
> + port->port_usb = gser;
> +
> + /* REVISIT unclear how best to handle this state...
> + * we don't really couple it with the Linux TTY.
> + */
> + gser->port_line_coding = port->port_line_coding;
> +
> + /* REVISIT if waiting on "carrier detect", signal. */
> + spin_unlock_irqrestore(&port->port_lock, flags);
> +}
> +
> +/**
> * gserial_connect - notify TTY I/O glue that USB link is active
> * @gser: the function, set up with endpoints and descriptors
> * @port_num: which port is active
> @@ -1244,18 +1280,6 @@ int gserial_connect(struct gserial *gser, u8 port_num)
> goto fail_out;
> gser->out->driver_data = port;
>
> - /* then tell the tty glue that I/O can work */
> - spin_lock_irqsave(&port->port_lock, flags);
.... the tty layer locking is a bit delicate and non-obvious
(though one hopes Alan's work has cleaned it up).
However, *NO* software will be happy with breaking basic
locking rules like this. Observe: you remove the lock,
leave sensitive code which relies on being locked in order
to guarantee correctness, then later you release a *lock
that you do not hold* ...
> - gser->ioport = port;
> - port->port_usb = gser;
> -
So you somehow want to couple the /dev/ttyGS* port
to a given function before that's even activated?
How's that supposed to work ... given that the same
port may need to be coupled to a *DIFFERENT* function
when another configuration is active? (Maybe using
a CDC veneer in one case, proprietary in another.)
> - /* REVISIT unclear how best to handle this state...
> - * we don't really couple it with the Linux TTY.
> - */
> - gser->port_line_coding = port->port_line_coding;
> -
> - /* REVISIT if waiting on "carrier detect", signal. */
> -
> /* if it's already open, start I/O ... and notify the serial
> * protocol about open/close status (connect/disconnect).
> */
> diff --git a/drivers/usb/gadget/u_serial.h b/drivers/usb/gadget/u_serial.h
> index 300f0ed..b4c791a 100644
> --- a/drivers/usb/gadget/u_serial.h
> +++ b/drivers/usb/gadget/u_serial.h
> @@ -56,6 +56,7 @@ int gserial_setup(struct usb_gadget *g, unsigned n_ports);
> void gserial_cleanup(void);
>
> /* connect/disconnect is handled by individual functions */
> +void gserial_start(struct gserial *, u8 port_num);
> int gserial_connect(struct gserial *, u8 port_num);
> void gserial_disconnect(struct gserial *);
>
> --
> 1.6.3.3.385.g60647
>
>
--
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
