Re: [PATCH] ptmx: adding handshake support

Alan Cox
2008-03-30T12:34:32Z

On Sun, 30 Mar 2008 14:09:48 +0200
postmaster@van-ginkel.eu wrote:

> This weekend I upgraded my kernel to 2.6.22.5 and tried my patch.
> Unfortunately pty.c seems to be a slightly different, so generated a new
> patch:

Really you want to be working against 2.6.25-rc5-mm1 or later as there
are big changes in the tty layer pending, some of which affect the
locking on stuff like this.

> +struct pty_mcrmsr {
> +
> + struct semaphore sem; /* locks this structure */

You create a lock but don't use it. Also btw the -mm tty layer has a
ctrl_lock you could use instead.

> + /* for tiocmget and tiocmset functions */
> +
> + int msr; /* MSR shadow */
> + int mcr; /* MCR shadow */

Could be one value using the existing bit masks.

> + mcrmsr = kmalloc(sizeof(*mcrmsr), GFP_KERNEL);
> +
> + init_MUTEX(&mcrmsr->sem);

Crashes on allocation failure

> + if (tty->driver_data != NULL) {
> + msr = mcrmsr->msr;
> + mcr = mcrmsr->mcr;
> + }

Always true

> +
> + result = ((mcr & 0x01) ? TIOCM_DTR : 0) | /* DTR is set */
> + ((mcr & 0x02) ? TIOCM_RTS : 0) | /* RTS is set */
> + ((mcr & 0x04) ? TIOCM_LOOP : 0) | /* LOOP is set */
> + ((msr & 0x08) ? TIOCM_CTS : 0) | /* CTS is set */
> + ((msr & 0x10) ? TIOCM_CAR : 0) | /* Carrier detect is set*/
> + ((msr & 0x20) ? TIOCM_RI : 0) | /* Ring Indicator is set */
> + ((msr & 0x40) ? TIOCM_DSR : 0); /* DSR is set */

Use the mcr/msr bits as is and you don't need this mess

> + if (set & TIOCM_DTR)
> + mcr |= 0x01;
> + if (set & TIOCM_RTS)
> + mcr |= 0x02;
> + if (set & TIOCM_LOOP)

Ditto

> + /* set the new MCR value in the device */
> +
> + if (tty->driver_data != NULL)
> + mcrmsr->mcr=mcr;

That may cause wakeups, open, hangup etc to occur - does that need to be
considered here.

> + case 0x5426: /* Set all of the handshake line, even the normally
> read only */

Use a proper value as I said before.