Re-align buffers, make RX buffer a power of two
[elmcan.git] / module / can327.c
index 75d1ec5bf118d39805adb8b70fefab7e544aac91..799905fd2d29fc4fa48fa3e96c03f8e08bd11b25 100644 (file)
 
 #include <linux/init.h>
 #include <linux/module.h>
-#include <linux/moduleparam.h>
 
-#include <linux/atomic.h>
 #include <linux/bitops.h>
 #include <linux/ctype.h>
-#include <linux/delay.h>
 #include <linux/errno.h>
-#include <linux/if_ether.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/lockdep.h>
@@ -39,7 +35,6 @@
 #include <linux/can.h>
 #include <linux/can/dev.h>
 #include <linux/can/error.h>
-#include <linux/can/led.h>
 #include <linux/can/rx-offload.h>
 
 /* Line discipline ID number.
@@ -57,8 +52,8 @@
 
 #define ELM327_NAPI_WEIGHT 4
 
-#define ELM327_SIZE_RXBUF 224
 #define ELM327_SIZE_TXBUF 32
+#define ELM327_SIZE_RXBUF 1024
 
 #define ELM327_CAN_CONFIG_SEND_SFF           0x8000
 #define ELM327_CAN_CONFIG_VARIABLE_DLC       0x4000
@@ -70,7 +65,7 @@
 #define ELM327_READY_CHAR '>'
 
 /* Bits in elm->cmds_todo */
