mbox series

[v3,00/11] LIN Bus support for Linux

Message ID 20240502182804.145926-1-christoph.fritz@hexdev.de
Headers show
Series LIN Bus support for Linux | expand

Message

Christoph Fritz May 2, 2024, 6:27 p.m. UTC
This series is introducing basic Local Interconnect Network (LIN) (ISO
17987) [0] support to the Linux kernel, along with two drivers that make
use of it: An advanced USB adapter and a lightweight serdev driver (for
UARTs equipped with a LIN transceiver).

The LIN bus is common in the automotive industry for connecting
low-level devices like side mirrors, seats, ambient lights, etc.

The LIN bus is a lower-cost bus system with a subset of features of CAN.
Its earlier specification (before ISO) is publicly accessible [1].

This series of patches follows up on a discussion initiated by an RFC
patch series [2].

The core of this series is the first patch, which implements the CAN_LIN
glue driver. It basically utilizes the CAN interface on one side and
for device drivers on the other side it creates a rx() function and
several callbacks.

This approach is non-invasive, as LIN frames (nearly identical to CAN
frames) are just treated as a special case of CAN frames. This approach
eliminates the need for a separate API for LIN, allowing the use of
existing CAN tools, including the CAN broadcast manager.

For the responder part of LIN, when a device responds to a controller
request, it can reply on up to LIN its 64 possible IDs (0...63) with a
maximum of 8 bytes payload.  The response must be sent relatively
quickly, so offloading is used (which is used by most devices anyway).
Devices that do not support offloading (like the lightweight serdev)
handle the list of responses in the driver on a best-effort basis.

The CAN broadcast manager (bcm) makes a good interface for the LIN
userland interface, bcm is therefore enhanced to handle the
configuration of these offload RX frames, so that the device can handle
the response on its own.  As a basic alternative, a sysfs file per LIN
identifier gets also introduced.

The USB device driver for the hexLIN [3] adapter uses the HID protocol
and is located in the drivers/hid directory. Which is a bit uncommon for
a CAN device, but this is a LIN device and mainly a hid driver.

The other driver, the UART lin-serdev driver requires support for break
detection, this is addressed by two serdev patches.

The lin-serdev driver has been tested on an ARM SoC, on its uart
(uart-pl011) an adapter board (hexLIN-tty [4]) has been used.  As a
sidenote, in that tty serial driver (amba-pl011.c) it was necessary to
disable DMA_ENGINE to accurately detect breaks [5].

The functions for generating LIN-Breaks and checksums, originally from
a line discipline driver named sllin [6], have been adopted into the
lin-serdev driver.

To make use of the LIN mode configuration (commander or responder)
option, a patch for iproute2 [7] has been made.

The lin-utils [8] provide userland tools for reference, testing, and
evaluation. These utilities are currently separate but may potentially
be integrated into can-utils in the future.

[0]: https://en.wikipedia.org/wiki/Local_Interconnect_Network
[1]: https://www.lin-cia.org/fileadmin/microsites/lin-cia.org/resources/documents/LIN_2.2A.pdf
[2]: https://lwn.net/Articles/916049/
[3]: https://hexdev.de/hexlin
[4]: https://hexdev.de/hexlin#tty
[5]: https://github.com/raspberrypi/linux/issues/5985
[6]: https://github.com/lin-bus/linux-lin/blob/master/sllin/sllin.c
[7]: https://github.com/ch-f/iproute2/tree/lin-feature
[8]: https://github.com/ch-f/lin-utils

Changes in v3:
 - drop unneccessary mutex_lock/unlock and _destroy() in hexlin
 - add Kconfig depends for HID_MCS_HEXDEV
 - simplify reset retry in hexlin
 - simplify hid driver init
 - fix dt-bindings and its commit message
 - adapt and reorder includes
 - use unsigned long instead of ulong
 - use macro USEC_PER_SEC in linser_derive_timings()
 - drop goto in linser_tx_frame_as_responder() and use unlock+return
 - simplify linser_pop_fifo() by using kfifo_skip()
 - squash [PATCH v2 08/12] can: lin: Add special frame id for rx...

Changes in v2:
 - add open/stop functions to also address teardown issues
 - adapt dt-bindings description and add hexdev
 - use 'unsigned int' instead of 'uint'
 - add and adapt macros
 - address review comments

Christoph Fritz (11):
  can: Add LIN bus as CAN abstraction
  HID: hexLIN: Add support for USB LIN bus adapter
  tty: serdev: Add flag buffer aware receive_buf_fp()
  tty: serdev: Add method to enable break flags
  dt-bindings: vendor-prefixes: Add hexDEV
  dt-bindings: net/can: Add serial (serdev) LIN adapter
  can: Add support for serdev LIN adapters
  can: bcm: Add LIN answer offloading for responder mode
  can: lin: Handle rx offload config frames
  can: lin: Support setting LIN mode
  HID: hexLIN: Implement ability to update lin mode

 .../bindings/net/can/hexdev,lin-serdev.yaml   |  32 +
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 drivers/hid/Kconfig                           |  19 +
 drivers/hid/Makefile                          |   1 +
 drivers/hid/hid-hexdev-hexlin.c               | 622 ++++++++++++++++++
 drivers/hid/hid-ids.h                         |   1 +
 drivers/hid/hid-quirks.c                      |   3 +
 drivers/net/can/Kconfig                       |  26 +
 drivers/net/can/Makefile                      |   2 +
 drivers/net/can/lin-serdev.c                  | 500 ++++++++++++++
 drivers/net/can/lin.c                         | 562 ++++++++++++++++
 drivers/tty/serdev/core.c                     |  11 +
 drivers/tty/serdev/serdev-ttyport.c           |  19 +-
 include/linux/serdev.h                        |  19 +-
 include/net/lin.h                             |  99 +++
 include/uapi/linux/can/bcm.h                  |   5 +-
 include/uapi/linux/can/netlink.h              |   2 +
 net/can/bcm.c                                 |  74 ++-
 18 files changed, 1993 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/can/hexdev,lin-serdev.yaml
 create mode 100644 drivers/hid/hid-hexdev-hexlin.c
 create mode 100644 drivers/net/can/lin-serdev.c
 create mode 100644 drivers/net/can/lin.c
 create mode 100644 include/net/lin.h

Comments

Ilpo Järvinen May 6, 2024, 4:24 p.m. UTC | #1
On Thu, 2 May 2024, Christoph Fritz wrote:

> This patch adds a LIN (local interconnect network) bus abstraction on
> top of CAN.  It is a glue driver adapting CAN on one side while offering
> LIN abstraction on the other side. So that upcoming LIN device drivers
> can make use of it.
> 
> Tested-by: Andreas Lauser <andreas.lauser@mercedes-benz.com>
> Signed-off-by: Christoph Fritz <christoph.fritz@hexdev.de>
> ---
>  drivers/net/can/Kconfig          |  10 +
>  drivers/net/can/Makefile         |   1 +
>  drivers/net/can/lin.c            | 495 +++++++++++++++++++++++++++++++
>  include/net/lin.h                |  92 ++++++
>  include/uapi/linux/can/netlink.h |   1 +
>  5 files changed, 599 insertions(+)
>  create mode 100644 drivers/net/can/lin.c
>  create mode 100644 include/net/lin.h
> 
> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index 2e31db55d9278..0934bbf8d03b2 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -171,6 +171,16 @@ config CAN_KVASER_PCIEFD
>  	    Kvaser M.2 PCIe 4xCAN
>  	    Kvaser PCIe 8xCAN
>  
> +config CAN_LIN
> +	tristate "LIN mode support"
> +	help
> +	  This is a glue driver for LIN-BUS support.
> +
> +	  The local interconnect (LIN) bus is a simple bus with a feature
> +	  subset of CAN. It is often combined with CAN for simple controls.
> +
> +	  Actual device drivers need to be enabled too.
> +
>  config CAN_SLCAN
>  	tristate "Serial / USB serial CAN Adaptors (slcan)"
>  	depends on TTY
> diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
> index 4669cd51e7bf5..0093ee9219ca8 100644
> --- a/drivers/net/can/Makefile
> +++ b/drivers/net/can/Makefile
> @@ -25,6 +25,7 @@ obj-$(CONFIG_CAN_GRCAN)		+= grcan.o
>  obj-$(CONFIG_CAN_IFI_CANFD)	+= ifi_canfd/
>  obj-$(CONFIG_CAN_JANZ_ICAN3)	+= janz-ican3.o
>  obj-$(CONFIG_CAN_KVASER_PCIEFD)	+= kvaser_pciefd.o
> +obj-$(CONFIG_CAN_LIN)		+= lin.o
>  obj-$(CONFIG_CAN_MSCAN)		+= mscan/
>  obj-$(CONFIG_CAN_M_CAN)		+= m_can/
>  obj-$(CONFIG_CAN_PEAK_PCIEFD)	+= peak_canfd/
> diff --git a/drivers/net/can/lin.c b/drivers/net/can/lin.c
> new file mode 100644
> index 0000000000000..95906003666fb
> --- /dev/null
> +++ b/drivers/net/can/lin.c
> @@ -0,0 +1,495 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/* Copyright (C) 2024 hexDEV GmbH - https://hexdev.de */
> +
> +#include <linux/can/core.h>
> +#include <linux/can/dev.h>
> +#include <linux/can/error.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/netdevice.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <net/lin.h>
> +
> +static const u8 lin_id_parity_tbl[] = {
> +	0x80, 0xC0, 0x40, 0x00, 0xC0, 0x80, 0x00, 0x40,
> +	0x00, 0x40, 0xC0, 0x80, 0x40, 0x00, 0x80, 0xC0,
> +	0x40, 0x00, 0x80, 0xC0, 0x00, 0x40, 0xC0, 0x80,
> +	0xC0, 0x80, 0x00, 0x40, 0x80, 0xC0, 0x40, 0x00,
> +	0x00, 0x40, 0xC0, 0x80, 0x40, 0x00, 0x80, 0xC0,
> +	0x80, 0xC0, 0x40, 0x00, 0xC0, 0x80, 0x00, 0x40,
> +	0xC0, 0x80, 0x00, 0x40, 0x80, 0xC0, 0x40, 0x00,
> +	0x40, 0x00, 0x80, 0xC0, 0x00, 0x40, 0xC0, 0x80,
> +};
> +
> +u8 lin_get_id_parity(u8 id)
> +{
> +	return lin_id_parity_tbl[id];
> +}
> +EXPORT_SYMBOL(lin_get_id_parity);
> +
> +static ssize_t lin_identifier_show(struct kobject *kobj,
> +				   struct kobj_attribute *attr, char *buf)
> +{
> +	struct lin_attr *lin_attr = container_of(attr, struct lin_attr, attr);
> +	struct lin_device *ldev = lin_attr->ldev;
> +	ssize_t count = 0;
> +	struct lin_responder_answer answ;
> +	int k, ret;
> +	long id;
> +
> +	if (!ldev->ldev_ops->get_responder_answer)
> +		return -EOPNOTSUPP;
> +
> +	ret = kstrtol(attr->attr.name, 16, &id);
> +	if (ret)
> +		return ret;
> +	if (id < 0 || id >= LIN_NUM_IDS)
> +		return -EINVAL;
> +
> +	count += scnprintf(buf + count, PAGE_SIZE - count,
> +			   "%-6s %-11s %-9s %-9s %-2s %-24s %-6s\n",
> +			   "state", "cksum-mode", "is_event", "event_id",
> +			   "n", "data", "cksum");

Onl use sysfs_emit() and sysfs_emit_at() in *_show functions.

> +
> +	ret = ldev->ldev_ops->get_responder_answer(ldev, id, &answ);
> +	if (ret)
> +		return ret;
> +
> +	count += scnprintf(buf + count, PAGE_SIZE - count,
> +			   "%-6s %-11s %-9s %-9u %-2u ",
> +			   answ.is_active ? "active" : "off",
> +			   answ.lf.checksum_mode ? "enhanced" : "classic",
> +			   answ.is_event_frame ? "yes" : "no",
> +			   answ.event_associated_id,
> +			   answ.lf.len);
> +
> +	for (k = 0; k < answ.lf.len; k++)
> +		count += scnprintf(buf + count, PAGE_SIZE - count,
> +				   "%02x ", answ.lf.data[k]);
> +	for (; k < 8; k++)
> +		count += scnprintf(buf + count, PAGE_SIZE - count,
> +				   "   ");
> +	if (answ.lf.len)
> +		count += scnprintf(buf + count, PAGE_SIZE - count,
> +				   " %02x", answ.lf.checksum);
> +
> +	count += scnprintf(buf + count, PAGE_SIZE - count, "\n");
> +
> +	return count;
> +}
> +
> +static const char *parse_and_advance(const char *buf, long *result,
> +				     unsigned int base)
> +{
> +	char num_str[5] = {0};
> +	int num_len = 0;
> +
> +	while (*buf && isspace(*buf))
> +		buf++;
> +	while (*buf && isalnum(*buf) && num_len < sizeof(num_str) - 1)
> +		num_str[num_len++] = *buf++;
> +	if (kstrtol(num_str, base, result))
> +		return NULL;
> +
> +	return buf;
> +}
> +
> +static ssize_t lin_identifier_store(struct kobject *kobj,
> +				    struct kobj_attribute *attr,
> +				    const char *buf, size_t count)
> +{
> +	struct lin_attr *lin_attr = container_of(attr, struct lin_attr, attr);
> +	struct lin_device *ldev = lin_attr->ldev;
> +	struct lin_responder_answer answ = { 0 };
> +	const char *ptr = buf;
> +	int ret;
> +	long v;
> +
> +	if (!ldev->ldev_ops->update_responder_answer)
> +		return -EOPNOTSUPP;
> +
> +	ret = kstrtol(attr->attr.name, 16, &v);
> +	if (ret)
> +		return ret;
> +	if (v < 0 || v >= LIN_NUM_IDS)
> +		return -EINVAL;
> +	answ.lf.lin_id = v;
> +
> +	ptr = parse_and_advance(ptr, &v, 2);
> +	if (!ptr)
> +		return -EINVAL;
> +	answ.is_active = v != 0;
> +
> +	ptr = parse_and_advance(ptr, &v, 2);
> +	if (!ptr)
> +		return -EINVAL;
> +	answ.lf.checksum_mode = v != 0;
> +
> +	ptr = parse_and_advance(ptr, &v, 2);
> +	if (!ptr)
> +		return -EINVAL;
> +	answ.is_event_frame = v != 0;
> +
> +	ptr = parse_and_advance(ptr, &v, 16);
> +	if (!ptr || v > LIN_ID_MASK)
> +		return -EINVAL;
> +	answ.event_associated_id = v;
> +
> +	ptr = parse_and_advance(ptr, &v, 16);
> +	if (!ptr || v > LIN_MAX_DLEN)
> +		return -EINVAL;
> +	answ.lf.len = v;
> +
> +	for (int i = 0; i < answ.lf.len; i++) {
> +		ptr = parse_and_advance(ptr, &v, 16);
> +		if (!ptr)
> +			return -EINVAL;
> +		answ.lf.data[i] = v;
> +	}
> +
> +	ret = ldev->ldev_ops->update_responder_answer(ldev, &answ);
> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}
> +
> +static int lin_create_sysfs_id_files(struct net_device *ndev)
> +{
> +	struct lin_device *ldev = netdev_priv(ndev);
> +	struct kobj_attribute *attr;
> +	int ret;
> +
> +	for (int id = 0; id < LIN_NUM_IDS; id++) {
> +		ldev->sysfs_entries[id].ldev = ldev;
> +		attr = &ldev->sysfs_entries[id].attr;
> +		attr->attr.name = kasprintf(GFP_KERNEL, "%02x", id);
> +		if (!attr->attr.name)
> +			return -ENOMEM;
> +		attr->attr.mode = 0644;
> +		attr->show = lin_identifier_show;
> +		attr->store = lin_identifier_store;
> +
> +		sysfs_attr_init(&attr->attr);
> +		ret = sysfs_create_file(ldev->lin_ids_kobj, &attr->attr);
> +		if (ret) {
> +			kfree(attr->attr.name);
> +			return -ENOMEM;
> +		}
> +	}
> +
> +	return 0;
> +}

