linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v6] can, tty: can327 CAN/ldisc driver for ELM327 based OBD-II adapters
@ 2022-05-12 18:29 Max Staudt
  2022-05-13  2:38 ` Vincent Mailhol
  2022-05-18 16:24 ` Vincent Mailhol
  0 siblings, 2 replies; 14+ messages in thread
From: Max Staudt @ 2022-05-12 18:29 UTC (permalink / raw)
  To: Wolfgang Grandegger, Marc Kleine-Budde
  Cc: linux-can, Greg Kroah-Hartman, Vincent Mailhol, Oliver Neukum,
	linux-kernel, Max Staudt

This is the can327 driver. It does a surprisingly good job at turning
ELM327 based OBD-II interfaces into cheap CAN interfaces for simple
homebrew projects.

Please see the included documentation for details and limitations:
Documentation/networking/device_drivers/can/can327.rst

Cc: linux-can <linux-can@vger.kernel.org>
Signed-off-by: Max Staudt <max@enpas.org>
---
Many thanks to Greg, Marc, Vincent for reviewing v3!

I've CC'd you hoping you'd check whether you are happy with the changes.
Please do let me know.


Changes in v6:
 - Renamed 'elmcan' to 'can327'
 - No other changes.

Changes in v5:
 - Fixed lockdep asserts (I didn't have lockdep enabled in my build)
 - Restore: TTY wakeup request before write() in elm327_send()
 - Clear TTY wakeup on uart_side_failure
 - checkpatch style fixes
 - Repack struct elmcan

Changes in v4:
 - Rebased on top of v5.18-rc5
 - Simplified TTY locking.
     The ldisc layer already blocks our ldisc ops from running concurrently.
     No elm_get(), no RCU are needed, except locking against the TX worker.
 - Removed .hangup() ldisc op which only called .close().
     The ldisc layer calls .close() in a good place anyway.
 - Restart on netdev down/up, even after UART failure.
     This is in line with Marc's line of thinking, and there was no
     good reason not to implement it. It also cleaned up the code.
 - hw_failure is now uart_side_failure and shutdown looks closer to es58x.
 - Clarified memory/string comparisons.
     One helper function remains, but it's hopefully clear now :)
 - lockdep_assert_held() instead of comments.
 - Cleaned up types in struct elmcan and used pahole to pack it.
     unsigned instead of int, u8 instead of char, ...
 - The TX buffer is now a static part of struct elmcan. No more kmalloc().
 - Removed dummy mailbox_read() for rx_offload.
     by using can_rx_offload_add_manual() instead of can_rx_offload_add_fifo().
 - netif_wake_queue() is moved to happen as late as possible.
     Wake when the ELM327 is going into monitoring mode, not every time
     after flushing whatever UART buffer we were TX'ing.
 - Use alloc_can_skb() and avoid late allocation and copy.
 - elmcan_netdev_start_xmit(): Used can_dropped_invalid_skb().
 - Replaced ->can_dlc with ->len.
 - Clarified size of local TX buffer in elm327_handle_prompt().
 - Renamed TODO_* to ELM327_TX_DO_* and ELM_* to ELM327_STATE_*.
 - Minor string and other cleanups.
     pr_fmt, MODULE_*, unnecessary checks, ...
 - Clarified file header.

Changes in v3:
 - Now depends on c2faf737abfb for new ldisc number 30.
 - Eliminated hardcoded string lengths (GCC will work its magic).
 - Emit generic error frames if an error message couldn't be parsed.
 - Silence driver startup and init (but still announce ldattach).
 - Cleaned up comments, strings, readme.
 - Removed sole module option "accept_flaky_uart".
   I likely had EMI in earlier testing, which is gone now.
   This means we can stay strict, unless anyone objects.

Changes in v2:
 - Moved to SocketCAN's rx-offload wrapper for NAPI, thus avoiding
   packets being reordered.
 - Updated TTY ldisc code for Linux v5.17-rc3. A lot of cleanup has
   happened there lately.
 - netif_stop_queue() is called earlier in _netdev_close().
 - Minor cleanup: More helpful strings and return values.

---
 .../networking/device_drivers/can/can327.rst  |  325 +++++
 .../networking/device_drivers/can/index.rst   |    1 +
 MAINTAINERS                                   |    7 +
 drivers/net/can/Kconfig                       |   17 +
 drivers/net/can/Makefile                      |    1 +
 drivers/net/can/can327.c                      | 1152 +++++++++++++++++
 include/uapi/linux/tty.h                      |    3 +-
 7 files changed, 1505 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/networking/device_drivers/can/can327.rst
 create mode 100644 drivers/net/can/can327.c

diff --git a/Documentation/networking/device_drivers/can/can327.rst b/Documentation/networking/device_drivers/can/can327.rst
new file mode 100644
index 000000000000..48508963dd1b
--- /dev/null
+++ b/Documentation/networking/device_drivers/can/can327.rst
@@ -0,0 +1,325 @@
+.. SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)
+
+can327: ELM327 driver for Linux SocketCAN
+==========================================
+
+Authors
+--------
+
+Max Staudt <max@enpas.org>
+
+
+
+Motivation
+-----------
+
+This driver aims to lower the initial cost for hackers interested in
+working with CAN buses.
+
+CAN adapters are expensive, few, and far between.
+ELM327 interfaces are cheap and plentiful.
+Let's use ELM327s as CAN adapters.
+
+
+
+Introduction
+-------------
+
+This driver is an effort to turn abundant ELM327 based OBD interfaces
+into full fledged (as far as possible) CAN interfaces.
+
+Since the ELM327 was never meant to be a stand alone CAN controller,
+the driver has to switch between its modes as quickly as possible in
+order to fake full-duplex operation.
+
+As such, can327 is a best effort driver. However, this is more than
+enough to implement simple request-response protocols (such as OBD II),
+and to monitor broadcast messages on a bus (such as in a vehicle).
+
+Most ELM327s come as nondescript serial devices, attached via USB or
+Bluetooth. The driver cannot recognize them by itself, and as such it
+is up to the user to attach it in form of a TTY line discipline
+(similar to PPP, SLIP, slcan, ...).
+
+This driver is meant for ELM327 versions 1.4b and up, see below for
+known limitations in older controllers and clones.
+
+
+
+Data sheet
+-----------
+
+The official data sheets can be found at ELM electronics' home page:
+
+  https://www.elmelectronics.com/
+
+
+
+How to check the controller version
+------------------------------------
+
+Use a terminal program to attach to the controller.
+The default settings are 38400 baud/s, 8 data bits, no parity, 1 stopbit.
+
+After issuing the "``AT WS``" command, the controller will respond with
+its version::
+
+    >AT WS
+
+
+    ELM327 v1.4b
+
+    >
+
+Note that clones may claim to be any version they like.
+It is not indicative of their actual feature set.
+
+
+
+How to attach the line discipline
+----------------------------------
+
+Every ELM327 chip is factory programmed to operate at a serial setting
+of 38400 baud/s, 8 data bits, no parity, 1 stopbit.
+
+The line discipline can be attached on a command prompt as follows::
+
+    sudo ldattach \
+           --debug \
+           --speed 38400 \
+           --eightbits \
+           --noparity \
+           --onestopbit \
+           --iflag -ICRNL,INLCR,-IXOFF \
+           30 \
+           /dev/ttyUSB0
+
+To change the ELM327's serial settings, please refer to its data
+sheet. This needs to be done before attaching the line discipline.
+
+Once the ldisc is attached, the CAN interface starts out unconfigured.
+Set the speed before starting it:
+
+    # The interface needs to be down to change parameters
+    sudo ip link set can0 down
+    sudo ip link set can0 type can bitrate 500000
+    sudo ip link set can0 up
+
+500000 bit/s is a common rate for OBD-II diagnostics.
+If you're connecting straight to a car's OBD port, this is the speed
+that most cars (but not all!) expect.
+
+After this, you can set out as usual with candump, cansniffer, etc.
+
+
+
+Known limitations of the controller
+------------------------------------
+
+- Clone devices ("v1.5" and others)
+
+  Sending RTR frames is not supported and will be dropped silently.
+
+  Receiving RTR with DLC 8 will appear to be a regular frame with
+  the last received frame's DLC and payload.
+
+  "``AT CSM``" not supported, thus no ACK-ing frames while listening:
+  "``AT MA``" will always be silent. However, immediately after
+  sending a frame, the ELM327 will be in "receive reply" mode, in
+  which it *does* ACK any received frames. Once the bus goes silent
+  or an error occurs (such as BUFFER FULL), the ELM327 will end reply
+  reception mode on its own and can327 will fall back to "``AT MA``"
+  in order to keep monitoring the bus.
+
+
+- All versions
+
+  No full duplex operation is supported. The driver will switch
+  between input/output mode as quickly as possible.
+
+  The length of outgoing RTR frames cannot be set. In fact, some
+  clones (tested with one identifying as "``v1.5``") are unable to
+  send RTR frames at all.
+
+  We don't have a way to get real-time notifications on CAN errors.
+  While there is a command (``AT CS``) to retrieve some basic stats,
+  we don't poll it as it would force us to interrupt reception mode.
+
+
+- Versions prior to 1.4b
+
+  These versions do not send CAN ACKs when in monitoring mode (AT MA).
+  However, they do send ACKs while waiting for a reply immediately
+  after sending a frame. The driver maximizes this time to make the
+  controller as useful as possible.
+
+  Starting with version 1.4b, the ELM327 supports the "``AT CSM``"
+  command, and the "listen-only" CAN option will take effect.
+
+
+- Versions prior to 1.4
+
+  These chips do not support the "``AT PB``" command, and thus cannot
+  change bitrate or SFF/EFF mode on-the-fly. This will have to be
+  programmed by the user before attaching the line discipline. See the
+  data sheet for details.
+
+
+- Versions prior to 1.3
+
+  These chips cannot be used at all with can327. They do not support
+  the "``AT D1``" command, which is necessary to avoid parsing conflicts
+  on incoming data, as well as distinction of RTR frame lengths.
+
+  Specifically, this allows for easy distinction of SFF and EFF
+  frames, and to check whether frames are complete. While it is possible
+  to deduce the type and length from the length of the line the ELM327
+  sends us, this method fails when the ELM327's UART output buffer
+  overruns. It may abort sending in the middle of the line, which will
+  then be mistaken for something else.
+
+
+
+Known limitations of the driver
+--------------------------------
+
+- No 8/7 timing.
+
+  ELM327 can only set CAN bitrates that are of the form 500000/n, where
+  n is an integer divisor.
+  However there is an exception: With a separate flag, it may set the
+  speed to be 8/7 of the speed indicated by the divisor.
+  This mode is not currently implemented.
+
+- No evaluation of command responses.
+
+  The ELM327 will reply with OK when a command is understood, and with ?
+  when it is not. The driver does not currently check this, and simply
+  assumes that the chip understands every command.
+  The driver is built such that functionality degrades gracefully
+  nevertheless. See the section on known limitations of the controller.
+
+- No use of hardware CAN ID filtering
+
+  An ELM327's UART sending buffer will easily overflow on heavy CAN bus
+  load, resulting in the "``BUFFER FULL``" message. Using the hardware
+  filters available through "``AT CF xxx``" and "``AT CM xxx``" would be
+  helpful here, however SocketCAN does not currently provide a facility
+  to make use of such hardware features.
+
+
+
+Communication example
+----------------------
+
+This is a short and incomplete introduction on how to talk to an ELM327.
+
+
+The ELM327 has two modes:
+
+- Command mode
+- Reception mode
+
+In command mode, it expects one command per line, terminated by CR.
+By default, the prompt is a "``>``", after which a command can be
+entered::
+
+    >ATE1
+    OK
+    >
+
+The init script in the driver switches off several configuration options
+that are only meaningful in the original OBD scenario the chip is meant
+for, and are actually a hindrance for can327.
+
+
+When a command is not recognized, such as by an older version of the
+ELM327, a question mark is printed as a response instead of OK::
+
+    >ATUNKNOWN
+    ?
+    >
+
+At present, can327 does not evaluate this response and silently assumes
+that all commands are recognized. It is structured such that it will
+degrade gracefully when a command is unknown. See the sections above on
+known limitations for details.
+
+
+When a CAN frame is to be sent, the target address is configured, after
+which the frame is sent as a command that consists of the data's hex
+dump::
+
+    >ATSH123
+    OK
+    >DEADBEEF12345678
+    OK
+    >
+
+The above interaction sends the frame "``DE AD BE EF 12 34 56 78``" with
+the 11 bit CAN ID ``0x123``.
+For this to function, the controller must be configured for 11 bit CAN
+ID sending mode (using "``AT PB``", see code or datasheet).
+
+
+Once a frame has been sent and wait-for-reply mode is on (``ATR1``,
+configured on ``listen-only=off``), or when the reply timeout expires and
+the driver sets the controller into monitoring mode (``ATMA``), the ELM327
+will send one line for each received CAN frame, consisting of CAN ID,
+DLC, and data::
+
+    123 8 DEADBEEF12345678
+
+For 29 bit CAN frames, the address format is slightly different, which
+can327 uses to tell the two apart::
+
+    12 34 56 78 8 DEADBEEF12345678
+
+The ELM327 will receive both 11 and 29 bit frames - the current CAN
+config (``ATPB``) does not matter.
+
+
+If the ELM327's internal UART sending buffer runs full, it will abort
+the monitoring mode, print "BUFFER FULL" and drop back into command
+mode. Note that in this case, unlike with other error messages, the
+error message may appear on the same line as the last (usually
+incomplete) data frame::
+
+    12 34 56 78 8 DEADBEEF123 BUFFER FULL
+
+
+
+Rationale behind the chosen configuration
+------------------------------------------
+
+``AT E1``
+  Echo on
+
+  We need this to be able to get a prompt reliably.
+
+``AT S1``
+  Spaces on
+
+  We need this to distinguish 11/29 bit CAN addresses received.
+
+  Note:
+  We can usually do this using the line length (odd/even),
+  but this fails if the line is not transmitted fully to
+  the host (BUFFER FULL).
+
+``AT D1``
+  DLC on
+
+  We need this to tell the "length" of RTR frames.
+
+
+
+A note on CAN bus termination
+------------------------------
+
+Your adapter may have resistors soldered in which are meant to terminate
+the bus. This is correct when it is plugged into a OBD-II socket, but
+not helpful when trying to tap into the middle of an existing CAN bus.
+
+If communications don't work with the adapter connected, check for the
+termination resistors on its PCB and try removing them.
diff --git a/Documentation/networking/device_drivers/can/index.rst b/Documentation/networking/device_drivers/can/index.rst
index 58b6e0ad3030..c4f724db4908 100644
--- a/Documentation/networking/device_drivers/can/index.rst
+++ b/Documentation/networking/device_drivers/can/index.rst
@@ -10,6 +10,7 @@ Contents:
 .. toctree::
    :maxdepth: 2
 
+   can327
    freescale/flexcan
 
 .. only::  subproject and html
diff --git a/MAINTAINERS b/MAINTAINERS
index edc96cdb85e8..ab7f0906f07b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7218,6 +7218,13 @@ L:	netdev@vger.kernel.org
 S:	Maintained
 F:	drivers/net/ethernet/ibm/ehea/
 
+ELM327 CAN NETWORK DRIVER
+M:	Max Staudt <max@enpas.org>
+L:	linux-can@vger.kernel.org
+S:	Maintained
+F:	Documentation/networking/device_drivers/can/can327.rst
+F:	drivers/net/can/can327.c
+
 EM28XX VIDEO4LINUX DRIVER
 M:	Mauro Carvalho Chehab <mchehab@kernel.org>
 L:	linux-media@vger.kernel.org
diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
index fff259247d52..7900f9852e03 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig
@@ -180,6 +180,23 @@ source "drivers/net/can/softing/Kconfig"
 source "drivers/net/can/spi/Kconfig"
 source "drivers/net/can/usb/Kconfig"
 
+config CAN_CAN327
+	tristate "Serial / USB serial ELM327 based OBD-II Interfaces (can327)"
+	depends on TTY
+	help
+	  CAN driver for several 'low cost' OBD-II interfaces based on the
+	  ELM327 OBD-II interpreter chip.
+
+	  This is a best effort driver - the ELM327 interface was never
+	  designed to be used as a standalone CAN interface. However, it can
+	  still be used for simple request-response protocols (such as OBD II),
+	  and to monitor broadcast messages on a bus (such as in a vehicle).
+
+	  Please refer to the documentation for information on how to use it:
+	  Documentation/networking/device_drivers/can/can327.rst
+
+	  If this driver is built as a module, it will be called can327.
+
 endif
 
 config CAN_DEBUG_DEVICES
diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
index 1e660afcb61b..ff53bc274ae1 100644
--- a/drivers/net/can/Makefile
+++ b/drivers/net/can/Makefile
@@ -6,6 +6,7 @@
 obj-$(CONFIG_CAN_VCAN)		+= vcan.o
 obj-$(CONFIG_CAN_VXCAN)		+= vxcan.o
 obj-$(CONFIG_CAN_SLCAN)		+= slcan.o
+obj-$(CONFIG_CAN_CAN327)	+= can327.o
 
 obj-y				+= dev/
 obj-y				+= rcar/
diff --git a/drivers/net/can/can327.c b/drivers/net/can/can327.c
new file mode 100644
index 000000000000..bbed01f90161
--- /dev/null
+++ b/drivers/net/can/can327.c
@@ -0,0 +1,1152 @@
+// SPDX-License-Identifier: GPL-2.0
+/* 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.
+ *
+ * can327.c Author : Max Staudt <max-linux@enpas.org>
+ * slcan.c Author  : Oliver Hartkopp <socketcan@hartkopp.net>
+ * slip.c Authors  : Laurence Culhane <loz@holmes.demon.co.uk>
+ *                   Fred N. van Kempen <waltje@uwalt.nl.mugnet.org>
+ */
+
+#define pr_fmt(fmt) "can327: " fmt
+
+#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>
+#include <linux/netdevice.h>
+#include <linux/skbuff.h>
+#include <linux/spinlock.h>
+#include <linux/string.h>
+#include <linux/tty.h>
+#include <linux/tty_ldisc.h>
+#include <linux/workqueue.h>
+
+#include <uapi/linux/tty.h>
+
+#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>
+
+#define ELM327_NAPI_WEIGHT 4
+
+#define ELM327_SIZE_RXBUF 224
+#define ELM327_SIZE_TXBUF 32
+
+#define ELM327_CAN_CONFIG_SEND_SFF           0x8000
+#define ELM327_CAN_CONFIG_VARIABLE_DLC       0x4000
+#define ELM327_CAN_CONFIG_RECV_BOTH_SFF_EFF  0x2000
+#define ELM327_CAN_CONFIG_BAUDRATE_MULT_8_7  0x1000
+
+#define ELM327_DUMMY_CHAR 'y'
+#define ELM327_DUMMY_STRING "y"
+#define ELM327_READY_CHAR '>'
+
+/* Bits in elm->cmds_todo */
+enum elm327_to_to_do_bits {
+	ELM327_TX_DO_CAN_DATA = 0,
+	ELM327_TX_DO_CANID_11BIT,
+	ELM327_TX_DO_CANID_29BIT_LOW,
+	ELM327_TX_DO_CANID_29BIT_HIGH,
+	ELM327_TX_DO_CAN_CONFIG_PART2,
+	ELM327_TX_DO_CAN_CONFIG,
+	ELM327_TX_DO_RESPONSES,
+	ELM327_TX_DO_SILENT_MONITOR,
+	ELM327_TX_DO_INIT
+};
+
+struct can327 {
+	/* This must be the first member when using alloc_candev() */
+	struct can_priv can;
+
+	struct can_rx_offload offload;
+
+	/* TTY buffers */
+	u8 rxbuf[ELM327_SIZE_RXBUF];
+	u8 txbuf[ELM327_SIZE_TXBUF] ____cacheline_aligned;
+
+	/* Per-channel lock */
+	spinlock_t lock;
+
+	/* TTY and netdev devices that we're bridging */
+	struct tty_struct *tty;
+	struct net_device *dev;
+
+	/* 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 {
+		ELM327_STATE_NOTINIT = 0,
+		ELM327_STATE_GETDUMMYCHAR,
+		ELM327_STATE_GETPROMPT,
+		ELM327_STATE_RECEIVING,
+	} state;
+
+	/* 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
+	 */
+	struct can_frame can_frame_to_send;
+	u16 can_config;
+	u8 can_bitrate_divisor;
+
+	/* 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_uart_side_failure(struct can327 *elm);
+
+static void elm327_send(struct can327 *elm, const void *buf, size_t len)
+{
+	int written;
+
+	lockdep_assert_held(&elm->lock);
+
+	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_uart_side_failure(elm);
+		return;
+	}
+
+	elm->txleft = len - written;
+	elm->txhead = elm->txbuf + written;
+}
+
+/* Take the ELM327 out of almost any state and back into command mode.
+ * We send ELM327_DUMMY_CHAR which will either abort any running
+ * operation, or be echoed back to us in case we're already in command
+ * mode.
+ */
+static void elm327_kick_into_cmd_mode(struct can327 *elm)
+{
+	lockdep_assert_held(&elm->lock);
+
+	if (elm->state != ELM327_STATE_GETDUMMYCHAR &&
+	    elm->state != ELM327_STATE_GETPROMPT) {
+		elm327_send(elm, ELM327_DUMMY_STRING, 1);
+
+		elm->state = ELM327_STATE_GETDUMMYCHAR;
+	}
+}
+
+/* Schedule a CAN frame and necessary config changes to be sent to the TTY. */
+static void elm327_send_frame(struct can327 *elm, struct can_frame *frame)
+{
+	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) {
+		/* Set the new CAN ID for transmission. */
+		if ((frame->can_id & CAN_EFF_FLAG) ^
+		    (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)
+					| ELM327_CAN_CONFIG_VARIABLE_DLC
+					| ELM327_CAN_CONFIG_RECV_BOTH_SFF_EFF
+					| elm->can_bitrate_divisor;
+
+			set_bit(ELM327_TX_DO_CAN_CONFIG, &elm->cmds_todo);
+		}
+
+		if (frame->can_id & CAN_EFF_FLAG) {
+			clear_bit(ELM327_TX_DO_CANID_11BIT, &elm->cmds_todo);
+			set_bit(ELM327_TX_DO_CANID_29BIT_LOW, &elm->cmds_todo);
+			set_bit(ELM327_TX_DO_CANID_29BIT_HIGH, &elm->cmds_todo);
+		} else {
+			set_bit(ELM327_TX_DO_CANID_11BIT, &elm->cmds_todo);
+			clear_bit(ELM327_TX_DO_CANID_29BIT_LOW, &elm->cmds_todo);
+			clear_bit(ELM327_TX_DO_CANID_29BIT_HIGH, &elm->cmds_todo);
+		}
+	}
+
+	/* Schedule the CAN frame itself. */
+	elm->can_frame_to_send = *frame;
+	set_bit(ELM327_TX_DO_CAN_DATA, &elm->cmds_todo);
+
+	elm327_kick_into_cmd_mode(elm);
+}
+
+/* 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 */
+	"AT M0\r",        /* v1.0: Memory Off */
+	"AT AL\r",        /* v1.0: Allow Long messages */
+	"AT BI\r",        /* v1.0: Bypass Initialisation */
+	"AT CAF0\r",      /* v1.0: CAN Auto Formatting Off */
+	"AT CFC0\r",      /* v1.0: CAN Flow Control Off */
+	"AT CF 000\r",    /* v1.0: Reset CAN ID Filter */
+	"AT CM 000\r",    /* v1.0: Reset CAN ID Mask */
+	"AT E1\r",        /* v1.0: Echo On */
+	"AT H1\r",        /* v1.0: Headers On */
+	"AT L0\r",        /* v1.0: Linefeeds Off */
+	"AT SH 7DF\r",    /* v1.0: Set CAN sending ID to 0x7df */
+	"AT ST FF\r",     /* v1.0: Set maximum Timeout for response after TX */
+	"AT AT0\r",       /* v1.2: Adaptive Timing Off */
+	"AT D1\r",        /* v1.3: Print DLC On */
+	"AT S1\r",        /* v1.3: Spaces On */
+	"AT TP B\r",      /* v1.0: Try Protocol B */
+	NULL
+};
+
+static void elm327_init(struct can327 *elm)
+{
+	lockdep_assert_held(&elm->lock);
+
+	elm->state = ELM327_STATE_NOTINIT;
+	elm->can_frame_to_send.can_id = 0x7df; /* ELM327 HW default */
+	elm->rxfill = 0;
+	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
+	 * limit the user to the right values.
+	 */
+	elm->can_bitrate_divisor = 500000 / elm->can.bittiming.bitrate;
+	elm->can_config = ELM327_CAN_CONFIG_SEND_SFF
+			| ELM327_CAN_CONFIG_VARIABLE_DLC
+			| ELM327_CAN_CONFIG_RECV_BOTH_SFF_EFF
+			| elm->can_bitrate_divisor;
+
+	/* Configure ELM327 and then start monitoring */
+	elm->next_init_cmd = &elm327_init_script[0];
+	set_bit(ELM327_TX_DO_INIT, &elm->cmds_todo);
+	set_bit(ELM327_TX_DO_SILENT_MONITOR, &elm->cmds_todo);
+	set_bit(ELM327_TX_DO_RESPONSES, &elm->cmds_todo);
+	set_bit(ELM327_TX_DO_CAN_CONFIG, &elm->cmds_todo);
+
+	elm327_kick_into_cmd_mode(elm);
+}
+
+static void elm327_feed_frame_to_netdev(struct can327 *elm,
+					struct sk_buff *skb)
+{
+	lockdep_assert_held(&elm->lock);
+
+	if (!netif_running(elm->dev))
+		return;
+
+	/* Queue for NAPI pickup.
+	 * rx-offload will update stats and LEDs for us.
+	 */
+	if (can_rx_offload_queue_tail(&elm->offload, skb))
+		elm->dev->stats.rx_fifo_errors++;
+
+	/* Wake NAPI */
+	can_rx_offload_irq_finish(&elm->offload);
+}
+
+/* Called when we're out of ideas and just want it all to end. */
+static inline void elm327_uart_side_failure(struct can327 *elm)
+{
+	struct can_frame *frame;
+	struct sk_buff *skb;
+
+	lockdep_assert_held(&elm->lock);
+
+	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);
+	elm->can.state = CAN_STATE_BUS_OFF;
+	can_bus_off(elm->dev);
+
+	netdev_err(elm->dev, "ELM327 misbehaved. Blocking further communication.\n");
+
+	skb = alloc_can_err_skb(elm->dev, &frame);
+	if (!skb)
+		return;
+
+	frame->can_id |= CAN_ERR_BUSOFF;
+	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.
+ *
+ * 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 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);
+}
+
+static void elm327_parse_error(struct can327 *elm, size_t len)
+{
+	struct can_frame *frame;
+	struct sk_buff *skb;
+
+	lockdep_assert_held(&elm->lock);
+
+	skb = alloc_can_err_skb(elm->dev, &frame);
+	if (!skb)
+		/* It's okay to return here:
+		 * The outer parsing loop will drop this UART buffer.
+		 */
+		return;
+
+	/* Filter possible error messages based on length of RX'd line */
+	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 (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 (check_len_then_cmp(elm->rxbuf, len, "BUS ERROR")) {
+		frame->can_id |= CAN_ERR_BUSERROR;
+	} else if (check_len_then_cmp(elm->rxbuf, len, "CAN ERROR")) {
+		frame->can_id |= CAN_ERR_PROT;
+	} else if (check_len_then_cmp(elm->rxbuf, len, "<RX ERROR")) {
+		frame->can_id |= CAN_ERR_PROT;
+	} 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 (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 && !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]);
+		frame->can_id |= CAN_ERR_CRTL;
+	} else {
+		/* Something else has happened.
+		 * Maybe garbage on the UART line.
+		 * Emit a generic error frame.
+		 */
+	}
+
+	elm327_feed_frame_to_netdev(elm, skb);
+}
+
+/* Parse CAN frames coming as ASCII from ELM327.
+ * They can be of various formats:
+ *
+ * 29-bit ID (EFF):  12 34 56 78 D PL PL PL PL PL PL PL PL
+ * 11-bit ID (!EFF): 123 D PL PL PL PL PL PL PL PL
+ *
+ * where D = DLC, PL = payload byte
+ *
+ * Instead of a payload, RTR indicates a remote request.
+ *
+ * We will use the spaces and line length to guess the format.
+ */
+static int elm327_parse_frame(struct can327 *elm, size_t len)
+{
+	struct can_frame *frame;
+	struct sk_buff *skb;
+	int hexlen;
+	int datastart;
+	int i;
+
+	lockdep_assert_held(&elm->lock);
+
+	skb = alloc_can_skb(elm->dev, &frame);
+	if (!skb)
+		return -ENOMEM;
+
+	/* Find first non-hex and non-space character:
+	 *  - In the simplest case, there is none.
+	 *  - For RTR frames, 'R' is the first non-hex character.
+	 *  - An error message may replace the end of the data line.
+	 */
+	for (hexlen = 0; hexlen <= len; hexlen++) {
+		if (hex_to_bin(elm->rxbuf[hexlen]) < 0 &&
+		    elm->rxbuf[hexlen] != ' ') {
+			break;
+		}
+	}
+
+	/* Sanity check whether the line is really a clean hexdump,
+	 * or terminated by an error message, or contains garbage.
+	 */
+	if (hexlen < len &&
+	    !isdigit(elm->rxbuf[hexlen]) &&
+	    !isupper(elm->rxbuf[hexlen]) &&
+	    '<' != elm->rxbuf[hexlen] &&
+	    ' ' != elm->rxbuf[hexlen]) {
+		/* The line is likely garbled anyway, so bail.
+		 * The main code will restart listening.
+		 */
+		return -ENODATA;
+	}
+
+	/* Use spaces in CAN ID to distinguish 29 or 11 bit address length.
+	 * No out-of-bounds access:
+	 * We use the fact that we can always read from elm->rxbuf.
+	 */
+	if (elm->rxbuf[2] == ' ' && elm->rxbuf[5] == ' ' &&
+	    elm->rxbuf[8] == ' ' && elm->rxbuf[11] == ' ' &&
+	    elm->rxbuf[13] == ' ') {
+		frame->can_id = CAN_EFF_FLAG;
+		datastart = 14;
+	} else if (elm->rxbuf[3] == ' ' && elm->rxbuf[5] == ' ') {
+		datastart = 6;
+	} else {
+		/* This is not a well-formatted data line.
+		 * Assume it's an error message.
+		 */
+		return -ENODATA;
+	}
+
+	if (hexlen < datastart) {
+		/* The line is too short to be a valid frame hex dump.
+		 * Something interrupted the hex dump or it is invalid.
+		 */
+		return -ENODATA;
+	}
+
+	/* From here on all chars up to buf[hexlen] are hex or spaces,
+	 * at well-defined offsets.
+	 */
+
+	/* Read CAN data length */
+	frame->len = (hex_to_bin(elm->rxbuf[datastart - 2]) << 0);
+
+	/* Read CAN ID */
+	if (frame->can_id & CAN_EFF_FLAG) {
+		frame->can_id |= (hex_to_bin(elm->rxbuf[0]) << 28)
+			       | (hex_to_bin(elm->rxbuf[1]) << 24)
+			       | (hex_to_bin(elm->rxbuf[3]) << 20)
+			       | (hex_to_bin(elm->rxbuf[4]) << 16)
+			       | (hex_to_bin(elm->rxbuf[6]) << 12)
+			       | (hex_to_bin(elm->rxbuf[7]) << 8)
+			       | (hex_to_bin(elm->rxbuf[9]) << 4)
+			       | (hex_to_bin(elm->rxbuf[10]) << 0);
+	} else {
+		frame->can_id |= (hex_to_bin(elm->rxbuf[0]) << 8)
+			       | (hex_to_bin(elm->rxbuf[1]) << 4)
+			       | (hex_to_bin(elm->rxbuf[2]) << 0);
+	}
+
+	/* Check for RTR frame */
+	if (elm->rxfill >= hexlen + 3 &&
+	    !memcmp(&elm->rxbuf[hexlen], "RTR", 3)) {
+		frame->can_id |= CAN_RTR_FLAG;
+	}
+
+	/* Is the line long enough to hold the advertised payload?
+	 * Note: RTR frames have a DLC, but no actual payload.
+	 */
+	if (!(frame->can_id & CAN_RTR_FLAG) &&
+	    (hexlen < frame->len * 3 + datastart)) {
+		/* Incomplete frame.
+		 * Probably the ELM327's RS232 TX buffer was full.
+		 * Emit an error frame and exit.
+		 */
+		frame->can_id = CAN_ERR_FLAG | CAN_ERR_CRTL;
+		frame->len = CAN_ERR_DLC;
+		frame->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
+		elm327_feed_frame_to_netdev(elm, skb);
+
+		/* Signal failure to parse.
+		 * The line will be re-parsed as an error line, which will fail.
+		 * However, this will correctly drop the state machine back into
+		 * command mode.
+		 */
+		return -ENODATA;
+	}
+
+	/* Parse the data nibbles. */
+	for (i = 0; i < frame->len; i++) {
+		frame->data[i] = (hex_to_bin(elm->rxbuf[datastart + 3*i]) << 4)
+			       | (hex_to_bin(elm->rxbuf[datastart + 3*i + 1]));
+	}
+
+	/* Feed the frame to the network layer. */
+	elm327_feed_frame_to_netdev(elm, skb);
+
+	return 0;
+}
+
+static void elm327_parse_line(struct can327 *elm, size_t len)
+{
+	lockdep_assert_held(&elm->lock);
+
+	/* Skip empty lines */
+	if (!len)
+		return;
+
+	/* Skip echo lines */
+	if (elm->drop_next_line) {
+		elm->drop_next_line = 0;
+		return;
+	} else if (!memcmp(elm->rxbuf, "AT", 2)) {
+		return;
+	}
+
+	/* Regular parsing */
+	if (elm->state == ELM327_STATE_RECEIVING &&
+	    elm327_parse_frame(elm, len)) {
+		/* Parse an error line. */
+		elm327_parse_error(elm, len);
+
+		/* Start afresh. */
+		elm327_kick_into_cmd_mode(elm);
+	}
+}
+
+static void elm327_handle_prompt(struct can327 *elm)
+{
+	struct can_frame *frame = &elm->can_frame_to_send;
+	/* 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);
+
+	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);
+
+		elm->next_init_cmd++;
+		if (!(*elm->next_init_cmd)) {
+			clear_bit(ELM327_TX_DO_INIT, &elm->cmds_todo);
+			/* Init finished. */
+		}
+
+	} 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)));
+
+	} 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));
+
+	} else if (test_and_clear_bit(ELM327_TX_DO_CAN_CONFIG, &elm->cmds_todo)) {
+		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)) {
+		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)) {
+		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)) {
+		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)) {
+		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)) {
+		if (frame->can_id & CAN_RTR_FLAG) {
+			/* Send an RTR frame. Their DLC is fixed.
+			 * Some chips don't send them at all.
+			 */
+			snprintf(local_txbuf, sizeof(local_txbuf),
+				 "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]);
+			}
+
+			snprintf(&local_txbuf[2 * i], sizeof(local_txbuf),
+				 "\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));
+}
+
+static bool elm327_is_ready_char(char c)
+{
+	/* Bits 0xc0 are sometimes set (randomly), hence the mask.
+	 * Probably bad hardware.
+	 */
+	return (c & 0x3f) == ELM327_READY_CHAR;
+}
+
+static void elm327_drop_bytes(struct can327 *elm, size_t i)
+{
+	lockdep_assert_held(&elm->lock);
+
+	memmove(&elm->rxbuf[0], &elm->rxbuf[i], ELM327_SIZE_RXBUF - i);
+	elm->rxfill -= i;
+}
+
+static void elm327_parse_rxbuf(struct can327 *elm)
+{
+	size_t len;
+	int i;
+
+	lockdep_assert_held(&elm->lock);
+
+	switch (elm->state) {
+	case ELM327_STATE_NOTINIT:
+		elm->rxfill = 0;
+		break;
+
+	case ELM327_STATE_GETDUMMYCHAR:
+	{
+		/* Wait for 'y' or '>' */
+		for (i = 0; i < elm->rxfill; i++) {
+			if (elm->rxbuf[i] == ELM327_DUMMY_CHAR) {
+				elm327_send(elm, "\r", 1);
+				elm->state = ELM327_STATE_GETPROMPT;
+				i++;
+				break;
+			} else if (elm327_is_ready_char(elm->rxbuf[i])) {
+				elm327_send(elm, ELM327_DUMMY_STRING, 1);
+				i++;
+				break;
+			}
+		}
+
+		elm327_drop_bytes(elm, i);
+
+		break;
+	}
+
+	case ELM327_STATE_GETPROMPT:
+		/* Wait for '>' */
+		if (elm327_is_ready_char(elm->rxbuf[elm->rxfill - 1]))
+			elm327_handle_prompt(elm);
+
+		elm->rxfill = 0;
+		break;
+
+	case ELM327_STATE_RECEIVING:
+		/* Find <CR> delimiting feedback lines. */
+		for (len = 0;
+		     (len < elm->rxfill) && (elm->rxbuf[len] != '\r');
+		     len++) {
+			/* empty loop */
+		}
+
+		if (len == ELM327_SIZE_RXBUF) {
+			/* Line exceeds buffer. It's probably all 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,
+				 * so we got a prompt.
+				 * Clear RX buffer and restart listening.
+				 */
+				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);
+
+		/* Remove parsed data from RX buffer. */
+		elm327_drop_bytes(elm, len + 1);
+
+		/* More data to parse? */
+		if (elm->rxfill)
+			elm327_parse_rxbuf(elm);
+	}
+}
+
+static int can327_netdev_open(struct net_device *dev)
+{
+	struct can327 *elm = netdev_priv(dev);
+	int err;
+
+	spin_lock_bh(&elm->lock);
+
+	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) {
+		spin_unlock_bh(&elm->lock);
+		return err;
+	}
+
+	elm327_init(elm);
+	spin_unlock_bh(&elm->lock);
+
+	err = can_rx_offload_add_manual(dev, &elm->offload, ELM327_NAPI_WEIGHT);
+	if (err) {
+		close_candev(dev);
+		return err;
+	}
+
+	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);
+
+	return 0;
+}
+
+static int can327_netdev_close(struct net_device *dev)
+{
+	struct can327 *elm = netdev_priv(dev);
+
+	/* Interrupt whatever the ELM327 is doing right now */
+	spin_lock_bh(&elm->lock);
+	elm327_send(elm, ELM327_DUMMY_STRING, 1);
+	spin_unlock_bh(&elm->lock);
+
+	netif_stop_queue(dev);
+
+	/* 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;
+	can_rx_offload_del(&elm->offload);
+	close_candev(dev);
+	can_led_event(dev, CAN_LED_EVENT_STOP);
+
+	return 0;
+}
+
+/* Send a can_frame to a TTY. */
+static netdev_tx_t can327_netdev_start_xmit(struct sk_buff *skb,
+					    struct net_device *dev)
+{
+	struct can327 *elm = netdev_priv(dev);
+	struct can_frame *frame = (struct can_frame *)skb->data;
+
+	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
+	 */
+	spin_lock(&elm->lock);
+
+	/* 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);
+		goto out;
+	}
+
+	netif_stop_queue(dev);
+
+	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;
+}
+
+static const struct net_device_ops can327_netdev_ops = {
+	.ndo_open	= can327_netdev_open,
+	.ndo_stop	= can327_netdev_close,
+	.ndo_start_xmit	= can327_netdev_start_xmit,
+	.ndo_change_mtu	= can_change_mtu,
+};
+
+static bool can327_is_valid_rx_char(char 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 == ' ');
+}
+
+/* Handle incoming ELM327 ASCII data.
+ * This will not be re-entered while running, but other ldisc
+ * functions may be called in parallel.
+ */
+static void can327_ldisc_rx(struct tty_struct *tty,
+			    const unsigned char *cp, const char *fp, int count)
+{
+	struct can327 *elm = (struct can327 *)tty->disc_data;
+
+	spin_lock_bh(&elm->lock);
+
+	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_uart_side_failure(elm);
+
+			goto out;
+		}
+
+		/* Ignore NUL characters, which the PIC microcontroller may
+		 * inadvertently insert due to a known hardware bug.
+		 * See ELM327 documentation, which refers to a Microchip PIC
+		 * bug description.
+		 */
+		if (*cp != 0) {
+			/* Check for stray characters on the UART line.
+			 * Likely caused by bad hardware.
+			 */
+			if (!can327_is_valid_rx_char(*cp)) {
+				netdev_err(elm->dev,
+					   "Received illegal character %02x.\n",
+					   *cp);
+				elm327_uart_side_failure(elm);
+
+				goto out;
+			}
+
+			elm->rxbuf[elm->rxfill++] = *cp;
+		}
+
+		cp++;
+	}
+
+	if (count >= 0) {
+		netdev_err(elm->dev, "Receive buffer overflowed. Bad chip or wiring?");
+
+		elm327_uart_side_failure(elm);
+
+		goto out;
+	}
+
+	elm327_parse_rxbuf(elm);
+
+out:
+	spin_unlock_bh(&elm->lock);
+}
+
+/* Write out remaining transmit buffer.
+ * Scheduled when TTY is writable.
+ */
+static void can327_ldisc_tx_worker(struct work_struct *work)
+{
+	struct can327 *elm = container_of(work, struct can327, tx_work);
+	ssize_t written;
+
+	if (elm->uart_side_failure)
+		return;
+
+	spin_lock_bh(&elm->lock);
+
+	if (elm->txleft) {
+		written = elm->tty->ops->write(elm->tty, elm->txhead, elm->txleft);
+		if (written < 0) {
+			netdev_err(elm->dev,
+				   "Failed to write to tty %s.\n",
+				   elm->tty->name);
+			elm327_uart_side_failure(elm);
+			spin_unlock_bh(&elm->lock);
+			return;
+		}
+
+		elm->txleft -= written;
+		elm->txhead += written;
+	}
+
+	if (!elm->txleft)  {
+		clear_bit(TTY_DO_WRITE_WAKEUP, &elm->tty->flags);
+		spin_unlock_bh(&elm->lock);
+	} else {
+		spin_unlock_bh(&elm->lock);
+	}
+}
+
+/* Called by the driver when there's room for more data. */
+static void can327_ldisc_tx_wakeup(struct tty_struct *tty)
+{
+	struct can327 *elm = (struct can327 *)tty->disc_data;
+
+	schedule_work(&elm->tx_work);
+}
+
+/* ELM327 can only handle bitrates that are integer divisors of 500 kHz,
+ * 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] = {
+	 7812,  7936,  8064,  8196,  8333,  8474,  8620,  8771,
+	 8928,  9090,  9259,  9433,  9615,  9803, 10000, 10204,
+	10416, 10638, 10869, 11111, 11363, 11627, 11904, 12195,
+	12500, 12820, 13157, 13513, 13888, 14285, 14705, 15151,
+	15625, 16129, 16666, 17241, 17857, 18518, 19230, 20000,
+	20833, 21739, 22727, 23809, 25000, 26315, 27777, 29411,
+	31250, 33333, 35714, 38461, 41666, 45454, 50000, 55555,
+	62500, 71428, 83333, 100000, 125000, 166666, 250000, 500000
+};
+
+/* Dummy needed to use bitrate_const */
+static int can327_do_set_bittiming(struct net_device *netdev)
+{
+	return 0;
+}
+
+static int can327_ldisc_open(struct tty_struct *tty)
+{
+	struct net_device *dev;
+	struct can327 *elm;
+	int err;
+
+	if (!capable(CAP_NET_ADMIN))
+		return -EPERM;
+
+	if (!tty->ops->write)
+		return -EOPNOTSUPP;
+
+	dev = alloc_candev(sizeof(struct can327), 0);
+	if (!dev)
+		return -ENFILE;
+	elm = netdev_priv(dev);
+
+	/* Configure TTY interface */
+	tty->receive_room = 65536; /* We don't flow control */
+	spin_lock_init(&elm->lock);
+	INIT_WORK(&elm->tx_work, can327_ldisc_tx_worker);
+
+	/* Configure CAN metadata */
+	elm->can.bitrate_const = can327_bitrate_const;
+	elm->can.bitrate_const_cnt = ARRAY_SIZE(can327_bitrate_const);
+	elm->can.do_set_bittiming = can327_do_set_bittiming;
+	elm->can.ctrlmode_supported = CAN_CTRLMODE_LISTENONLY;
+
+	/* Configure netdev interface */
+	elm->dev = dev;
+	dev->netdev_ops = &can327_netdev_ops;
+
+	/* Mark ldisc channel as alive */
+	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;
+
+	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.
+ * This means flushing out any pending queues, and then returning.
+ * This call is serialized against other ldisc functions:
+ * Once this is called, no other ldisc function of ours is entered.
+ *
+ * We also use this function for a hangup event.
+ */
+static void can327_ldisc_close(struct tty_struct *tty)
+{
+	struct can327 *elm = (struct can327 *)tty->disc_data;
+
+	/* 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);
+
+	/* Mark channel as dead */
+	spin_lock_bh(&elm->lock);
+	tty->disc_data = NULL;
+	elm->tty = NULL;
+	spin_unlock_bh(&elm->lock);
+
+	netdev_info(elm->dev, "can327 off %s.\n", tty->name);
+
+	free_candev(elm->dev);
+}
+
+static int can327_ldisc_ioctl(struct tty_struct *tty,
+			      unsigned int cmd, unsigned long arg)
+{
+	struct can327 *elm = (struct can327 *)tty->disc_data;
+	unsigned int tmp;
+
+	switch (cmd) {
+	case SIOCGIFNAME:
+		tmp = strnlen(elm->dev->name, IFNAMSIZ - 1) + 1;
+		if (copy_to_user((void __user *)arg, elm->dev->name, tmp))
+			return -EFAULT;
+		return 0;
+
+	case SIOCSIFHWADDR:
+		return -EINVAL;
+
+	default:
+		return tty_mode_ioctl(tty, cmd, arg);
+	}
+}
+
+static struct tty_ldisc_ops can327_ldisc = {
+	.owner		= THIS_MODULE,
+	.name		= "can327",
+	.num		= N_CAN327,
+	.receive_buf	= can327_ldisc_rx,
+	.write_wakeup	= can327_ldisc_tx_wakeup,
+	.open		= can327_ldisc_open,
+	.close		= can327_ldisc_close,
+	.ioctl		= can327_ldisc_ioctl,
+};
+
+static int __init can327_init(void)
+{
+	int status;
+
+	status = tty_register_ldisc(&can327_ldisc);
+	if (status)
+		pr_err("Can't register line discipline\n");
+
+	return status;
+}
+
+static void __exit can327_exit(void)
+{
+	/* This will only be called when all channels have been closed by
+	 * userspace - tty_ldisc.c takes care of the module's refcount.
+	 */
+	tty_unregister_ldisc(&can327_ldisc);
+}
+
+module_init(can327_init);
+module_exit(can327_exit);
+
+MODULE_ALIAS_LDISC(N_CAN327);
+MODULE_DESCRIPTION("ELM327 based CAN interface");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Max Staudt <max@enpas.org>");
diff --git a/include/uapi/linux/tty.h b/include/uapi/linux/tty.h
index 9d0f06bfbac3..68aeae2addec 100644
--- a/include/uapi/linux/tty.h
+++ b/include/uapi/linux/tty.h
@@ -38,8 +38,9 @@
 #define N_NULL		27	/* Null ldisc used for error handling */
 #define N_MCTP		28	/* MCTP-over-serial */
 #define N_DEVELOPMENT	29	/* Manual out-of-tree testing */
+#define N_CAN327	30	/* ELM327 based OBD-II interfaces */
 
 /* Always the newest line discipline + 1 */
-#define NR_LDISCS	30
+#define NR_LDISCS	31
 
 #endif /* _UAPI_LINUX_TTY_H */
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v6] can, tty: can327 CAN/ldisc driver for ELM327 based OBD-II adapters
  2022-05-12 18:29 [PATCH v6] can, tty: can327 CAN/ldisc driver for ELM327 based OBD-II adapters Max Staudt
