Hi,
On Mon, Jul 27, 2009 at 10:50:53PM +0200, ext David Brownell wrote:
> > 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()?
so we load g_serial with f_obex, then obexd opens /dev/ttyGS0 then we
connect the cable and gser->connect() is not called since port->port_usb
only becomes a valid pointer after gserial_connect() is called. And
gserial_connect() is only called when host succesfully sets alternate
setting 1 (on obex case).
You see the problem ?
>
>
> > + */
> > +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.)
ooh too bad, I should have reviewed better before sending, it was a
problem when porting the patch from internal nokia tree to mainline.
Sorry for that, I will resend once you agree there's a real bug there.
--
balbi
--
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
