Re: [PATCH 2.6.36-rc3] USB: serial: handle Data Carrier Detectchanges

[Libor Pechacek]

On Tue 04-01-11 13:53:13, Alan Cox wrote:
> On Fri, 17 Dec 2010 16:34:23 +0100 Libor Pechacek <lpechacek@xxxxxxx> wrote:
> > @Greg: There are two drivers left to be fixed - drivers/usb/serial/cp210x.c and
> > drivers/usb/serial/keyspan_pda.c.  cp210x device does not seem to have an
> > interrupt endpoint to report the changes so a polling thread seems to be needed
> > to detect DCD changes.  I can implement the polling for cp210x if you don't
> > have a better idea.
> 
> It might be worth making tty_port_open know how to handle such devices as
> I'd bet there will be others out there somewhere. Perhaps add a dcd_poll
> field that holds the poll time in ms then in the tty_port_block_til_ready
> code change from
> 
> 	tty_unlock();
> 	schedule();
> 	tty_lock();
> 
> to 
> 
> 	tty_unlock();
> 	if (port->dcd_poll)
> 		schedule_timeout(port->dcd_poll);
> 	else
> 		schedule();
> 	tty_lock

I fell in love with the idea and implemented it.   Then between implementation
and the first testing I realized that we also need to handle the complementary
case - DCD drop - where we indicate hangup.

At the moment I don't see an elegant solution to sense DCD drop.  For some
devices reading the status lines also involves an extra USB control transaction
that should be done quite often.  That looks too clumsy to me and I'd like to
avoid that if possible.

> > +/**
> > + *	usb_serial_handle_dcd_change - handle a change of carrier detect state
> > + *	@port: usb_serial_port structure for the open port
> > + *	@status: new carrier detect status, nonzero if active
> > + */
> > +void usb_serial_handle_dcd_change(struct usb_serial_port *usb_port,
> > +						unsigned int status)
> > +{
> > +	struct tty_port *port = &usb_port->port;
> > +	struct tty_struct *tty = port->tty;
> > +
> > +	dbg("%s - port %d, status %d", __func__, usb_port->number, status);
> > +
> > +	if (status)
> > +		wake_up_interruptible(&port->open_wait);
> > +	else if (tty && !C_CLOCAL(tty))
> > +		tty_hangup(tty);
> > +}
> > +EXPORT_SYMBOL_GPL(usb_serial_handle_dcd_change);
> 
> This looks good except that port->tty isn't necessarily a safe
> de-reference so it would be better to pass the tty into the function so
> the caller must think about that problem (and all the calling points
> appear to have a tty reference held ready)

Thanks for the hint.  I like the separation and added the parameter.

Updated patch will follow.

Libor
-- 
Libor Pechacek
SUSE L3 Team, Prague
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/