Date: Sun, 30 Mar 2008 13:16:52 +0100
From: Alan Cox <alan@lxorguk.ukuu.org.uk>
To: postmaster@van-ginkel.eu
Cc: linux-kernel@vger.kernel.org
Subject: Re: [PATCH] ptmx: adding handshake support
Message-ID: <20080330131652.7073589c@core>
In-Reply-To: <20080330140948.0cqmtqb9us880cw8@62.129.139.44>
References: <20080311212830.5f4apfr4uqs884cc@62.129.139.44>
	<20080330140948.0cqmtqb9us880cw8@62.129.139.44>
Organization: Red Hat UK Cyf., Amberley Place, 107-111 Peascod Street,
 Windsor, Berkshire, SL4 1TE, Y Deyrnas Gyfunol. Cofrestrwyd yng Nghymru a
 Lloegr o'r rhif cofrestru 3798903
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2222
Lines: 77

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.


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