linux-kernel - Re: [PATCH] serial: samsung: add support for manual RTS setting

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-id: <35383907.gKXZyKLrYq@amdc1227>
Date:	Tue, 17 Sep 2013 12:18:50 +0200
From:	Tomasz Figa <t.figa@...sung.com>
To:	José Miguel Gonçalves <jose.goncalves@...v.pt>
Cc:	linux-samsung-soc@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-serial@...r.kernel.org
Subject: Re: [PATCH] serial: samsung: add support for manual RTS setting

Hi José,

Please see my comments below.

On Wednesday 11 of September 2013 11:08:27 José Miguel Gonçalves wrote:
> The Samsung serial driver currently does not support setting the
> RTS pin with an ioctl(TIOCMSET) call. This patch adds this support.
> 
> Signed-off-by: José Miguel Gonçalves <jose.goncalves@...v.pt>
> ---
>  drivers/tty/serial/samsung.c |   17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
> index f3dfa19..e5dd808 100644
> --- a/drivers/tty/serial/samsung.c
> +++ b/drivers/tty/serial/samsung.c
> @@ -407,7 +407,14 @@ static unsigned int s3c24xx_serial_get_mctrl(struct
> uart_port *port)
> 
>  static void s3c24xx_serial_set_mctrl(struct uart_port *port, unsigned
> int mctrl) {
> -	/* todo - possibly remove AFC and do manual CTS */
> +	unsigned int umcon = rd_regl(port, S3C2410_UMCON);
> +
> +	if (mctrl & TIOCM_RTS)
> +		umcon |= S3C2410_UMCOM_RTS_LOW;
> +	else
> +		umcon &= ~S3C2410_UMCOM_RTS_LOW;
> +
> +	wr_regl(port, S3C2410_UMCON, umcon);

I wonder if port capability shouldn't be considered here. Depending on SoC, 
only selected ports provide modem control capability.

For example on S3C64xx only ports 0 and 1 support modem control, while 
ports 2 and 3 don't.

>  }
> 
>  static void s3c24xx_serial_break_ctl(struct uart_port *port, int
> break_state) @@ -774,8 +781,6 @@ static void
> s3c24xx_serial_set_termios(struct uart_port *port, if (termios->c_cflag
> & CSTOPB)
>  		ulcon |= S3C2410_LCON_STOPB;
> 
> -	umcon = (termios->c_cflag & CRTSCTS) ? S3C2410_UMCOM_AFC : 0;
> -
>  	if (termios->c_cflag & PARENB) {
>  		if (termios->c_cflag & PARODD)
>  			ulcon |= S3C2410_LCON_PODD;
> @@ -792,6 +797,12 @@ static void s3c24xx_serial_set_termios(struct
> uart_port *port,
> 
>  	wr_regl(port, S3C2410_ULCON, ulcon);
>  	wr_regl(port, S3C2410_UBRDIV, quot);
> +
> +	if (termios->c_cflag & CRTSCTS)
> +		umcon = S3C2410_UMCOM_AFC;

Is it correct to override the last manual RTS value set to this register 
when activating manual flow control?

Shouldn't the code be more like the following:

	umcon = rd_regb(port, S3C2410_UMCON);
	if (termios->c_cflag & CRTSCTS)
		umcon |= S3C2410_UMCOM_AFC;
	else
		umcon &= ~S3C2410_UMCOM_AFC;
	wr_regl(port, S3C2410_UMCON, umcon);

Probably port capability should be considered here as well.

Best regards,
Tomasz

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