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

[Alan Cox <alan@lxorguk.ukuu.org.uk>]

On Fri, 17 Dec 2010 16:34:23 +0100
Libor Pechacek <lpechacek@suse.cz> wrote:

> Alan's commit 335f8514f200e63d689113d29cb7253a5c282967 introduced
> .carrier_raised function in several drivers.  That also means
> tty_port_block_til_ready can now suspend the process trying to open the serial
> port when Carrier Detect is low and put it into tty_port.open_wait queue.  We
> need to wake up the process when the Carrier Detect goes high.

(catching up after Christmas break)

> @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

??

> The keyspan_pda can report the changes asynchronously, but I haven't found the
> message format description anywhere.  Any idea how that driver can be fixed
> besides dropping .carrier_raised?

I don't - Greg may have info or of course you could cheat and wake the
queue whenever you see an unknown message format, then ask the device 8)

> +/**
> + *	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)

Looks good

Alan