| 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
| ||
|
Message-ID: <20160503093751.GI25025@localhost>
Date: Tue, 3 May 2016 11:37:51 +0200
From: Johan Hovold <johan@...nel.org>
To: Konstantin Shkolnyy <konstantin.shkolnyy@...il.com>
Cc: johan@...nel.org, linux-usb@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/3] USB: serial: cp210x: Got rid of magic numbers in
CRTSCTS flag code.
On Sat, Apr 30, 2016 at 09:49:38AM -0500, Konstantin Shkolnyy wrote:
> Replaced magic numbers used in the CRTSCTS flag code with symbolic names
> from the chip specification.
>
> Signed-off-by: Konstantin Shkolnyy <konstantin.shkolnyy@...il.com>
> ---
> v3:
> Regenerated the patches correctly against the latest usb-next branch.
> v2
> Improved CRTSCTS fix by feedback. Dropped get_termios error handling fix.
>
> drivers/usb/serial/cp210x.c | 93 +++++++++++++++++++++++++++++++++------------
> 1 file changed, 69 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index fef7a51..24955a7 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -327,6 +327,40 @@ struct cp210x_comm_status {
> */
> #define PURGE_ALL 0x000f
>
> +/* CP210X_GET_FLOW/CP210X_SET_FLOW read/write these 0x10 bytes */
> +struct cp210x_flow_ctl {
> + __le32 ulControlHandshake;
> + __le32 ulFlowReplace;
> + __le32 ulXonLimit;
> + __le32 ulXoffLimit;
> +} __packed;
> +
> +/* cp210x_flow_ctl::ulControlHandshake */
> +#define SERIAL_DTR_MASK 0x00000003
> +#define SERIAL_CTS_HANDSHAKE 0x00000008
> +#define SERIAL_DSR_HANDSHAKE 0x00000010
> +#define SERIAL_DCD_HANDSHAKE 0x00000020
> +#define SERIAL_DSR_SENSITIVITY 0x00000040
> +
> +/* values for cp210x_flow_ctl::ulControlHandshake::SERIAL_DTR_MASK */
> +#define SERIAL_DTR_INACTIVE 0x00000000
> +#define SERIAL_DTR_ACTIVE 0x00000001
> +#define SERIAL_DTR_FLOW_CTL 0x00000002
> +
> +/* cp210x_flow_ctl::ulFlowReplace */
> +#define SERIAL_AUTO_TRANSMIT 0x00000001
> +#define SERIAL_AUTO_RECEIVE 0x00000002
> +#define SERIAL_ERROR_CHAR 0x00000004
> +#define SERIAL_NULL_STRIPPING 0x00000008
> +#define SERIAL_BREAK_CHAR 0x00000010
> +#define SERIAL_RTS_MASK 0x000000c0
> +#define SERIAL_XOFF_CONTINUE 0x80000000
> +
> +/* values for cp210x_flow_ctl::ulFlowReplace::SERIAL_RTS_MASK */
> +#define SERIAL_RTS_INACTIVE 0x00000000
> +#define SERIAL_RTS_ACTIVE 0x00000040
> +#define SERIAL_RTS_FLOW_CTL 0x00000080
Please use the BIT and GENMASK macros for the above. Also add shift
defines for the DTR and RTS fields instead of including it in the
values.
> +
> /*
> * Reads a variable-sized block of CP210X_ registers, identified by req.
> * Returns data into buf in native USB byte order.
> @@ -694,7 +728,7 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
> {
> struct device *dev = &port->dev;
> unsigned int cflag;
> - u8 modem_ctl[16];
> + struct cp210x_flow_ctl flow_ctl;
> u32 baud;
> u16 bits;
>
> @@ -792,9 +826,9 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
> break;
> }
>
> - cp210x_read_reg_block(port, CP210X_GET_FLOW, modem_ctl,
> - sizeof(modem_ctl));
> - if (modem_ctl[0] & 0x08) {
> + cp210x_read_reg_block(port, CP210X_GET_FLOW, &flow_ctl,
> + sizeof(flow_ctl));
> + if (le32_to_cpu(flow_ctl.ulControlHandshake) & SERIAL_CTS_HANDSHAKE) {
Use a temporary for the control-handshake field to make this a bit more
readable.
> dev_dbg(dev, "%s - flow control = CRTSCTS\n", __func__);
> cflag |= CRTSCTS;
> } else {
> @@ -863,7 +897,6 @@ static void cp210x_set_termios(struct tty_struct *tty,
> struct device *dev = &port->dev;
> unsigned int cflag, old_cflag;
> u16 bits;
> - u8 modem_ctl[16];
>
> cflag = tty->termios.c_cflag;
> old_cflag = old_termios->c_cflag;
> @@ -948,33 +981,45 @@ static void cp210x_set_termios(struct tty_struct *tty,
>
> if ((cflag & CRTSCTS) != (old_cflag & CRTSCTS)) {
>
You can remove this stray newline as well.
> - /* Only bytes 0, 4 and 7 out of first 8 have functional bits */
> + struct cp210x_flow_ctl flow_ctl;
> + u32 ControlHandshake;
> + u32 FlowReplace;
Don't use CamelCase. The only exception would be for message structures
going out over the wire were we allow using the specification field
names.
Use something short as ctrl and flow for these temporaries.
> - cp210x_read_reg_block(port, CP210X_GET_FLOW, modem_ctl,
> - sizeof(modem_ctl));
> - dev_dbg(dev, "%s - read modem controls = %02x .. .. .. %02x .. .. %02x\n",
> - __func__, modem_ctl[0], modem_ctl[4], modem_ctl[7]);
> + cp210x_read_reg_block(port, CP210X_GET_FLOW, &flow_ctl,
> + sizeof(flow_ctl));
> + ControlHandshake = le32_to_cpu(flow_ctl.ulControlHandshake);
> + FlowReplace = le32_to_cpu(flow_ctl.ulFlowReplace);
> + dev_dbg(dev, "%s - read ulControlHandshake=%08x ulFlowReplace=%08x\n",
> + __func__, ControlHandshake, FlowReplace);
Please add a 0x prefix after the equal signs. Add a comma after the
first value?
> if (cflag & CRTSCTS) {
> - modem_ctl[0] &= ~0x7B;
> - modem_ctl[0] |= 0x09;
> - modem_ctl[4] = 0x80;
> - /* FIXME - why clear reserved bits just read? */
> - modem_ctl[5] = 0;
> - modem_ctl[6] = 0;
> - modem_ctl[7] = 0;
> + ControlHandshake &= ~(SERIAL_DTR_MASK |
> + SERIAL_CTS_HANDSHAKE | SERIAL_DSR_HANDSHAKE |
> + SERIAL_DCD_HANDSHAKE | SERIAL_DSR_SENSITIVITY);
> + ControlHandshake |= SERIAL_DTR_ACTIVE;
> + ControlHandshake |= SERIAL_CTS_HANDSHAKE;
> + /* FIXME why clear bits unrelated to flow control */
> + /* FIXME why clear _XOFF_CONTINUE which is never set */
Please add colon after "FIXME". Stray _ in _XOFF_...?
> + FlowReplace &= ~0xffffffff;
> + FlowReplace |= SERIAL_RTS_FLOW_CTL;
Just use assignment (or set to 0 before ORing) until this is fixed.
> dev_dbg(dev, "%s - flow control = CRTSCTS\n", __func__);
> } else {
> - modem_ctl[0] &= ~0x7B;
> - modem_ctl[0] |= 0x01;
> - modem_ctl[4] = 0x40;
> + ControlHandshake &= ~(SERIAL_DTR_MASK |
> + SERIAL_CTS_HANDSHAKE | SERIAL_DSR_HANDSHAKE |
> + SERIAL_DCD_HANDSHAKE | SERIAL_DSR_SENSITIVITY);
> + ControlHandshake |= SERIAL_DTR_ACTIVE;
> + /* FIXME - why clear bits unrelated to flow control */
> + FlowReplace &= ~0xff;
> + FlowReplace |= SERIAL_RTS_ACTIVE;
Ditto.
> dev_dbg(dev, "%s - flow control = NONE\n", __func__);
> }
>
> - dev_dbg(dev, "%s - write modem controls = %02x .. .. .. %02x .. .. %02x\n",
> - __func__, modem_ctl[0], modem_ctl[4], modem_ctl[7]);
> - cp210x_write_reg_block(port, CP210X_SET_FLOW, modem_ctl,
> - sizeof(modem_ctl));
> + dev_dbg(dev, "%s - write ulControlHandshake=%08x ulFlowReplace=%08x\n",
> + __func__, ControlHandshake, FlowReplace);
See above.
> + flow_ctl.ulControlHandshake = cpu_to_le32(ControlHandshake);
> + flow_ctl.ulFlowReplace = cpu_to_le32(FlowReplace);
> + cp210x_write_reg_block(port, CP210X_SET_FLOW, &flow_ctl,
> + sizeof(flow_ctl));
> }
>
> }
Thanks,
Johan
Powered by blists - more mailing lists