@ 2022-05-13  2:38 ` Vincent Mailhol
  2022-05-13  6:31   ` Vincent Mailhol
                     ` (3 more replies)
  2022-05-18 16:24 ` Vincent Mailhol
  1 sibling, 4 replies; 14+ messages in thread
From: Vincent Mailhol @ 2022-05-13  2:38 UTC (permalink / raw)
  To: Max Staudt
  Cc: Wolfgang Grandegger, Marc Kleine-Budde, linux-can,
	Greg Kroah-Hartman, Oliver Neukum, linux-kernel

Hi Max,

Nice improvement since v3, thanks! Here are my new comments.

On Fri. 13 May 2022 at 03:29, Max Staudt <max@enpas.org> wrote:
> This is the can327 driver. It does a surprisingly good job at turning
> ELM327 based OBD-II interfaces into cheap CAN interfaces for simple
> homebrew projects.
>
> Please see the included documentation for details and limitations:
> Documentation/networking/device_drivers/can/can327.rst
>
> Cc: linux-can <linux-can@vger.kernel.org>
> Signed-off-by: Max Staudt <max@enpas.org>
> ---
> Many thanks to Greg, Marc, Vincent for reviewing v3!
>
> I've CC'd you hoping you'd check whether you are happy with the changes.
> Please do let me know.
>
>
> Changes in v6:
>  - Renamed 'elmcan' to 'can327'
>  - No other changes.
>
> Changes in v5:
>  - Fixed lockdep asserts (I didn't have lockdep enabled in my build)
>  - Restore: TTY wakeup request before write() in elm327_send()
>  - Clear TTY wakeup on uart_side_failure
>  - checkpatch style fixes
>  - Repack struct elmcan
>
> Changes in v4:
>  - Rebased on top of v5.18-rc5
>  - Simplified TTY locking.
>      The ldisc layer already blocks our ldisc ops from running concurrently.
>      No elm_get(), no RCU are needed, except locking against the TX worker.
>  - Removed .hangup() ldisc op which only called .close().
>      The ldisc layer calls .close() in a good place anyway.
>  - Restart on netdev down/up, even after UART failure.
>      This is in line with Marc's line of thinking, and there was no
>      good reason not to implement it. It also cleaned up the code.
>  - hw_failure is now uart_side_failure and shutdown looks closer to es58x.
>  - Clarified memory/string comparisons.
>      One helper function remains, but it's hopefully clear now :)
>  - lockdep_assert_held() instead of comments.
>  - Cleaned up types in struct elmcan and used pahole to pack it.
>      unsigned instead of int, u8 instead of char, ...
>  - The TX buffer is now a static part of struct elmcan. No more kmalloc().
>  - Removed dummy mailbox_read() for rx_offload.
>      by using can_rx_offload_add_manual() instead of can_rx_offload_add_fifo().
>  - netif_wake_queue() is moved to happen as late as possible.
>      Wake when the ELM327 is going into monitoring mode, not every time
>      after flushing whatever UART buffer we were TX'ing.
>  - Use alloc_can_skb() and avoid late allocation and copy.
>  - elmcan_netdev_start_xmit(): Used can_dropped_invalid_skb().
>  - Replaced ->can_dlc with ->len.
>  - Clarified size of local TX buffer in elm327_handle_prompt().
>  - Renamed TODO_* to ELM327_TX_DO_* and ELM_* to ELM327_STATE_*.
>  - Minor string and other cleanups.
>      pr_fmt, MODULE_*, unnecessary checks, ...
>  - Clarified file header.
>
> Changes in v3:
>  - Now depends on c2faf737abfb for new ldisc number 30.
>  - Eliminated hardcoded string lengths (GCC will work its magic).
>  - Emit generic error frames if an error message couldn't be parsed.
>  - Silence driver startup and init (but still announce ldattach).
>  - Cleaned up comments, strings, readme.
>  - Removed sole module option "accept_flaky_uart".
>    I likely had EMI in earlier testing, which is gone now.
>    This means we can stay strict, unless anyone objects.
>
> Changes in v2:
>  - Moved to SocketCAN's rx-offload wrapper for NAPI, thus avoiding
>    packets being reordered.
>  - Updated TTY ldisc code for Linux v5.17-rc3. A lot of cleanup has
>    happened there lately.
>  - netif_stop_queue() is called earlier in _netdev_close().
>  - Minor cleanup: More helpful strings and return values.
>
> ---
>  .../networking/device_drivers/can/can327.rst  |  325 +++++
>  .../networking/device_drivers/can/index.rst   |    1 +
>  MAINTAINERS                                   |    7 +
>  drivers/net/can/Kconfig                       |   17 +
>  drivers/net/can/Makefile                      |    1 +
>  drivers/net/can/can327.c                      | 1152 +++++++++++++++++
>  include/uapi/linux/tty.h                      |    3 +-
>  7 files changed, 1505 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/networking/device_drivers/can/can327.rst
>  create mode 100644 drivers/net/can/can327.c
>
> diff --git a/Documentation/networking/device_drivers/can/can327.rst b/Documentation/networking/device_drivers/can/can327.rst
> new file mode 100644
> index 000000000000..48508963dd1b
> --- /dev/null
> +++ b/Documentation/networking/device_drivers/can/can327.rst
> @@ -0,0 +1,325 @@
> +.. SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)
> +
> +can327: ELM327 driver for Linux SocketCAN
> +==========================================
> +
> +Authors
> +--------
> +
> +Max Staudt <max@enpas.org>
> +
> +
> +
> +Motivation
> +-----------
> +
> +This driver aims to lower the initial cost for hackers interested in
> +working with CAN buses.
> +
> +CAN adapters are expensive, few, and far between.
> +ELM327 interfaces are cheap and plentiful.
> +Let's use ELM327s as CAN adapters.
> +
> +
> +
> +Introduction
> +-------------
> +
> +This driver is an effort to turn abundant ELM327 based OBD interfaces
> +into full fledged (as far as possible) CAN interfaces.
> +
> +Since the ELM327 was never meant to be a stand alone CAN controller,
> +the driver has to switch between its modes as quickly as possible in
> +order to fake full-duplex operation.
> +
> +As such, can327 is a best effort driver. However, this is more than
> +enough to implement simple request-response protocols (such as OBD II),
> +and to monitor broadcast messages on a bus (such as in a vehicle).
> +
> +Most ELM327s come as nondescript serial devices, attached via USB or
> +Bluetooth. The driver cannot recognize them by itself, and as such it
> +is up to the user to attach it in form of a TTY line discipline
> +(similar to PPP, SLIP, slcan, ...).
> +
> +This driver is meant for ELM327 versions 1.4b and up, see below for
> +known limitations in older controllers and clones.
> +
> +
> +
> +Data sheet
> +-----------
> +
> +The official data sheets can be found at ELM electronics' home page:
> +
> +  https://www.elmelectronics.com/
> +
> +
> +
> +How to check the controller version
> +------------------------------------
> +
> +Use a terminal program to attach to the controller.
> +The default settings are 38400 baud/s, 8 data bits, no parity, 1 stopbit.
> +
> +After issuing the "``AT WS``" command, the controller will respond with
> +its version::
> +
> +    >AT WS
> +
> +
> +    ELM327 v1.4b
> +
> +    >
> +
> +Note that clones may claim to be any version they like.
> +It is not indicative of their actual feature set.
> +
> +
> +
> +How to attach the line discipline
> +----------------------------------
> +
> +Every ELM327 chip is factory programmed to operate at a serial setting
> +of 38400 baud/s, 8 data bits, no parity, 1 stopbit.
> +
> +The line discipline can be attached on a command prompt as follows::
> +
> +    sudo ldattach \
> +           --debug \
> +           --speed 38400 \
> +           --eightbits \
> +           --noparity \
> +           --onestopbit \
> +           --iflag -ICRNL,INLCR,-IXOFF \
> +           30 \
> +           /dev/ttyUSB0
> +
> +To change the ELM327's serial settings, please refer to its data
> +sheet. This needs to be done before attaching the line discipline.
> +
> +Once the ldisc is attached, the CAN interface starts out unconfigured.
> +Set the speed before starting it:
> +
> +    # The interface needs to be down to change parameters
> +    sudo ip link set can0 down
> +    sudo ip link set can0 type can bitrate 500000
> +    sudo ip link set can0 up
> +
> +500000 bit/s is a common rate for OBD-II diagnostics.
> +If you're connecting straight to a car's OBD port, this is the speed
> +that most cars (but not all!) expect.
> +
> +After this, you can set out as usual with candump, cansniffer, etc.
> +
> +
> +
> +Known limitations of the controller
> +------------------------------------
> +
> +- Clone devices ("v1.5" and others)
> +
> +  Sending RTR frames is not supported and will be dropped silently.
> +
> +  Receiving RTR with DLC 8 will appear to be a regular frame with
> +  the last received frame's DLC and payload.
> +
> +  "``AT CSM``" not supported, thus no ACK-ing frames while listening:
> +  "``AT MA``" will always be silent. However, immediately after
> +  sending a frame, the ELM327 will be in "receive reply" mode, in
> +  which it *does* ACK any received frames. Once the bus goes silent
> +  or an error occurs (such as BUFFER FULL), the ELM327 will end reply
> +  reception mode on its own and can327 will fall back to "``AT MA``"
> +  in order to keep monitoring the bus.
> +
> +
> +- All versions
> +
> +  No full duplex operation is supported. The driver will switch
> +  between input/output mode as quickly as possible.
> +
> +  The length of outgoing RTR frames cannot be set. In fact, some
> +  clones (tested with one identifying as "``v1.5``") are unable to
> +  send RTR frames at all.
> +
> +  We don't have a way to get real-time notifications on CAN errors.
> +  While there is a command (``AT CS``) to retrieve some basic stats,
> +  we don't poll it as it would force us to interrupt reception mode.
> +
> +
> +- Versions prior to 1.4b
> +
> +  These versions do not send CAN ACKs when in monitoring mode (AT MA).
> +  However, they do send ACKs while waiting for a reply immediately
> +  after sending a frame. The driver maximizes this time to make the
> +  controller as useful as possible.
> +
> +  Starting with version 1.4b, the ELM327 supports the "``AT CSM``"
> +  command, and the "listen-only" CAN option will take effect.
> +
> +
> +- Versions prior to 1.4
> +
> +  These chips do not support the "``AT PB``" command, and thus cannot
> +  change bitrate or SFF/EFF mode on-the-fly. This will have to be
> +  programmed by the user before attaching the line discipline. See the
> +  data sheet for details.
> +
> +
> +- Versions prior to 1.3
> +
> +  These chips cannot be used at all with can327. They do not support
> +  the "``AT D1``" command, which is necessary to avoid parsing conflicts
> +  on incoming data, as well as distinction of RTR frame lengths.
> +
> +  Specifically, this allows for easy distinction of SFF and EFF
> +  frames, and to check whether frames are complete. While it is possible
> +  to deduce the type and length from the length of the line the ELM327
> +  sends us, this method fails when the ELM327's UART output buffer
> +  overruns. It may abort sending in the middle of the line, which will
> +  then be mistaken for something else.
> +
> +
> +
> +Known limitations of the driver
> +--------------------------------
> +
> +- No 8/7 timing.
> +
> +  ELM327 can only set CAN bitrates that are of the form 500000/n, where
> +  n is an integer divisor.
> +  However there is an exception: With a separate flag, it may set the
> +  speed to be 8/7 of the speed indicated by the divisor.
> +  This mode is not currently implemented.
> +
> +- No evaluation of command responses.
> +
> +  The ELM327 will reply with OK when a command is understood, and with ?
> +  when it is not. The driver does not currently check this, and simply
> +  assumes that the chip understands every command.
> +  The driver is built such that functionality degrades gracefully
> +  nevertheless. See the section on known limitations of the controller.
> +
> +- No use of hardware CAN ID filtering
> +
> +  An ELM327's UART sending buffer will easily overflow on heavy CAN bus
> +  load, resulting in the "``BUFFER FULL``" message. Using the hardware
> +  filters available through "``AT CF xxx``" and "``AT CM xxx``" would be
> +  helpful here, however SocketCAN does not currently provide a facility
> +  to make use of such hardware features.
> +
> +
> +
> +Communication example
> +----------------------
> +
> +This is a short and incomplete introduction on how to talk to an ELM327.
> +
> +
> +The ELM327 has two modes:
> +
> +- Command mode
> +- Reception mode
> +
> +In command mode, it expects one command per line, terminated by CR.
> +By default, the prompt is a "``>``", after which a command can be
> +entered::
> +
> +    >ATE1
> +    OK
> +    >
> +
> +The init script in the driver switches off several configuration options
> +that are only meaningful in the original OBD scenario the chip is meant
> +for, and are actually a hindrance for can327.
> +
> +
> +When a command is not recognized, such as by an older version of the
> +ELM327, a question mark is printed as a response instead of OK::
> +
> +    >ATUNKNOWN
> +    ?
> +    >
> +
> +At present, can327 does not evaluate this response and silently assumes
> +that all commands are recognized. It is structured such that it will
> +degrade gracefully when a command is unknown. See the sections above on
> +known limitations for details.
> +
> +
> +When a CAN frame is to be sent, the target address is configured, after
> +which the frame is sent as a command that consists of the data's hex
> +dump::
> +
> +    >ATSH123
> +    OK
> +    >DEADBEEF12345678
> +    OK
> +    >
> +
> +The above interaction sends the frame "``DE AD BE EF 12 34 56 78``" with
> +the 11 bit CAN ID ``0x123``.
> +For this to function, the controller must be configured for 11 bit CAN
> +ID sending mode (using "``AT PB``", see code or datasheet).
> +
> +
> +Once a frame has been sent and wait-for-reply mode is on (``ATR1``,
> +configured on ``listen-only=off``), or when the reply timeout expires and
> +the driver sets the controller into monitoring mode (``ATMA``), the ELM327
> +will send one line for each received CAN frame, consisting of CAN ID,
> +DLC, and data::
> +
> +    123 8 DEADBEEF12345678
> +
> +For 29 bit CAN frames, the address format is slightly different, which
> +can327 uses to tell the two apart::
> +
> +    12 34 56 78 8 DEADBEEF12345678
> +
> +The ELM327 will receive both 11 and 29 bit frames - the current CAN
> +config (``ATPB``) does not matter.
> +
> +
> +If the ELM327's internal UART sending buffer runs full, it will abort
> +the monitoring mode, print "BUFFER FULL" and drop back into command
> +mode. Note that in this case, unlike with other error messages, the
> +error message may appear on the same line as the last (usually
> +incomplete) data frame::
> +
> +    12 34 56 78 8 DEADBEEF123 BUFFER FULL
> +
> +
> +
> +Rationale behind the chosen configuration
> +------------------------------------------
> +
> +``AT E1``
> +  Echo on
> +
> +  We need this to be able to get a prompt reliably.
> +
> +``AT S1``
> +  Spaces on
> +
> +  We need this to distinguish 11/29 bit CAN addresses received.
> +
> +  Note:
> +  We can usually do this using the line length (odd/even),
> +  but this fails if the line is not transmitted fully to
> +  the host (BUFFER FULL).
> +
> +``AT D1``
> +  DLC on
> +
> +  We need this to tell the "length" of RTR frames.
> +
> +
> +
> +A note on CAN bus termination
> +------------------------------
> +
> +Your adapter may have resistors soldered in which are meant to terminate
> +the bus. This is correct when it is plugged into a OBD-II socket, but
> +not helpful when trying to tap into the middle of an existing CAN bus.
> +
> +If communications don't work with the adapter connected, check for the
> +termination resistors on its PCB and try removing them.
> diff --git a/Documentation/networking/device_drivers/can/index.rst b/Documentation/networking/device_drivers/can/index.rst
> index 58b6e0ad3030..c4f724db4908 100644
> --- a/Documentation/networking/device_drivers/can/index.rst
> +++ b/Documentation/networking/device_drivers/can/index.rst
> @@ -10,6 +10,7 @@ Contents:
>  .. toctree::
>     :maxdepth: 2
>
> +   can327
>     freescale/flexcan
>
>  .. only::  subproject and html
> diff --git a/MAINTAINERS b/MAINTAINERS
> index edc96cdb85e8..ab7f0906f07b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7218,6 +7218,13 @@ L:       netdev@vger.kernel.org
>  S:     Maintained
>  F:     drivers/net/ethernet/ibm/ehea/
>
> +ELM327 CAN NETWORK DRIVER
> +M:     Max Staudt <max@enpas.org>
> +L:     linux-can@vger.kernel.org
> +S:     Maintained
> +F:     Documentation/networking/device_drivers/can/can327.rst
> +F:     drivers/net/can/can327.c
> +
>  EM28XX VIDEO4LINUX DRIVER
>  M:     Mauro Carvalho Chehab <mchehab@kernel.org>
>  L:     linux-media@vger.kernel.org
> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index fff259247d52..7900f9852e03 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -180,6 +180,23 @@ source "drivers/net/can/softing/Kconfig"
>  source "drivers/net/can/spi/Kconfig"
>  source "drivers/net/can/usb/Kconfig"
>
> +config CAN_CAN327
> +       tristate "Serial / USB serial ELM327 based OBD-II Interfaces (can327)"
> +       depends on TTY
> +       help
> +         CAN driver for several 'low cost' OBD-II interfaces based on the
> +         ELM327 OBD-II interpreter chip.
> +
> +         This is a best effort driver - the ELM327 interface was never
> +         designed to be used as a standalone CAN interface. However, it can
> +         still be used for simple request-response protocols (such as OBD II),
> +         and to monitor broadcast messages on a bus (such as in a vehicle).
> +
> +         Please refer to the documentation for information on how to use it:
> +         Documentation/networking/device_drivers/can/can327.rst
> +
> +         If this driver is built as a module, it will be called can327.
> +
>  endif
>
>  config CAN_DEBUG_DEVICES
> diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
> index 1e660afcb61b..ff53bc274ae1 100644
> --- a/drivers/net/can/Makefile
> +++ b/drivers/net/can/Makefile
> @@ -6,6 +6,7 @@
>  obj-$(CONFIG_CAN_VCAN)         += vcan.o
>  obj-$(CONFIG_CAN_VXCAN)                += vxcan.o
>  obj-$(CONFIG_CAN_SLCAN)                += slcan.o
> +obj-$(CONFIG_CAN_CAN327)       += can327.o
>
>  obj-y                          += dev/
>  obj-y                          += rcar/
> diff --git a/drivers/net/can/can327.c b/drivers/net/can/can327.c
> new file mode 100644
> index 000000000000..bbed01f90161
> --- /dev/null
> +++ b/drivers/net/can/can327.c
> @@ -0,0 +1,1152 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* 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.
> + *
> + * can327.c Author : Max Staudt <max-linux@enpas.org>
> + * slcan.c Author  : Oliver Hartkopp <socketcan@hartkopp.net>
> + * slip.c Authors  : Laurence Culhane <loz@holmes.demon.co.uk>
> + *                   Fred N. van Kempen <waltje@uwalt.nl.mugnet.org>
> + */
> +
> +#define pr_fmt(fmt) "can327: " fmt
> +
> +#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>
> +#include <linux/netdevice.h>
> +#include <linux/skbuff.h>
> +#include <linux/spinlock.h>
> +#include <linux/string.h>
> +#include <linux/tty.h>
> +#include <linux/tty_ldisc.h>
> +#include <linux/workqueue.h>
> +
> +#include <uapi/linux/tty.h>
> +
> +#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>
> +
> +#define ELM327_NAPI_WEIGHT 4
> +
> +#define ELM327_SIZE_RXBUF 224
> +#define ELM327_SIZE_TXBUF 32
> +
> +#define ELM327_CAN_CONFIG_SEND_SFF           0x8000
> +#define ELM327_CAN_CONFIG_VARIABLE_DLC       0x4000
> +#define ELM327_CAN_CONFIG_RECV_BOTH_SFF_EFF  0x2000
> +#define ELM327_CAN_CONFIG_BAUDRATE_MULT_8_7  0x1000
> +
> +#define ELM327_DUMMY_CHAR 'y'
> +#define ELM327_DUMMY_STRING "y"
> +#define ELM327_READY_CHAR '>'
> +
> +/* Bits in elm->cmds_todo */
> +enum elm327_to_to_do_bits {

to_to_do? Name looks like a typo. Also, do not see the value on the
_bits suffix. I suggest:

enum elm327_tx_do {

> +       ELM327_TX_DO_CAN_DATA = 0,
> +       ELM327_TX_DO_CANID_11BIT,
> +       ELM327_TX_DO_CANID_29BIT_LOW,
> +       ELM327_TX_DO_CANID_29BIT_HIGH,
> +       ELM327_TX_DO_CAN_CONFIG_PART2,
> +       ELM327_TX_DO_CAN_CONFIG,
> +       ELM327_TX_DO_RESPONSES,
> +       ELM327_TX_DO_SILENT_MONITOR,
> +       ELM327_TX_DO_INIT
> +};
> +
> +struct can327 {
> +       /* This must be the first member when using alloc_candev() */
> +       struct can_priv can;
> +
> +       struct can_rx_offload offload;
> +
> +       /* TTY buffers */
> +       u8 rxbuf[ELM327_SIZE_RXBUF];
> +       u8 txbuf[ELM327_SIZE_TXBUF] ____cacheline_aligned;

Out of curiosity, any rationale for the use of ____cacheline_aligned here?

> +       /* Per-channel lock */
> +       spinlock_t lock;
> +
> +       /* TTY and netdev devices that we're bridging */
> +       struct tty_struct *tty;
> +       struct net_device *dev;
> +
> +       /* 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 {
> +               ELM327_STATE_NOTINIT = 0,
> +               ELM327_STATE_GETDUMMYCHAR,
> +               ELM327_STATE_GETPROMPT,
> +               ELM327_STATE_RECEIVING,
> +       } state;
> +
> +       /* 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
> +        */
> +       struct can_frame can_frame_to_send;
> +       u16 can_config;
> +       u8 can_bitrate_divisor;
> +
> +       /* 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_uart_side_failure(struct can327 *elm);
> +
> +static void elm327_send(struct can327 *elm, const void *buf, size_t len)
> +{
> +       int written;
> +
> +       lockdep_assert_held(&elm->lock);
> +
> +       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_uart_side_failure(elm);
> +               return;
> +       }
> +
> +       elm->txleft = len - written;
> +       elm->txhead = elm->txbuf + written;
> +}
> +
> +/* Take the ELM327 out of almost any state and back into command mode.
> + * We send ELM327_DUMMY_CHAR which will either abort any running
> + * operation, or be echoed back to us in case we're already in command
> + * mode.
> + */
> +static void elm327_kick_into_cmd_mode(struct can327 *elm)
> +{
> +       lockdep_assert_held(&elm->lock);
> +
> +       if (elm->state != ELM327_STATE_GETDUMMYCHAR &&
> +           elm->state != ELM327_STATE_GETPROMPT) {
> +               elm327_send(elm, ELM327_DUMMY_STRING, 1);
> +
> +               elm->state = ELM327_STATE_GETDUMMYCHAR;
> +       }
> +}
> +
> +/* Schedule a CAN frame and necessary config changes to be sent to the TTY. */
> +static void elm327_send_frame(struct can327 *elm, struct can_frame *frame)
> +{
> +       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) {
> +               /* Set the new CAN ID for transmission. */
> +               if ((frame->can_id & CAN_EFF_FLAG) ^
> +                   (elm->can_frame_to_send.can_id & CAN_EFF_FLAG)) {

(frame->can_id & CAN_EFF_FLAG) ^ (elm->can_frame_to_send.can_id & CAN_EFF_FLAG)

can be factorized as:

(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)
> +                                       | ELM327_CAN_CONFIG_VARIABLE_DLC
> +                                       | ELM327_CAN_CONFIG_RECV_BOTH_SFF_EFF
> +                                       | elm->can_bitrate_divisor;
> +
> +                       set_bit(ELM327_TX_DO_CAN_CONFIG, &elm->cmds_todo);
> +               }
> +
> +               if (frame->can_id & CAN_EFF_FLAG) {
> +                       clear_bit(ELM327_TX_DO_CANID_11BIT, &elm->cmds_todo);
> +                       set_bit(ELM327_TX_DO_CANID_29BIT_LOW, &elm->cmds_todo);
> +                       set_bit(ELM327_TX_DO_CANID_29BIT_HIGH, &elm->cmds_todo);
> +               } else {
> +                       set_bit(ELM327_TX_DO_CANID_11BIT, &elm->cmds_todo);
> +                       clear_bit(ELM327_TX_DO_CANID_29BIT_LOW, &elm->cmds_todo);
> +                       clear_bit(ELM327_TX_DO_CANID_29BIT_HIGH, &elm->cmds_todo);
> +               }
> +       }
> +
> +       /* Schedule the CAN frame itself. */
> +       elm->can_frame_to_send = *frame;
> +       set_bit(ELM327_TX_DO_CAN_DATA, &elm->cmds_todo);
> +
> +       elm327_kick_into_cmd_mode(elm);
> +}
> +
> +/* 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 */
> +       "AT M0\r",        /* v1.0: Memory Off */
> +       "AT AL\r",        /* v1.0: Allow Long messages */
> +       "AT BI\r",        /* v1.0: Bypass Initialisation */
> +       "AT CAF0\r",      /* v1.0: CAN Auto Formatting Off */
> +       "AT CFC0\r",      /* v1.0: CAN Flow Control Off */
> +       "AT CF 000\r",    /* v1.0: Reset CAN ID Filter */
> +       "AT CM 000\r",    /* v1.0: Reset CAN ID Mask */
> +       "AT E1\r",        /* v1.0: Echo On */
> +       "AT H1\r",        /* v1.0: Headers On */
> +       "AT L0\r",        /* v1.0: Linefeeds Off */
> +       "AT SH 7DF\r",    /* v1.0: Set CAN sending ID to 0x7df */
> +       "AT ST FF\r",     /* v1.0: Set maximum Timeout for response after TX */
> +       "AT AT0\r",       /* v1.2: Adaptive Timing Off */
> +       "AT D1\r",        /* v1.3: Print DLC On */
> +       "AT S1\r",        /* v1.3: Spaces On */
> +       "AT TP B\r",      /* v1.0: Try Protocol B */
> +       NULL
> +};
> +
> +static void elm327_init(struct can327 *elm)
> +{
> +       lockdep_assert_held(&elm->lock);
> +
> +       elm->state = ELM327_STATE_NOTINIT;
> +       elm->can_frame_to_send.can_id = 0x7df; /* ELM327 HW default */
> +       elm->rxfill = 0;
> +       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
> +        * limit the user to the right values.
> +        */
> +       elm->can_bitrate_divisor = 500000 / elm->can.bittiming.bitrate;
> +       elm->can_config = ELM327_CAN_CONFIG_SEND_SFF
> +                       | ELM327_CAN_CONFIG_VARIABLE_DLC
> +                       | ELM327_CAN_CONFIG_RECV_BOTH_SFF_EFF
> +                       | elm->can_bitrate_divisor;
> +
> +       /* Configure ELM327 and then start monitoring */
> +       elm->next_init_cmd = &elm327_init_script[0];
> +       set_bit(ELM327_TX_DO_INIT, &elm->cmds_todo);
> +       set_bit(ELM327_TX_DO_SILENT_MONITOR, &elm->cmds_todo);
> +       set_bit(ELM327_TX_DO_RESPONSES, &elm->cmds_todo);
> +       set_bit(ELM327_TX_DO_CAN_CONFIG, &elm->cmds_todo);
> +
> +       elm327_kick_into_cmd_mode(elm);
> +}
> +
> +static void elm327_feed_frame_to_netdev(struct can327 *elm,
> +                                       struct sk_buff *skb)
> +{
> +       lockdep_assert_held(&elm->lock);
> +
> +       if (!netif_running(elm->dev))
> +               return;
> +
> +       /* Queue for NAPI pickup.
> +        * rx-offload will update stats and LEDs for us.
> +        */
> +       if (can_rx_offload_queue_tail(&elm->offload, skb))
> +               elm->dev->stats.rx_fifo_errors++;
> +
> +       /* Wake NAPI */
> +       can_rx_offload_irq_finish(&elm->offload);
> +}
> +
> +/* Called when we're out of ideas and just want it all to end. */
> +static inline void elm327_uart_side_failure(struct can327 *elm)
> +{
> +       struct can_frame *frame;
> +       struct sk_buff *skb;
> +
> +       lockdep_assert_held(&elm->lock);
> +
> +       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);
> +       elm->can.state = CAN_STATE_BUS_OFF;
> +       can_bus_off(elm->dev);
> +
> +       netdev_err(elm->dev, "ELM327 misbehaved. Blocking further communication.\n");
> +
> +       skb = alloc_can_err_skb(elm->dev, &frame);
> +       if (!skb)
> +               return;
> +
> +       frame->can_id |= CAN_ERR_BUSOFF;
> +       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.
> + *
> + * strncmp() cannot be used here because it accepts the following wrong case:
> + *   strncmp("CAN ER", "CAN ERROR", 6);