Can you use .dev_groups instead ?

FWIW, this function doesn't do rollback when error occurs.

> +
> +static void lin_remove_sysfs_id_files(struct net_device *ndev)
> +{
> +	struct lin_device *ldev = netdev_priv(ndev);
> +	struct kobj_attribute *attr;
> +
> +	for (int id = 0; id < LIN_NUM_IDS; id++) {
> +		attr = &ldev->sysfs_entries[id].attr;
> +		sysfs_remove_file(ldev->lin_ids_kobj, &attr->attr);
> +		kfree(attr->attr.name);
> +	}
> +}
> +
> +static void lin_tx_work_handler(struct work_struct *ws)
> +{
> +	struct lin_device *ldev = container_of(ws, struct lin_device,
> +					       tx_work);
> +	struct net_device *ndev = ldev->ndev;
> +	struct canfd_frame *cfd;
> +	struct lin_frame lf;
> +	int ret;
> +
> +	ldev->tx_busy = true;
> +
> +	cfd = (struct canfd_frame *)ldev->tx_skb->data;
> +	lf.checksum_mode = (cfd->can_id & LIN_ENHANCED_CKSUM_FLAG) ?
> +			   LINBUS_ENHANCED : LINBUS_CLASSIC;
> +	lf.lin_id = cfd->can_id & LIN_ID_MASK;
> +	lf.len = min(cfd->len, LIN_MAX_DLEN);
> +	memcpy(lf.data, cfd->data, lf.len);
> +
> +	ret = ldev->ldev_ops->ldo_tx(ldev, &lf);
> +	if (ret) {
> +		DEV_STATS_INC(ndev, tx_dropped);
> +		netdev_err_once(ndev, "transmission failure %d\n", ret);
> +		goto lin_tx_out;
> +	}
> +
> +	DEV_STATS_INC(ndev, tx_packets);
> +	DEV_STATS_ADD(ndev, tx_bytes, lf.len);
> +
> +lin_tx_out:
> +	ldev->tx_busy = false;
> +	netif_wake_queue(ndev);
> +}
> +
> +static netdev_tx_t lin_start_xmit(struct sk_buff *skb,
> +				  struct net_device *ndev)
> +{
> +	struct lin_device *ldev = netdev_priv(ndev);
> +
> +	if (ldev->tx_busy)
> +		return NETDEV_TX_BUSY;
> +
> +	netif_stop_queue(ndev);
> +	ldev->tx_skb = skb;
> +	queue_work(ldev->wq, &ldev->tx_work);
> +
> +	return NETDEV_TX_OK;
> +}
> +
> +static int lin_open(struct net_device *ndev)
> +{
> +	struct lin_device *ldev = netdev_priv(ndev);
> +	int ret;
> +
> +	ldev->tx_busy = false;
> +
> +	ret = open_candev(ndev);
> +	if (ret)
> +		return ret;
> +
> +	netif_wake_queue(ndev);
> +
> +	ldev->can.state = CAN_STATE_ERROR_ACTIVE;
> +	ndev->mtu = CANFD_MTU;
> +
> +	return ldev->ldev_ops->ldo_open(ldev);
> +}
> +
> +static int lin_stop(struct net_device *ndev)
> +{
> +	struct lin_device *ldev = netdev_priv(ndev);
> +
> +	close_candev(ndev);
> +
> +	flush_work(&ldev->tx_work);
> +
> +	ldev->can.state = CAN_STATE_STOPPED;
> +
> +	return ldev->ldev_ops->ldo_stop(ldev);
> +}
> +
> +static const struct net_device_ops lin_netdev_ops = {
> +	.ndo_open = lin_open,
> +	.ndo_stop = lin_stop,
> +	.ndo_start_xmit = lin_start_xmit,
> +	.ndo_change_mtu = can_change_mtu,
> +};
> +
> +u8 lin_get_checksum(u8 pid, u8 n_of_bytes, const u8 *bytes,
> +		    enum lin_checksum_mode cm)
> +{
> +	unsigned int csum = 0;
> +	int i;
> +
> +	if (cm == LINBUS_ENHANCED)
> +		csum += pid;
> +
> +	for (i = 0; i < n_of_bytes; i++) {
> +		csum += bytes[i];
> +		if (csum > 255)
> +			csum -= 255;
> +	}
> +
> +	return (~csum & 0xff);
> +}
> +EXPORT_SYMBOL_GPL(lin_get_checksum);
> +
> +static int lin_bump_rx_err(struct lin_device *ldev, const struct lin_frame *lf)
> +{
> +	struct net_device *ndev = ldev->ndev;
> +	struct can_frame cf = {0 };
> +
> +	if (lf->lin_id > LIN_ID_MASK) {
> +		netdev_dbg(ndev, "id exceeds LIN max id\n");
> +		cf.can_id = CAN_ERR_FLAG | CAN_ERR_PROT;
> +		cf.data[3] = CAN_ERR_PROT_LOC_ID12_05;
> +	}
> +
> +	if (lf->len > LIN_MAX_DLEN) {
> +		netdev_dbg(ndev, "frame exceeds number of bytes\n");
> +		cf.can_id = CAN_ERR_FLAG | CAN_ERR_PROT;
> +		cf.data[3] = CAN_ERR_PROT_LOC_DLC;
> +	}
> +
> +	if (lf->len) {
> +		u8 checksum = lin_get_checksum(LIN_FORM_PID(lf->lin_id),
> +					       lf->len, lf->data,
> +					       lf->checksum_mode);
> +
> +		if (checksum != lf->checksum) {
> +			netdev_dbg(ndev, "expected cksm: 0x%02x got: 0x%02x\n",
> +				   checksum, lf->checksum);
> +			cf.can_id = CAN_ERR_FLAG | CAN_ERR_PROT;
> +			cf.data[2] = CAN_ERR_PROT_FORM;
> +		}
> +	}
> +
> +	if (cf.can_id & CAN_ERR_FLAG) {
> +		struct can_frame *err_cf;
> +		struct sk_buff *skb = alloc_can_err_skb(ndev, &err_cf);
> +
> +		if (unlikely(!skb))
> +			return -ENOMEM;
> +
> +		err_cf->can_id |= cf.can_id;
> +		memcpy(err_cf->data, cf.data, CAN_MAX_DLEN);
> +
> +		netif_rx(skb);
> +
> +		return -EREMOTEIO;
> +	}
> +
> +	return 0;
> +}
> +
> +int lin_rx(struct lin_device *ldev, const struct lin_frame *lf)
> +{
> +	struct net_device *ndev = ldev->ndev;
> +	struct can_frame *cf;
> +	struct sk_buff *skb;
> +	int ret;
> +
> +	if (ldev->can.state == CAN_STATE_STOPPED)
> +		return 0;
> +
> +	netdev_dbg(ndev, "id:%02x, len:%u, data:%*ph, checksum:%02x (%s)\n",
> +		   lf->lin_id, lf->len, lf->len, lf->data, lf->checksum,
> +		   lf->checksum_mode ? "enhanced" : "classic");
> +
> +	ret = lin_bump_rx_err(ldev, lf);
> +	if (ret) {
> +		DEV_STATS_INC(ndev, rx_dropped);
> +		return ret;
> +	}
> +
> +	skb = alloc_can_skb(ndev, &cf);
> +	if (unlikely(!skb)) {
> +		DEV_STATS_INC(ndev, rx_dropped);
> +		return -ENOMEM;
> +	}
> +
> +	cf->can_id = lf->lin_id;
> +	cf->len = min(lf->len, LIN_MAX_DLEN);
> +	memcpy(cf->data, lf->data, cf->len);
> +
> +	DEV_STATS_INC(ndev, rx_packets);
> +	DEV_STATS_ADD(ndev, rx_bytes, cf->len);
> +
> +	netif_receive_skb(skb);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(lin_rx);
> +
> +static int lin_set_bittiming(struct net_device *ndev)
> +{
> +	struct lin_device *ldev = netdev_priv(ndev);
> +	unsigned int bitrate = ldev->can.bittiming.bitrate;
> +
> +	return ldev->ldev_ops->update_bitrate(ldev, bitrate);
> +}
> +
> +static const u32 lin_bitrate[] = { 1200, 2400, 4800, 9600, 19200 };
> +
> +struct lin_device *register_lin(struct device *dev,
> +				const struct lin_device_ops *ldops)
> +{
> +	struct net_device *ndev;
> +	struct lin_device *ldev;
> +	int ret;
> +
> +	if (!ldops || !ldops->ldo_tx || !ldops->update_bitrate  ||
> +	    !ldops->ldo_open || !ldops->ldo_stop) {
> +		netdev_err(ndev, "missing mandatory lin_device_ops\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	ndev = alloc_candev(sizeof(struct lin_device), 1);
> +	if (!ndev)
> +		return ERR_PTR(-ENOMEM);
> +
> +	ldev = netdev_priv(ndev);
> +
> +	ldev->ldev_ops = ldops;
> +	ndev->netdev_ops = &lin_netdev_ops;
> +	ndev->flags |= IFF_ECHO;
> +	ndev->mtu = CANFD_MTU;
> +	ldev->can.bittiming.bitrate = LIN_DEFAULT_BAUDRATE;
> +	ldev->can.ctrlmode = CAN_CTRLMODE_LIN;
> +	ldev->can.ctrlmode_supported = 0;
> +	ldev->can.bitrate_const = lin_bitrate;
> +	ldev->can.bitrate_const_cnt = ARRAY_SIZE(lin_bitrate);
> +	ldev->can.do_set_bittiming = lin_set_bittiming;
> +	ldev->ndev = ndev;
> +	ldev->dev = dev;
> +
> +	SET_NETDEV_DEV(ndev, dev);
> +
> +	ret = lin_set_bittiming(ndev);
> +	if (ret) {
> +		netdev_err(ndev, "set bittiming failed\n");
> +		goto exit_candev;
> +	}
> +
> +	ret = register_candev(ndev);
> +	if (ret)
> +		goto exit_candev;
> +
> +	ldev->lin_ids_kobj = kobject_create_and_add("lin_ids", &ndev->dev.kobj);
> +	if (!ldev->lin_ids_kobj) {
> +		netdev_err(ndev, "Failed to create sysfs directory\n");
> +		ret = -ENOMEM;
> +		goto exit_unreg;
> +	}
> +
> +	ret = lin_create_sysfs_id_files(ndev);
> +	if (ret) {
> +		netdev_err(ndev, "Failed to create sysfs entry: %d\n", ret);
> +		goto exit_kobj_put;
> +	}
> +
> +	/* Using workqueue as tx over USB/SPI/... may sleep */
> +	ldev->wq = alloc_workqueue(dev_name(dev), WQ_FREEZABLE | WQ_MEM_RECLAIM,
> +				   0);
> +	if (!ldev->wq)
> +		goto exit_rm_files;
> +
> +	INIT_WORK(&ldev->tx_work, lin_tx_work_handler);
> +
> +	netdev_info(ndev, "LIN initialized.\n");
> +
> +	return ldev;
> +
> +exit_rm_files:
> +	lin_remove_sysfs_id_files(ndev);
> +exit_kobj_put:
> +	kobject_put(ldev->lin_ids_kobj);
> +exit_unreg:
> +	unregister_candev(ndev);
> +exit_candev:
> +	free_candev(ndev);
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(register_lin);
> +
> +void unregister_lin(struct lin_device *ldev)
> +{
> +	struct net_device *ndev = ldev->ndev;
> +
> +	lin_remove_sysfs_id_files(ndev);
> +	kobject_put(ldev->lin_ids_kobj);
> +	unregister_candev(ndev);
> +	destroy_workqueue(ldev->wq);
> +	free_candev(ndev);
> +}
> +EXPORT_SYMBOL_GPL(unregister_lin);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Christoph Fritz <christoph.fritz@hexdev.de>");
> +MODULE_DESCRIPTION("LIN bus to CAN glue driver");
> diff --git a/include/net/lin.h b/include/net/lin.h
> new file mode 100644
> index 0000000000000..e7c7c820a6e18
> --- /dev/null
> +++ b/include/net/lin.h
> @@ -0,0 +1,92 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/* Copyright (C) 2024 hexDEV GmbH - https://hexdev.de */
> +
> +#ifndef _NET_LIN_H_
> +#define _NET_LIN_H_
> +
> +#include <linux/bitfield.h>
> +#include <linux/can/dev.h>
> +#include <linux/device.h>
> +
> +#define LIN_NUM_IDS		64
> +#define LIN_HEADER_SIZE		3
> +#define LIN_MAX_DLEN		8
> +
> +#define LIN_MAX_BAUDRATE	20000
> +#define LIN_MIN_BAUDRATE	1000
> +#define LIN_DEFAULT_BAUDRATE	9600
> +#define LIN_SYNC_BYTE		0x55
> +
> +#define LIN_ID_MASK		GENMASK(5, 0)
> +/* special ID descriptions for LIN */
> +#define LIN_RXOFFLOAD_DATA_FLAG	0x00000200U
> +#define LIN_ENHANCED_CKSUM_FLAG	0x00000100U

BIT(x) x 2

> +
> +extern u8 lin_get_id_parity(u8 id);
> +
> +#define LIN_GET_ID(PID)		FIELD_GET(LIN_ID_MASK, PID)
> +#define LIN_FORM_PID(ID)	(LIN_GET_ID(ID) | \
> +					lin_get_id_parity(LIN_GET_ID(ID)))
> +#define LIN_GET_PARITY(PID)	((PID) & ~LIN_ID_MASK)
> +#define LIN_CHECK_PID(PID)	(LIN_GET_PARITY(PID) == \
> +					LIN_GET_PARITY(LIN_FORM_PID(PID)))
> +
> +struct lin_attr {
> +	struct kobj_attribute attr;
> +	struct lin_device *ldev;
> +};
> +
> +struct lin_device {
> +	struct can_priv can;  /* must be the first member */
> +	struct net_device *ndev;
> +	struct device *dev;
> +	const struct lin_device_ops *ldev_ops;
> +	struct workqueue_struct *wq;
> +	struct work_struct tx_work;
> +	bool tx_busy;
> +	struct sk_buff *tx_skb;
> +	struct kobject *lin_ids_kobj;
> +	struct lin_attr sysfs_entries[LIN_NUM_IDS];
> +};
> +
> +enum lin_checksum_mode {
> +	LINBUS_CLASSIC = 0,
> +	LINBUS_ENHANCED,
> +};
> +
> +struct lin_frame {
> +	u8 lin_id;
> +	u8 len;
> +	u8 data[LIN_MAX_DLEN];
> +	u8 checksum;
> +	enum lin_checksum_mode checksum_mode;
> +};
> +
> +struct lin_responder_answer {
> +	bool is_active;
> +	bool is_event_frame;
> +	u8 event_associated_id;
> +	struct lin_frame lf;
> +};
> +
> +struct lin_device_ops {
> +	int (*ldo_open)(struct lin_device *ldev);
> +	int (*ldo_stop)(struct lin_device *ldev);
> +	int (*ldo_tx)(struct lin_device *ldev, const struct lin_frame *frame);
> +	int (*update_bitrate)(struct lin_device *ldev, u16 bitrate);
> +	int (*update_responder_answer)(struct lin_device *ldev,
> +				       const struct lin_responder_answer *answ);
> +	int (*get_responder_answer)(struct lin_device *ldev, u8 id,
> +				    struct lin_responder_answer *answ);
> +};
> +
> +int lin_rx(struct lin_device *ldev, const struct lin_frame *lf);
> +
> +u8 lin_get_checksum(u8 pid, u8 n_of_bytes, const u8 *bytes,
> +		    enum lin_checksum_mode cm);
> +
> +struct lin_device *register_lin(struct device *dev,
> +				const struct lin_device_ops *ldops);
> +void unregister_lin(struct lin_device *ldev);
> +
> +#endif /* _NET_LIN_H_ */
> diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
> index 02ec32d694742..51b0e2a7624e4 100644
> --- a/include/uapi/linux/can/netlink.h
> +++ b/include/uapi/linux/can/netlink.h
> @@ -103,6 +103,7 @@ struct can_ctrlmode {
>  #define CAN_CTRLMODE_CC_LEN8_DLC	0x100	/* Classic CAN DLC option */
>  #define CAN_CTRLMODE_TDC_AUTO		0x200	/* CAN transiver automatically calculates TDCV */
>  #define CAN_CTRLMODE_TDC_MANUAL		0x400	/* TDCV is manually set up by user */

BIT(x) is these days available also for uapi I think.

> +#define CAN_CTRLMODE_LIN		0x800	/* LIN bus mode */
>  
>  /*
>   * CAN device statistics
>
Ilpo Järvinen May 6, 2024, 5:03 p.m. UTC | #2
On Thu, 2 May 2024, Christoph Fritz wrote:

> This commit introduces LIN-Bus support for UART devices equipped with
> LIN transceivers, utilizing the Serial Device Bus (serdev) interface.
> 
> For more details on an adapter, visit: https://hexdev.de/hexlin#tty
> 
> Signed-off-by: Christoph Fritz <christoph.fritz@hexdev.de>
> ---
>  drivers/net/can/Kconfig      |  16 ++
>  drivers/net/can/Makefile     |   1 +
>  drivers/net/can/lin-serdev.c | 500 +++++++++++++++++++++++++++++++++++
>  3 files changed, 517 insertions(+)
>  create mode 100644 drivers/net/can/lin-serdev.c
> 
> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index 0934bbf8d03b2..91c6cdeefe440 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -181,6 +181,22 @@ config CAN_LIN
>  
>  	  Actual device drivers need to be enabled too.
>  
> +config CAN_LIN_SERDEV
> +	tristate "Serial LIN Adaptors"
> +	depends on CAN_LIN && SERIAL_DEV_BUS && OF
> +	help
> +	  LIN-Bus support for serial devices equipped with LIN transceievers.
> +	  This device driver is using CAN_LIN for a userland connection on
> +	  one side and the kernel its serial device bus (serdev) interface
> +	  on the other side.
> +
> +	  If you have a hexlin tty adapter, say Y here and see
> +	  <https://hexdev.de/hexlin#tty>.
> +
> +	  This driver can also be built as a module. If so, the module will be
> +	  called lin-serdev.ko.
> +
> +
>  config CAN_SLCAN
>  	tristate "Serial / USB serial CAN Adaptors (slcan)"
>  	depends on TTY
> diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
> index 0093ee9219ca8..21ca514a42439 100644
> --- a/drivers/net/can/Makefile
> +++ b/drivers/net/can/Makefile
> @@ -26,6 +26,7 @@ obj-$(CONFIG_CAN_IFI_CANFD)	+= ifi_canfd/
>  obj-$(CONFIG_CAN_JANZ_ICAN3)	+= janz-ican3.o
>  obj-$(CONFIG_CAN_KVASER_PCIEFD)	+= kvaser_pciefd.o
>  obj-$(CONFIG_CAN_LIN)		+= lin.o
> +obj-$(CONFIG_CAN_LIN_SERDEV)	+= lin-serdev.o
>  obj-$(CONFIG_CAN_MSCAN)		+= mscan/
>  obj-$(CONFIG_CAN_M_CAN)		+= m_can/
>  obj-$(CONFIG_CAN_PEAK_PCIEFD)	+= peak_canfd/
> diff --git a/drivers/net/can/lin-serdev.c b/drivers/net/can/lin-serdev.c
> new file mode 100644
> index 0000000000000..13fd9a54c6c93
> --- /dev/null
> +++ b/drivers/net/can/lin-serdev.c
> @@ -0,0 +1,500 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/* Copyright (C) 2024 hexDEV GmbH - https://hexdev.de */
> +
> +#include <linux/delay.h>
> +#include <linux/init.h>
> +#include <linux/kfifo.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/serdev.h>
> +#include <linux/slab.h>
> +#include <linux/tty.h>
> +#include <net/lin.h>
> +
> +#define LINSER_SAMPLES_PER_CHAR		10
> +#define LINSER_TX_BUFFER_SIZE		11
> +#define LINSER_RX_FIFO_SIZE		256
> +#define LINSER_PARSE_BUFFER		24
> +
> +struct linser_rx {
> +	u8 data;
> +	u8 flag;
> +};
> +
> +enum linser_rx_status {
> +	NEED_MORE = -1,
> +	MODE_OK = 0,
> +	NEED_FORCE,
> +};
> +
> +struct linser_priv {
> +	struct lin_device *lin_dev;
> +	struct serdev_device *serdev;
> +	DECLARE_KFIFO_PTR(rx_fifo, struct linser_rx);
> +	struct delayed_work rx_work;
> +	unsigned long break_usleep_min;
> +	unsigned long break_usleep_max;
> +	unsigned long post_break_usleep_min;
> +	unsigned long post_break_usleep_max;
> +	unsigned long force_timeout_jfs;
> +	struct lin_responder_answer respond_answ[LIN_NUM_IDS];
> +	struct mutex resp_lock; /* protects respond_answ */
> +	bool is_stopped;
> +};
> +
> +static int linser_open(struct lin_device *ldev)
> +{
> +	struct serdev_device *serdev = to_serdev_device(ldev->dev);
> +	struct linser_priv *priv = serdev_device_get_drvdata(serdev);
> +	int ret;
> +
> +	if (priv->is_stopped) {
> +		ret = serdev_device_open(serdev);
> +		if (ret) {
> +			dev_err(&serdev->dev, "Unable to open device\n");
> +			return ret;
> +		}
> +
> +		serdev_device_set_flow_control(serdev, false);
> +		serdev_device_set_break_detection(serdev, true);
> +
> +		priv->is_stopped = false;
> +	}
> +
> +	return 0;
> +}
> +
> +static int linser_stop(struct lin_device *ldev)
> +{
> +	struct serdev_device *serdev = to_serdev_device(ldev->dev);
> +	struct linser_priv *priv = serdev_device_get_drvdata(serdev);
> +
> +	if (priv->is_stopped)
> +		return 0;
> +
> +	serdev_device_close(serdev);
> +	priv->is_stopped = true;
> +
> +	return 0;
> +}
> +
> +static int linser_send_break(struct linser_priv *priv)
> +{
> +	struct serdev_device *serdev = priv->serdev;
> +	int ret;
> +
> +	ret = serdev_device_break_ctl(serdev, -1);
> +	if (ret)
> +		return ret;
> +
> +	usleep_range(priv->break_usleep_min, priv->break_usleep_max);
> +
> +	ret = serdev_device_break_ctl(serdev, 0);
> +	if (ret)
> +		return ret;
> +
> +	usleep_range(priv->post_break_usleep_min, priv->post_break_usleep_max);
> +
> +	return 0;
> +}
> +
> +static int linser_ldo_tx(struct lin_device *ldev, const struct lin_frame *lf)
> +{
> +	struct serdev_device *serdev = to_serdev_device(ldev->dev);
> +	struct linser_priv *priv = serdev_device_get_drvdata(serdev);
> +	u8 pid = LIN_FORM_PID(lf->lin_id);
> +	u8 buf[LINSER_TX_BUFFER_SIZE];
> +	ssize_t written_len, total_len;
> +	u8 checksum;
> +	int ret;
> +
> +	if (lf->len + 3 > LINSER_TX_BUFFER_SIZE) {
> +		dev_err(&serdev->dev, "Frame length %u exceeds buffer size\n", lf->len);
> +		return -EINVAL;
> +	}
> +
> +	buf[0] = LIN_SYNC_BYTE;
> +	buf[1] = pid;
> +	total_len = 2;
> +
> +	if (lf->len) {
> +		memcpy(&buf[2], lf->data, lf->len);
> +		checksum = lin_get_checksum(pid, lf->len, lf->data,
> +					    lf->checksum_mode);
> +		buf[lf->len + 2] = checksum;
> +		total_len += lf->len + 1;
> +	}
> +
> +	ret = linser_send_break(priv);
> +	if (ret)
> +		return ret;
> +
> +	written_len = serdev_device_write(serdev, buf, total_len, 0);
> +	if (written_len < total_len)
> +		return written_len < 0 ? (int)written_len : -EIO;
> +
> +	dev_dbg(&serdev->dev, "sent out: %*ph\n", (int)total_len, buf);
> +
> +	serdev_device_wait_until_sent(serdev, 0);
> +
> +	return 0;
> +}
> +
> +static void linser_derive_timings(struct linser_priv *priv, u16 bitrate)
> +{
> +	unsigned long break_baud = (bitrate * 2) / 3;
> +	unsigned long timeout_us;
> +
> +	priv->break_usleep_min = (USEC_PER_SEC * LINSER_SAMPLES_PER_CHAR) /
> +				 break_baud;
> +	priv->break_usleep_max = priv->break_usleep_min + 50;
> +	priv->post_break_usleep_min = (USEC_PER_SEC * 1 /* 1 bit */) / break_baud;

Don't mix comments and code like this.

> +	priv->post_break_usleep_max = priv->post_break_usleep_min + 30;
> +
> +	timeout_us = DIV_ROUND_CLOSEST(USEC_PER_SEC * 256 /* bit */, bitrate);

Ditto.

> +	priv->force_timeout_jfs = usecs_to_jiffies(timeout_us);
> +}
> +
> +static int linser_update_bitrate(struct lin_device *ldev, u16 bitrate)
> +{
> +	struct serdev_device *serdev = to_serdev_device(ldev->dev);
> +	struct linser_priv *priv = serdev_device_get_drvdata(serdev);
> +	unsigned int speed;
> +	int ret;
> +
> +	ret = linser_open(ldev);
> +	if (ret)
> +		return ret;
> +
> +	speed = serdev_device_set_baudrate(serdev, bitrate);
> +	if (!bitrate || speed != bitrate)
> +		return -EINVAL;
> +
> +	linser_derive_timings(priv, bitrate);
> +
> +	return 0;
> +}
> +
> +static int linser_get_responder_answer(struct lin_device *ldev, u8 id,
> +				       struct lin_responder_answer *answ)
> +{
> +	struct serdev_device *serdev = to_serdev_device(ldev->dev);
> +	struct linser_priv *priv = serdev_device_get_drvdata(serdev);
> +	struct lin_responder_answer *r = &priv->respond_answ[id];
> +
> +	if (!answ)
> +		return -EINVAL;
> +
> +	mutex_lock(&priv->resp_lock);

guard(mutex)(&priv->resp_lock);

> +	memcpy(answ, r, sizeof(struct lin_responder_answer));

sizeof(*answ);

> +	mutex_unlock(&priv->resp_lock);
> +
> +	return 0;
> +}
> +
> +static int linser_update_resp_answer(struct lin_device *ldev,
> +				     const struct lin_responder_answer *answ)
> +{
> +	struct serdev_device *serdev = to_serdev_device(ldev->dev);
> +	struct linser_priv *priv = serdev_device_get_drvdata(serdev);
> +	struct lin_responder_answer *r = &priv->respond_answ[answ->lf.lin_id];
> +
> +	if (!answ)
> +		return -EINVAL;
> +
> +	mutex_lock(&priv->resp_lock);
> +	memcpy(r, answ, sizeof(struct lin_responder_answer));

Ditto.

> +	r->lf.checksum = lin_get_checksum(LIN_FORM_PID(answ->lf.lin_id),
> +					  answ->lf.len,
> +					  answ->lf.data,
> +					  answ->lf.checksum_mode);
> +	mutex_unlock(&priv->resp_lock);
> +
> +	return 0;
> +}
> +
> +static struct lin_device_ops linser_lindev_ops = {
> +	.ldo_open = linser_open,
> +	.ldo_stop = linser_stop,
> +	.ldo_tx = linser_ldo_tx,
> +	.update_bitrate = linser_update_bitrate,
> +	.get_responder_answer = linser_get_responder_answer,
> +	.update_responder_answer = linser_update_resp_answer,
> +};
> +
> +static bool linser_tx_frame_as_responder(struct linser_priv *priv, u8 id)
> +{
> +	struct lin_responder_answer *answ = &priv->respond_answ[id];
> +	struct serdev_device *serdev = priv->serdev;
> +	u8 buf[LINSER_TX_BUFFER_SIZE];
> +	u8 checksum, count, n;
> +	ssize_t write_len;
> +
> +	mutex_lock(&priv->resp_lock);

scoped_guard() can be used here.

> +
> +	if (!answ->is_active) {
> +		mutex_unlock(&priv->resp_lock);
> +		return false;
> +	}
> +
> +	if (answ->is_event_frame) {
> +		struct lin_responder_answer *e_answ;
> +
> +		e_answ = &priv->respond_answ[answ->event_associated_id];
> +		n = min(e_answ->lf.len, LIN_MAX_DLEN);
> +		if (memcmp(answ->lf.data, e_answ->lf.data, n) != 0) {
> +			memcpy(answ->lf.data, e_answ->lf.data, n);
> +			checksum = lin_get_checksum(LIN_FORM_PID(answ->lf.lin_id),
> +						    n, e_answ->lf.data,
> +						    answ->lf.checksum_mode);
> +			answ = e_answ;
> +		} else {
> +			mutex_unlock(&priv->resp_lock);
> +			return false;

Use reverse logic so you can reduce indent.

> +		}
> +	} else {
> +		checksum = answ->lf.checksum;
> +	}
> +
> +	count = min(answ->lf.len, LIN_MAX_DLEN);
> +	memcpy(&buf[0], answ->lf.data, count);
> +	buf[count] = checksum;
> +
> +	mutex_unlock(&priv->resp_lock);
> +
> +	write_len = serdev_device_write(serdev, buf, count + 1, 0);
> +	if (write_len < count + 1)
> +		return false;
> +
> +	serdev_device_wait_until_sent(serdev, 0);
> +
> +	return true;
> +}
> +
> +static void linser_pop_fifo(struct linser_priv *priv, size_t n)
> +{
> +	for (size_t i = 0; i < n; i++)
> +		kfifo_skip(&priv->rx_fifo);
> +}
> +
> +static int linser_fill_frame(struct linser_priv *priv, struct lin_frame *lf)
> +{
> +	struct serdev_device *serdev = priv->serdev;
> +	struct linser_rx buf[LINSER_PARSE_BUFFER];
> +	unsigned int count, i, brk = 0;
> +
> +	count = kfifo_out_peek(&priv->rx_fifo, buf, LINSER_PARSE_BUFFER);
> +
> +	memset(lf, 0, sizeof(struct lin_frame));
> +
> +	for (i = 0; i < count; i++) {
> +		dev_dbg(&serdev->dev, "buf[%d]: data=%02x, flag=%02x\n",
> +			i, buf[i].data, buf[i].flag);
> +	}
> +
> +	if (count < 3)
> +		return NEED_MORE;
> +
> +	if (buf[0].flag != TTY_BREAK || buf[1].data != LIN_SYNC_BYTE) {
> +		linser_pop_fifo(priv, 1); /* pop incorrect start */
> +		return NEED_MORE;
> +	} else if (!LIN_CHECK_PID(buf[2].data)) {
> +		linser_pop_fifo(priv, 3); /* pop incorrect header */
> +		return NEED_MORE;
> +	}
> +
> +	lf->lin_id = LIN_GET_ID(buf[2].data);
> +
> +	/* from here on we do have a correct LIN header */
> +
> +	if (count == 3)
> +		return linser_tx_frame_as_responder(priv, lf->lin_id) ?
> +		       NEED_MORE : NEED_FORCE;
> +
> +	for (i = 3; i < count && i < LINSER_PARSE_BUFFER && i < 12; i++) {
> +		if (buf[i].flag == TTY_BREAK) {
> +			brk = i;
> +			break;
> +		}
> +		lf->len++;
> +	}
> +	if (lf->len)
> +		lf->len -= 1; /* account for checksum */
> +
> +	if (brk == 3)
> +		return MODE_OK;
> +
> +	if (brk == 4) {
> +		/* suppress wrong answer data-byte in between PID and break
> +		 * because checksum is missing
> +		 */
> +		return MODE_OK;
> +	}
> +
> +	for (i = 0; i < lf->len; i++)
> +		lf->data[i] = buf[3 + i].data;
> +	lf->checksum = buf[2 + lf->len + 1].data;
> +	mutex_lock(&priv->resp_lock);
> +	lf->checksum_mode = priv->respond_answ[lf->lin_id].lf.checksum_mode;
> +	mutex_unlock(&priv->resp_lock);
> +
> +	dev_dbg(&serdev->dev, "brk:%i, len:%u, data:%*ph, checksum:%x (%s)\n",
> +		brk, lf->len, lf->len, lf->data, lf->checksum,
> +		lf->checksum_mode ? "enhanced" : "classic");
> +
> +	if (brk > 4)
> +		return MODE_OK;	/* frame in between two breaks: so complete */
> +
> +	if (lf->len == 8)
> +		return MODE_OK;
> +
> +	return NEED_FORCE;
> +}
> +
> +static int linser_process_frame(struct linser_priv *priv, bool force)
> +{
> +	struct serdev_device *serdev = priv->serdev;
> +	struct lin_frame lf;
> +	size_t bytes_to_pop;
> +	int ret = NEED_MORE;
> +
> +	while (kfifo_len(&priv->rx_fifo) >= LIN_HEADER_SIZE) {
> +		ret = linser_fill_frame(priv, &lf);
> +
> +		if (ret == MODE_OK || (ret == NEED_FORCE && force)) {
> +			dev_dbg(&serdev->dev, "lin_rx: %s\n",
> +				force ? "force" : "normal");
> +			lin_rx(priv->lin_dev, &lf);
> +			bytes_to_pop = LIN_HEADER_SIZE + lf.len +
> +				       (lf.len ? 1 : 0);
> +			linser_pop_fifo(priv, bytes_to_pop);
> +			force = false;
> +			ret = MODE_OK;
> +		} else {
> +			return ret;

Reverse logic & deindent the main block.

> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static void linser_process_delayed(struct work_struct *work)
> +{
> +	struct linser_priv *priv = container_of(work, struct linser_priv,
> +						rx_work.work);
> +
> +	linser_process_frame(priv, true);
> +}
> +
> +static ssize_t linser_receive_buf_fp(struct serdev_device *serdev,
> +				     const u8 *data, const u8 *fp,
> +				     size_t count)
> +{
> +	struct linser_priv *priv = serdev_device_get_drvdata(serdev);
> +	enum linser_rx_status rx_status;
> +	ssize_t n = 0;
> +	int i;
> +
> +	cancel_delayed_work_sync(&priv->rx_work);
> +
> +	for (i = 0; i < count; i++) {
> +		struct linser_rx rx;
> +
> +		rx.data = data[i];
> +		rx.flag = (fp ? fp[i] : 0);
> +		n += kfifo_in(&priv->rx_fifo, &rx, 1);
> +		dev_dbg(&serdev->dev, "%s: n:%zd, flag:0x%02x, data:0x%02x\n",
> +			__func__, n, rx.flag, data[i]);
> +	}
> +
> +	rx_status = linser_process_frame(priv, false);
> +
> +	if (rx_status == NEED_FORCE)
> +		schedule_delayed_work(&priv->rx_work, priv->force_timeout_jfs);
> +
> +	return n;
> +}
> +
> +static const struct serdev_device_ops linser_ops = {
> +	.receive_buf_fp = linser_receive_buf_fp,
> +	.write_wakeup = serdev_device_write_wakeup,
> +};
> +
> +static int linser_probe(struct serdev_device *serdev)
> +{
> +	struct linser_priv *priv;
> +	int ret;
> +
> +	priv = devm_kzalloc(&serdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	ret = kfifo_alloc(&priv->rx_fifo, LINSER_RX_FIFO_SIZE, GFP_KERNEL);
> +	if (ret)
> +		return ret;
> +
> +	INIT_DELAYED_WORK(&priv->rx_work, linser_process_delayed);
> +
> +	priv->serdev = serdev;
> +	serdev_device_set_drvdata(serdev, priv);
> +	serdev_device_set_client_ops(serdev, &linser_ops);
> +
> +	ret = serdev_device_open(serdev);
> +	if (ret) {
> +		dev_err(&serdev->dev, "Unable to open device\n");
> +		goto err_open;
> +	}
> +
> +	serdev_device_set_flow_control(serdev, false);
> +	serdev_device_set_break_detection(serdev, true);
> +	linser_derive_timings(priv, LIN_DEFAULT_BAUDRATE);
> +
> +	mutex_init(&priv->resp_lock);
> +
> +	priv->lin_dev = register_lin(&serdev->dev, &linser_lindev_ops);
> +	if (IS_ERR_OR_NULL(priv->lin_dev)) {
> +		ret = PTR_ERR(priv->lin_dev);
> +		goto err_register_lin;
> +	}
> +
> +	serdev_device_close(serdev);
> +	priv->is_stopped = true;
> +
> +	return 0;
> +
> +err_register_lin:
> +	serdev_device_close(serdev);
> +err_open:
> +	kfifo_free(&priv->rx_fifo);
> +	return ret;
> +}
> +
> +static void linser_remove(struct serdev_device *serdev)
> +{
> +	struct linser_priv *priv = serdev_device_get_drvdata(serdev);
> +
> +	unregister_lin(priv->lin_dev);
> +}
> +
> +static const struct of_device_id linser_of_match[] = {
> +	{
> +		.compatible = "hexdev,lin-serdev",
> +	},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, linser_of_match);
> +
> +static struct serdev_device_driver linser_driver = {
> +	.probe = linser_probe,
> +	.remove = linser_remove,
> +	.driver = {
> +		.name = KBUILD_MODNAME,
> +		.of_match_table = linser_of_match,
> +	}
> +};
> +
> +module_serdev_device_driver(linser_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Christoph Fritz <christoph.fritz@hexdev.de>");
> +MODULE_DESCRIPTION("LIN-Bus serdev driver");
>
Ilpo Järvinen May 6, 2024, 5:08 p.m. UTC | #3
On Thu, 2 May 2024, Christoph Fritz wrote:

> Enhance CAN broadcast manager with RX_LIN_SETUP and RX_LIN_DELETE
> operations to setup automatic LIN frame responses in responder mode.
> 
> Additionally, the patch introduces the LIN_EVENT_FRAME flag to
> setup event-triggered LIN frames.
> 
> Signed-off-by: Christoph Fritz <christoph.fritz@hexdev.de>
> ---
>  include/uapi/linux/can/bcm.h |  5 ++-
>  net/can/bcm.c                | 74 +++++++++++++++++++++++++++++++++++-
>  2 files changed, 77 insertions(+), 2 deletions(-)
> 
> diff --git a/include/uapi/linux/can/bcm.h b/include/uapi/linux/can/bcm.h
> index f1e45f533a72c..c46268a114078 100644
> --- a/include/uapi/linux/can/bcm.h
> +++ b/include/uapi/linux/can/bcm.h
> @@ -86,7 +86,9 @@ enum {
>  	TX_EXPIRED,	/* notification on performed transmissions (count=0) */
>  	RX_STATUS,	/* reply to RX_READ request */
>  	RX_TIMEOUT,	/* cyclic message is absent */
> -	RX_CHANGED	/* updated CAN frame (detected content change) */
> +	RX_CHANGED,	/* updated CAN frame (detected content change) */
> +	RX_LIN_SETUP,	/* create auto-response for LIN frame */
> +	RX_LIN_DELETE,  /* remove auto-response for LIN frame */
>  };
>  
>  #define SETTIMER            0x0001
> @@ -101,5 +103,6 @@ enum {
>  #define TX_RESET_MULTI_IDX  0x0200
>  #define RX_RTR_FRAME        0x0400
>  #define CAN_FD_FRAME        0x0800
> +#define LIN_EVENT_FRAME     0x1000
>  
>  #endif /* !_UAPI_CAN_BCM_H */
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 27d5fcf0eac9d..a717e594234d1 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -59,6 +59,7 @@
>  #include <linux/can/bcm.h>
>  #include <linux/slab.h>
>  #include <net/sock.h>
> +#include <net/lin.h>
>  #include <net/net_namespace.h>
>  
>  /*
> @@ -1330,6 +1331,59 @@ static int bcm_tx_send(struct msghdr *msg, int ifindex, struct sock *sk,
>  	return cfsiz + MHSIZ;
>  }
>  
> +static int bcm_lin_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
> +			 int ifindex, struct sock *sk, int cfsiz, int is_active)
> +{
> +	struct lin_responder_answer answ;
> +	struct net_device *dev;
> +	struct sk_buff *skb;
> +	struct canfd_frame cf;
> +	netdevice_tracker tracker;
> +	size_t sz;
> +	int ret;
> +
> +	if (msg_head->nframes > 1)
> +		return -EINVAL;
> +
> +	if (!(msg_head->flags & CAN_FD_FRAME))
> +		return -EINVAL;
> +
> +	ret = memcpy_from_msg(&cf, msg, cfsiz);
> +	if (ret < 0)
> +		return ret;
> +
> +	answ.lf.lin_id = cf.can_id & LIN_ID_MASK;
> +	answ.is_active = is_active;
> +	answ.is_event_frame = !!(msg_head->flags & LIN_EVENT_FRAME);
> +	answ.event_associated_id = msg_head->can_id;
> +	answ.lf.len = min(cf.len, LIN_MAX_DLEN);
> +	memcpy(answ.lf.data, cf.data, answ.lf.len);
> +	sz = min(sizeof(struct lin_responder_answer), sizeof(cf.data));
> +	cf.can_id |= LIN_RXOFFLOAD_DATA_FLAG;
> +	memcpy(cf.data, &answ, sz);
> +
> +	dev = netdev_get_by_index(sock_net(sk), ifindex, &tracker, GFP_KERNEL);
> +	if (!dev)
> +		return -ENODEV;
> +
> +	skb = alloc_skb(cfsiz + sizeof(struct can_skb_priv), gfp_any());

You just called the other function with GFP_KERNEL and you now need 
gfp_any(). Which is correct??

> +	if (!skb)
> +		goto lin_out;
> +
> +	can_skb_reserve(skb);
> +	can_skb_prv(skb)->ifindex = dev->ifindex;
> +	can_skb_prv(skb)->skbcnt = 0;
> +	skb_put_data(skb, &cf, cfsiz);
> +
> +	skb->dev = dev;
> +	can_skb_set_owner(skb, sk);
> +	ret = can_send(skb, 1); /* send with loopback */
> +
> +lin_out:
> +	netdev_put(dev, &tracker);
> +	return ret;
> +}
> +
>  /*
>   * bcm_sendmsg - process BCM commands (opcodes) from the userspace
>   */
> @@ -1429,12 +1483,30 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>  
>  	case TX_SEND:
>  		/* we need exactly one CAN frame behind the msg head */
> -		if ((msg_head.nframes != 1) || (size != cfsiz + MHSIZ))
> +		if (msg_head.nframes != 1 || size != cfsiz + MHSIZ)

Unrelated style fix, doesn't belong to this patch.

>  			ret = -EINVAL;
>  		else
>  			ret = bcm_tx_send(msg, ifindex, sk, cfsiz);
>  		break;
>  
> +	case RX_LIN_SETUP:
> +		/* we need exactly one CAN frame behind the msg head */
> +		if (msg_head.nframes != 1 || size != cfsiz + MHSIZ)
> +			ret = -EINVAL;
> +		else
> +			ret = bcm_lin_setup(&msg_head, msg, ifindex, sk, cfsiz,
> +					    1);
> +		break;
> +
> +	case RX_LIN_DELETE:
> +		/* we need exactly one CAN frame behind the msg head */
> +		if (msg_head.nframes != 1 || size != cfsiz + MHSIZ)
> +			ret = -EINVAL;
> +		else
> +			ret = bcm_lin_setup(&msg_head, msg, ifindex, sk, cfsiz,
> +					    0);
> +		break;
> +
>  	default:
>  		ret = -EINVAL;
>  		break;
>
Christoph Fritz May 8, 2024, 12:47 p.m. UTC | #4
On Mon, 2024-05-06 at 19:24 +0300, Ilpo Järvinen wrote:
> On Thu, 2 May 2024, Christoph Fritz wrote:
> 
> > This patch adds a LIN (local interconnect network) bus abstraction on
> > top of CAN.  It is a glue driver adapting CAN on one side while offering
> > LIN abstraction on the other side. So that upcoming LIN device drivers
> > can make use of it.
...
> > +static ssize_t lin_identifier_show(struct kobject *kobj,
> > +				   struct kobj_attribute *attr, char *buf)
> > +{
> > +	struct lin_attr *lin_attr = container_of(attr, struct lin_attr, attr);
> > +	struct lin_device *ldev = lin_attr->ldev;
> > +	ssize_t count = 0;
> > +	struct lin_responder_answer answ;
> > +	int k, ret;
> > +	long id;
> > +
> > +	if (!ldev->ldev_ops->get_responder_answer)
> > +		return -EOPNOTSUPP;
> > +
> > +	ret = kstrtol(attr->attr.name, 16, &id);
> > +	if (ret)
> > +		return ret;
> > +	if (id < 0 || id >= LIN_NUM_IDS)
> > +		return -EINVAL;
> > +
> > +	count += scnprintf(buf + count, PAGE_SIZE - count,
> > +			   "%-6s %-11s %-9s %-9s %-2s %-24s %-6s\n",
> > +			   "state", "cksum-mode", "is_event", "event_id",
> > +			   "n", "data", "cksum");
> 
> Onl use sysfs_emit() and sysfs_emit_at() in *_show functions.

OK

> > +
> > +	ret = ldev->ldev_ops->get_responder_answer(ldev, id, &answ);
> > +	if (ret)
> > +		return ret;
> > +
> > +	count += scnprintf(buf + count, PAGE_SIZE - count,
> > +			   "%-6s %-11s %-9s %-9u %-2u ",
> > +			   answ.is_active ? "active" : "off",
> > +			   answ.lf.checksum_mode ? "enhanced" : "classic",
> > +			   answ.is_event_frame ? "yes" : "no",
> > +			   answ.event_associated_id,
> > +			   answ.lf.len);
> > +
> > +	for (k = 0; k < answ.lf.len; k++)
> > +		count += scnprintf(buf + count, PAGE_SIZE - count,
> > +				   "%02x ", answ.lf.data[k]);
> > +	for (; k < 8; k++)
> > +		count += scnprintf(buf + count, PAGE_SIZE - count,
> > +				   "   ");
> > +	if (answ.lf.len)
> > +		count += scnprintf(buf + count, PAGE_SIZE - count,
> > +				   " %02x", answ.lf.checksum);
> > +
> > +	count += scnprintf(buf + count, PAGE_SIZE - count, "\n");
> > +
> > +	return count;
> > +}
...
> > +
> > +static int lin_create_sysfs_id_files(struct net_device *ndev)
> > +{
> > +	struct lin_device *ldev = netdev_priv(ndev);
> > +	struct kobj_attribute *attr;
> > +	int ret;
> > +
> > +	for (int id = 0; id < LIN_NUM_IDS; id++) {
> > +		ldev->sysfs_entries[id].ldev = ldev;
> > +		attr = &ldev->sysfs_entries[id].attr;
> > +		attr->attr.name = kasprintf(GFP_KERNEL, "%02x", id);
> > +		if (!attr->attr.name)
> > +			return -ENOMEM;
> > +		attr->attr.mode = 0644;
> > +		attr->show = lin_identifier_show;
> > +		attr->store = lin_identifier_store;
> > +
> > +		sysfs_attr_init(&attr->attr);
> > +		ret = sysfs_create_file(ldev->lin_ids_kobj, &attr->attr);
> > +		if (ret) {
> > +			kfree(attr->attr.name);
> > +			return -ENOMEM;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> 
> Can you use .dev_groups instead ?

I'm not sure where to attach this in this glue code here. Should I do a
class_register() and add the .dev_groups there?

> FWIW, this function doesn't do rollback when error occurs.

OK, this issue can be fixed in revision v4.

...
> > diff --git a/include/net/lin.h b/include/net/lin.h
> > new file mode 100644
> > index 0000000000000..e7c7c820a6e18
> > --- /dev/null
> > +++ b/include/net/lin.h
> > @@ -0,0 +1,92 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/* Copyright (C) 2024 hexDEV GmbH - https://hexdev.de */
> > +
> > +#ifndef _NET_LIN_H_
> > +#define _NET_LIN_H_
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/can/dev.h>
> > +#include <linux/device.h>
> > +
> > +#define LIN_NUM_IDS		64
> > +#define LIN_HEADER_SIZE		3
> > +#define LIN_MAX_DLEN		8
> > +
> > +#define LIN_MAX_BAUDRATE	20000
> > +#define LIN_MIN_BAUDRATE	1000
> > +#define LIN_DEFAULT_BAUDRATE	9600
> > +#define LIN_SYNC_BYTE		0x55
> > +
> > +#define LIN_ID_MASK		GENMASK(5, 0)
> > +/* special ID descriptions for LIN */
> > +#define LIN_RXOFFLOAD_DATA_FLAG	0x00000200U
> > +#define LIN_ENHANCED_CKSUM_FLAG	0x00000100U
> 
> BIT(x) x 2

OK

> 
> > +
> > +extern u8 lin_get_id_parity(u8 id);
> > +
> > +#define LIN_GET_ID(PID)		FIELD_GET(LIN_ID_MASK, PID)
> > +#define LIN_FORM_PID(ID)	(LIN_GET_ID(ID) | \
> > +					lin_get_id_parity(LIN_GET_ID(ID)))
> > +#define LIN_GET_PARITY(PID)	((PID) & ~LIN_ID_MASK)
> > +#define LIN_CHECK_PID(PID)	(LIN_GET_PARITY(PID) == \
> > +					LIN_GET_PARITY(LIN_FORM_PID(PID)))
> > +
> > +struct lin_attr {
> > +	struct kobj_attribute attr;
> > +	struct lin_device *ldev;
> > +};
> > +
> > +struct lin_device {
> > +	struct can_priv can;  /* must be the first member */
> > +	struct net_device *ndev;
> > +	struct device *dev;
> > +	const struct lin_device_ops *ldev_ops;
> > +	struct workqueue_struct *wq;
> > +	struct work_struct tx_work;
> > +	bool tx_busy;
> > +	struct sk_buff *tx_skb;
> > +	struct kobject *lin_ids_kobj;
> > +	struct lin_attr sysfs_entries[LIN_NUM_IDS];
> > +};
> > +
> > +enum lin_checksum_mode {
> > +	LINBUS_CLASSIC = 0,
> > +	LINBUS_ENHANCED,
> > +};
> > +
> > +struct lin_frame {
> > +	u8 lin_id;
> > +	u8 len;
> > +	u8 data[LIN_MAX_DLEN];
> > +	u8 checksum;
> > +	enum lin_checksum_mode checksum_mode;
> > +};
> > +
> > +struct lin_responder_answer {
> > +	bool is_active;
> > +	bool is_event_frame;
> > +	u8 event_associated_id;
> > +	struct lin_frame lf;
> > +};
> > +
> > +struct lin_device_ops {
> > +	int (*ldo_open)(struct lin_device *ldev);
> > +	int (*ldo_stop)(struct lin_device *ldev);
> > +	int (*ldo_tx)(struct lin_device *ldev, const struct lin_frame *frame);
> > +	int (*update_bitrate)(struct lin_device *ldev, u16 bitrate);
> > +	int (*update_responder_answer)(struct lin_device *ldev,
> > +				       const struct lin_responder_answer *answ);
> > +	int (*get_responder_answer)(struct lin_device *ldev, u8 id,
> > +				    struct lin_responder_answer *answ);
> > +};
> > +
> > +int lin_rx(struct lin_device *ldev, const struct lin_frame *lf);
> > +
> > +u8 lin_get_checksum(u8 pid, u8 n_of_bytes, const u8 *bytes,
> > +		    enum lin_checksum_mode cm);
> > +
> > +struct lin_device *register_lin(struct device *dev,
> > +				const struct lin_device_ops *ldops);
> > +void unregister_lin(struct lin_device *ldev);
> > +
> > +#endif /* _NET_LIN_H_ */
> > diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
> > index 02ec32d694742..51b0e2a7624e4 100644
> > --- a/include/uapi/linux/can/netlink.h
> > +++ b/include/uapi/linux/can/netlink.h
> > @@ -103,6 +103,7 @@ struct can_ctrlmode {
> >  #define CAN_CTRLMODE_CC_LEN8_DLC	0x100	/* Classic CAN DLC option */
> >  #define CAN_CTRLMODE_TDC_AUTO		0x200	/* CAN transiver automatically calculates TDCV */
> >  #define CAN_CTRLMODE_TDC_MANUAL		0x400	/* TDCV is manually set up by user */
> 
> BIT(x) is these days available also for uapi I think.
> 
> > +#define CAN_CTRLMODE_LIN		0x800	/* LIN bus mode */

So, should I use just BIT(11) for the new define, or should I also
refactor the whole list while at it?

Thanks
  -- Christoph
Ilpo Järvinen May 8, 2024, 1:08 p.m. UTC | #5
On Wed, 8 May 2024, Christoph Fritz wrote:

> On Mon, 2024-05-06 at 19:24 +0300, Ilpo Järvinen wrote:
> > On Thu, 2 May 2024, Christoph Fritz wrote:
> > 
> > > This patch adds a LIN (local interconnect network) bus abstraction on
> > > top of CAN.  It is a glue driver adapting CAN on one side while offering
> > > LIN abstraction on the other side. So that upcoming LIN device drivers
> > > can make use of it.

> > > +static int lin_create_sysfs_id_files(struct net_device *ndev)
> > > +{
> > > +	struct lin_device *ldev = netdev_priv(ndev);
> > > +	struct kobj_attribute *attr;
> > > +	int ret;
> > > +
> > > +	for (int id = 0; id < LIN_NUM_IDS; id++) {
> > > +		ldev->sysfs_entries[id].ldev = ldev;
> > > +		attr = &ldev->sysfs_entries[id].attr;
> > > +		attr->attr.name = kasprintf(GFP_KERNEL, "%02x", id);
> > > +		if (!attr->attr.name)
> > > +			return -ENOMEM;
> > > +		attr->attr.mode = 0644;
> > > +		attr->show = lin_identifier_show;
> > > +		attr->store = lin_identifier_store;
> > > +
> > > +		sysfs_attr_init(&attr->attr);
> > > +		ret = sysfs_create_file(ldev->lin_ids_kobj, &attr->attr);
> > > +		if (ret) {
> > > +			kfree(attr->attr.name);
> > > +			return -ENOMEM;
> > > +		}
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > 
> > Can you use .dev_groups instead ?
> 
> I'm not sure where to attach this in this glue code here. Should I do a
> class_register() and add the .dev_groups there?

I guess struct class would be correct direction but I'm not sure if it's 
viable in this case. It would avoid the need for custom sysfs setup code
if it's workable.

> > FWIW, this function doesn't do rollback when error occurs.
> 
> OK, this issue can be fixed in revision v4.
> 
> ...

> > > diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
> > > index 02ec32d694742..51b0e2a7624e4 100644
> > > --- a/include/uapi/linux/can/netlink.h
> > > +++ b/include/uapi/linux/can/netlink.h
> > > @@ -103,6 +103,7 @@ struct can_ctrlmode {
> > >  #define CAN_CTRLMODE_CC_LEN8_DLC	0x100	/* Classic CAN DLC option */
> > >  #define CAN_CTRLMODE_TDC_AUTO		0x200	/* CAN transiver automatically calculates TDCV */
> > >  #define CAN_CTRLMODE_TDC_MANUAL		0x400	/* TDCV is manually set up by user */
> > 
> > BIT(x) is these days available also for uapi I think.
> > 
> > > +#define CAN_CTRLMODE_LIN		0x800	/* LIN bus mode */
> 
> So, should I use just BIT(11) for the new define, or should I also
> refactor the whole list while at it?

Either is fine for me.
Christoph Fritz May 8, 2024, 6:20 p.m. UTC | #6
...
> ...
> > > > +static int lin_create_sysfs_id_files(struct net_device *ndev)
> > > > +{
> > > > +	struct lin_device *ldev = netdev_priv(ndev);
> > > > +	struct kobj_attribute *attr;
> > > > +	int ret;
> > > > +
> > > > +	for (int id = 0; id < LIN_NUM_IDS; id++) {
> > > > +		ldev->sysfs_entries[id].ldev = ldev;
> > > > +		attr = &ldev->sysfs_entries[id].attr;
> > > > +		attr->attr.name = kasprintf(GFP_KERNEL, "%02x", id);
> > > > +		if (!attr->attr.name)
> > > > +			return -ENOMEM;
> > > > +		attr->attr.mode = 0644;
> > > > +		attr->show = lin_identifier_show;
> > > > +		attr->store = lin_identifier_store;
> > > > +
> > > > +		sysfs_attr_init(&attr->attr);
> > > > +		ret = sysfs_create_file(ldev->lin_ids_kobj, &attr->attr);
> > > > +		if (ret) {
> > > > +			kfree(attr->attr.name);
> > > > +			return -ENOMEM;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > 
> > > Can you use .dev_groups instead ?
> > 
> > I'm not sure where to attach this in this glue code here. Should I do a
> > class_register() and add the .dev_groups there?
> 
> I guess struct class would be correct direction but I'm not sure if it's 
> viable in this case. It would avoid the need for custom sysfs setup code
> if it's workable.

I just tried to find a way, but these are 64 sysfs files and declaring
them all static looks a bit odd to me. I might be missing something
here.

For v4 I would stick to the dynamic setup and fix the rollback.

Any objections?


Thanks
  -- Christoph
Greg KH May 8, 2024, 6:48 p.m. UTC | #7
On Wed, May 08, 2024 at 08:20:51PM +0200, Christoph Fritz wrote:
> ...
> > ...
> > > > > +static int lin_create_sysfs_id_files(struct net_device *ndev)
> > > > > +{
> > > > > +	struct lin_device *ldev = netdev_priv(ndev);
> > > > > +	struct kobj_attribute *attr;
> > > > > +	int ret;
> > > > > +
> > > > > +	for (int id = 0; id < LIN_NUM_IDS; id++) {
> > > > > +		ldev->sysfs_entries[id].ldev = ldev;
> > > > > +		attr = &ldev->sysfs_entries[id].attr;
> > > > > +		attr->attr.name = kasprintf(GFP_KERNEL, "%02x", id);
> > > > > +		if (!attr->attr.name)
> > > > > +			return -ENOMEM;
> > > > > +		attr->attr.mode = 0644;
> > > > > +		attr->show = lin_identifier_show;
> > > > > +		attr->store = lin_identifier_store;
> > > > > +
> > > > > +		sysfs_attr_init(&attr->attr);
> > > > > +		ret = sysfs_create_file(ldev->lin_ids_kobj, &attr->attr);
> > > > > +		if (ret) {
> > > > > +			kfree(attr->attr.name);
> > > > > +			return -ENOMEM;
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > 
> > > > Can you use .dev_groups instead ?
> > > 
> > > I'm not sure where to attach this in this glue code here. Should I do a
> > > class_register() and add the .dev_groups there?
> > 
> > I guess struct class would be correct direction but I'm not sure if it's 
> > viable in this case. It would avoid the need for custom sysfs setup code
> > if it's workable.
> 
> I just tried to find a way, but these are 64 sysfs files and declaring
> them all static looks a bit odd to me. I might be missing something
> here.
> 
> For v4 I would stick to the dynamic setup and fix the rollback.
> 
> Any objections?

Yes, you race with userspace and loose by trying to do this "by hand".
Make this static please.

thanks,

greg k-h
Christoph Fritz May 9, 2024, 5:06 p.m. UTC | #8
On Wed, 2024-05-08 at 19:48 +0100, Greg Kroah-Hartman wrote:
> On Wed, May 08, 2024 at 08:20:51PM +0200, Christoph Fritz wrote:
> > ...
> > > ...
> > > > > > +static int lin_create_sysfs_id_files(struct net_device *ndev)
> > > > > > +{
> > > > > > +	struct lin_device *ldev = netdev_priv(ndev);
> > > > > > +	struct kobj_attribute *attr;
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	for (int id = 0; id < LIN_NUM_IDS; id++) {
> > > > > > +		ldev->sysfs_entries[id].ldev = ldev;
> > > > > > +		attr = &ldev->sysfs_entries[id].attr;
> > > > > > +		attr->attr.name = kasprintf(GFP_KERNEL, "%02x", id);
> > > > > > +		if (!attr->attr.name)
> > > > > > +			return -ENOMEM;
> > > > > > +		attr->attr.mode = 0644;
> > > > > > +		attr->show = lin_identifier_show;
> > > > > > +		attr->store = lin_identifier_store;
> > > > > > +
> > > > > > +		sysfs_attr_init(&attr->attr);
> > > > > > +		ret = sysfs_create_file(ldev->lin_ids_kobj, &attr->attr);
> > > > > > +		if (ret) {
> > > > > > +			kfree(attr->attr.name);
> > > > > > +			return -ENOMEM;
> > > > > > +		}
> > > > > > +	}
> > > > > > +
> > > > > > +	return 0;
> > > > > > +}
> > > > > 
> > > > > Can you use .dev_groups instead ?
> > > > 
> > > > I'm not sure where to attach this in this glue code here. Should I do a
> > > > class_register() and add the .dev_groups there?
> > > 
> > > I guess struct class would be correct direction but I'm not sure if it's 
> > > viable in this case. It would avoid the need for custom sysfs setup code
> > > if it's workable.
> > 
> > I just tried to find a way, but these are 64 sysfs files and declaring
> > them all static looks a bit odd to me. I might be missing something
> > here.
> > 
> > For v4 I would stick to the dynamic setup and fix the rollback.
> > 
> > Any objections?
> 
> Yes, you race with userspace and loose by trying to do this "by hand".
> Make this static please.

OK, static init coming up in v4

thanks
  -- Christoph
Christoph Fritz May 9, 2024, 5:06 p.m. UTC | #9
On Mon, 2024-05-06 at 20:03 +0300, Ilpo Järvinen wrote:
> On Thu, 2 May 2024, Christoph Fritz wrote:
> 
> > This commit introduces LIN-Bus support for UART devices equipped with
> > LIN transceivers, utilizing the Serial Device Bus (serdev) interface.
> > 
> > For more details on an adapter, visit: https://hexdev.de/hexlin#tty
> > 
> > Signed-off-by: Christoph Fritz <christoph.fritz@hexdev.de>
> > ---
> >  drivers/net/can/Kconfig      |  16 ++
> >  drivers/net/can/Makefile     |   1 +
> >  drivers/net/can/lin-serdev.c | 500 +++++++++++++++++++++++++++++++++++
> >  3 files changed, 517 insertions(+)
> >  create mode 100644 drivers/net/can/lin-serdev.c
> > 
> > diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> > index 0934bbf8d03b2..91c6cdeefe440 100644
> > --- a/drivers/net/can/Kconfig
> > +++ b/drivers/net/can/Kconfig
> > @@ -181,6 +181,22 @@ config CAN_LIN
> >  
> >  	  Actual device drivers need to be enabled too.
> >  
> > +config CAN_LIN_SERDEV
> > +	tristate "Serial LIN Adaptors"
> > +	depends on CAN_LIN && SERIAL_DEV_BUS && OF
> > +	help
> > +	  LIN-Bus support for serial devices equipped with LIN transceievers.
> > +	  This device driver is using CAN_LIN for a userland connection on
> > +	  one side and the kernel its serial device bus (serdev) interface
> > +	  on the other side.
> > +
> > +	  If you have a hexlin tty adapter, say Y here and see
> > +	  <https://hexdev.de/hexlin#tty>.
> > +
> > +	  This driver can also be built as a module. If so, the module will be
> > +	  called lin-serdev.ko.
> > +
> > +
> >  config CAN_SLCAN
> >  	tristate "Serial / USB serial CAN Adaptors (slcan)"
> >  	depends on TTY
> > diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
> > index 0093ee9219ca8..21ca514a42439 100644
> > --- a/drivers/net/can/Makefile
> > +++ b/drivers/net/can/Makefile
> > @@ -26,6 +26,7 @@ obj-$(CONFIG_CAN_IFI_CANFD)	+= ifi_canfd/
> >  obj-$(CONFIG_CAN_JANZ_ICAN3)	+= janz-ican3.o
> >  obj-$(CONFIG_CAN_KVASER_PCIEFD)	+= kvaser_pciefd.o
> >  obj-$(CONFIG_CAN_LIN)		+= lin.o
> > +obj-$(CONFIG_CAN_LIN_SERDEV)	+= lin-serdev.o
> >  obj-$(CONFIG_CAN_MSCAN)		+= mscan/
> >  obj-$(CONFIG_CAN_M_CAN)		+= m_can/
> >  obj-$(CONFIG_CAN_PEAK_PCIEFD)	+= peak_canfd/
> > diff --git a/drivers/net/can/lin-serdev.c b/drivers/net/can/lin-serdev.c
> > new file mode 100644
> > index 0000000000000..13fd9a54c6c93
> > --- /dev/null
> > +++ b/drivers/net/can/lin-serdev.c
> > @@ -0,0 +1,500 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/* Copyright (C) 2024 hexDEV GmbH - https://hexdev.de */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/init.h>
> > +#include <linux/kfifo.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/serdev.h>
> > +#include <linux/slab.h>
> > +#include <linux/tty.h>
> > +#include <net/lin.h>
> > +
> > +#define LINSER_SAMPLES_PER_CHAR		10
> > +#define LINSER_TX_BUFFER_SIZE		11
> > +#define LINSER_RX_FIFO_SIZE		256
> > +#define LINSER_PARSE_BUFFER		24
> > +
> > +struct linser_rx {
> > +	u8 data;
> > +	u8 flag;
> > +};
> > +
> > +enum linser_rx_status {
> > +	NEED_MORE = -1,
> > +	MODE_OK = 0,
> > +	NEED_FORCE,
> > +};
> > +
> > +struct linser_priv {
> > +	struct lin_device *lin_dev;
> > +	struct serdev_device *serdev;
> > +	DECLARE_KFIFO_PTR(rx_fifo, struct linser_rx);
> > +	struct delayed_work rx_work;
> > +	unsigned long break_usleep_min;
> > +	unsigned long break_usleep_max;
> > +	unsigned long post_break_usleep_min;
> > +	unsigned long post_break_usleep_max;
> > +	unsigned long force_timeout_jfs;
> > +	struct lin_responder_answer respond_answ[LIN_NUM_IDS];
> > +	struct mutex resp_lock; /* protects respond_answ */
> > +	bool is_stopped;
> > +};
> > +
> > +static int linser_open(struct lin_device *ldev)
> > +{
> > +	struct serdev_device *serdev = to_serdev_device(ldev->dev);
> > +	struct linser_priv *priv = serdev_device_get_drvdata(serdev);
> > +	int ret;
> > +
> > +	if (priv->is_stopped) {
> > +		ret = serdev_device_open(serdev);
> > +		if (ret) {
> > +			dev_err(&serdev->dev, "Unable to open device\n");
> > +			return ret;
> > +		}
> > +
> > +		serdev_device_set_flow_control(serdev, false);
> > +		serdev_device_set_break_detection(serdev, true);
> > +
> > +		priv->is_stopped = false;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int linser_stop(struct lin_device *ldev)
> > +{
> > +	struct serdev_device *serdev = to_serdev_device(ldev->dev);
> > +	struct linser_priv *priv = serdev_device_get_drvdata(serdev);
> > +
> > +	if (priv->is_stopped)
> > +		return 0;
> > +
> > +	serdev_device_close(serdev);
> > +	priv->is_stopped = true;
> > +
> > +	return 0;
> > +}
> > +
> > +static int linser_send_break(struct linser_priv *priv)
> > +{
> > +	struct serdev_device *serdev = priv->serdev;
> > +	int ret;
> > +
> > +	ret = serdev_device_break_ctl(serdev, -1);
> > +	if (ret)
> > +		return ret;
> > +
> > +	usleep_range(priv->break_usleep_min, priv->break_usleep_max);
> > +
> > +	ret = serdev_device_break_ctl(serdev, 0);
> > +	if (ret)
> > +		return ret;
> > +
> > +	usleep_range(priv->post_break_usleep_min, priv->post_break_usleep_max);
> > +
> > +	return 0;
> > +}
> > +
> > +static int linser_ldo_tx(struct lin_device *ldev, const struct lin_frame *lf)
> > +{
> > +	struct serdev_device *serdev = to_serdev_device(ldev->dev);
> > +	struct linser_priv *priv = serdev_device_get_drvdata(serdev);
> > +	u8 pid = LIN_FORM_PID(lf->lin_id);
> > +	u8 buf[LINSER_TX_BUFFER_SIZE];
> > +	ssize_t written_len, total_len;
> > +	u8 checksum;
> > +	int ret;
> > +
> > +	if (lf->len + 3 > LINSER_TX_BUFFER_SIZE) {
> > +		dev_err(&serdev->dev, "Frame length %u exceeds buffer size\n", lf->len);
> > +		return -EINVAL;
> > +	}
> > +
> > +	buf[0] = LIN_SYNC_BYTE;
> > +	buf[1] = pid;
> > +	total_len = 2;
> > +
> > +	if (lf->len) {
> > +		memcpy(&buf[2], lf->data, lf->len);
> > +		checksum = lin_get_checksum(pid, lf->len, lf->data,
> > +					    lf->checksum_mode);
> > +		buf[lf->len + 2] = checksum;
> > +		total_len += lf->len + 1;
> > +	}
> > +
> > +	ret = linser_send_break(priv);
> > +	if (ret)
> > +		return ret;
> > +
> > +	written_len = serdev_device_write(serdev, buf, total_len, 0);
> > +	if (written_len < total_len)
> > +		return written_len < 0 ? (int)written_len : -EIO;
> > +
> > +	dev_dbg(&serdev->dev, "sent out: %*ph\n", (int)total_len, buf);
> > +
> > +	serdev_device_wait_until_sent(serdev, 0);
> > +
> > +	return 0;
> > +}
> > +
> > +static void linser_derive_timings(struct linser_priv *priv, u16 bitrate)
> > +{
> > +	unsigned long break_baud = (bitrate * 2) / 3;
> > +	unsigned long timeout_us;
> > +
> > +	priv->break_usleep_min = (USEC_PER_SEC * LINSER_SAMPLES_PER_CHAR) /
> > +				 break_baud;
> > +	priv->break_usleep_max = priv->break_usleep_min + 50;
> > +	priv->post_break_usleep_min = (USEC_PER_SEC * 1 /* 1 bit */) / break_baud;
> 
> Don't mix comments and code like this.

OK

> 
> > +	priv->post_break_usleep_max = priv->post_break_usleep_min + 30;
> > +
> > +	timeout_us = DIV_ROUND_CLOSEST(USEC_PER_SEC * 256 /* bit */, bitrate);
> 
> Ditto.

OK

> 
> > +	priv->force_timeout_jfs = usecs_to_jiffies(timeout_us);
> > +}
> > +
> > +static int linser_update_bitrate(struct lin_device *ldev, u16 bitrate)
> > +{
> > +	struct serdev_device *serdev = to_serdev_device(ldev->dev);
> > +	struct linser_priv *priv = serdev_device_get_drvdata(serdev);
> > +	unsigned int speed;
> > +	int ret;
> > +
> > +	ret = linser_open(ldev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	speed = serdev_device_set_baudrate(serdev, bitrate);
> > +	if (!bitrate || speed != bitrate)
> > +		return -EINVAL;
> > +
> > +	linser_derive_timings(priv, bitrate);
> > +
> > +	return 0;
> > +}
> > +
> > +static int linser_get_responder_answer(struct lin_device *ldev, u8 id,
> > +				       struct lin_responder_answer *answ)
> > +{
> > +	struct serdev_device *serdev = to_serdev_device(ldev->dev);
> > +	struct linser_priv *priv = serdev_device_get_drvdata(serdev);
> > +	struct lin_responder_answer *r = &priv->respond_answ[id];
> > +
> > +	if (!answ)
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&priv->resp_lock);
> 
> guard(mutex)(&priv->resp_lock);

OK

> 
> > +	memcpy(answ, r, sizeof(struct lin_responder_answer));
> 
> sizeof(*answ);

OK

> 
> > +	mutex_unlock(&priv->resp_lock);
> > +
> > +	return 0;
> > +}
> > +
> > +static int linser_update_resp_answer(struct lin_device *ldev,
> > +				     const struct lin_responder_answer *answ)
> > +{
> > +	struct serdev_device *serdev = to_serdev_device(ldev->dev);
> > +	struct linser_priv *priv = serdev_device_get_drvdata(serdev);
> > +	struct lin_responder_answer *r = &priv->respond_answ[answ->lf.lin_id];
> > +
> > +	if (!answ)
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&priv->resp_lock);
> > +	memcpy(r, answ, sizeof(struct lin_responder_answer));
> 
> Ditto.

OK

> 
> > +	r->lf.checksum = lin_get_checksum(LIN_FORM_PID(answ->lf.lin_id),
> > +					  answ->lf.len,
> > +					  answ->lf.data,
> > +					  answ->lf.checksum_mode);
> > +	mutex_unlock(&priv->resp_lock);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct lin_device_ops linser_lindev_ops = {
> > +	.ldo_open = linser_open,
> > +	.ldo_stop = linser_stop,
> > +	.ldo_tx = linser_ldo_tx,
> > +	.update_bitrate = linser_update_bitrate,
> > +	.get_responder_answer = linser_get_responder_answer,
> > +	.update_responder_answer = linser_update_resp_answer,
> > +};
> > +
> > +static bool linser_tx_frame_as_responder(struct linser_priv *priv, u8 id)
> > +{
> > +	struct lin_responder_answer *answ = &priv->respond_answ[id];
> > +	struct serdev_device *serdev = priv->serdev;
> > +	u8 buf[LINSER_TX_BUFFER_SIZE];
> > +	u8 checksum, count, n;
> > +	ssize_t write_len;
> > +
> > +	mutex_lock(&priv->resp_lock);
> 
> scoped_guard() can be used here.

OK

> 
> > +
> > +	if (!answ->is_active) {
> > +		mutex_unlock(&priv->resp_lock);
> > +		return false;
> > +	}
> > +
> > +	if (answ->is_event_frame) {
> > +		struct lin_responder_answer *e_answ;
> > +
> > +		e_answ = &priv->respond_answ[answ->event_associated_id];
> > +		n = min(e_answ->lf.len, LIN_MAX_DLEN);
> > +		if (memcmp(answ->lf.data, e_answ->lf.data, n) != 0) {
> > +			memcpy(answ->lf.data, e_answ->lf.data, n);
> > +			checksum = lin_get_checksum(LIN_FORM_PID(answ->lf.lin_id),
> > +						    n, e_answ->lf.data,
> > +						    answ->lf.checksum_mode);
> > +			answ = e_answ;
> > +		} else {
> > +			mutex_unlock(&priv->resp_lock);
> > +			return false;
> 
> Use reverse logic so you can reduce indent.

OK

> 
> > +		}
> > +	} else {
> > +		checksum = answ->lf.checksum;
> > +	}
> > +
> > +	count = min(answ->lf.len, LIN_MAX_DLEN);
> > +	memcpy(&buf[0], answ->lf.data, count);
> > +	buf[count] = checksum;
> > +
> > +	mutex_unlock(&priv->resp_lock);
> > +
> > +	write_len = serdev_device_write(serdev, buf, count + 1, 0);
> > +	if (write_len < count + 1)
> > +		return false;
> > +
> > +	serdev_device_wait_until_sent(serdev, 0);
> > +
> > +	return true;
> > +}
> > +
> > +static void linser_pop_fifo(struct linser_priv *priv, size_t n)
> > +{
> > +	for (size_t i = 0; i < n; i++)
> > +		kfifo_skip(&priv->rx_fifo);
> > +}
> > +
> > +static int linser_fill_frame(struct linser_priv *priv, struct lin_frame *lf)
> > +{
> > +	struct serdev_device *serdev = priv->serdev;
> > +	struct linser_rx buf[LINSER_PARSE_BUFFER];
> > +	unsigned int count, i, brk = 0;
> > +
> > +	count = kfifo_out_peek(&priv->rx_fifo, buf, LINSER_PARSE_BUFFER);
> > +
> > +	memset(lf, 0, sizeof(struct lin_frame));
> > +
> > +	for (i = 0; i < count; i++) {
> > +		dev_dbg(&serdev->dev, "buf[%d]: data=%02x, flag=%02x\n",
> > +			i, buf[i].data, buf[i].flag);
> > +	}
> > +
> > +	if (count < 3)
> > +		return NEED_MORE;
> > +
> > +	if (buf[0].flag != TTY_BREAK || buf[1].data != LIN_SYNC_BYTE) {
> > +		linser_pop_fifo(priv, 1); /* pop incorrect start */
> > +		return NEED_MORE;
> > +	} else if (!LIN_CHECK_PID(buf[2].data)) {
> > +		linser_pop_fifo(priv, 3); /* pop incorrect header */
> > +		return NEED_MORE;
> > +	}
> > +
> > +	lf->lin_id = LIN_GET_ID(buf[2].data);
> > +
> > +	/* from here on we do have a correct LIN header */
> > +
> > +	if (count == 3)
> > +		return linser_tx_frame_as_responder(priv, lf->lin_id) ?
> > +		       NEED_MORE : NEED_FORCE;
> > +
> > +	for (i = 3; i < count && i < LINSER_PARSE_BUFFER && i < 12; i++) {
> > +		if (buf[i].flag == TTY_BREAK) {
> > +			brk = i;
> > +			break;
> > +		}
> > +		lf->len++;
> > +	}
> > +	if (lf->len)
> > +		lf->len -= 1; /* account for checksum */
> > +
> > +	if (brk == 3)
> > +		return MODE_OK;
> > +
> > +	if (brk == 4) {
> > +		/* suppress wrong answer data-byte in between PID and break
> > +		 * because checksum is missing
> > +		 */
> > +		return MODE_OK;
> > +	}
> > +
> > +	for (i = 0; i < lf->len; i++)
> > +		lf->data[i] = buf[3 + i].data;
> > +	lf->checksum = buf[2 + lf->len + 1].data;
> > +	mutex_lock(&priv->resp_lock);
> > +	lf->checksum_mode = priv->respond_answ[lf->lin_id].lf.checksum_mode;
> > +	mutex_unlock(&priv->resp_lock);
> > +
> > +	dev_dbg(&serdev->dev, "brk:%i, len:%u, data:%*ph, checksum:%x (%s)\n",
> > +		brk, lf->len, lf->len, lf->data, lf->checksum,
> > +		lf->checksum_mode ? "enhanced" : "classic");
> > +
> > +	if (brk > 4)
> > +		return MODE_OK;	/* frame in between two breaks: so complete */
> > +
> > +	if (lf->len == 8)
> > +		return MODE_OK;
> > +
> > +	return NEED_FORCE;
> > +}
> > +
> > +static int linser_process_frame(struct linser_priv *priv, bool force)
> > +{
> > +	struct serdev_device *serdev = priv->serdev;
> > +	struct lin_frame lf;
> > +	size_t bytes_to_pop;
> > +	int ret = NEED_MORE;
> > +
> > +	while (kfifo_len(&priv->rx_fifo) >= LIN_HEADER_SIZE) {
> > +		ret = linser_fill_frame(priv, &lf);
> > +
> > +		if (ret == MODE_OK || (ret == NEED_FORCE && force)) {
> > +			dev_dbg(&serdev->dev, "lin_rx: %s\n",
> > +				force ? "force" : "normal");
> > +			lin_rx(priv->lin_dev, &lf);
> > +			bytes_to_pop = LIN_HEADER_SIZE + lf.len +
> > +				       (lf.len ? 1 : 0);
> > +			linser_pop_fifo(priv, bytes_to_pop);
> > +			force = false;
> > +			ret = MODE_OK;
> > +		} else {
> > +			return ret;
> 
> Reverse logic & deindent the main block.

OK

...

thanks
  -- Christoph
Christoph Fritz May 9, 2024, 5:06 p.m. UTC | #10
On Mon, 2024-05-06 at 20:08 +0300, Ilpo Järvinen wrote:
> On Thu, 2 May 2024, Christoph Fritz wrote:
> 
> > Enhance CAN broadcast manager with RX_LIN_SETUP and RX_LIN_DELETE
> > operations to setup automatic LIN frame responses in responder mode.
> > 
> > Additionally, the patch introduces the LIN_EVENT_FRAME flag to
> > setup event-triggered LIN frames.
> > 
> > Signed-off-by: Christoph Fritz <christoph.fritz@hexdev.de>
> > ---
> >  include/uapi/linux/can/bcm.h |  5 ++-
> >  net/can/bcm.c                | 74 +++++++++++++++++++++++++++++++++++-
> >  2 files changed, 77 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/uapi/linux/can/bcm.h b/include/uapi/linux/can/bcm.h
> > index f1e45f533a72c..c46268a114078 100644
> > --- a/include/uapi/linux/can/bcm.h
> > +++ b/include/uapi/linux/can/bcm.h
> > @@ -86,7 +86,9 @@ enum {
> >  	TX_EXPIRED,	/* notification on performed transmissions (count=0) */
> >  	RX_STATUS,	/* reply to RX_READ request */
> >  	RX_TIMEOUT,	/* cyclic message is absent */
> > -	RX_CHANGED	/* updated CAN frame (detected content change) */
> > +	RX_CHANGED,	/* updated CAN frame (detected content change) */
> > +	RX_LIN_SETUP,	/* create auto-response for LIN frame */
> > +	RX_LIN_DELETE,  /* remove auto-response for LIN frame */
> >  };
> >  
> >  #define SETTIMER            0x0001
> > @@ -101,5 +103,6 @@ enum {
> >  #define TX_RESET_MULTI_IDX  0x0200
> >  #define RX_RTR_FRAME        0x0400
> >  #define CAN_FD_FRAME        0x0800
> > +#define LIN_EVENT_FRAME     0x1000
> >  
> >  #endif /* !_UAPI_CAN_BCM_H */
> > diff --git a/net/can/bcm.c b/net/can/bcm.c
> > index 27d5fcf0eac9d..a717e594234d1 100644
> > --- a/net/can/bcm.c
> > +++ b/net/can/bcm.c
> > @@ -59,6 +59,7 @@
> >  #include <linux/can/bcm.h>
> >  #include <linux/slab.h>
> >  #include <net/sock.h>
> > +#include <net/lin.h>
> >  #include <net/net_namespace.h>
> >  
> >  /*
> > @@ -1330,6 +1331,59 @@ static int bcm_tx_send(struct msghdr *msg, int ifindex, struct sock *sk,
> >  	return cfsiz + MHSIZ;
> >  }
> >  
> > +static int bcm_lin_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
> > +			 int ifindex, struct sock *sk, int cfsiz, int is_active)
> > +{
> > +	struct lin_responder_answer answ;
> > +	struct net_device *dev;
> > +	struct sk_buff *skb;
> > +	struct canfd_frame cf;
> > +	netdevice_tracker tracker;
> > +	size_t sz;
> > +	int ret;
> > +
> > +	if (msg_head->nframes > 1)
> > +		return -EINVAL;
> > +
> > +	if (!(msg_head->flags & CAN_FD_FRAME))
> > +		return -EINVAL;
> > +
> > +	ret = memcpy_from_msg(&cf, msg, cfsiz);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	answ.lf.lin_id = cf.can_id & LIN_ID_MASK;
> > +	answ.is_active = is_active;
> > +	answ.is_event_frame = !!(msg_head->flags & LIN_EVENT_FRAME);
> > +	answ.event_associated_id = msg_head->can_id;
> > +	answ.lf.len = min(cf.len, LIN_MAX_DLEN);
> > +	memcpy(answ.lf.data, cf.data, answ.lf.len);
> > +	sz = min(sizeof(struct lin_responder_answer), sizeof(cf.data));
> > +	cf.can_id |= LIN_RXOFFLOAD_DATA_FLAG;
> > +	memcpy(cf.data, &answ, sz);
> > +
> > +	dev = netdev_get_by_index(sock_net(sk), ifindex, &tracker, GFP_KERNEL);
> > +	if (!dev)
> > +		return -ENODEV;
> > +
> > +	skb = alloc_skb(cfsiz + sizeof(struct can_skb_priv), gfp_any());
> 
> You just called the other function with GFP_KERNEL and you now need 
> gfp_any(). Which is correct??

I guess GFP_KERNEL but I'm not sure so I'll let gfp_any() decide for
netdev_get_by_index() too.

> 
> > +	if (!skb)
> > +		goto lin_out;
> > +
> > +	can_skb_reserve(skb);
> > +	can_skb_prv(skb)->ifindex = dev->ifindex;
> > +	can_skb_prv(skb)->skbcnt = 0;
> > +	skb_put_data(skb, &cf, cfsiz);
> > +
> > +	skb->dev = dev;
> > +	can_skb_set_owner(skb, sk);
> > +	ret = can_send(skb, 1); /* send with loopback */
> > +
> > +lin_out:
> > +	netdev_put(dev, &tracker);
> > +	return ret;
> > +}
> > +
> >  /*
> >   * bcm_sendmsg - process BCM commands (opcodes) from the userspace
> >   */
> > @@ -1429,12 +1483,30 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
> >  
> >  	case TX_SEND:
> >  		/* we need exactly one CAN frame behind the msg head */
> > -		if ((msg_head.nframes != 1) || (size != cfsiz + MHSIZ))
> > +		if (msg_head.nframes != 1 || size != cfsiz + MHSIZ)
> 
> Unrelated style fix, doesn't belong to this patch.

OK

...

thanks
  -- Christoph