-enum elm327_to_to_do_bits {
+enum elm327_tx_do {
        ELM327_TX_DO_CAN_DATA = 0,
        ELM327_TX_DO_CANID_11BIT,
        ELM327_TX_DO_CANID_29BIT_LOW,
@@ -89,8 +84,8 @@ struct can327 {
        struct can_rx_offload offload;
 
        /* TTY buffers */
+       u8 txbuf[ELM327_SIZE_TXBUF];
        u8 rxbuf[ELM327_SIZE_RXBUF];
-       u8 txbuf[ELM327_SIZE_TXBUF] ____cacheline_aligned;
 
        /* Per-channel lock */
        spinlock_t lock;
@@ -195,8 +190,8 @@ static void elm327_send_frame(struct can327 *elm, struct can_frame *frame)
        /* Schedule any necessary changes in ELM327's CAN configuration */
        if (elm->can_frame_to_send.can_id != frame->can_id) {
                /* Set the new CAN ID for transmission. */
-               if ((frame->can_id & CAN_EFF_FLAG) ^
-                   (elm->can_frame_to_send.can_id & CAN_EFF_FLAG)) {
+               if ((frame->can_id ^ elm->can_frame_to_send.can_id)
+                   & CAN_EFF_FLAG) {
                        elm->can_config = (frame->can_id & CAN_EFF_FLAG
                                                ? 0
                                                : ELM327_CAN_CONFIG_SEND_SFF)
@@ -260,7 +255,7 @@ static void elm327_init(struct can327 *elm)
        elm->drop_next_line = 0;
 
        /* We can only set the bitrate as a fraction of 500000.
-        * The bit timing constants in can327_bittiming_const will
+        * The bitrates listed in can327_bitrate_const will
         * limit the user to the right values.
         */
        elm->can_bitrate_divisor = 500000 / elm->can.bittiming.bitrate;
@@ -326,21 +321,21 @@ static inline void elm327_uart_side_failure(struct can327 *elm)
        elm327_feed_frame_to_netdev(elm, skb);
 }
 
-/* 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.
+/* Compares a byte buffer (non-NUL terminated) to the payload part of a string,
+ * and returns true iff the buffer (content *and* length) is exactly that
+ * string, without the terminating NUL byte.
  *
- * 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.
+ * Example: If reference is "BUS ERROR", then this returns true iff nbytes == 9
+ *          and !memcmp(buf, "BUS ERROR", 9).
+ *
+ * The reason to use strings is so we can easily include them in the C code,
+ * and to avoid hardcoding lengths.
  */
-static inline int check_len_then_cmp(const u8 *mem, size_t mem_len, const char *str)
+static inline bool elm327_rxbuf_cmp(const u8 *buf, size_t nbytes, const char *reference)
 {
-       size_t str_len = strlen(str);
+       size_t ref_len = strlen(reference);
 
-       return (mem_len == str_len) && !memcmp(mem, str, str_len);
+       return (nbytes == ref_len) && !memcmp(buf, reference, ref_len);
 }
 
 static void elm327_parse_error(struct can327 *elm, size_t len)
@@ -358,26 +353,26 @@ static void elm327_parse_error(struct can327 *elm, size_t len)
                return;
 
        /* Filter possible error messages based on length of RX'd line */
-       if (check_len_then_cmp(elm->rxbuf, len, "UNABLE TO CONNECT")) {
+       if (elm327_rxbuf_cmp(elm->rxbuf, len, "UNABLE TO CONNECT")) {
                netdev_err(elm->dev,
                           "ELM327 reported UNABLE TO CONNECT. Please check your setup.\n");
-       } else if (check_len_then_cmp(elm->rxbuf, len, "BUFFER FULL")) {
+       } else if (elm327_rxbuf_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 (check_len_then_cmp(elm->rxbuf, len, "BUS ERROR")) {
+       } else if (elm327_rxbuf_cmp(elm->rxbuf, len, "BUS ERROR")) {
                frame->can_id |= CAN_ERR_BUSERROR;
-       } else if (check_len_then_cmp(elm->rxbuf, len, "CAN ERROR")) {
+       } else if (elm327_rxbuf_cmp(elm->rxbuf, len, "CAN ERROR")) {
                frame->can_id |= CAN_ERR_PROT;
-       } else if (check_len_then_cmp(elm->rxbuf, len, "<RX ERROR")) {
+       } else if (elm327_rxbuf_cmp(elm->rxbuf, len, "<RX ERROR")) {
                frame->can_id |= CAN_ERR_PROT;
-       } else if (check_len_then_cmp(elm->rxbuf, len, "BUS BUSY")) {
+       } else if (elm327_rxbuf_cmp(elm->rxbuf, len, "BUS BUSY")) {
                frame->can_id |= CAN_ERR_PROT;
                frame->data[2] = CAN_ERR_PROT_OVERLOAD;
-       } else if (check_len_then_cmp(elm->rxbuf, len, "FB ERROR")) {
+       } else if (elm327_rxbuf_cmp(elm->rxbuf, len, "FB ERROR")) {
                frame->can_id |= CAN_ERR_PROT;
                frame->data[2] = CAN_ERR_PROT_TX;
        } else if (len == 5 && !memcmp(elm->rxbuf, "ERR", 3)) {
@@ -444,6 +439,7 @@ static int elm327_parse_frame(struct can327 *elm, size_t len)
                /* The line is likely garbled anyway, so bail.
                 * The main code will restart listening.
                 */
+               kfree_skb(skb);
                return -ENODATA;
        }
 
@@ -462,6 +458,7 @@ static int elm327_parse_frame(struct can327 *elm, size_t len)
                /* This is not a well-formatted data line.
                 * Assume it's an error message.
                 */
+               kfree_skb(skb);
                return -ENODATA;
        }
 
@@ -469,6 +466,7 @@ static int elm327_parse_frame(struct can327 *elm, size_t len)
                /* The line is too short to be a valid frame hex dump.
                 * Something interrupted the hex dump or it is invalid.
                 */
+               kfree_skb(skb);
                return -ENODATA;
        }
 
@@ -520,6 +518,7 @@ static int elm327_parse_frame(struct can327 *elm, size_t len)
                 * However, this will correctly drop the state machine back into
                 * command mode.
                 */
+               kfree_skb(skb);
                return -ENODATA;
        }
 
@@ -601,7 +600,7 @@ static void elm327_handle_prompt(struct can327 *elm)
        } else if (test_and_clear_bit(ELM327_TX_DO_SILENT_MONITOR, &elm->cmds_todo)) {
                snprintf(local_txbuf, sizeof(local_txbuf),
                         "ATCSM%i\r",
-                        !(!(elm->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)));
+                        !!(elm->can.ctrlmode & CAN_CTRLMODE_LISTENONLY));
 
        } else if (test_and_clear_bit(ELM327_TX_DO_RESPONSES, &elm->cmds_todo)) {
                snprintf(local_txbuf, sizeof(local_txbuf),
@@ -682,10 +681,9 @@ static void elm327_drop_bytes(struct can327 *elm, size_t i)
        elm->rxfill -= i;
 }
 
-static void elm327_parse_rxbuf(struct can327 *elm)
+static void elm327_parse_rxbuf(struct can327 *elm, size_t first_new_char_idx)
 {
-       size_t len;
-       int i;
+       size_t len, pos;
 
        lockdep_assert_held(&elm->lock);
 
@@ -695,25 +693,22 @@ static void elm327_parse_rxbuf(struct can327 *elm)
                break;
 
        case ELM327_STATE_GETDUMMYCHAR:
-       {
                /* Wait for 'y' or '>' */
-               for (i = 0; i < elm->rxfill; i++) {
-                       if (elm->rxbuf[i] == ELM327_DUMMY_CHAR) {
+               for (pos = 0; pos < elm->rxfill; pos++) {
+                       if (elm->rxbuf[pos] == ELM327_DUMMY_CHAR) {
                                elm327_send(elm, "\r", 1);
                                elm->state = ELM327_STATE_GETPROMPT;
-                               i++;
+                               pos++;
                                break;
-                       } else if (elm327_is_ready_char(elm->rxbuf[i])) {
+                       } else if (elm327_is_ready_char(elm->rxbuf[pos])) {
                                elm327_send(elm, ELM327_DUMMY_STRING, 1);
-                               i++;
+                               pos++;
                                break;
                        }
                }
 
-               elm327_drop_bytes(elm, i);
-
+               elm327_drop_bytes(elm, pos);
                break;
-       }
 
        case ELM327_STATE_GETPROMPT:
                /* Wait for '>' */
@@ -725,20 +720,17 @@ static void elm327_parse_rxbuf(struct can327 *elm)
 
        case ELM327_STATE_RECEIVING:
                /* Find <CR> delimiting feedback lines. */
-               for (len = 0;
-                    (len < elm->rxfill) && (elm->rxbuf[len] != '\r');
-                    len++) {
-                       /* empty loop */
-               }
+               len = first_new_char_idx;
+               while (len < elm->rxfill && elm->rxbuf[len] != '\r')
+                       len++;
 
                if (len == ELM327_SIZE_RXBUF) {
-                       /* Line exceeds buffer. It's probably all garbage.
+                       /* Assume the buffer ran full with garbage.
                         * Did we even connect at the right baud rate?
                         */
                        netdev_err(elm->dev,
                                   "RX buffer overflow. Faulty ELM327 or UART?\n");
                        elm327_uart_side_failure(elm);
-                       break;
                } else if (len == elm->rxfill) {
                        if (elm327_is_ready_char(elm->rxbuf[elm->rxfill - 1])) {
                                /* The ELM327's AT ST response timeout ran out,
@@ -748,29 +740,37 @@ static void elm327_parse_rxbuf(struct can327 *elm)
                                elm->rxfill = 0;
 
                                elm327_handle_prompt(elm);
-                               break;
                        }
 
                        /* No <CR> found - we haven't received a full line yet.
                         * Wait for more data.
                         */
-                       break;
-               }
-
-               /* We have a full line to parse. */
-               elm327_parse_line(elm, len);
+               } else {
+                       /* We have a full line to parse. */
+                       elm327_parse_line(elm, len);
 
-               /* Remove parsed data from RX buffer. */
-               elm327_drop_bytes(elm, len + 1);
+                       /* Remove parsed data from RX buffer. */
+                       elm327_drop_bytes(elm, len + 1);
 
-               /* More data to parse? */
-               if (elm->rxfill)
-                       elm327_parse_rxbuf(elm);
+                       /* More data to parse? */
+                       if (elm->rxfill)
+                               elm327_parse_rxbuf(elm, 0);
+               }
        }
 }
 
 #if LINUX_VERSION_CODE < KERNEL_VERSION(5,10,0)
 /* Dummy needed to use can_rx_offload */
+#if LINUX_VERSION_CODE < KERNEL_VERSION(5,5,0)
+static unsigned int *can327_mailbox_read(struct can_rx_offload *offload,
+                                        struct can_frame *cf,
+                                        u32 *timestamp, unsigned int n)
+{
+       WARN_ON_ONCE(1); /* This function is a dummy, so don't call it! */
+
+       return -ENOBUFS;
+}
+#else /* Since 4e9c9484b085 (included in v5.5) */
 static struct sk_buff *can327_mailbox_read(struct can_rx_offload *offload,
                                           unsigned int n, u32 *timestamp,
                                           bool drop)
@@ -780,6 +780,7 @@ static struct sk_buff *can327_mailbox_read(struct can_rx_offload *offload,
        return ERR_PTR(-ENOBUFS);
 }
 #endif
+#endif
 
 static int can327_netdev_open(struct net_device *dev)
 {
@@ -823,7 +824,6 @@ static int can327_netdev_open(struct net_device *dev)
 
        can_rx_offload_enable(&elm->offload);
 
-       can_led_event(dev, CAN_LED_EVENT_OPEN);
        elm->can.state = CAN_STATE_ERROR_ACTIVE;
        netif_start_queue(dev);
 
@@ -849,7 +849,6 @@ static int can327_netdev_close(struct net_device *dev)
        elm->can.state = CAN_STATE_STOPPED;
        can_rx_offload_del(&elm->offload);
        close_candev(dev);
-       can_led_event(dev, CAN_LED_EVENT_STOP);
 
        return 0;
 }
@@ -864,33 +863,32 @@ static netdev_tx_t can327_netdev_start_xmit(struct sk_buff *skb,
        if (can_dropped_invalid_skb(dev, skb))
                return NETDEV_TX_OK;
 
-       /* BHs are already disabled, so no spin_lock_bh().
-        * See Documentation/networking/netdevices.txt
+       /* This check will be part of can_dropped_invalid_skb()
+        * in future kernels.
         */
-       spin_lock(&elm->lock);
+       if (elm->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
+               goto out;
 
        /* We shouldn't get here after a hardware fault:
         * can_bus_off() calls netif_carrier_off()
         */
-       WARN_ON_ONCE(elm->uart_side_failure);
-
-       if (!elm->tty ||
-           elm->uart_side_failure ||
-           elm->can.ctrlmode & CAN_CTRLMODE_LISTENONLY) {
-               spin_unlock(&elm->lock);
+       if (elm->uart_side_failure) {
+               WARN_ON_ONCE(elm->uart_side_failure);
                goto out;
        }
 
        netif_stop_queue(dev);
 
+       /* BHs are already disabled, so no spin_lock_bh().
+        * See Documentation/networking/netdevices.txt
+        */
+       spin_lock(&elm->lock);
        elm327_send_frame(elm, frame);
        spin_unlock(&elm->lock);
 
        dev->stats.tx_packets++;
        dev->stats.tx_bytes += frame->len;
 
-       can_led_event(dev, CAN_LED_EVENT_TX);
-
 out:
        kfree_skb(skb);
        return NETDEV_TX_OK;
@@ -903,20 +901,30 @@ static const struct net_device_ops can327_netdev_ops = {
        .ndo_change_mtu = can_change_mtu,
 };
 
-static bool can327_is_valid_rx_char(char c)
+static bool can327_is_valid_rx_char(u8 c)
 {
-       return (isdigit(c) ||
-               isupper(c) ||
-               c == ELM327_DUMMY_CHAR ||
-               c == ELM327_READY_CHAR ||
-               c == '<' ||
-               c == 'a' ||
-               c == 'b' ||
-               c == 'v' ||
-               c == '.' ||
-               c == '?' ||
-               c == '\r' ||
-               c == ' ');
+       static const bool lut_char_is_valid['z'] = {
+               ['\r'] = true,
+               [' '] = true,
+               ['.'] = true,
+               ['0'] = true, true, true, true, true,
+               ['5'] = true, true, true, true, true,
+               ['<'] = true,
+               [ELM327_READY_CHAR] = true,
+               ['?'] = true,
+               ['A'] = true, true, true, true, true, true, true,
+               ['H'] = true, true, true, true, true, true, true,
+               ['O'] = true, true, true, true, true, true, true,
+               ['V'] = true, true, true, true, true,
+               ['a'] = true,
+               ['b'] = true,
+               ['v'] = true,
+               [ELM327_DUMMY_CHAR] = true,
+       };
+       BUILD_BUG_ON(ELM327_DUMMY_CHAR >= 'z');
+
+       return (c < ARRAY_SIZE(lut_char_is_valid) &&
+               lut_char_is_valid[c]);
 }
 
 /* Handle incoming ELM327 ASCII data.
@@ -932,11 +940,17 @@ static void can327_ldisc_rx(struct tty_struct *tty,
 #endif
 {
        struct can327 *elm = (struct can327 *)tty->disc_data;
+       size_t first_new_char_idx;
+
+       if (elm->uart_side_failure)
+               return;
 
        spin_lock_bh(&elm->lock);
 
-       if (elm->uart_side_failure)
-               goto out;
+       /* Store old rxfill, so elm327_parse_rxbuf() will have
+        * the option of skipping already checked characters.
+        */
+       first_new_char_idx = elm->rxfill;
 
        while (count-- && elm->rxfill < ELM327_SIZE_RXBUF) {
                if (fp && *fp++) {
@@ -944,7 +958,8 @@ static void can327_ldisc_rx(struct tty_struct *tty,
 
                        elm327_uart_side_failure(elm);
 
-                       goto out;
+                       spin_unlock_bh(&elm->lock);
+                       return;
                }
 
                /* Ignore NUL characters, which the PIC microcontroller may
@@ -952,7 +967,7 @@ static void can327_ldisc_rx(struct tty_struct *tty,
                 * See ELM327 documentation, which refers to a Microchip PIC
                 * bug description.
                 */
-               if (*cp != 0) {
+               if (*cp) {
                        /* Check for stray characters on the UART line.
                         * Likely caused by bad hardware.
                         */
@@ -962,7 +977,8 @@ static void can327_ldisc_rx(struct tty_struct *tty,
                                           *cp);
                                elm327_uart_side_failure(elm);
 
-                               goto out;
+                               spin_unlock_bh(&elm->lock);
+                               return;
                        }
 
                        elm->rxbuf[elm->rxfill++] = *cp;
@@ -972,16 +988,15 @@ static void can327_ldisc_rx(struct tty_struct *tty,
        }
 
        if (count >= 0) {
-               netdev_err(elm->dev, "Receive buffer overflowed. Bad chip or wiring?");
+               netdev_err(elm->dev, "Receive buffer overflowed. Bad chip or wiring? count = %i", count);
 
                elm327_uart_side_failure(elm);
 
-               goto out;
+               spin_unlock_bh(&elm->lock);
+               return;
        }
 
-       elm327_parse_rxbuf(elm);
-
-out:
+       elm327_parse_rxbuf(elm, first_new_char_idx);
        spin_unlock_bh(&elm->lock);
 }
 
@@ -1005,6 +1020,7 @@ static void can327_ldisc_tx_worker(struct work_struct *work)
                                   "Failed to write to tty %s.\n",
                                   elm->tty->name);
                        elm327_uart_side_failure(elm);
+
                        spin_unlock_bh(&elm->lock);
                        return;
                }
@@ -1013,12 +1029,10 @@ static void can327_ldisc_tx_worker(struct work_struct *work)
                elm->txhead += written;
        }
 
-       if (!elm->txleft)  {
+       if (!elm->txleft)
                clear_bit(TTY_DO_WRITE_WAKEUP, &elm->tty->flags);
-               spin_unlock_bh(&elm->lock);
-       } else {
-               spin_unlock_bh(&elm->lock);
-       }
+
+       spin_unlock_bh(&elm->lock);
 }
 
 /* Called by the driver when there's room for more data. */
@@ -1033,7 +1047,7 @@ static void can327_ldisc_tx_wakeup(struct tty_struct *tty)
  * or 7/8 of that. Divisors are 1 to 64.
  * Currently we don't implement support for 7/8 rates.
  */
-static const u32 can327_bitrate_const[64] = {
+static const u32 can327_bitrate_const[] = {
         7812,  7936,  8064,  8196,  8333,  8474,  8620,  8771,
         8928,  9090,  9259,  9433,  9615,  9803, 10000, 10204,
        10416, 10638, 10869, 11111, 11363, 11627, 11904, 12195,
@@ -1086,20 +1100,16 @@ static int can327_ldisc_open(struct tty_struct *tty)
        elm->tty = tty;
        tty->disc_data = elm;
 
-       devm_can_led_init(elm->dev);
-
        /* Let 'er rip */
        err = register_candev(elm->dev);
-       if (err)
-               goto out_err;
+       if (err) {
+               free_candev(elm->dev);
+               return err;
+       }
 
        netdev_info(elm->dev, "can327 on %s.\n", tty->name);
 
        return 0;
-
-out_err:
-       free_candev(elm->dev);
-       return err;
 }
 
 /* Close down a can327 channel.