What about:
strncmp("CAN ER", "CAN ERROR", 7);
?

> + * This must fail, hence this helper function.
> + */
> +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);
> +}
> +
> +static void elm327_parse_error(struct can327 *elm, size_t len)
> +{
> +       struct can_frame *frame;
> +       struct sk_buff *skb;
> +
> +       lockdep_assert_held(&elm->lock);
> +
> +       skb = alloc_can_err_skb(elm->dev, &frame);
> +       if (!skb)
> +               /* It's okay to return here:
> +                * The outer parsing loop will drop this UART buffer.
> +                */
> +               return;
> +
> +       /* Filter possible error messages based on length of RX'd line */
> +       if (check_len_then_cmp(elm->rxbuf, len, "UNABLE TO CONNECT")) {

Is this equivalent?
      if (!strncmp(elm->rxbuf, "UNABLE TO CONNECT", len + 1)) {

> +               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")) {
> +               /* 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")) {
> +               frame->can_id |= CAN_ERR_BUSERROR;
> +       } else if (check_len_then_cmp(elm->rxbuf, len, "CAN ERROR")) {
> +               frame->can_id |= CAN_ERR_PROT;
> +       } else if (check_len_then_cmp(elm->rxbuf, len, "<RX ERROR")) {
> +               frame->can_id |= CAN_ERR_PROT;
> +       } 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 (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 && !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]);
> +               frame->can_id |= CAN_ERR_CRTL;
> +       } else {
> +               /* Something else has happened.
> +                * Maybe garbage on the UART line.
> +                * Emit a generic error frame.
> +                */
> +       }
> +
> +       elm327_feed_frame_to_netdev(elm, skb);
> +}
> +
> +/* Parse CAN frames coming as ASCII from ELM327.
> + * They can be of various formats:
> + *
> + * 29-bit ID (EFF):  12 34 56 78 D PL PL PL PL PL PL PL PL
> + * 11-bit ID (!EFF): 123 D PL PL PL PL PL PL PL PL
> + *
> + * where D = DLC, PL = payload byte
> + *
> + * Instead of a payload, RTR indicates a remote request.
> + *
> + * We will use the spaces and line length to guess the format.
> + */
> +static int elm327_parse_frame(struct can327 *elm, size_t len)
> +{
> +       struct can_frame *frame;
> +       struct sk_buff *skb;
> +       int hexlen;
> +       int datastart;
> +       int i;
> +
> +       lockdep_assert_held(&elm->lock);
> +
> +       skb = alloc_can_skb(elm->dev, &frame);
> +       if (!skb)
> +               return -ENOMEM;
> +
> +       /* Find first non-hex and non-space character:
> +        *  - In the simplest case, there is none.
> +        *  - For RTR frames, 'R' is the first non-hex character.
> +        *  - An error message may replace the end of the data line.
> +        */
> +       for (hexlen = 0; hexlen <= len; hexlen++) {
> +               if (hex_to_bin(elm->rxbuf[hexlen]) < 0 &&
> +                   elm->rxbuf[hexlen] != ' ') {
> +                       break;
> +               }
> +       }
> +
> +       /* Sanity check whether the line is really a clean hexdump,
> +        * or terminated by an error message, or contains garbage.
> +        */
> +       if (hexlen < len &&
> +           !isdigit(elm->rxbuf[hexlen]) &&
> +           !isupper(elm->rxbuf[hexlen]) &&
> +           '<' != elm->rxbuf[hexlen] &&
> +           ' ' != elm->rxbuf[hexlen]) {
> +               /* The line is likely garbled anyway, so bail.
> +                * The main code will restart listening.
> +                */
> +               return -ENODATA;
> +       }
> +
> +       /* Use spaces in CAN ID to distinguish 29 or 11 bit address length.
> +        * No out-of-bounds access:
> +        * We use the fact that we can always read from elm->rxbuf.
> +        */
> +       if (elm->rxbuf[2] == ' ' && elm->rxbuf[5] == ' ' &&
> +           elm->rxbuf[8] == ' ' && elm->rxbuf[11] == ' ' &&
> +           elm->rxbuf[13] == ' ') {
> +               frame->can_id = CAN_EFF_FLAG;
> +               datastart = 14;
> +       } else if (elm->rxbuf[3] == ' ' && elm->rxbuf[5] == ' ') {
> +               datastart = 6;
> +       } else {
> +               /* This is not a well-formatted data line.
> +                * Assume it's an error message.
> +                */
> +               return -ENODATA;
> +       }
> +
> +       if (hexlen < datastart) {
> +               /* The line is too short to be a valid frame hex dump.
> +                * Something interrupted the hex dump or it is invalid.
> +                */
> +               return -ENODATA;
> +       }
> +
> +       /* From here on all chars up to buf[hexlen] are hex or spaces,
> +        * at well-defined offsets.
> +        */
> +
> +       /* Read CAN data length */
> +       frame->len = (hex_to_bin(elm->rxbuf[datastart - 2]) << 0);
> +
> +       /* Read CAN ID */
> +       if (frame->can_id & CAN_EFF_FLAG) {
> +               frame->can_id |= (hex_to_bin(elm->rxbuf[0]) << 28)
> +                              | (hex_to_bin(elm->rxbuf[1]) << 24)
> +                              | (hex_to_bin(elm->rxbuf[3]) << 20)
> +                              | (hex_to_bin(elm->rxbuf[4]) << 16)
> +                              | (hex_to_bin(elm->rxbuf[6]) << 12)
> +                              | (hex_to_bin(elm->rxbuf[7]) << 8)
> +                              | (hex_to_bin(elm->rxbuf[9]) << 4)
> +                              | (hex_to_bin(elm->rxbuf[10]) << 0);
> +       } else {
> +               frame->can_id |= (hex_to_bin(elm->rxbuf[0]) << 8)
> +                              | (hex_to_bin(elm->rxbuf[1]) << 4)
> +                              | (hex_to_bin(elm->rxbuf[2]) << 0);
> +       }
> +
> +       /* Check for RTR frame */
> +       if (elm->rxfill >= hexlen + 3 &&
> +           !memcmp(&elm->rxbuf[hexlen], "RTR", 3)) {
> +               frame->can_id |= CAN_RTR_FLAG;
> +       }
> +
> +       /* Is the line long enough to hold the advertised payload?
> +        * Note: RTR frames have a DLC, but no actual payload.
> +        */
> +       if (!(frame->can_id & CAN_RTR_FLAG) &&
> +           (hexlen < frame->len * 3 + datastart)) {
> +               /* Incomplete frame.
> +                * Probably the ELM327's RS232 TX buffer was full.
> +                * Emit an error frame and exit.
> +                */
> +               frame->can_id = CAN_ERR_FLAG | CAN_ERR_CRTL;
> +               frame->len = CAN_ERR_DLC;
> +               frame->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
> +               elm327_feed_frame_to_netdev(elm, skb);
> +
> +               /* Signal failure to parse.
> +                * The line will be re-parsed as an error line, which will fail.
> +                * However, this will correctly drop the state machine back into
> +                * command mode.
> +                */
> +               return -ENODATA;
> +       }
> +
> +       /* Parse the data nibbles. */
> +       for (i = 0; i < frame->len; i++) {
> +               frame->data[i] = (hex_to_bin(elm->rxbuf[datastart + 3*i]) << 4)
> +                              | (hex_to_bin(elm->rxbuf[datastart + 3*i + 1]));
> +       }
> +
> +       /* Feed the frame to the network layer. */
> +       elm327_feed_frame_to_netdev(elm, skb);
> +
> +       return 0;
> +}
> +
> +static void elm327_parse_line(struct can327 *elm, size_t len)
> +{
> +       lockdep_assert_held(&elm->lock);

You are using many of those lockdep_assert_held(&elm->lock);

I guess you put them for debug purpose and probaly some of those can
be removed (if you see a genuine risk at some places, then OK to keep
as a safety net, but a bit of clean up can be done here, I think).

This comment applies to all use of lockdep_assert_held().

> +
> +       /* Skip empty lines */
> +       if (!len)
> +               return;
> +
> +       /* Skip echo lines */
> +       if (elm->drop_next_line) {
> +               elm->drop_next_line = 0;
> +               return;
> +       } else if (!memcmp(elm->rxbuf, "AT", 2)) {
> +               return;
> +       }
> +
> +       /* Regular parsing */
> +       if (elm->state == ELM327_STATE_RECEIVING &&
> +           elm327_parse_frame(elm, len)) {
> +               /* Parse an error line. */
> +               elm327_parse_error(elm, len);
> +
> +               /* Start afresh. */
> +               elm327_kick_into_cmd_mode(elm);
> +       }
> +}
> +
> +static void elm327_handle_prompt(struct can327 *elm)
> +{
> +       struct can_frame *frame = &elm->can_frame_to_send;
> +       /* 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);
> +
> +       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);
> +
> +               elm->next_init_cmd++;
> +               if (!(*elm->next_init_cmd)) {
> +                       clear_bit(ELM327_TX_DO_INIT, &elm->cmds_todo);
> +                       /* Init finished. */
> +               }
> +
> +       } 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)));

