Restore WAKEUP set_bit vs. write() order in elm327_send()
[elmcan.git] / module / elmcan.c
index 9c19d76e141606ad8e52c4e56843d973f2fd9e05..e2d616f2b5c631666fb44363d4bbde528a339fd9 100644 (file)
@@ -2,8 +2,7 @@
 /* ELM327 based CAN interface driver (tty line discipline)
  *
  * This driver started as a derivative of linux/drivers/net/can/slcan.c
- * and my thanks go to the original authors for their inspiration, even
- * after almost none of their code is left.
+ * and my thanks go to the original authors for their inspiration.
  *
  * elmcan.c Author : Max Staudt <max-linux@enpas.org>
  * slcan.c Author  : Oliver Hartkopp <socketcan@hartkopp.net>
@@ -44,8 +43,8 @@
 #include <linux/can/rx-offload.h>
 
 /* Line discipline ID number.
- * N_DEVELOPMENT will likely be defined from Linux 5.18 onwards:
- * https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git/commit/?h=tty-next&id=c2faf737abfb10f88f2d2612d573e9edc3c42c37
+ * Starting with Linux v5.18-rc1, N_DEVELOPMENT is defined as 29:
+ * https://github.com/torvalds/linux/commit/c2faf737abfb10f88f2d2612d573e9edc3c42c37
  */
 #ifndef N_DEVELOPMENT
 #define N_DEVELOPMENT 29
@@ -58,7 +57,7 @@
 
 #define ELM327_NAPI_WEIGHT 4
 
-#define ELM327_SIZE_RXBUF 256
+#define ELM327_SIZE_RXBUF 224
 #define ELM327_SIZE_TXBUF 32
 
 #define ELM327_CAN_CONFIG_SEND_SFF           0x8000
