| 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: <20160228132704.GA32461@localhost>
Date: Sun, 28 Feb 2016 14:27:04 +0100
From: Johan Hovold <johan@...nel.org>
To: Mathieu OTHACEHE <m.othacehe@...il.com>
Cc: johan@...nel.org, gregkh@...uxfoundation.org,
linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [PATCH v2 1/4] USB: serial: fix compare_const_fl.cocci warnings
On Thu, Feb 04, 2016 at 07:01:27PM +0100, Mathieu OTHACEHE wrote:
> Move constants to the right of binary operators.
>
> Generated by: scripts/coccinelle/misc/compare_const_fl.cocci
>
> Signed-off-by: Mathieu OTHACEHE <m.othacehe@...il.com>
> ---
> Move constants to the right improve readability in my opinion.
> But it's a matter of taste, and nothing is specified in CodingStyle.
>
> drivers/usb/serial/ch341.c | 2 +-
> drivers/usb/serial/f81232.c | 2 +-
> drivers/usb/serial/ftdi_sio.c | 16 +++++++-------
> drivers/usb/serial/ftdi_sio.h | 8 +++----
> drivers/usb/serial/garmin_gps.c | 48 ++++++++++++++++++++---------------------
> drivers/usb/serial/mos7840.c | 4 ++--
> 6 files changed, 40 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> index c73808f..f139488 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -370,7 +370,7 @@ static void ch341_set_termios(struct tty_struct *tty,
> static void ch341_break_ctl(struct tty_struct *tty, int break_state)
> {
> const uint16_t ch341_break_reg =
> - CH341_REG_BREAK1 | ((uint16_t) CH341_REG_BREAK2 << 8);
> + ((uint16_t) CH341_REG_BREAK2 << 8) | CH341_REG_BREAK1;
> struct usb_serial_port *port = tty->driver_data;
> int r;
> uint16_t reg_contents;
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index 972f5a5..589d856 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -190,7 +190,7 @@ static int f81232_set_mctrl(struct usb_serial_port *port,
>
> /* force enable interrupt with OUT2 */
> mutex_lock(&priv->lock);
> - val = UART_MCR_OUT2 | priv->modem_control;
> + val = priv->modem_control | UART_MCR_OUT2;
I don't consider this an improvement as the modem-control bits are the
least significant bits of the register.
>
> if (clear & TIOCM_DTR)
> val &= ~UART_MCR_DTR;
> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> index 8c660ae..07b0040 100644
> --- a/drivers/usb/serial/ftdi_sio.c
> +++ b/drivers/usb/serial/ftdi_sio.c
> @@ -1320,11 +1320,11 @@ static __u32 get_ftdi_divisor(struct tty_struct *tty,
> if (baud <= 3000000) {
> __u16 product_id = le16_to_cpu(
> port->serial->dev->descriptor.idProduct);
> - if (((FTDI_NDI_HUC_PID == product_id) ||
> - (FTDI_NDI_SPECTRA_SCU_PID == product_id) ||
> - (FTDI_NDI_FUTURE_2_PID == product_id) ||
> - (FTDI_NDI_FUTURE_3_PID == product_id) ||
> - (FTDI_NDI_AURORA_SCU_PID == product_id)) &&
> + if (((product_id == FTDI_NDI_HUC_PID) ||
> + (product_id == FTDI_NDI_SPECTRA_SCU_PID) ||
> + (product_id == FTDI_NDI_FUTURE_2_PID) ||
> + (product_id == FTDI_NDI_FUTURE_3_PID) ||
> + (product_id == FTDI_NDI_AURORA_SCU_PID)) &&
> (baud == 19200)) {
> baud = 1200000;
> }
> @@ -2325,7 +2325,7 @@ no_c_cflag_changes:
> usb_sndctrlpipe(dev, 0),
> FTDI_SIO_SET_FLOW_CTRL_REQUEST,
> FTDI_SIO_SET_FLOW_CTRL_REQUEST_TYPE,
> - 0 , (FTDI_SIO_RTS_CTS_HS | priv->interface),
> + 0, (priv->interface | FTDI_SIO_RTS_CTS_HS),
Similarly, here FTDI_SIO_RTS_CTS_HS is in the most significant byte.
> NULL, 0, WDR_TIMEOUT) < 0) {
> dev_err(ddev, "urb failed to set to rts/cts flow control\n");
> }
> @@ -2354,8 +2354,8 @@ no_c_cflag_changes:
> usb_sndctrlpipe(dev, 0),
> FTDI_SIO_SET_FLOW_CTRL_REQUEST,
> FTDI_SIO_SET_FLOW_CTRL_REQUEST_TYPE,
> - urb_value , (FTDI_SIO_XON_XOFF_HS
> - | priv->interface),
> + urb_value, (priv->interface |
> + FTDI_SIO_XON_XOFF_HS),
Same here.
> NULL, 0, WDR_TIMEOUT) < 0) {
> dev_err(&port->dev, "urb failed to set to "
> "xon/xoff flow control\n");
> diff --git a/drivers/usb/serial/ftdi_sio.h b/drivers/usb/serial/ftdi_sio.h
> index ed58c6f..bbcc13df 100644
> --- a/drivers/usb/serial/ftdi_sio.h
> +++ b/drivers/usb/serial/ftdi_sio.h
> @@ -239,11 +239,11 @@ enum ftdi_sio_baudrate {
> */
>
> #define FTDI_SIO_SET_DTR_MASK 0x1
> -#define FTDI_SIO_SET_DTR_HIGH (1 | (FTDI_SIO_SET_DTR_MASK << 8))
> -#define FTDI_SIO_SET_DTR_LOW (0 | (FTDI_SIO_SET_DTR_MASK << 8))
> +#define FTDI_SIO_SET_DTR_HIGH ((FTDI_SIO_SET_DTR_MASK << 8) | 1)
> +#define FTDI_SIO_SET_DTR_LOW ((FTDI_SIO_SET_DTR_MASK << 8) | 0)
> #define FTDI_SIO_SET_RTS_MASK 0x2
> -#define FTDI_SIO_SET_RTS_HIGH (2 | (FTDI_SIO_SET_RTS_MASK << 8))
> -#define FTDI_SIO_SET_RTS_LOW (0 | (FTDI_SIO_SET_RTS_MASK << 8))
> +#define FTDI_SIO_SET_RTS_HIGH ((FTDI_SIO_SET_RTS_MASK << 8) | 2)
> +#define FTDI_SIO_SET_RTS_LOW ((FTDI_SIO_SET_RTS_MASK << 8) | 0)
>
> /*
> * ControlValue
> diff --git a/drivers/usb/serial/garmin_gps.c b/drivers/usb/serial/garmin_gps.c
> index db591d1..c0bbe44 100644
> --- a/drivers/usb/serial/garmin_gps.c
> +++ b/drivers/usb/serial/garmin_gps.c
> @@ -237,10 +237,10 @@ static inline int getDataLength(const __u8 *usbPacket)
> */
> static inline int isAbortTrfCmnd(const unsigned char *buf)
> {
> - if (0 == memcmp(buf, GARMIN_STOP_TRANSFER_REQ,
> - sizeof(GARMIN_STOP_TRANSFER_REQ)) ||
> - 0 == memcmp(buf, GARMIN_STOP_TRANSFER_REQ_V2,
> - sizeof(GARMIN_STOP_TRANSFER_REQ_V2)))
> + if (memcmp(buf, GARMIN_STOP_TRANSFER_REQ,
> + sizeof(GARMIN_STOP_TRANSFER_REQ)) == 0 ||
> + memcmp(buf, GARMIN_STOP_TRANSFER_REQ_V2,
> + sizeof(GARMIN_STOP_TRANSFER_REQ_V2)) == 0)
> return 1;
> else
> return 0;
> @@ -350,7 +350,7 @@ static int gsp_send_ack(struct garmin_data *garmin_data_p, __u8 pkt_id)
> unsigned l = 0;
>
> dev_dbg(&garmin_data_p->port->dev, "%s - pkt-id: 0x%X.\n", __func__,
> - 0xFF & pkt_id);
> + pkt_id & 0xFF);
pkt_id is u8
>
> *ptr++ = DLE;
> *ptr++ = ACK;
> @@ -366,7 +366,7 @@ static int gsp_send_ack(struct garmin_data *garmin_data_p, __u8 pkt_id)
> *ptr++ = DLE;
>
> *ptr++ = 0;
> - *ptr++ = 0xFF & (-cksum);
> + *ptr++ = (-cksum) & 0xFF;
> *ptr++ = DLE;
> *ptr++ = ETX;
>
> @@ -423,9 +423,9 @@ static int gsp_rec_packet(struct garmin_data *garmin_data_p, int count)
> n++;
> }
>
> - if ((0xff & (cksum + *recpkt)) != 0) {
> + if (((cksum + *recpkt) & 0xff) != 0) {
> dev_dbg(dev, "%s - invalid checksum, expected %02x, got %02x\n",
> - __func__, 0xff & -cksum, 0xff & *recpkt);
> + __func__, -cksum & 0xff, *recpkt & 0xff);
and so is recpkt
> return -EINVPKT;
> }
>
> @@ -528,7 +528,7 @@ static int gsp_receive(struct garmin_data *garmin_data_p,
> dev_dbg(dev, "NAK packet complete.\n");
> } else {
> dev_dbg(dev, "packet complete - id=0x%X.\n",
> - 0xFF & data);
> + data & 0xFF);
and data
> gsp_rec_packet(garmin_data_p, size);
> }
>
> @@ -636,7 +636,7 @@ static int gsp_send(struct garmin_data *garmin_data_p,
>
> garmin_data_p->outsize = 0;
>
> - if (GARMIN_LAYERID_APPL != getLayerId(garmin_data_p->outbuffer)) {
> + if (getLayerId(garmin_data_p->outbuffer) != GARMIN_LAYERID_APPL) {
> dev_dbg(dev, "not an application packet (%d)\n",
> getLayerId(garmin_data_p->outbuffer));
> return -1;
> @@ -688,7 +688,7 @@ static int gsp_send(struct garmin_data *garmin_data_p,
> *dst++ = DLE;
> }
>
> - cksum = 0xFF & -cksum;
> + cksum = -cksum & 0xFF;
> *dst++ = cksum;
> if (cksum == DLE)
> *dst++ = DLE;
> @@ -970,7 +970,7 @@ static void garmin_write_bulk_callback(struct urb *urb)
> struct garmin_data *garmin_data_p =
> usb_get_serial_port_data(port);
>
> - if (GARMIN_LAYERID_APPL == getLayerId(urb->transfer_buffer)) {
> + if (getLayerId(urb->transfer_buffer) == GARMIN_LAYERID_APPL) {
>
> if (garmin_data_p->mode == MODE_GARMIN_SERIAL) {
> gsp_send_ack(garmin_data_p,
> @@ -1025,7 +1025,7 @@ static int garmin_write_bulk(struct usb_serial_port *port,
> dismiss_ack ? NULL : port);
> urb->transfer_flags |= URB_ZERO_PACKET;
>
> - if (GARMIN_LAYERID_APPL == getLayerId(buffer)) {
> + if (getLayerId(buffer) == GARMIN_LAYERID_APPL) {
>
> spin_lock_irqsave(&garmin_data_p->lock, flags);
> garmin_data_p->flags |= APP_REQ_SEEN;
> @@ -1077,9 +1077,9 @@ static int garmin_write(struct tty_struct *tty, struct usb_serial_port *port,
> pktsiz = getDataLength(garmin_data_p->privpkt);
> pktid = getPacketId(garmin_data_p->privpkt);
>
> - if (count == (GARMIN_PKTHDR_LENGTH+pktsiz)
> - && GARMIN_LAYERID_PRIVATE ==
> - getLayerId(garmin_data_p->privpkt)) {
> + if ((GARMIN_PKTHDR_LENGTH+pktsiz) == count &&
Here I believe count should remain on the left side.
> + getLayerId(garmin_data_p->privpkt) ==
> + GARMIN_LAYERID_PRIVATE) {
>
> dev_dbg(dev, "%s - processing private request %d\n",
> __func__, pktid);
> @@ -1192,7 +1192,7 @@ static void garmin_read_bulk_callback(struct urb *urb)
> garmin_read_process(garmin_data_p, data, urb->actual_length, 1);
>
> if (urb->actual_length == 0 &&
> - 0 != (garmin_data_p->flags & FLAGS_BULK_IN_RESTART)) {
> + (garmin_data_p->flags & FLAGS_BULK_IN_RESTART) != 0) {
> spin_lock_irqsave(&garmin_data_p->lock, flags);
> garmin_data_p->flags &= ~FLAGS_BULK_IN_RESTART;
> spin_unlock_irqrestore(&garmin_data_p->lock, flags);
> @@ -1203,7 +1203,7 @@ static void garmin_read_bulk_callback(struct urb *urb)
> __func__, retval);
> } else if (urb->actual_length > 0) {
> /* Continue trying to read until nothing more is received */
> - if (0 == (garmin_data_p->flags & FLAGS_THROTTLED)) {
> + if ((garmin_data_p->flags & FLAGS_THROTTLED) == 0) {
> retval = usb_submit_urb(port->read_urb, GFP_ATOMIC);
> if (retval)
> dev_err(&port->dev,
> @@ -1249,12 +1249,12 @@ static void garmin_read_int_callback(struct urb *urb)
> urb->transfer_buffer);
>
> if (urb->actual_length == sizeof(GARMIN_BULK_IN_AVAIL_REPLY) &&
> - 0 == memcmp(data, GARMIN_BULK_IN_AVAIL_REPLY,
> - sizeof(GARMIN_BULK_IN_AVAIL_REPLY))) {
> + memcmp(data, GARMIN_BULK_IN_AVAIL_REPLY,
> + sizeof(GARMIN_BULK_IN_AVAIL_REPLY)) == 0) {
>
> dev_dbg(&port->dev, "%s - bulk data available.\n", __func__);
>
> - if (0 == (garmin_data_p->flags & FLAGS_BULK_IN_ACTIVE)) {
> + if ((garmin_data_p->flags & FLAGS_BULK_IN_ACTIVE) == 0) {
>
> /* bulk data available */
> retval = usb_submit_urb(port->read_urb, GFP_ATOMIC);
> @@ -1276,8 +1276,8 @@ static void garmin_read_int_callback(struct urb *urb)
> }
>
> } else if (urb->actual_length == (4+sizeof(GARMIN_START_SESSION_REPLY))
> - && 0 == memcmp(data, GARMIN_START_SESSION_REPLY,
> - sizeof(GARMIN_START_SESSION_REPLY))) {
> + && memcmp(data, GARMIN_START_SESSION_REPLY,
> + sizeof(GARMIN_START_SESSION_REPLY)) == 0) {
>
> spin_lock_irqsave(&garmin_data_p->lock, flags);
> garmin_data_p->flags |= FLAGS_SESSION_REPLY1_SEEN;
> @@ -1356,7 +1356,7 @@ static void garmin_unthrottle(struct tty_struct *tty)
> if (garmin_data_p->mode == MODE_NATIVE)
> garmin_flush_queue(garmin_data_p);
>
> - if (0 != (garmin_data_p->flags & FLAGS_BULK_IN_ACTIVE)) {
> + if ((garmin_data_p->flags & FLAGS_BULK_IN_ACTIVE) != 0) {
> status = usb_submit_urb(port->read_urb, GFP_KERNEL);
> if (status)
> dev_err(&port->dev,
> diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
> index 2c69bfc..6a2fab8 100644
> --- a/drivers/usb/serial/mos7840.c
> +++ b/drivers/usb/serial/mos7840.c
> @@ -2039,7 +2039,7 @@ static int mos7810_check(struct usb_serial *serial)
> /* Send the 1-bit test pattern out to MCS7810 test pin */
> usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev, 0),
> MCS_WRREQ, MCS_WR_RTYPE,
> - (0x0300 | (((test_pattern >> i) & 0x0001) << 1)),
> + ((((test_pattern >> i) & 0x0001) << 1) | 0x0300),
Again, I find it more natural to keep the most-significant bits to the
left.
> MODEM_CONTROL_REGISTER, NULL, 0, MOS_WDR_TIMEOUT);
>
> /* Read the test pattern back */
> @@ -2059,7 +2059,7 @@ static int mos7810_check(struct usb_serial *serial)
>
> /* Restore MCR setting */
> usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev, 0), MCS_WRREQ,
> - MCS_WR_RTYPE, 0x0300 | mcr_data, MODEM_CONTROL_REGISTER, NULL,
> + MCS_WR_RTYPE, mcr_data | 0x0300, MODEM_CONTROL_REGISTER, NULL,
Ditto.
> 0, MOS_WDR_TIMEOUT);
>
> kfree(buf);
Ok, so maybe partially accepting this patch wasn't such a good idea, but
I do find that at least the "y == f()" => "f() == y" transformations
increase readability.
Thanks,
Johan
Powered by blists - more mailing lists