The second pair of brackets look superficial:
               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)) {
> +               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)) {
> +               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)) {
> +               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)) {
> +               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)) {
> +               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)) {
> +               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)) {
> +               if (frame->can_id & CAN_RTR_FLAG) {
> +                       /* Send an RTR frame. Their DLC is fixed.
> +                        * Some chips don't send them at all.
> +                        */
> +                       snprintf(local_txbuf, sizeof(local_txbuf),
> +                                "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]);
> +                       }
> +
> +                       snprintf(&local_txbuf[2 * i], sizeof(local_txbuf),
> +                                "\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));
> +}
> +
> +static bool elm327_is_ready_char(char c)
> +{
> +       /* Bits 0xc0 are sometimes set (randomly), hence the mask.
> +        * Probably bad hardware.
> +        */
> +       return (c & 0x3f) == ELM327_READY_CHAR;
> +}
> +
> +static void elm327_drop_bytes(struct can327 *elm, size_t i)
> +{
> +       lockdep_assert_held(&elm->lock);
> +
> +       memmove(&elm->rxbuf[0], &elm->rxbuf[i], ELM327_SIZE_RXBUF - i);
> +       elm->rxfill -= i;
> +}
> +
> +static void elm327_parse_rxbuf(struct can327 *elm)
> +{
> +       size_t len;
> +       int i;
> +
> +       lockdep_assert_held(&elm->lock);
> +
> +       switch (elm->state) {
> +       case ELM327_STATE_NOTINIT:
> +               elm->rxfill = 0;
> +               break;
> +
> +       case ELM327_STATE_GETDUMMYCHAR:
> +       {

Is this braket need?

> +               /* Wait for 'y' or '>' */
> +               for (i = 0; i < elm->rxfill; i++) {
> +                       if (elm->rxbuf[i] == ELM327_DUMMY_CHAR) {
> +                               elm327_send(elm, "\r", 1);
> +                               elm->state = ELM327_STATE_GETPROMPT;
> +                               i++;
> +                               break;
> +                       } else if (elm327_is_ready_char(elm->rxbuf[i])) {
> +                               elm327_send(elm, ELM327_DUMMY_STRING, 1);
> +                               i++;
> +                               break;
> +                       }
> +               }
> +
> +               elm327_drop_bytes(elm, i);
> +
> +               break;
> +       }

Same.

> +
> +       case ELM327_STATE_GETPROMPT:
> +               /* Wait for '>' */
> +               if (elm327_is_ready_char(elm->rxbuf[elm->rxfill - 1]))
> +                       elm327_handle_prompt(elm);
> +
> +               elm->rxfill = 0;
> +               break;
> +
> +       case ELM327_STATE_RECEIVING:
> +               /* Find <CR> delimiting feedback lines. */
> +               for (len = 0;
> +                    (len < elm->rxfill) && (elm->rxbuf[len] != '\r');
> +                    len++) {
> +                       /* empty loop */

Question of taste but would prefer a while look with the len++ in the
body (if you prever to do as above, no need to argue, just keep it
like it is).

> +               }
> +
> +               if (len == ELM327_SIZE_RXBUF) {
> +                       /* Line exceeds buffer. It's probably all 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;

Can you have just a single break at the end of the case instead of
having it in every branches of the if/else?

> +               } else if (len == elm->rxfill) {
> +                       if (elm327_is_ready_char(elm->rxbuf[elm->rxfill - 1])) {
> +                               /* The ELM327's AT ST response timeout ran out,
> +                                * so we got a prompt.
> +                                * Clear RX buffer and restart listening.
> +                                */
> +                               elm->rxfill = 0;
> +
> +                               elm327_handle_prompt(elm);
> +                               break;
> +                       }
> +
> +                       /* No <CR> found - we haven't received a full line yet.
> +                        * Wait for more data.
> +                        */
> +                       break;
> +               }

You just need to introduce a final else block here in order to remove
all those breaks:
              } 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);
> +
> +               /* More data to parse? */
> +               if (elm->rxfill)
> +                       elm327_parse_rxbuf(elm);
> +       }
> +}
> +
> +static int can327_netdev_open(struct net_device *dev)
> +{
> +       struct can327 *elm = netdev_priv(dev);
> +       int err;
> +
> +       spin_lock_bh(&elm->lock);
> +
> +       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) {
> +               spin_unlock_bh(&elm->lock);
> +               return err;
> +       }
> +
> +       elm327_init(elm);
> +       spin_unlock_bh(&elm->lock);
> +
> +       err = can_rx_offload_add_manual(dev, &elm->offload, ELM327_NAPI_WEIGHT);
> +       if (err) {
> +               close_candev(dev);
> +               return err;
> +       }
> +
> +       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);
> +
> +       return 0;
> +}
> +
> +static int can327_netdev_close(struct net_device *dev)
> +{
> +       struct can327 *elm = netdev_priv(dev);
> +
> +       /* Interrupt whatever the ELM327 is doing right now */
> +       spin_lock_bh(&elm->lock);
> +       elm327_send(elm, ELM327_DUMMY_STRING, 1);
> +       spin_unlock_bh(&elm->lock);
> +
> +       netif_stop_queue(dev);
> +
> +       /* 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;
> +       can_rx_offload_del(&elm->offload);
> +       close_candev(dev);
> +       can_led_event(dev, CAN_LED_EVENT_STOP);
> +
> +       return 0;
> +}
> +
> +/* Send a can_frame to a TTY. */
> +static netdev_tx_t can327_netdev_start_xmit(struct sk_buff *skb,
> +                                           struct net_device *dev)
> +{
> +       struct can327 *elm = netdev_priv(dev);
> +       struct can_frame *frame = (struct can_frame *)skb->data;
> +
> +       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
> +        */
> +       spin_lock(&elm->lock);

Do you need to hold the lock here? Isn't it possible to move this line
after the next if so that you do not have to unclock in the error
path?

> +
> +       /* 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) {

Can the xmit function get called when CAN_CTRLMODE_LISTENONLY is on?

> +               spin_unlock(&elm->lock);
> +               goto out;
> +       }
> +
> +       netif_stop_queue(dev);
> +
> +       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:

The benefit of the goto label is to factorize code. If you have only
one goto, you might as well prefer to remove the label and do the
kfree_skb inside the if block.

> +       kfree_skb(skb);

Maybe you want to increment dev->stats here?

> +       return NETDEV_TX_OK;
> +}
> +
> +static const struct net_device_ops can327_netdev_ops = {
> +       .ndo_open       = can327_netdev_open,
> +       .ndo_stop       = can327_netdev_close,
> +       .ndo_start_xmit = can327_netdev_start_xmit,
> +       .ndo_change_mtu = can_change_mtu,
> +};
> +
> +static bool can327_is_valid_rx_char(char 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 == ' ');

Remark: if this function gets called a lot, you might what so create
an lookup array and:

return c < sizeof(can327_is_valid_rx_char_lookup) &&
can327_is_valid_rx_char_lookup[c];

This should be faster.

> +}
> +
> +/* Handle incoming ELM327 ASCII data.
> + * This will not be re-entered while running, but other ldisc
> + * functions may be called in parallel.
> + */
> +static void can327_ldisc_rx(struct tty_struct *tty,
> +                           const unsigned char *cp, const char *fp, int count)
> +{
> +       struct can327 *elm = (struct can327 *)tty->disc_data;
> +
> +       spin_lock_bh(&elm->lock);
> +
> +       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_uart_side_failure(elm);
> +
> +                       goto out;
> +               }
> +
> +               /* Ignore NUL characters, which the PIC microcontroller may
> +                * inadvertently insert due to a known hardware bug.
> +                * 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.
> +                        */
> +                       if (!can327_is_valid_rx_char(*cp)) {
> +                               netdev_err(elm->dev,
> +                                          "Received illegal character %02x.\n",
> +                                          *cp);

Might make sense to put the netdev_err message inside can327_is_valid_rx_char().

> +                               elm327_uart_side_failure(elm);
> +
> +                               goto out;
> +                       }
> +
> +                       elm->rxbuf[elm->rxfill++] = *cp;
> +               }
> +
> +               cp++;
> +       }
> +
> +       if (count >= 0) {
> +               netdev_err(elm->dev, "Receive buffer overflowed. Bad chip or wiring?");
> +
> +               elm327_uart_side_failure(elm);
> +
> +               goto out;
> +       }
> +
> +       elm327_parse_rxbuf(elm);
> +
> +out:
> +       spin_unlock_bh(&elm->lock);