@@ -71,7 +70,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,
@@ -89,27 +88,22 @@ struct elmcan {
 
        struct can_rx_offload offload;
 
-       /* TTY and netdev devices that we're bridging */
-       struct tty_struct *tty;
-       struct net_device *dev;
+       /* TTY buffers */
+       u8 rxbuf[ELM327_SIZE_RXBUF];
+       u8 txbuf[ELM327_SIZE_TXBUF] ____cacheline_aligned;
 
        /* Per-channel lock */
        spinlock_t lock;
 
-       /* Stop the channel on hardware failure.
-        * Once this is true, nothing will be sent to the TTY.
-        */
-       bool hw_failure;
-
-       /* TTY TX helpers */
-       struct work_struct tx_work;     /* Flushes TTY TX buffer   */
-       u8 *txbuf;                      /* Pointer to our TX buffer */
-       u8 *txhead;                     /* Pointer to next TX byte */
-       unsigned txleft;                /* Bytes left to TX */
+       /* TTY and netdev devices that we're bridging */
+       struct tty_struct *tty;
+       struct net_device *dev;
 
-       /* TTY RX helpers */
-       u8 rxbuf[ELM327_SIZE_RXBUF];
-       int rxfill;
+       /* TTY buffer accounting */
+       struct work_struct tx_work;     /* Flushes TTY TX buffer */
+       u8 *txhead;                     /* Next TX byte */
+       size_t txleft;                  /* Bytes left to TX */
+       int rxfill;                     /* Bytes already RX'd in buffer */
 
        /* State machine */
        enum {
@@ -119,7 +113,9 @@ struct elmcan {
                ELM327_STATE_RECEIVING,
        } state;
 
-       bool drop_next_line;
+       /* Things we have yet to send */
+       char **next_init_cmd;
+       unsigned long cmds_todo;
 
        /* The CAN frame and config the ELM327 is sending/using,
         * or will send/use after finishing all cmds_todo
@@ -128,40 +124,50 @@ struct elmcan {
        u16 can_config;
        u8 can_bitrate_divisor;
 
-       /* Things we have yet to send */
-       char **next_init_cmd;
-       unsigned long cmds_todo;
+       /* Parser state */
+       bool drop_next_line;
+
+       /* 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 uart_side_failure;
 };
 
-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)
 {
        int written;
 
-       lockdep_assert_held(elm->lock);
+       lockdep_assert_held(&elm->lock);
 
-       if (elm->hw_failure)
+       if (elm->uart_side_failure)
                return;
 
        memcpy(elm->txbuf, buf, len);
 
+       /* Order of next two lines is *very* important.
+        * When we are sending a little amount of data,
+        * the transfer may be completed inside the ops->write()
+        * routine, because it's running with interrupts enabled.
+        * In this case we *never* got WRITE_WAKEUP event,
+        * if we did not request it before write operation.
+        *       14 Oct 1994  Dmitry Gorodchanin.
+        */
+       set_bit(TTY_DO_WRITE_WAKEUP, &elm->tty->flags);
        written = elm->tty->ops->write(elm->tty, elm->txbuf, len);
        if (written < 0) {
                netdev_err(elm->dev,
                           "Failed to write to tty %s.\n",
                           elm->tty->name);
-               elm327_hw_failure(elm);
+               elm327_uart_side_failure(elm);
                return;
        }
 
        elm->txleft = len - written;
        elm->txhead = elm->txbuf + written;
-
-       if (!elm->txleft)
-               netif_wake_queue(elm->dev);
-       else
-               set_bit(TTY_DO_WRITE_WAKEUP, &elm->tty->flags);
 }
 
 /* Take the ELM327 out of almost any state and back into command mode.
@@ -171,7 +177,7 @@ static void elm327_send(struct elmcan *elm, const void *buf, size_t len)
  */
 static void elm327_kick_into_cmd_mode(struct elmcan *elm)
 {
-       lockdep_assert_held(elm->lock);
+       lockdep_assert_held(&elm->lock);
 
        if (elm->state != ELM327_STATE_GETDUMMYCHAR &&
            elm->state != ELM327_STATE_GETPROMPT) {
@@ -184,7 +190,7 @@ static void elm327_kick_into_cmd_mode(struct elmcan *elm)
 /* Schedule a CAN frame and necessary config changes to be sent to the TTY. */
 static void elm327_send_frame(struct elmcan *elm, struct can_frame *frame)
 {
-       lockdep_assert_held(elm->lock);
+       lockdep_assert_held(&elm->lock);
 
        /* Schedule any necessary changes in ELM327's CAN configuration */
        if (elm->can_frame_to_send.can_id != frame->can_id) {
@@ -246,7 +252,7 @@ static char *elm327_init_script[] = {
 
 static void elm327_init(struct elmcan *elm)
 {
-       lockdep_assert_held(elm->lock);
+       lockdep_assert_held(&elm->lock);
 
        elm->state = ELM327_STATE_NOTINIT;
        elm->can_frame_to_send.can_id = 0x7df; /* ELM327 HW default */
@@ -276,7 +282,7 @@ static void elm327_init(struct elmcan *elm)
 static void elm327_feed_frame_to_netdev(struct elmcan *elm,
                                        struct sk_buff *skb)
 {
-       lockdep_assert_held(elm->lock);
+       lockdep_assert_held(&elm->lock);
 
        if (!netif_running(elm->dev))
                return;
@@ -294,14 +300,16 @@ 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);
+       lockdep_assert_held(&elm->lock);
 
-       elm->hw_failure = true;
+       elm->uart_side_failure = true;
+
+       clear_bit(TTY_DO_WRITE_WAKEUP, &elm->tty->flags);
 
        elm->can.can_stats.bus_off++;
        netif_stop_queue(elm->dev);
@@ -340,7 +348,7 @@ static void elm327_parse_error(struct elmcan *elm, size_t len)
        struct can_frame *frame;
        struct sk_buff *skb;
 
-       lockdep_assert_held(elm->lock);
+       lockdep_assert_held(&elm->lock);
 
        skb = alloc_can_err_skb(elm->dev, &frame);
        if (!skb)
@@ -407,7 +415,7 @@ static int elm327_parse_frame(struct elmcan *elm, size_t len)
        int datastart;
        int i;
 
-       lockdep_assert_held(elm->lock);
+       lockdep_assert_held(&elm->lock);
 
        skb = alloc_can_skb(elm->dev, &frame);
        if (!skb)
@@ -529,7 +537,7 @@ static int elm327_parse_frame(struct elmcan *elm, size_t len)
 
 static void elm327_parse_line(struct elmcan *elm, size_t len)
 {
-       lockdep_assert_held(elm->lock);
+       lockdep_assert_held(&elm->lock);
 
        /* Skip empty lines */
        if (!len)
@@ -544,8 +552,8 @@ static void elm327_parse_line(struct elmcan *elm, size_t len)
        }
 
        /* Regular parsing */
-       if (elm->state == ELM327_STATE_RECEIVING
-           && elm327_parse_frame(elm, len)) {
+       if (elm->state == ELM327_STATE_RECEIVING &&
+           elm327_parse_frame(elm, len)) {
                /* Parse an error line. */
                elm327_parse_error(elm, len);
 
@@ -563,21 +571,26 @@ static void elm327_handle_prompt(struct elmcan *elm)
         */
        char local_txbuf[sizeof("0102030405060708\r")];
 
-       lockdep_assert_held(elm->lock);
+       lockdep_assert_held(&elm->lock);
 
        if (!elm->cmds_todo) {
                /* Enter CAN monitor mode */
                elm327_send(elm, "ATMA\r", 5);
                elm->state = ELM327_STATE_RECEIVING;
 
+               /* We will be in the default state once this command is
+                * sent, so enable the TX packet queue.
+                */
+               netif_wake_queue(elm->dev);
+
                return;
        }
 
        /* Reconfigure ELM327 step by step as indicated by elm->cmds_todo */
        if (test_bit(ELM327_TX_DO_INIT, &elm->cmds_todo)) {
                snprintf(local_txbuf, sizeof(local_txbuf),
-                       "%s",
-                       *elm->next_init_cmd);
+                        "%s",
+                        *elm->next_init_cmd);
 
                elm->next_init_cmd++;
                if (!(*elm->next_init_cmd)) {
@@ -587,38 +600,38 @@ static void elm327_handle_prompt(struct elmcan *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)));
+                        "ATCSM%i\r",
+                        !(!(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),
-                       "ATR%i\r",
-                       !(elm->can.ctrlmode & CAN_CTRLMODE_LISTENONLY));
+                        "ATR%i\r",
+                        !(elm->can.ctrlmode & CAN_CTRLMODE_LISTENONLY));
 
        } else if (test_and_clear_bit(ELM327_TX_DO_CAN_CONFIG, &elm->cmds_todo)) {
                snprintf(local_txbuf, sizeof(local_txbuf),
-                       "ATPC\r");
+                        "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)) {
                snprintf(local_txbuf, sizeof(local_txbuf),
-                       "ATPB%04X\r",
-                       elm->can_config);
+                        "ATPB%04X\r",
+                        elm->can_config);
 
        } else if (test_and_clear_bit(ELM327_TX_DO_CANID_29BIT_HIGH, &elm->cmds_todo)) {
                snprintf(local_txbuf, sizeof(local_txbuf),
-                       "ATCP%02X\r",
-                       (frame->can_id & CAN_EFF_MASK) >> 24);
+                        "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)) {
                snprintf(local_txbuf, sizeof(local_txbuf),
-                       "ATSH%06X\r",
-                       frame->can_id & CAN_EFF_MASK & ((1 << 24) - 1));
+                        "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)) {
                snprintf(local_txbuf, sizeof(local_txbuf),
-                       "ATSH%03X\r",
-                       frame->can_id & CAN_SFF_MASK);
+                        "ATSH%03X\r",
+                        frame->can_id & CAN_SFF_MASK);
 
        } else if (test_and_clear_bit(ELM327_TX_DO_CAN_DATA, &elm->cmds_todo)) {
                if (frame->can_id & CAN_RTR_FLAG) {
@@ -626,23 +639,28 @@ static void elm327_handle_prompt(struct elmcan *elm)
                         * Some chips don't send them at all.
                         */
                        snprintf(local_txbuf, sizeof(local_txbuf),
-                               "ATRTR\r");
+                                "ATRTR\r");
                } else {
                        /* Send a regular CAN data frame */
                        int i;
 
                        for (i = 0; i < frame->len; i++) {
                                snprintf(&local_txbuf[2 * i], sizeof(local_txbuf),
-                                       "%02X",
-                                       frame->data[i]);
+                                        "%02X",
+                                        frame->data[i]);
                        }
 
                        snprintf(&local_txbuf[2 * i], sizeof(local_txbuf),
-                               "\r");
+                                "\r");
                }
 
                elm->drop_next_line = 1;
                elm->state = ELM327_STATE_RECEIVING;
+
+               /* We will be in the default state once this command is
+                * sent, so enable the TX packet queue.
+                */
+               netif_wake_queue(elm->dev);
        }
 
        elm327_send(elm, local_txbuf, strlen(local_txbuf));
@@ -658,7 +676,7 @@ static bool elm327_is_ready_char(char c)
 
 static void elm327_drop_bytes(struct elmcan *elm, size_t i)
 {
-       lockdep_assert_held(elm->lock);
+       lockdep_assert_held(&elm->lock);
 
        memmove(&elm->rxbuf[0], &elm->rxbuf[i], ELM327_SIZE_RXBUF - i);
        elm->rxfill -= i;
@@ -669,7 +687,7 @@ static void elm327_parse_rxbuf(struct elmcan *elm)
        size_t len;
        int i;
 
-       lockdep_assert_held(elm->lock);
+       lockdep_assert_held(&elm->lock);
 
        switch (elm->state) {
        case ELM327_STATE_NOTINIT:
@@ -719,7 +737,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])) {
@@ -769,17 +787,19 @@ static int elmcan_netdev_open(struct net_device *dev)
        int err;
 
        spin_lock_bh(&elm->lock);
-       if (elm->hw_failure) {
-               netdev_err(elm->dev, "Refusing to open interface after a hardware fault has been detected.\n");
-               spin_unlock_bh(&elm->lock);
-               return -EIO;
-       }
 
        if (!elm->tty) {
                spin_unlock_bh(&elm->lock);
                return -ENODEV;
        }
 
+       if (elm->uart_side_failure)
+               netdev_warn(elm->dev, "Reopening netdev after a UART side fault has been detected.\n");
+
+       /* Clear TTY buffers */
+       elm->rxfill = 0;
+       elm->txleft = 0;
+
        /* open_candev() checks for elm->can.bittiming.bitrate != 0 */
        err = open_candev(dev);
        if (err) {
@@ -814,24 +834,16 @@ static int elmcan_netdev_close(struct net_device *dev)
 {
        struct elmcan *elm = netdev_priv(dev);
 
-       netif_stop_queue(dev);
-
+       /* Interrupt whatever the ELM327 is doing right now */
        spin_lock_bh(&elm->lock);
-       if (elm->tty) {
-               /* Interrupt whatever we're doing right now */
-               elm327_send(elm, ELM327_DUMMY_STRING, 1);
-
-               /* Clear the wakeup bit, as the netdev will be down and thus
-                * the wakeup handler won't clear it
-                */
-               clear_bit(TTY_DO_WRITE_WAKEUP, &elm->tty->flags);
+       elm327_send(elm, ELM327_DUMMY_STRING, 1);
+       spin_unlock_bh(&elm->lock);
 
-               spin_unlock_bh(&elm->lock);
+       netif_stop_queue(dev);
 
-               flush_work(&elm->tx_work);
-       } else {
-               spin_unlock_bh(&elm->lock);
-       }
+       /* Give UART one final chance to flush. */
+       clear_bit(TTY_DO_WRITE_WAKEUP, &elm->tty->flags);
+       flush_work(&elm->tx_work);
 
        can_rx_offload_disable(&elm->offload);
        elm->can.state = CAN_STATE_STOPPED;
@@ -860,10 +872,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;
@@ -923,14 +935,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;
                }
@@ -948,7 +960,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;
                        }
@@ -962,7 +974,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;
        }
