Name enum elm327_to_to_do_bits in lowercase for kernel code style
[elmcan.git] / module / elmcan.c
index 75f13590ceb3ddd63582a4f929d7c1e6b6cf74a8..1ac12913d78b5f1466bbb8ff18e3ef827c87ec84 100644 (file)
@@ -71,7 +71,7 @@
 #define ELM327_READY_CHAR '>'
 
 /* Bits in elm->cmds_todo */
-enum ELM327_TX_DO_BITS {
+enum elm327_to_to_do_bits {
        ELM327_TX_DO_CAN_DATA = 0,
        ELM327_TX_DO_CANID_11BIT,
        ELM327_TX_DO_CANID_29BIT_LOW,
@@ -96,10 +96,12 @@ struct elmcan {
        /* Per-channel lock */
        spinlock_t lock;
 
-       /* Stop the channel on hardware failure.
+       /* Stop the channel on UART side hardware failure, e.g. stray
+        * characters or neverending lines. This may be caused by bad
+        * UART wiring, a bad ELM327, a bad UART bridge...
         * Once this is true, nothing will be sent to the TTY.
         */
-       bool hw_failure;
+       bool uart_side_failure;
 
        /* TTY TX helpers */
        struct work_struct tx_work;     /* Flushes TTY TX buffer   */
@@ -133,7 +135,7 @@ struct elmcan {
        unsigned long cmds_todo;
 };
 
-static inline void elm327_hw_failure(struct elmcan *elm);
+static inline void elm327_uart_side_failure(struct elmcan *elm);
 
 static void elm327_send(struct elmcan *elm, const void *buf, size_t len)
 {
@@ -141,7 +143,7 @@ static void elm327_send(struct elmcan *elm, const void *buf, size_t len)
 
        lockdep_assert_held(elm->lock);
 
-       if (elm->hw_failure)
+       if (elm->uart_side_failure)
                return;
 
        memcpy(elm->txbuf, buf, len);
@@ -151,7 +153,7 @@ static void elm327_send(struct elmcan *elm, const void *buf, size_t len)
                netdev_err(elm->dev,
                           "Failed to write to tty %s.\n",
                           elm->tty->name);
-               elm327_hw_failure(elm);
+               elm327_uart_side_failure(elm);
                return;
        }
 
@@ -219,7 +221,9 @@ static void elm327_send_frame(struct elmcan *elm, struct can_frame *frame)
        elm327_kick_into_cmd_mode(elm);
 }
 
-/* ELM327 initialisation sequence. */
+/* ELM327 initialisation sequence.
+ * The line length is limited by the buffer in elm327_handle_prompt().
+ */
 static char *elm327_init_script[] = {
        "AT WS\r",        /* v1.0: Warm Start */
        "AT PP FF OFF\r", /* v1.0: All Programmable Parameters Off */
@@ -292,14 +296,14 @@ static void elm327_feed_frame_to_netdev(struct elmcan *elm,
 }
 
 /* Called when we're out of ideas and just want it all to end. */
-static inline void elm327_hw_failure(struct elmcan *elm)
+static inline void elm327_uart_side_failure(struct elmcan *elm)
 {
        struct can_frame *frame;
        struct sk_buff *skb;
 
        lockdep_assert_held(elm->lock);
 
-       elm->hw_failure = true;
+       elm->uart_side_failure = true;
 
        elm->can.can_stats.bus_off++;
        netif_stop_queue(elm->dev);
@@ -316,23 +320,21 @@ static inline void elm327_hw_failure(struct elmcan *elm)
        elm327_feed_frame_to_netdev(elm, skb);
 }
 
-/* Compare a buffer to a fixed string */
-static inline int _memstrcmp(const u8 *mem, const char *str)
-{
-       return memcmp(mem, str, strlen(str));
-}
-
 /* Compare buffer to string length, then compare buffer to fixed string.
  * This ensures two things:
  *  - It flags cases where the fixed string is only the start of the
  *    buffer, rather than exactly all of it.
  *  - It avoids byte comparisons in case the length doesn't match.
+ *
+ * strncmp() cannot be used here because it accepts the following wrong case:
+ *   strncmp("CAN ER", "CAN ERROR", 6);
+ * This must fail, hence this helper function.
  */
-static inline int _len_memstrcmp(const u8 *mem, size_t mem_len, const char *str)
+static inline int check_len_then_cmp(const u8 *mem, size_t mem_len, const char *str)
 {
        size_t str_len = strlen(str);
 
-       return (mem_len != str_len) || memcmp(mem, str, str_len);
+       return (mem_len == str_len) && !memcmp(mem, str, str_len);
 }
 
 static void elm327_parse_error(struct elmcan *elm, size_t len)
@@ -350,29 +352,29 @@ static void elm327_parse_error(struct elmcan *elm, size_t len)
                return;
 
        /* Filter possible error messages based on length of RX'd line */
-       if (!_len_memstrcmp(elm->rxbuf, len, "UNABLE TO CONNECT")) {
+       if (check_len_then_cmp(elm->rxbuf, len, "UNABLE TO CONNECT")) {
                netdev_err(elm->dev,
                           "ELM327 reported UNABLE TO CONNECT. Please check your setup.\n");
-       } else if (!_len_memstrcmp(elm->rxbuf, len, "BUFFER FULL")) {
+       } else if (check_len_then_cmp(elm->rxbuf, len, "BUFFER FULL")) {
                /* This will only happen if the last data line was complete.
                 * Otherwise, elm327_parse_frame() will heuristically
                 * emit this kind of error frame instead.
                 */
                frame->can_id |= CAN_ERR_CRTL;
                frame->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
-       } else if (!_len_memstrcmp(elm->rxbuf, len, "BUS ERROR")) {
+       } else if (check_len_then_cmp(elm->rxbuf, len, "BUS ERROR")) {
                frame->can_id |= CAN_ERR_BUSERROR;
-       } else if (!_len_memstrcmp(elm->rxbuf, len, "CAN ERROR")) {
+       } else if (check_len_then_cmp(elm->rxbuf, len, "CAN ERROR")) {
                frame->can_id |= CAN_ERR_PROT;
-       } else if (!_len_memstrcmp(elm->rxbuf, len, "<RX ERROR")) {
+       } else if (check_len_then_cmp(elm->rxbuf, len, "<RX ERROR")) {
                frame->can_id |= CAN_ERR_PROT;
-       } else if (!_len_memstrcmp(elm->rxbuf, len, "BUS BUSY")) {
+       } else if (check_len_then_cmp(elm->rxbuf, len, "BUS BUSY")) {
                frame->can_id |= CAN_ERR_PROT;
                frame->data[2] = CAN_ERR_PROT_OVERLOAD;
-       } else if (!_len_memstrcmp(elm->rxbuf, len, "FB ERROR")) {
+       } else if (check_len_then_cmp(elm->rxbuf, len, "FB ERROR")) {
                frame->can_id |= CAN_ERR_PROT;
                frame->data[2] = CAN_ERR_PROT_TX;
-       } else if (len == 5 && !_memstrcmp(elm->rxbuf, "ERR")) {
+       } else if (len == 5 && !memcmp(elm->rxbuf, "ERR", 3)) {
                /* ERR is followed by two digits, hence line length 5 */
                netdev_err(elm->dev, "ELM327 reported an ERR%c%c. Please power it off and on again.\n",
                           elm->rxbuf[3], elm->rxbuf[4]);
@@ -489,7 +491,7 @@ static int elm327_parse_frame(struct elmcan *elm, size_t len)
 
        /* Check for RTR frame */
        if (elm->rxfill >= hexlen + 3 &&
-           !_memstrcmp(&elm->rxbuf[hexlen], "RTR")) {
+           !memcmp(&elm->rxbuf[hexlen], "RTR", 3)) {
                frame->can_id |= CAN_RTR_FLAG;
        }
 
@@ -539,7 +541,7 @@ static void elm327_parse_line(struct elmcan *elm, size_t len)
        if (elm->drop_next_line) {
                elm->drop_next_line = 0;
                return;
-       } else if (!_memstrcmp(elm->rxbuf, "AT")) {
+       } else if (!memcmp(elm->rxbuf, "AT", 2)) {
                return;
        }
 
@@ -557,7 +559,11 @@ static void elm327_parse_line(struct elmcan *elm, size_t len)
 static void elm327_handle_prompt(struct elmcan *elm)
 {
        struct can_frame *frame = &elm->can_frame_to_send;
-       char local_txbuf[20];
+       /* Size this buffer for the largest ELM327 line we may generate,
+        * which is currently an 8 byte CAN frame's payload hexdump.
+        * Items in elm327_init_script must fit here, too!
+        */
+       char local_txbuf[sizeof("0102030405060708\r")];
 
        lockdep_assert_held(elm->lock);
 
@@ -571,7 +577,9 @@ static void elm327_handle_prompt(struct elmcan *elm)
 
        /* Reconfigure ELM327 step by step as indicated by elm->cmds_todo */
        if (test_bit(ELM327_TX_DO_INIT, &elm->cmds_todo)) {
-               strcpy(local_txbuf, *elm->next_init_cmd);
+               snprintf(local_txbuf, sizeof(local_txbuf),
+                       "%s",
+                       *elm->next_init_cmd);
 
                elm->next_init_cmd++;
                if (!(*elm->next_init_cmd)) {
@@ -580,31 +588,38 @@ static void elm327_handle_prompt(struct elmcan *elm)
                }
 
        } else if (test_and_clear_bit(ELM327_TX_DO_SILENT_MONITOR, &elm->cmds_todo)) {
-               sprintf(local_txbuf, "ATCSM%i\r",
+               snprintf(local_txbuf, sizeof(local_txbuf),
+                       "ATCSM%i\r",
                        !(!(elm->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)));
 
        } else if (test_and_clear_bit(ELM327_TX_DO_RESPONSES, &elm->cmds_todo)) {
-               sprintf(local_txbuf, "ATR%i\r",
+               snprintf(local_txbuf, sizeof(local_txbuf),
+                       "ATR%i\r",
                        !(elm->can.ctrlmode & CAN_CTRLMODE_LISTENONLY));
 
        } else if (test_and_clear_bit(ELM327_TX_DO_CAN_CONFIG, &elm->cmds_todo)) {
-               sprintf(local_txbuf, "ATPC\r");
+               snprintf(local_txbuf, sizeof(local_txbuf),
+                       "ATPC\r");
                set_bit(ELM327_TX_DO_CAN_CONFIG_PART2, &elm->cmds_todo);
 
        } else if (test_and_clear_bit(ELM327_TX_DO_CAN_CONFIG_PART2, &elm->cmds_todo)) {
-               sprintf(local_txbuf, "ATPB%04X\r",
+               snprintf(local_txbuf, sizeof(local_txbuf),
+                       "ATPB%04X\r",
                        elm->can_config);
 
        } else if (test_and_clear_bit(ELM327_TX_DO_CANID_29BIT_HIGH, &elm->cmds_todo)) {
-               sprintf(local_txbuf, "ATCP%02X\r",
+               snprintf(local_txbuf, sizeof(local_txbuf),
+                       "ATCP%02X\r",
                        (frame->can_id & CAN_EFF_MASK) >> 24);
 
        } else if (test_and_clear_bit(ELM327_TX_DO_CANID_29BIT_LOW, &elm->cmds_todo)) {
-               sprintf(local_txbuf, "ATSH%06X\r",
+               snprintf(local_txbuf, sizeof(local_txbuf),
+                       "ATSH%06X\r",
                        frame->can_id & CAN_EFF_MASK & ((1 << 24) - 1));
 
        } else if (test_and_clear_bit(ELM327_TX_DO_CANID_11BIT, &elm->cmds_todo)) {
-               sprintf(local_txbuf, "ATSH%03X\r",
+               snprintf(local_txbuf, sizeof(local_txbuf),
+                       "ATSH%03X\r",
                        frame->can_id & CAN_SFF_MASK);
 
        } else if (test_and_clear_bit(ELM327_TX_DO_CAN_DATA, &elm->cmds_todo)) {
@@ -612,17 +627,20 @@ static void elm327_handle_prompt(struct elmcan *elm)
                        /* Send an RTR frame. Their DLC is fixed.
                         * Some chips don't send them at all.
                         */
-                       sprintf(local_txbuf, "ATRTR\r");
+                       snprintf(local_txbuf, sizeof(local_txbuf),
+                               "ATRTR\r");
                } else {
                        /* Send a regular CAN data frame */
                        int i;
 
                        for (i = 0; i < frame->len; i++) {
-                               sprintf(&local_txbuf[2 * i], "%02X",
+                               snprintf(&local_txbuf[2 * i], sizeof(local_txbuf),
+                                       "%02X",
                                        frame->data[i]);
                        }
 
-                       sprintf(&local_txbuf[2 * i], "\r");
+                       snprintf(&local_txbuf[2 * i], sizeof(local_txbuf),
+                               "\r");
                }
 
                elm->drop_next_line = 1;
@@ -703,7 +721,7 @@ static void elm327_parse_rxbuf(struct elmcan *elm)
                         */
                        netdev_err(elm->dev,
                                   "RX buffer overflow. Faulty ELM327 or UART?\n");
-                       elm327_hw_failure(elm);
+                       elm327_uart_side_failure(elm);
                        break;
                } else if (len == elm->rxfill) {
                        if (elm327_is_ready_char(elm->rxbuf[elm->rxfill - 1])) {
@@ -753,7 +771,7 @@ static int elmcan_netdev_open(struct net_device *dev)
        int err;
 
        spin_lock_bh(&elm->lock);
-       if (elm->hw_failure) {
+       if (elm->uart_side_failure) {
                netdev_err(elm->dev, "Refusing to open interface after a hardware fault has been detected.\n");
                spin_unlock_bh(&elm->lock);
                return -EIO;
@@ -844,10 +862,10 @@ static netdev_tx_t elmcan_netdev_start_xmit(struct sk_buff *skb,
        /* We shouldn't get here after a hardware fault:
         * can_bus_off() calls netif_carrier_off()
         */
-       WARN_ON_ONCE(elm->hw_failure);
+       WARN_ON_ONCE(elm->uart_side_failure);
 
        if (!elm->tty ||
-           elm->hw_failure ||
+           elm->uart_side_failure ||
            elm->can.ctrlmode & CAN_CTRLMODE_LISTENONLY) {
                spin_unlock(&elm->lock);
                goto out;
@@ -907,14 +925,14 @@ static void elmcan_ldisc_rx(struct tty_struct *tty,
 
        spin_lock_bh(&elm->lock);
 
-       if (elm->hw_failure)
+       if (elm->uart_side_failure)
                goto out;
 
        while (count-- && elm->rxfill < ELM327_SIZE_RXBUF) {
                if (fp && *fp++) {
                        netdev_err(elm->dev, "Error in received character stream. Check your wiring.");
 
-                       elm327_hw_failure(elm);
+                       elm327_uart_side_failure(elm);
 
                        goto out;
                }
@@ -932,7 +950,7 @@ static void elmcan_ldisc_rx(struct tty_struct *tty,
                                netdev_err(elm->dev,
                                           "Received illegal character %02x.\n",
                                           *cp);
-                               elm327_hw_failure(elm);
+                               elm327_uart_side_failure(elm);
 
                                goto out;
                        }
@@ -946,7 +964,7 @@ static void elmcan_ldisc_rx(struct tty_struct *tty,
        if (count >= 0) {
                netdev_err(elm->dev, "Receive buffer overflowed. Bad chip or wiring?");
 
-               elm327_hw_failure(elm);
+               elm327_uart_side_failure(elm);
 
                goto out;
        }
@@ -965,7 +983,7 @@ static void elmcan_ldisc_tx_worker(struct work_struct *work)
        struct elmcan *elm = container_of(work, struct elmcan, tx_work);
        ssize_t written;
 
-       if (elm->hw_failure)
+       if (elm->uart_side_failure)
                return;
 
        spin_lock_bh(&elm->lock);
@@ -976,7 +994,7 @@ static void elmcan_ldisc_tx_worker(struct work_struct *work)
                        netdev_err(elm->dev,
                                   "Failed to write to tty %s.\n",
                                   elm->tty->name);
-                       elm327_hw_failure(elm);
+                       elm327_uart_side_failure(elm);
                        spin_unlock_bh(&elm->lock);
                        return;
                } else {
@@ -1113,18 +1131,6 @@ static void elmcan_ldisc_close(struct tty_struct *tty)
        free_candev(elm->dev);
 }
 
-#if LINUX_VERSION_CODE < KERNEL_VERSION(5,16,0)
-static int elmcan_ldisc_hangup(struct tty_struct *tty)
-#else
-static void elmcan_ldisc_hangup(struct tty_struct *tty)
-#endif
-{
-       elmcan_ldisc_close(tty);
-#if LINUX_VERSION_CODE < KERNEL_VERSION(5,16,0)
-       return 0;
-#endif
-}
-
 static int elmcan_ldisc_ioctl(struct tty_struct *tty,
 #if LINUX_VERSION_CODE < KERNEL_VERSION(5,17,0)
                              struct file *file,
@@ -1161,7 +1167,6 @@ static struct tty_ldisc_ops elmcan_ldisc = {
        .write_wakeup   = elmcan_ldisc_tx_wakeup,
        .open           = elmcan_ldisc_open,
        .close          = elmcan_ldisc_close,
-       .hangup         = elmcan_ldisc_hangup,
        .ioctl          = elmcan_ldisc_ioctl,
 };