If the out label has a single statement, can be better to just repalce
all the goto out with spin_unlock_bh(&elm->lock);

> +}
> +
> +/* Write out remaining transmit buffer.
> + * Scheduled when TTY is writable.
> + */
> +static void can327_ldisc_tx_worker(struct work_struct *work)
> +{
> +       struct can327 *elm = container_of(work, struct can327, tx_work);
> +       ssize_t written;
> +
> +       if (elm->uart_side_failure)
> +               return;
> +
> +       spin_lock_bh(&elm->lock);
> +
> +       if (elm->txleft) {
> +               written = elm->tty->ops->write(elm->tty, elm->txhead, elm->txleft);
> +               if (written < 0) {
> +                       netdev_err(elm->dev,
> +                                  "Failed to write to tty %s.\n",
> +                                  elm->tty->name);
> +                       elm327_uart_side_failure(elm);
> +                       spin_unlock_bh(&elm->lock);
> +                       return;
> +               }
> +
> +               elm->txleft -= written;
> +               elm->txhead += written;
> +       }
> +
> +       if (!elm->txleft)  {
> +               clear_bit(TTY_DO_WRITE_WAKEUP, &elm->tty->flags);
> +               spin_unlock_bh(&elm->lock);
> +       } else {
> +               spin_unlock_bh(&elm->lock);
> +       }

Remove the else branch and do a common spin_unlock_bh(&elm->lock);

> +}
> +
> +/* Called by the driver when there's room for more data. */
> +static void can327_ldisc_tx_wakeup(struct tty_struct *tty)
> +{
> +       struct can327 *elm = (struct can327 *)tty->disc_data;
> +
> +       schedule_work(&elm->tx_work);
> +}
> +
> +/* ELM327 can only handle bitrates that are integer divisors of 500 kHz,
> + * 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] = {
> +        7812,  7936,  8064,  8196,  8333,  8474,  8620,  8771,
> +        8928,  9090,  9259,  9433,  9615,  9803, 10000, 10204,
> +       10416, 10638, 10869, 11111, 11363, 11627, 11904, 12195,
> +       12500, 12820, 13157, 13513, 13888, 14285, 14705, 15151,
> +       15625, 16129, 16666, 17241, 17857, 18518, 19230, 20000,
> +       20833, 21739, 22727, 23809, 25000, 26315, 27777, 29411,
> +       31250, 33333, 35714, 38461, 41666, 45454, 50000, 55555,
> +       62500, 71428, 83333, 100000, 125000, 166666, 250000, 500000
> +};
> +
> +/* Dummy needed to use bitrate_const */
> +static int can327_do_set_bittiming(struct net_device *netdev)
> +{
> +       return 0;
> +}
> +
> +static int can327_ldisc_open(struct tty_struct *tty)
> +{
> +       struct net_device *dev;
> +       struct can327 *elm;
> +       int err;
> +
> +       if (!capable(CAP_NET_ADMIN))
> +               return -EPERM;
> +
> +       if (!tty->ops->write)
> +               return -EOPNOTSUPP;
> +
> +       dev = alloc_candev(sizeof(struct can327), 0);
> +       if (!dev)
> +               return -ENFILE;
> +       elm = netdev_priv(dev);
> +
> +       /* Configure TTY interface */
> +       tty->receive_room = 65536; /* We don't flow control */
> +       spin_lock_init(&elm->lock);
> +       INIT_WORK(&elm->tx_work, can327_ldisc_tx_worker);
> +
> +       /* Configure CAN metadata */
> +       elm->can.bitrate_const = can327_bitrate_const;
> +       elm->can.bitrate_const_cnt = ARRAY_SIZE(can327_bitrate_const);
> +       elm->can.do_set_bittiming = can327_do_set_bittiming;
> +       elm->can.ctrlmode_supported = CAN_CTRLMODE_LISTENONLY;

Question: did you try to send/receive DLC greater than 8? (c.f.
CAN_CTRLMODE_CC_LEN8_DLC)

> +
> +       /* Configure netdev interface */
> +       elm->dev = dev;
> +       dev->netdev_ops = &can327_netdev_ops;
> +
> +       /* Mark ldisc channel as alive */
> +       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;
> +
> +       netdev_info(elm->dev, "can327 on %s.\n", tty->name);
> +
> +       return 0;
> +
> +out_err:

Don't use the goto/lable for a single case.

> +       free_candev(elm->dev);
> +       return err;
> +}
> +
> +/* Close down a can327 channel.
> + * This means flushing out any pending queues, and then returning.
> + * This call is serialized against other ldisc functions:
> + * Once this is called, no other ldisc function of ours is entered.
> + *
> + * We also use this function for a hangup event.
> + */
> +static void can327_ldisc_close(struct tty_struct *tty)
> +{
> +       struct can327 *elm = (struct can327 *)tty->disc_data;
> +
> +       /* 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);
> +
> +       /* Mark channel as dead */
> +       spin_lock_bh(&elm->lock);
> +       tty->disc_data = NULL;
> +       elm->tty = NULL;
> +       spin_unlock_bh(&elm->lock);
> +
> +       netdev_info(elm->dev, "can327 off %s.\n", tty->name);
> +
> +       free_candev(elm->dev);
> +}
> +
> +static int can327_ldisc_ioctl(struct tty_struct *tty,
> +                             unsigned int cmd, unsigned long arg)
> +{
> +       struct can327 *elm = (struct can327 *)tty->disc_data;
> +       unsigned int tmp;
> +
> +       switch (cmd) {
> +       case SIOCGIFNAME:
> +               tmp = strnlen(elm->dev->name, IFNAMSIZ - 1) + 1;
> +               if (copy_to_user((void __user *)arg, elm->dev->name, tmp))
> +                       return -EFAULT;
> +               return 0;
> +
> +       case SIOCSIFHWADDR:
> +               return -EINVAL;
> +
> +       default:
> +               return tty_mode_ioctl(tty, cmd, arg);
> +       }
> +}
> +
> +static struct tty_ldisc_ops can327_ldisc = {
> +       .owner          = THIS_MODULE,
> +       .name           = "can327",
> +       .num            = N_CAN327,
> +       .receive_buf    = can327_ldisc_rx,
> +       .write_wakeup   = can327_ldisc_tx_wakeup,
> +       .open           = can327_ldisc_open,
> +       .close          = can327_ldisc_close,
> +       .ioctl          = can327_ldisc_ioctl,
> +};
> +
> +static int __init can327_init(void)
> +{
> +       int status;
> +
> +       status = tty_register_ldisc(&can327_ldisc);
> +       if (status)
> +               pr_err("Can't register line discipline\n");
> +
> +       return status;
> +}
> +
> +static void __exit can327_exit(void)
> +{
> +       /* This will only be called when all channels have been closed by
> +        * userspace - tty_ldisc.c takes care of the module's refcount.
> +        */
> +       tty_unregister_ldisc(&can327_ldisc);
> +}
> +
> +module_init(can327_init);
> +module_exit(can327_exit);
> +
> +MODULE_ALIAS_LDISC(N_CAN327);
> +MODULE_DESCRIPTION("ELM327 based CAN interface");
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Max Staudt <max@enpas.org>");
> diff --git a/include/uapi/linux/tty.h b/include/uapi/linux/tty.h
> index 9d0f06bfbac3..68aeae2addec 100644
> --- a/include/uapi/linux/tty.h
> +++ b/include/uapi/linux/tty.h
> @@ -38,8 +38,9 @@
>  #define N_NULL         27      /* Null ldisc used for error handling */
>  #define N_MCTP         28      /* MCTP-over-serial */
>  #define N_DEVELOPMENT  29      /* Manual out-of-tree testing */
> +#define N_CAN327       30      /* ELM327 based OBD-II interfaces */
>
>  /* Always the newest line discipline + 1 */
> -#define NR_LDISCS      30
> +#define NR_LDISCS      31
>
>  #endif /* _UAPI_LINUX_TTY_H */
> --
> 2.30.2
>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v6] can, tty: can327 CAN/ldisc driver for ELM327 based OBD-II adapters
  2022-05-13  2:38 ` Vincent Mailhol
@ 2022-05-13  6:31   ` Vincent Mailhol
  2022-05-13 18:59     ` Max Staudt
  2022-05-13 11:46   ` Marc Kleine-Budde
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 14+ messages in thread
From: Vincent Mailhol @ 2022-05-13  6:31 UTC (permalink / raw)
  To: Max Staudt
  Cc: Wolfgang Grandegger, Marc Kleine-Budde, linux-can,
	Greg Kroah-Hartman, Oliver Neukum, linux-kernel