@@ -981,7 +993,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);
@@ -992,19 +1004,18 @@ 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 {
-                       elm->txleft -= written;
-                       elm->txhead += written;
                }
+
+               elm->txleft -= written;
+               elm->txhead += written;
        }
 
        if (!elm->txleft)  {
                clear_bit(TTY_DO_WRITE_WAKEUP, &elm->tty->flags);
                spin_unlock_bh(&elm->lock);
-               netif_wake_queue(elm->dev);
        } else {
                spin_unlock_bh(&elm->lock);
        }
@@ -1056,15 +1067,8 @@ static int elmcan_ldisc_open(struct tty_struct *tty)
                return -ENFILE;
        elm = netdev_priv(dev);
 
-       elm->txbuf = kmalloc(ELM327_SIZE_TXBUF, GFP_KERNEL);
-       if (!elm->txbuf) {
-               err = -ENOMEM;
-               goto out_err;
-       }
-
        /* Configure TTY interface */
        tty->receive_room = 65536; /* We don't flow control */
-       elm->txleft = 0; /* Clear TTY TX buffer */
        spin_lock_init(&elm->lock);
        INIT_WORK(&elm->tx_work, elmcan_ldisc_tx_worker);
 
@@ -1094,7 +1098,6 @@ static int elmcan_ldisc_open(struct tty_struct *tty)
        return 0;
 
 out_err:
-       kfree(elm->txbuf);
        free_candev(elm->dev);
        return err;
 }
@@ -1110,13 +1113,12 @@ static void elmcan_ldisc_close(struct tty_struct *tty)
 {
        struct elmcan *elm = (struct elmcan *)tty->disc_data;
 
-       /* unregister_netdev() calls .ndo_stop() so we don't have to. */
+       /* unregister_netdev() calls .ndo_stop() so we don't have to.
+        * Our .ndo_stop() also flushes the TTY write wakeup handler,
+        * so we can safely set elm->tty = NULL after this.
+        */
        unregister_candev(elm->dev);
 
-       /* Ensure that our worker won't be rescheduled */
-       clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
-       flush_work(&elm->tx_work);
-
        /* Mark channel as dead */
        spin_lock_bh(&elm->lock);
        tty->disc_data = NULL;
@@ -1125,7 +1127,6 @@ static void elmcan_ldisc_close(struct tty_struct *tty)
 
        netdev_info(elm->dev, "elmcan off %s.\n", tty->name);
 
-       kfree(elm->txbuf);
        free_candev(elm->dev);
 }