On Fri. 13 May 2022 at 11:38, Vincent Mailhol <vincent.mailhol@gmail.com> wrote:
[...]
> > +       case ELM327_STATE_RECEIVING:
> > +               /* Find <CR> delimiting feedback lines. */
> > +               for (len = 0;
> > +                    (len < elm->rxfill) && (elm->rxbuf[len] != '\r');
> > +                    len++) {
> > +                       /* empty loop */
>
> Question of taste but would prefer a while look with the len++ in the
> body (if you prefer to do as above, no need to argue, just keep it
> like it is).

Actually, what about this?

len = strnchr(elm->rxbuf, elm->rxfill, '\r');

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v6] can, tty: can327 CAN/ldisc driver for ELM327 based OBD-II adapters
  2022-05-13  2:38 ` Vincent Mailhol
  2022-05-13  6:31   ` Vincent Mailhol
@ 2022-05-13 11:46   ` Marc Kleine-Budde
  2022-05-13 11:52   ` Marc Kleine-Budde
  2022-05-14 11:04   ` Max Staudt
  3 siblings, 0 replies; 14+ messages in thread
From: Marc Kleine-Budde @ 2022-05-13 11:46 UTC (permalink / raw)
  To: Vincent Mailhol
  Cc: Max Staudt, Wolfgang Grandegger, linux-can, Greg Kroah-Hartman,
	Oliver Neukum, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 964 bytes --]

On 13.05.2022 11:38:31, Vincent Mailhol wrote:
> > +static void elm327_parse_line(struct can327 *elm, size_t len)
> > +{
> > +       lockdep_assert_held(&elm->lock);
> 
> You are using many of those lockdep_assert_held(&elm->lock);
> 
> I guess you put them for debug purpose and probaly some of those can
> be removed (if you see a genuine risk at some places, then OK to keep
> as a safety net, but a bit of clean up can be done here, I think).
> 
> This comment applies to all use of lockdep_assert_held().

These statements document that the code must be called with the lock
held. They optimize away if lockdep is not enabled. Better keep them in
place.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v6] can, tty: can327 CAN/ldisc driver for ELM327 based OBD-II adapters
  2022-05-13  2:38 ` Vincent Mailhol
  2022-05-13  6:31   ` Vincent Mailhol
  2022-05-13 11:46   ` Marc Kleine-Budde
@ 2022-05-13 11:52   ` Marc Kleine-Budde
  2022-05-13 12:14     ` Vincent Mailhol
  2022-05-14 11:04   ` Max Staudt
  3 siblings, 1 reply; 14+ messages in thread
From: Marc Kleine-Budde @ 2022-05-13 11:52 UTC (permalink / raw)
  To: Vincent Mailhol
  Cc: Max Staudt, Wolfgang Grandegger, linux-can, Greg Kroah-Hartman,
	Oliver Neukum, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 910 bytes --]

On 13.05.2022 11:38:31, Vincent Mailhol wrote:
> > +
> > +       /* 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) {
> 
> Can the xmit function get called when CAN_CTRLMODE_LISTENONLY is on?

I think yes. You can skip the whole CAN stack by injecting CAN frames in
the kernel via the packet socket. Maybe we should add a check to
can_dropped_invalid_skb().

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v6] can, tty: can327 CAN/ldisc driver for ELM327 based OBD-II adapters
  2022-05-13 11:52   ` Marc Kleine-Budde
@ 2022-05-13 12:14     ` Vincent Mailhol
  0 siblings, 0 replies; 14+ messages in thread
From: Vincent Mailhol @ 2022-05-13 12:14 UTC (permalink / raw)
  To: Marc Kleine-Budde
  Cc: Max Staudt, Wolfgang Grandegger, linux-can, Greg Kroah-Hartman,
	Oliver Neukum, linux-kernel

On Fri 13 May 2022 at 20:52, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 13.05.2022 11:38:31, Vincent Mailhol wrote:
> > > +
> > > +       /* 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) {
> >
> > Can the xmit function get called when CAN_CTRLMODE_LISTENONLY is on?
>
> I think yes. You can skip the whole CAN stack by injecting CAN frames in
> the kernel via the packet socket. Maybe we should add a check to
> can_dropped_invalid_skb().

Ack. Most of the drivers do not check for it (my doesn't). So better
to put it in can_dropped_invalid_skb().
I can do the patch for this.

And also noted for your previous comment on lockdep_assert_held().
@Max: please ignore this particular remark.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v6] can, tty: can327 CAN/ldisc driver for ELM327 based OBD-II adapters
  2022-05-13  6:31   ` Vincent Mailhol
@ 2022-05-13 18:59     ` Max Staudt
  2022-05-14  3:14       ` Vincent Mailhol
  0 siblings, 1 reply; 14+ messages in thread
From: Max Staudt @ 2022-05-13 18:59 UTC (permalink / raw)
  To: Vincent Mailhol
  Cc: Wolfgang Grandegger, Marc Kleine-Budde, linux-can,
	Greg Kroah-Hartman, Oliver Neukum, linux-kernel

On Fri, 13 May 2022 15:31:20 +0900
Vincent Mailhol <vincent.mailhol@gmail.com> wrote:

> On Fri. 13 May 2022 at 11:38, Vincent Mailhol
> <vincent.mailhol@gmail.com> wrote: [...]
> > > +       case ELM327_STATE_RECEIVING:
> > > +               /* Find <CR> delimiting feedback lines. */
> > > +               for (len = 0;
> > > +                    (len < elm->rxfill) && (elm->rxbuf[len] !=
> > > '\r');
> > > +                    len++) {
> > > +                       /* empty loop */  
> >
> > Question of taste but would prefer a while look with the len++ in
> > the body (if you prefer to do as above, no need to argue, just keep
> > it like it is).  
> 
> Actually, what about this?
> 
> len = strnchr(elm->rxbuf, elm->rxfill, '\r');

Actually I'd use memchr() if anything, but not really here. I do end up
using the actual index. And since both strchr() and mrmchr() return
pointers, I'd rather avoid them because I prefer to use indices
whenever possible.


Max

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v6] can, tty: can327 CAN/ldisc driver for ELM327 based OBD-II adapters
  2022-05-13 18:59     ` Max Staudt
@ 2022-05-14  3:14       ` Vincent Mailhol
  2022-05-14 11:11         ` Max Staudt
  0 siblings, 1 reply; 14+ messages in thread
From: Vincent Mailhol @ 2022-05-14  3:14 UTC (permalink / raw)
  To: Max Staudt
  Cc: Wolfgang Grandegger, Marc Kleine-Budde, linux-can,
	Greg Kroah-Hartman, Oliver Neukum, linux-kernel

On Sat. 14 mai 2022 at 03:59, Max Staudt <max@enpas.org> wrote:
> On Fri, 13 May 2022 15:31:20 +0900
> Vincent Mailhol <vincent.mailhol@gmail.com> wrote:
> > On Fri. 13 May 2022 at 11:38, Vincent Mailhol
> > <vincent.mailhol@gmail.com> wrote: [...]
> > > > +       case ELM327_STATE_RECEIVING:
> > > > +               /* Find <CR> delimiting feedback lines. */
> > > > +               for (len = 0;
> > > > +                    (len < elm->rxfill) && (elm->rxbuf[len] !=
> > > > '\r');
> > > > +                    len++) {
> > > > +                       /* empty loop */
> > >
> > > Question of taste but would prefer a while look with the len++ in
> > > the body (if you prefer to do as above, no need to argue, just keep
> > > it like it is).
> >
> > Actually, what about this?
> >
> > len = strnchr(elm->rxbuf, elm->rxfill, '\r');
>
> Actually I'd use memchr() if anything, but not really here. I do end up
> using the actual index. And since both strchr() and mrmchr() return
> pointers, I'd rather avoid them because I prefer to use indices
> whenever possible.

You are right. strnchr()'s result can not be used as is. I was a bit
careless when writing my response.

But I still think it is possible to do pointer arithmetic.

len = strnchr(elm->rxbuf, elm->rxfill, '\r') - elm->rxbuf;
(I let you check that I did not do an off by one mistake).

The above should also work with memchr(). Although the C standard
doesn't allow pointer arithmetic on void *, GNU C adds an extension
for that: https://gcc.gnu.org/onlinedocs/gcc/Pointer-Arith.html

As I said before, your for loop is not fundamentally wrong, this is
just not my prefered approach. You have my suggestion, choose what you
prefer.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v6] can, tty: can327 CAN/ldisc driver for ELM327 based OBD-II adapters
  2022-05-13  2:38 ` Vincent Mailhol
                     ` (2 preceding siblings ...)
  2022-05-13 11:52   ` Marc Kleine-Budde
@ 2022-05-14 11:04   ` Max Staudt
  2022-05-14 12:10     ` Vincent Mailhol
  3 siblings, 1 reply; 14+ messages in thread
From: Max Staudt @ 2022-05-14 11:04 UTC (permalink / raw)
  To: Vincent Mailhol
  Cc: Wolfgang Grandegger, Marc Kleine-Budde, linux-can,
	Greg Kroah-Hartman, Oliver Neukum, linux-kernel

On Fri, 13 May 2022 11:38:31 +0900
Vincent Mailhol <vincent.mailhol@gmail.com> wrote:

> Nice improvement since v3, thanks! Here are my new comments.

Well, thanks for and thanks to your review!

Replies below, a few style things (branching/goto) omitted.


> > +/* Bits in elm->cmds_todo */
> > +enum elm327_to_to_do_bits {  
> 
> to_to_do? Name looks like a typo. Also, do not see the value on the
> _bits suffix. I suggest:
> 
> enum elm327_tx_do {

Indeed, a typo! It should have been elm327_to_to_do_bits. It's named
*_bits because the elem's indices are used to index bits in cmds_todo
in struct can327.

I guess your suggestion is nice, because it fits with the named indices.


> > +struct can327 {
> > +       /* This must be the first member when using alloc_candev()
> > */
> > +       struct can_priv can;
> > +
> > +       struct can_rx_offload offload;
> > +
> > +       /* TTY buffers */
> > +       u8 rxbuf[ELM327_SIZE_RXBUF];
> > +       u8 txbuf[ELM327_SIZE_TXBUF] ____cacheline_aligned;  
> 
> Out of curiosity, any rationale for the use of ____cacheline_aligned
> here?

Actually, I've checked now and it seems to not be necessary at all.

This was originally a kmalloc()'d buffer, in order to pass aligned
memory to tty->ops->write(). However, there are other (though few)
ldiscs that pass unaligned memory, and there aren't any checks in
serdev either, so I'll drop the forced alignment.


> > +static void elm327_kick_into_cmd_mode(struct can327 *elm)
> > +{
> > [...]
> 
> (frame->can_id & CAN_EFF_FLAG) ^ (elm->can_frame_to_send.can_id &
> CAN_EFF_FLAG)
> 
> can be factorized as:
> 
> (frame->can_id ^ elm->can_frame_to_send.can_id) & CAN_EFF_FLAG

Yes :)


> > +/* 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);  
> 
> What about:
> strncmp("CAN ER", "CAN ERROR", 7);
> ?

NAK, because this may overread the buffer by one byte (the NUL byte).
I am comparing naked bytes, not NUL-terminated strings.


> > +static void elm327_parse_error(struct can327 *elm, size_t len)
> > +{
> > +       struct can_frame *frame;
> > +       struct sk_buff *skb;
> > +
> > +       lockdep_assert_held(&elm->lock);
> > +
> > +       skb = alloc_can_err_skb(elm->dev, &frame);
> > +       if (!skb)
> > +               /* It's okay to return here:
> > +                * The outer parsing loop will drop this UART
> > buffer.
> > +                */
> > +               return;
> > +
> > +       /* Filter possible error messages based on length of RX'd
> > line */
> > +       if (check_len_then_cmp(elm->rxbuf, len, "UNABLE TO
> > CONNECT")) {  
> 
> Is this equivalent?
>       if (!strncmp(elm->rxbuf, "UNABLE TO CONNECT", len + 1)) {

No. We can use (len) bytes in elm->rxbuf, not (len+1).

There are no C strings in this buffer. There are no NUL chars in this
buffer. There are no trailing bytes we can overread into.


> > +static void elm327_handle_prompt(struct can327 *elm)
> > +{
> > +       struct can_frame *frame = &elm->can_frame_to_send;
> > +       /* 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);
> > +
> > +       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);
> > +
> > +               elm->next_init_cmd++;
> > +               if (!(*elm->next_init_cmd)) {
> > +                       clear_bit(ELM327_TX_DO_INIT,
> > &elm->cmds_todo);
> > +                       /* Init finished. */
> > +               }
> > +
> > +       } 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)));  
> 
> The second pair of brackets look superficial:
>                snprintf(local_txbuf, sizeof(local_txbuf),
>                         "ATCSM%i\r",
>                         !!(elm->can.ctrlmode &
> CAN_CTRLMODE_LISTENONLY));

True, thanks for catching this.


> > +static void elm327_parse_rxbuf(struct can327 *elm)
> > +{
> > +       size_t len;
> > +       int i;
> > +
> > +       lockdep_assert_held(&elm->lock);
> > +
> > +       switch (elm->state) {
> > +       case ELM327_STATE_NOTINIT:
> > +               elm->rxfill = 0;
> > +               break;
> > +
> > +       case ELM327_STATE_GETDUMMYCHAR:
> > +       {  
> 
> Is this braket need?

No it's not, it's a leftover from before moving "int i;" to the
function head.


> > +
> > +       case ELM327_STATE_GETPROMPT:
> > +               /* Wait for '>' */
> > +               if (elm327_is_ready_char(elm->rxbuf[elm->rxfill -
> > 1]))
> > +                       elm327_handle_prompt(elm);
> > +
> > +               elm->rxfill = 0;
> > +               break;
> > +
> > +       case ELM327_STATE_RECEIVING:
> > +               /* Find <CR> delimiting feedback lines. */
> > +               for (len = 0;
> > +                    (len < elm->rxfill) && (elm->rxbuf[len] !=
> > '\r');
> > +                    len++) {
> > +                       /* empty loop */  
> 
> Question of taste but would prefer a while look with the len++ in the
> body (if you prever to do as above, no need to argue, just keep it
> like it is).

Good point, I think a while() would be easier on the eyes indeed.


> 
> > +               }
> > +
> > +               if (len == ELM327_SIZE_RXBUF) {
> > +                       /* Line exceeds buffer. It's probably all
> > 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;  
> 
> Can you have just a single break at the end of the case instead of
> having it in every branches of the if/else?

Agreed, the status quo is ugly. I'll look into it.


> > +/* Send a can_frame to a TTY. */
> > +static netdev_tx_t can327_netdev_start_xmit(struct sk_buff *skb,
> > +                                           struct net_device *dev)
> > +{
> > +       struct can327 *elm = netdev_priv(dev);
> > +       struct can_frame *frame = (struct can_frame *)skb->data;
> > +
> > +       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
> > +        */
> > +       spin_lock(&elm->lock);  
> 
> Do you need to hold the lock here? Isn't it possible to move this line
> after the next if so that you do not have to unclock in the error
> path?

It is possible.

Even better: I forgot to remove the elm->tty check as part of PATCHv4,
and elm->uart_side_failure cannot be unset while the netdev is up. So
we can push the locking way further down after any remaining checks :)


> > +               spin_unlock(&elm->lock);
> > +               goto out;
> > +       }
> > +
> > +       netif_stop_queue(dev);
> > +
> > +       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:  
> 
> The benefit of the goto label is to factorize code. If you have only
> one goto, you might as well prefer to remove the label and do the
> kfree_skb inside the if block.

But then I also have to duplicate the return... matter of taste :)


> > +       kfree_skb(skb);  
> 
> Maybe you want to increment dev->stats here?

This already happens above, but only in case the frame is actually
queued on the UART.


> > +       return NETDEV_TX_OK;
> > +}
> > +
> > +static const struct net_device_ops can327_netdev_ops = {
> > +       .ndo_open       = can327_netdev_open,
> > +       .ndo_stop       = can327_netdev_close,
> > +       .ndo_start_xmit = can327_netdev_start_xmit,
> > +       .ndo_change_mtu = can_change_mtu,
> > +};
> > +
> > +static bool can327_is_valid_rx_char(char 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 == ' ');  
> 
> Remark: if this function gets called a lot, you might what so create
> an lookup array and:
> 
> return c < sizeof(can327_is_valid_rx_char_lookup) &&
> can327_is_valid_rx_char_lookup[c];
> 
> This should be faster.

It does get called for every byte RX'd via UART. I'll have a look at
your LUT idea, it does sound good :)


> > +                       /* Check for stray characters on the UART
> > line.
> > +                        * Likely caused by bad hardware.
> > +                        */
> > +                       if (!can327_is_valid_rx_char(*cp)) {
> > +                               netdev_err(elm->dev,
> > +                                          "Received illegal
> > character %02x.\n",
> > +                                          *cp);  
> 
> Might make sense to put the netdev_err message inside
> can327_is_valid_rx_char().

Sorry, I prefer to keep can327_is_valid_rx_char() purely functional,
and the side effects in this function here. :)


> > +                               elm327_uart_side_failure(elm);
> > +
> > +                               goto out;
> > +                       }
> > +
> > +                       elm->rxbuf[elm->rxfill++] = *cp;
> > +               }
> > +
> > +               cp++;
> > +       }
> > +
> > +       if (count >= 0) {
> > +               netdev_err(elm->dev, "Receive buffer overflowed.
> > Bad chip or wiring?"); +
> > +               elm327_uart_side_failure(elm);
> > +
> > +               goto out;
> > +       }
> > +
> > +       elm327_parse_rxbuf(elm);
> > +
> > +out:
> > +       spin_unlock_bh(&elm->lock);  
> 
> If the out label has a single statement, can be better to just repalce
> all the goto out with spin_unlock_bh(&elm->lock);

It would be spin_unlock_bh() plus return...
Yeah, I guess it's easier to read.


> Question: did you try to send/receive DLC greater than 8? (c.f.
> CAN_CTRLMODE_CC_LEN8_DLC)

Yes, I have :)

It is entirely unsupported by the hardware:
 - On RX, DLC > 8 is reported as 8.
 - On TX, the DLC is constructed by the ELM327 depending on the
   payload, so DLC > 8 is impossible.



Thanks for your review!

Max

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v6] can, tty: can327 CAN/ldisc driver for ELM327 based OBD-II adapters
  2022-05-14  3:14       ` Vincent Mailhol
@ 2022-05-14 11:11         ` Max Staudt
  2022-05-14 12:24           ` Vincent Mailhol
  0 siblings, 1 reply; 14+ messages in thread
From: Max Staudt @ 2022-05-14 11:11 UTC (permalink / raw)
  To: Vincent Mailhol
  Cc: Wolfgang Grandegger, Marc Kleine-Budde, linux-can,
	Greg Kroah-Hartman, Oliver Neukum, linux-kernel

On Sat, 14 May 2022 12:14:24 +0900
Vincent Mailhol <vincent.mailhol@gmail.com> wrote:

> But I still think it is possible to do pointer arithmetic.
> 
> len = strnchr(elm->rxbuf, elm->rxfill, '\r') - elm->rxbuf;
> (I let you check that I did not do an off by one mistake).
> 
> The above should also work with memchr(). Although the C standard
> doesn't allow pointer arithmetic on void *, GNU C adds an extension
> for that: https://gcc.gnu.org/onlinedocs/gcc/Pointer-Arith.html
> 
> As I said before, your for loop is not fundamentally wrong, this is
> just not my prefered approach. You have my suggestion, choose what you
> prefer.

Yeah, this is the arithmetic that I'd like to avoid here. In my
opinion, it is clearer with indices.

If I were searching through a UTF-8 string (i.e. with variable width
chars), that'd be another matter entirely IMHO, and I'd rely on C
library functions that know more about UTF that I do. But it's really
just naked ASCII bytes this time.


...unless memchr() may be faster than the loop? Could this happen?



Max

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v6] can, tty: can327 CAN/ldisc driver for ELM327 based OBD-II adapters
  2022-05-14 11:04   ` Max Staudt
@ 2022-05-14 12:10     ` Vincent Mailhol
  0 siblings, 0 replies; 14+ messages in thread
From: Vincent Mailhol @ 2022-05-14 12:10 UTC (permalink / raw)
  To: Max Staudt
  Cc: Wolfgang Grandegger, Marc Kleine-Budde, linux-can,
	Greg Kroah-Hartman, Oliver Neukum, linux-kernel

On Fri. 14 May 2022 at 20:04, Max Staudt <max@enpas.org> wrote:
> On Fri, 13 May 2022 11:38:31 +0900
> Vincent Mailhol <vincent.mailhol@gmail.com> wrote:
> > > +/* 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);
> >
> > What about:
> > strncmp("CAN ER", "CAN ERROR", 7);
> > ?
>
> NAK, because this may overread the buffer by one byte (the NUL byte).
> I am comparing naked bytes, not NUL-terminated strings.

Right, I missed the fact that the first argument was not Null terminated.
Your example is misleading: "CAN ER" is a string literal which is NULL
terminated, it doesn't reflect the use case.

My suggestion: first rename the function to reflect the feature it
brings. For example: elm327_rxbuff_cmp(). Naming it as if it was a
library is also misleading.

Also make it return bool.

For example, I would write it like in this fashion:

/*
 * elm327_rxfuff_cmp - compare received buffer against expected error message
 * @rxbuff: received buffer. Not NUL-terminated.
 * @rxbuff_len: size of @rxbuff.
 * @err_msg: error message against which we compare. Must be NUL-terminated.
 *
 * This function flags cases where the @rxbuff is only the start of @err_msg
 *  rather than exactly all of it.
 */
static inline bool elm327_rxbuff_cmp(const u8 *rxbuff, size_t
rxbuff_len, const char *err_msg)
{
       size_t err_len = strlen(err_msg);

       return (rxbuff_len == err_len) && !memcmp(rxbuff, err_msg, rxbuff_len);
}

Naming the arguments instead of using generic terms such as buffer and
string make it easier to follow what you are doing. The important
message is that @rxbuff is not terminated. With this information, it
becomes clear that the string functions can not be used in
replacement, no need to explicitly tell it.

[...]

Other answers are OK.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v6] can, tty: can327 CAN/ldisc driver for ELM327 based OBD-II adapters
  2022-05-14 11:11         ` Max Staudt
@ 2022-05-14 12:24           ` Vincent Mailhol
  0 siblings, 0 replies; 14+ messages in thread
From: Vincent Mailhol @ 2022-05-14 12:24 UTC (permalink / raw)
  To: Max Staudt
  Cc: Wolfgang Grandegger, Marc Kleine-Budde, linux-can,
	Greg Kroah-Hartman, Oliver Neukum, linux-kernel

On Tue. 14 May 2022 à 20:11, Max Staudt <max@enpas.org> wrote:
> On Sat, 14 May 2022 12:14:24 +0900
> Vincent Mailhol <vincent.mailhol@gmail.com> wrote:
>
> > But I still think it is possible to do pointer arithmetic.
> >
> > len = strnchr(elm->rxbuf, elm->rxfill, '\r') - elm->rxbuf;
> > (I let you check that I did not do an off by one mistake).
> >
> > The above should also work with memchr(). Although the C standard
> > doesn't allow pointer arithmetic on void *, GNU C adds an extension
> > for that: https://gcc.gnu.org/onlinedocs/gcc/Pointer-Arith.html
> >
> > As I said before, your for loop is not fundamentally wrong, this is
> > just not my prefered approach. You have my suggestion, choose what you
> > prefer.
>
> Yeah, this is the arithmetic that I'd like to avoid here. In my
> opinion, it is clearer with indices.
>
> If I were searching through a UTF-8 string (i.e. with variable width
> chars), that'd be another matter entirely IMHO, and I'd rely on C
> library functions that know more about UTF that I do. But it's really
> just naked ASCII bytes this time.
>
>
> ...unless memchr() may be faster than the loop? Could this happen?

It depends on many factors, the length of the string, the architecture
on which you compile it, whether the compiler decides to inline it or
not…

For long strings, there are some tricks to scan through the memory
using 64 bits operation instead of doing it bytes per bytes. But there
is a preparation overhead which makes it not worth it for small
buffers.

You can look at glibc implementation for reference:
https://github.com/lattera/glibc/blob/master/string/memchr.c

Of course, the kernel does not use glibc but often compilers
__builtin_memchr() instead (one more time, it depends on the
architecture). But it will give you an idea of why memchr() can be
faster than the loop.

Finally, the compiler might also detect your loop and take the
initiative to replace it with a call to memchr(). You have to check
the assembly to see if this is the case.

For a device which speaks on UART which by nature is slow,
optimizations are not critical, so readability is the priority, and is
why I prefer using the library functions.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v6] can, tty: can327 CAN/ldisc driver for ELM327 based OBD-II adapters
  2022-05-12 18:29 [PATCH v6] can, tty: can327 CAN/ldisc driver for ELM327 based OBD-II adapters Max Staudt
  2022-05-13  2:38 ` Vincent Mailhol
@ 2022-05-18 16:24 ` Vincent Mailhol
  2022-05-18 16:31   ` Vincent Mailhol
  1 sibling, 1 reply; 14+ messages in thread
From: Vincent Mailhol @ 2022-05-18 16:24 UTC (permalink / raw)
  To: Max Staudt
  Cc: Wolfgang Grandegger, Marc Kleine-Budde, linux-can,
	Greg Kroah-Hartman, Oliver Neukum, linux-kernel

Forgot one comment.

On Fri 13 May 2022 at 03:29, Max Staudt <max@enpas.org> wrote:
[...]
> +/* Send a can_frame to a TTY. */
> +static netdev_tx_t can327_netdev_start_xmit(struct sk_buff *skb,
> +                                           struct net_device *dev)
> +{
> +       struct can327 *elm = netdev_priv(dev);
> +       struct can_frame *frame = (struct can_frame *)skb->data;
> +
> +       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
> +        */
> +       spin_lock(&elm->lock);
> +
> +       /* 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);
> +               goto out;
> +       }
> +
> +       netif_stop_queue(dev);
> +
> +       elm327_send_frame(elm, frame);
> +       spin_unlock(&elm->lock);
> +
> +       dev->stats.tx_packets++;
> +       dev->stats.tx_bytes += frame->len;

Do not increase tx_bytes for RTR frame. c.f.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=cc4b08c31b5c51352f258032cc65e884b3e61e6a

Also, when is the frame freed? Did you double check there is no race
condition resulting in a use after free on frame->len? Similar to:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=03f16c5075b22c8902d2af739969e878b0879c94

> +       can_led_event(dev, CAN_LED_EVENT_TX);

Please adjust according to Oliver's patch.

> +out:
> +       kfree_skb(skb);
> +       return NETDEV_TX_OK;
> +}
> +
> +static const struct net_device_ops can327_netdev_ops = {
> +       .ndo_open       = can327_netdev_open,
> +       .ndo_stop       = can327_netdev_close,
> +       .ndo_start_xmit = can327_netdev_start_xmit,
> +       .ndo_change_mtu = can_change_mtu,
> +};
[...]

Yours sincerely,
Vincent Mailhol

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v6] can, tty: can327 CAN/ldisc driver for ELM327 based OBD-II adapters
  2022-05-18 16:24 ` Vincent Mailhol
@ 2022-05-18 16:31   ` Vincent Mailhol
  0 siblings, 0 replies; 14+ messages in thread
From: Vincent Mailhol @ 2022-05-18 16:31 UTC (permalink / raw)
  To: Max Staudt
  Cc: Wolfgang Grandegger, Marc Kleine-Budde, linux-can,
	Greg Kroah-Hartman, Oliver Neukum, linux-kernel

On Thu. 19 May 2022 at 01:24, Vincent Mailhol <vincent.mailhol@gmail.com> wrote:
> Forgot one comment.
>
> On Fri 13 May 2022 at 03:29, Max Staudt <max@enpas.org> wrote:
> [...]
> > +/* Send a can_frame to a TTY. */
> > +static netdev_tx_t can327_netdev_start_xmit(struct sk_buff *skb,
> > +                                           struct net_device *dev)
> > +{
> > +       struct can327 *elm = netdev_priv(dev);
> > +       struct can_frame *frame = (struct can_frame *)skb->data;
> > +
> > +       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
> > +        */
> > +       spin_lock(&elm->lock);
> > +
> > +       /* 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);
> > +               goto out;
> > +       }
> > +
> > +       netif_stop_queue(dev);
> > +
> > +       elm327_send_frame(elm, frame);
> > +       spin_unlock(&elm->lock);
> > +
> > +       dev->stats.tx_packets++;
> > +       dev->stats.tx_bytes += frame->len;
>
> Do not increase tx_bytes for RTR frame. c.f.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=cc4b08c31b5c51352f258032cc65e884b3e61e6a
>
> Also, when is the frame freed? Did you double check there is no race
> condition resulting in a use after free on frame->len? Similar to:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=03f16c5075b22c8902d2af739969e878b0879c94

Never mind, the free is just two lines below inside the out label.
Sorry for the noise.

> > +       can_led_event(dev, CAN_LED_EVENT_TX);
>
> Please adjust according to Oliver's patch.
>
> > +out:
> > +       kfree_skb(skb);
> > +       return NETDEV_TX_OK;
> > +}
> > +
> > +static const struct net_device_ops can327_netdev_ops = {
> > +       .ndo_open       = can327_netdev_open,
> > +       .ndo_stop       = can327_netdev_close,
> > +       .ndo_start_xmit = can327_netdev_start_xmit,
> > +       .ndo_change_mtu = can_change_mtu,
> > +};
> [...]
>
> Yours sincerely,
> Vincent Mailhol

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2022-05-18 16:32 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-12 18:29 [PATCH v6] can, tty: can327 CAN/ldisc driver for ELM327 based OBD-II adapters Max Staudt
2022-05-13  2:38 ` Vincent Mailhol
2022-05-13  6:31   ` Vincent Mailhol
2022-05-13 18:59     ` Max Staudt
2022-05-14  3:14       ` Vincent Mailhol
2022-05-14 11:11         ` Max Staudt
2022-05-14 12:24           ` Vincent Mailhol
2022-05-13 11:46   ` Marc Kleine-Budde
2022-05-13 11:52   ` Marc Kleine-Budde
2022-05-13 12:14     ` Vincent Mailhol
2022-05-14 11:04   ` Max Staudt
2022-05-14 12:10     ` Vincent Mailhol
2022-05-18 16:24 ` Vincent Mailhol
2022-05-18 16:31   ` Vincent Mailhol

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).