mbox series

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

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

Message

Christoph Fritz May 9, 2024, 5:17 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#hexLINSER
[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 v4:
 - rebase to next-20240509
 - dt-bindings: rename lin-serdev to hexlinser
 - lin: don't use ndev uninitialized, use dev_err() instead
 - lin: fix missing failure return value
 - lin: use sysfs_emit() instead of scnprintf()
 - lin: use static init of sysfs files
 - hexlin: fix hid_dbg indent
 - hexlin: use __le16 for req.baudrate
 - hexlin: refactor to be able to drop kmemdup()
 - linser and hexlin: s/IS_ERR_OR_NULL()/IS_ERR()/
 - serdev treewide: refactor receive_buf() by adding argument flags
 - address minor changes from review

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 adapter
  treewide, serdev: add flags argument to receive_buf()
  tty: serdev: Add method to enable break flags
  dt-bindings: vendor-prefixes: Add hexDEV
  dt-bindings: net/can: Add serial LIN adapter hexLINSER
  can: Add support for hexDEV serial LIN adapter hexLINSER
  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,hex-linser.yaml   |  32 +
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 drivers/bluetooth/btmtkuart.c                 |   3 +-
 drivers/bluetooth/btnxpuart.c                 |   3 +-
 drivers/bluetooth/hci_serdev.c                |   3 +-
 drivers/gnss/serial.c                         |   2 +-
 drivers/gnss/sirf.c                           |   2 +-
 drivers/greybus/gb-beagleplay.c               |   2 +-
 drivers/hid/Kconfig                           |  19 +
 drivers/hid/Makefile                          |   1 +
 drivers/hid/hid-hexdev-hexlin.c               | 620 ++++++++++++++++++
 drivers/hid/hid-ids.h                         |   1 +
 drivers/hid/hid-quirks.c                      |   3 +
 drivers/iio/chemical/pms7003.c                |   2 +-
 drivers/iio/chemical/scd30_serial.c           |   3 +-
 drivers/iio/chemical/sps30_serial.c           |   3 +-
 drivers/iio/imu/bno055/bno055_ser_core.c      |   3 +-
 drivers/mfd/rave-sp.c                         |   2 +-
 drivers/net/can/Kconfig                       |  25 +
 drivers/net/can/Makefile                      |   2 +
 drivers/net/can/hex-linser.c                  | 505 ++++++++++++++
 drivers/net/can/lin.c                         | 537 +++++++++++++++
 drivers/net/ethernet/qualcomm/qca_uart.c      |   3 +-
 drivers/nfc/pn533/uart.c                      |   2 +-
 drivers/nfc/s3fwrn5/uart.c                    |   3 +-
 drivers/platform/chrome/cros_ec_uart.c        |   3 +-
 drivers/platform/surface/aggregator/core.c    |   2 +-
 .../lenovo-yoga-tab2-pro-1380-fastcharger.c   |   3 +-
 drivers/tty/serdev/core.c                     |  11 +
 drivers/tty/serdev/serdev-ttyport.c           |  19 +-
 drivers/w1/masters/w1-uart.c                  |   3 +-
 include/linux/serdev.h                        |  12 +-
 include/net/lin.h                             |  97 +++
 include/uapi/linux/can/bcm.h                  |   5 +-
 include/uapi/linux/can/netlink.h              |   2 +
 net/can/bcm.c                                 |  72 ++
 sound/drivers/serial-generic.c                |   3 +-
 37 files changed, 1991 insertions(+), 24 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/can/hexdev,hex-linser.yaml
 create mode 100644 drivers/hid/hid-hexdev-hexlin.c
 create mode 100644 drivers/net/can/hex-linser.c
 create mode 100644 drivers/net/can/lin.c
 create mode 100644 include/net/lin.h

Comments

Ilpo Järvinen May 10, 2024, 1:23 p.m. UTC | #1
On Thu, 9 May 2024, Christoph Fritz wrote:

> Introduce a LIN (local interconnect network) abstraction on top of CAN.
> This 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            | 470 +++++++++++++++++++++++++++++++
>  include/net/lin.h                |  90 ++++++
>  include/uapi/linux/can/netlink.h |   1 +
>  5 files changed, 572 insertions(+)
>  create mode 100644 drivers/net/can/lin.c
>  create mode 100644 include/net/lin.h
> 
> diff --git a/drivers/net/can/lin.c b/drivers/net/can/lin.c
> new file mode 100644
> index 0000000000000..a22768c17e3f8
> --- /dev/null
> +++ b/drivers/net/can/lin.c
> @@ -0,0 +1,470 @@
> +// 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 device *dev,
> +				   struct device_attribute *attr, char *buf)
> +{
> +	struct lin_device *ldev = netdev_priv(to_net_dev(dev));
> +	struct lin_responder_answer answ;
> +	int k, count, 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)

How can this happen?

Perhaps you can determine the id arithmetically from a pointer within an 
array?

> +		return -EINVAL;
> +
> +	count = sysfs_emit(buf, "%-6s %-11s %-9s %-9s %-2s %-24s %-6s\n",
> +			   "state", "cksum-mode", "is_event", "event_id",
> +			   "n", "data", "cksum");
> +
> +	ret = ldev->ldev_ops->get_responder_answer(ldev, id, &answ);
> +	if (ret)
> +		return ret;
> +
> +	count += sysfs_emit_at(buf, 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 += sysfs_emit_at(buf, count, "%02x ", answ.lf.data[k]);
> +	for (; k < 8; k++)
> +		count += sysfs_emit_at(buf, count, "   ");
> +	if (answ.lf.len)
> +		count += sysfs_emit_at(buf, count, " %02x", answ.lf.checksum);
> +
> +	count += sysfs_emit_at(buf, 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))

Missing #include for isspace().

> +		buf++;
> +	while (*buf && isalnum(*buf) && num_len < sizeof(num_str) - 1)
> +		num_str[num_len++] = *buf++;

This is likely broken when the number is too long because the next 
number is not correctly parsed? I think you should bail out with an error 
when a number is invalid (too long).

> +	if (kstrtol(num_str, base, result))

Missing #include for kstrtol() but see b.

Do you care for signed numbers? If not use unsigned kstrtox variant and 
typing.

> +		return NULL;
> +
> +	return buf;
> +}
> +
> +static ssize_t lin_identifier_store(struct device *dev,
> +				    struct device_attribute *attr,
> +				    const char *buf, size_t count)
> +{
> +	struct lin_device *ldev = netdev_priv(to_net_dev(dev));
> +	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;

Can this happen?

> +	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;
> +}

When adding sysfs entries, they should be documented under 
Documentation/ABI/

> +#define LID(_name) \
> +	struct device_attribute linid_##_name = __ATTR(_name, 0644, \
> +	lin_identifier_show, lin_identifier_store)
> +
> +LID(00); LID(01); LID(02); LID(03); LID(04); LID(05); LID(06); LID(07);
> +LID(08); LID(09); LID(0a); LID(0b); LID(0c); LID(0d); LID(0e); LID(0f);
> +LID(10); LID(11); LID(12); LID(13); LID(14); LID(15); LID(16); LID(17);
> +LID(18); LID(19); LID(1a); LID(1b); LID(1c); LID(1d); LID(1e); LID(1f);
> +LID(20); LID(21); LID(22); LID(23); LID(24); LID(25); LID(26); LID(27);
> +LID(28); LID(29); LID(2a); LID(2b); LID(2c); LID(2d); LID(2e); LID(2f);
> +LID(30); LID(31); LID(32); LID(33); LID(34); LID(35); LID(36); LID(37);
> +LID(38); LID(39); LID(3a); LID(3b); LID(3c); LID(3d); LID(3e); LID(3f);
> +
> +static struct attribute *lin_sysfs_attrs[] = {
> +	&linid_00.attr, &linid_01.attr, &linid_02.attr, &linid_03.attr,
> +	&linid_04.attr, &linid_05.attr, &linid_06.attr, &linid_07.attr,
> +	&linid_08.attr, &linid_09.attr, &linid_0a.attr, &linid_0b.attr,
> +	&linid_0c.attr, &linid_0d.attr, &linid_0e.attr, &linid_0f.attr,
> +	&linid_10.attr, &linid_11.attr, &linid_12.attr, &linid_13.attr,
> +	&linid_14.attr, &linid_15.attr, &linid_16.attr, &linid_17.attr,
> +	&linid_18.attr, &linid_19.attr, &linid_1a.attr, &linid_1b.attr,
> +	&linid_1c.attr, &linid_1d.attr, &linid_1e.attr, &linid_1f.attr,
> +	&linid_20.attr, &linid_21.attr, &linid_22.attr, &linid_23.attr,
> +	&linid_24.attr, &linid_25.attr, &linid_26.attr, &linid_27.attr,
> +	&linid_28.attr, &linid_29.attr, &linid_2a.attr, &linid_2b.attr,
> +	&linid_2c.attr, &linid_2d.attr, &linid_2e.attr, &linid_2f.attr,
> +	&linid_30.attr, &linid_31.attr, &linid_32.attr, &linid_33.attr,
> +	&linid_34.attr, &linid_35.attr, &linid_36.attr, &linid_37.attr,
> +	&linid_38.attr, &linid_39.attr, &linid_3a.attr, &linid_3b.attr,
> +	&linid_3c.attr, &linid_3d.attr, &linid_3e.attr, &linid_3f.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group lin_sysfs_group = {
> +	.name = "lin_ids",
> +	.attrs = lin_sysfs_attrs,
> +};
> +
> +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);

I wonder if it is the correct approach to continue in this case? Something 
is getting truncated here, right?

> +	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)

I'm skeptical this could be correct without any concurrency control 
related primitives.

> +		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;

Use unsigned int for loop variables that are counting positive things.

> +	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);

Unnecessary parenthesis.

> +}
> +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 };

Extra space.

I think ... = {} is enough if you want to just to fully initialize it.

> +
> +	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  ||

Extra space.

> +	    !ldops->ldo_open || !ldops->ldo_stop) {
> +		dev_err(dev, "missing mandatory lin_device_ops\n");

Is this a programming error? WARN_ON_ONCE() around the condition would be 
warranted in that case.

> +		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;
> +	ndev->sysfs_groups[0] = &lin_sysfs_group;
> +	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;
> +
> +	/* 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) {
> +		ret = -ENOMEM;
> +		goto exit_unreg;
> +	}
> +
> +	INIT_WORK(&ldev->tx_work, lin_tx_work_handler);
> +
> +	netdev_info(ndev, "LIN initialized\n");
> +
> +	return ldev;
> +
> +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;
> +
> +	unregister_candev(ndev);
> +	destroy_workqueue(ldev->wq);

Why is the order different from that in register side?

> +	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..31bb0feefd188
> --- /dev/null
> +++ b/include/net/lin.h
> @@ -0,0 +1,90 @@
> +/* 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	BIT(9)
> +#define LIN_ENHANCED_CKSUM_FLAG	BIT(8)
> +
> +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;
> +};
> +
> +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..a37f56d86c5f2 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 */
> +#define CAN_CTRLMODE_LIN		BIT(11)	/* LIN bus mode */
>  
>  /*
>   * CAN device statistics
>
Ilpo Järvinen May 10, 2024, 1:46 p.m. UTC | #2
On Thu, 9 May 2024, Christoph Fritz wrote:

> Introduce driver support for hexDEV hexLIN USB LIN adapter, enabling LIN
> communication over USB for both controller and responder modes. The
> driver interfaces with the CAN_LIN framework for userland connectivity.
> 
> For more details on the adapter, visit: https://hexdev.de/hexlin/
> 
> Tested-by: Andreas Lauser <andreas.lauser@mercedes-benz.com>
> Signed-off-by: Christoph Fritz <christoph.fritz@hexdev.de>
> ---
>  drivers/hid/Kconfig             |  19 +
>  drivers/hid/Makefile            |   1 +
>  drivers/hid/hid-hexdev-hexlin.c | 609 ++++++++++++++++++++++++++++++++
>  drivers/hid/hid-ids.h           |   1 +
>  drivers/hid/hid-quirks.c        |   3 +
>  5 files changed, 633 insertions(+)
>  create mode 100644 drivers/hid/hid-hexdev-hexlin.c
> 
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 08446c89eff6e..682e3ab5fdfe5 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -496,6 +496,25 @@ config HID_GYRATION
>  	help
>  	Support for Gyration remote control.
>  
> +config HID_MCS_HEXDEV
> +	tristate "hexDEV LIN-BUS adapter support"
> +	depends on HID && CAN_NETLINK && CAN_DEV
> +	select CAN_LIN
> +	help
> +	  Support for hexDEV its hexLIN USB LIN bus adapter.
> +
> +	  Local Interconnect Network (LIN) to USB adapter for controller and
> +	  responder usage.
> +	  This device driver is using CAN_LIN for a userland connection on
> +	  one side and USB HID for the actual hardware adapter on the other
> +	  side.
> +
> +	  If you have such an adapter, say Y here and see
> +	  <https://hexdev.de/hexlin>.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called hid-hexlin.
> +
>  config HID_ICADE
>  	tristate "ION iCade arcade controller"
>  	help
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index ce71b53ea6c54..6af678f283548 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -59,6 +59,7 @@ obj-$(CONFIG_HID_GOOGLE_STADIA_FF)	+= hid-google-stadiaff.o
>  obj-$(CONFIG_HID_VIVALDI)	+= hid-vivaldi.o
>  obj-$(CONFIG_HID_GT683R)	+= hid-gt683r.o
>  obj-$(CONFIG_HID_GYRATION)	+= hid-gyration.o
> +obj-$(CONFIG_HID_MCS_HEXDEV)	+= hid-hexdev-hexlin.o
>  obj-$(CONFIG_HID_HOLTEK)	+= hid-holtek-kbd.o
>  obj-$(CONFIG_HID_HOLTEK)	+= hid-holtek-mouse.o
>  obj-$(CONFIG_HID_HOLTEK)	+= hid-holtekff.o
> diff --git a/drivers/hid/hid-hexdev-hexlin.c b/drivers/hid/hid-hexdev-hexlin.c
> new file mode 100644
> index 0000000000000..a9ed080b3e33e
> --- /dev/null
> +++ b/drivers/hid/hid-hexdev-hexlin.c
> @@ -0,0 +1,609 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * LIN bus USB adapter driver https://hexdev.de/hexlin
> + *
> + * Copyright (C) 2024 hexDEV GmbH
> + */
> +
> +#include <linux/completion.h>
> +#include <linux/hid.h>
> +#include <linux/module.h>
> +#include <linux/wait.h>
> +#include <net/lin.h>
> +#include "hid-ids.h"
> +
> +#define HEXLIN_PKGLEN_MAX	64
> +#define HEXLIN_LEN_RETCODE	1
> +
> +enum {
> +	/* answers */
> +	HEXLIN_SUCCESS			= 0x01,
> +	HEXLIN_FRAME			= 0x02,
> +	HEXLIN_ERROR			= 0x03,
> +	HEXLIN_FAIL			= 0x0F,
> +	/* lin-responder */
> +	HEXLIN_SET_MODE_RESPONDER	= 0x10,
> +	HEXLIN_SET_RESPONDER_ANSWER_ID	= 0x11,
> +	HEXLIN_GET_RESPONDER_ANSWER_ID	= 0x12,
> +	/* lin-controller */
> +	HEXLIN_SET_MODE_CONTROLLER	= 0x20,
> +	HEXLIN_SEND_BREAK		= 0x21,
> +	HEXLIN_SEND_UNCONDITIONAL_FRAME	= 0x22,
> +	/* lin-div */
> +	HEXLIN_SET_BAUDRATE		= 0x34,
> +	HEXLIN_GET_BAUDRATE		= 0x35,
> +	/* div */
> +	HEXLIN_RESET			= 0xF0,
> +	HEXLIN_GET_VERSION		= 0xF1,
> +};
> +
> +struct hexlin_val8_req {
> +	u8 cmd;
> +	u8 v;
> +} __packed;
> +
> +struct hexlin_baudrate_req {
> +	u8 cmd;
> +	__le16 baudrate;
> +} __packed;
> +
> +struct hexlin_frame {
> +	__le32 flags;
> +	u8 len;
> +	u8 lin_id;
> +	u8 data[LIN_MAX_DLEN];
> +	u8 checksum;
> +	u8 checksum_mode;
> +} __packed;
> +
> +struct hexlin_unconditional_req {
> +	u8 cmd;
> +	struct hexlin_frame frm;
> +} __packed;
> +
> +struct hexlin_responder_answer {
> +	u8 is_active;
> +	u8 is_event_frame;
> +	u8 event_associated_id;
> +	struct hexlin_frame frm;
> +} __packed;
> +
> +struct hexlin_responder_answer_req {
> +	u8 cmd;
> +	struct hexlin_responder_answer answ;
> +} __packed;
> +
> +struct hexlin_priv_data {
> +	struct hid_device *hid_dev;
> +	struct lin_device *ldev;
> +	u16 baudrate;
> +	struct completion wait_in_report;
> +	bool is_error;
> +	struct mutex tx_lock;  /* protects hexlin_tx_report() */
> +	struct hexlin_responder_answer answ;
> +	u8 fw_version;
> +};
> +
> +static int hexlin_tx_req_status(struct hexlin_priv_data *priv,
> +				const void *out_report, int len)
> +{
> +	unsigned long t;
> +	int n, ret = 0;
> +
> +	mutex_lock(&priv->tx_lock);

Use guard().

In general btw, if you're instructed to change something, it's useful to 
check your own patch series for similar cases as review pass is likely to 
miss some or not even spend time to point out the same thing over and 
over again. Then they are raised on the next review round which could have 
been avoided.

> +	reinit_completion(&priv->wait_in_report);
> +
> +	n = hid_hw_output_report(priv->hid_dev, (__u8 *) out_report, len);

The usual is to not leave space between cast and what is being cast. I 
know hid functions seem to use __u8 but that's intended for uapi and in 
kernel, u8 should be used (somebody should eventually cleanup the hid 
function types too).

> +	if (n < 0) {
> +		mutex_unlock(&priv->tx_lock);
> +		return n;
> +	}
> +	if (n != len) {
> +		mutex_unlock(&priv->tx_lock);
> +		return -EIO;
> +	}
> +
> +	t = wait_for_completion_killable_timeout(&priv->wait_in_report, HZ);
> +	if (!t)
> +		ret = -ETIMEDOUT;
> +
> +	if (priv->is_error)
> +		ret = -EINVAL;

These can return directly when you use guard().

> +	mutex_unlock(&priv->tx_lock);
> +
> +	return ret;

return 0; + drop ret variable.

> +}
> +
> +#define HEXLIN_GET_CMD(name, enum_cmd)					\
> +	static int hexlin_##name(struct hexlin_priv_data *p)		\
> +	{								\
> +		u8 *req;						\
> +		int ret;						\
> +									\
> +		req = kmalloc(sizeof(*req), GFP_KERNEL)	;		\

Extra space.

Use:

u8 *req __free(kfree) = kmalloc(sizeof(*req), GFP_KERNEL);

> +		if (!req)						\
> +			return -ENOMEM;					\
> +									\
> +		*req = enum_cmd;					\
> +									\
> +		ret = hexlin_tx_req_status(p, req, sizeof(*req));	\
> +		if (ret)						\
> +			hid_err(p->hid_dev, "%s failed, error: %d\n",	\
> +				#name, ret);				\
> +									\
> +		kfree(req);						\

Not needed after using __free().

> +		return ret;						\
> +	}
> +
> +HEXLIN_GET_CMD(get_version, HEXLIN_GET_VERSION)
> +HEXLIN_GET_CMD(reset_dev, HEXLIN_RESET)
> +HEXLIN_GET_CMD(get_baudrate, HEXLIN_GET_BAUDRATE)

Could you convert the function in the macro into a helper function which 
is just called by a simple function with the relevant parameters for 
these 3 cases?

> +#define HEXLIN_VAL_CMD(name, enum_cmd, struct_type, vtype)		\
> +	static int hexlin_##name(struct hexlin_priv_data *p, vtype val)	\
> +	{								\
> +		struct struct_type *req;				\
> +		int ret;						\
> +									\
> +		req = kmalloc(sizeof(*req), GFP_KERNEL)	;		\

Extra space.

Use __free().

> +		if (!req)						\
> +			return -ENOMEM;					\
> +									\
> +		req->cmd = enum_cmd;					\
> +		req->v = val;						\
> +									\
> +		ret = hexlin_tx_req_status(p, req, sizeof(*req));	\
> +		if (ret)						\
> +			hid_err(p->hid_dev, "%s failed, error: %d\n",	\
> +				#name, ret);				\
> +									\
> +		kfree(req);						\
> +		return ret;						\
> +	}
> +
> +HEXLIN_VAL_CMD(send_break, HEXLIN_SEND_BREAK, hexlin_val8_req, u8)
> +
> +static int hexlin_send_unconditional(struct hexlin_priv_data *priv,
> +				     const struct hexlin_frame *hxf)
> +{
> +	struct hexlin_unconditional_req *req;
> +	int ret;
> +
> +	if (hxf->lin_id > LIN_ID_MASK)
> +		return -EINVAL;
> +
> +	req = kmalloc(sizeof(*req), GFP_KERNEL);
> +	if (!req)
> +		return -ENOMEM;
> +
> +	req->cmd = HEXLIN_SEND_UNCONDITIONAL_FRAME;
> +	memcpy(&req->frm, hxf, sizeof(*hxf));
> +
> +	ret = hexlin_tx_req_status(priv, req, sizeof(*req));
> +	if (ret)
> +		hid_err(priv->hid_dev, "unconditional tx failed: %d\n", ret);
> +
> +	kfree(req);
> +	return ret;
> +}
> +
> +static int hexlin_set_baudrate(struct hexlin_priv_data *priv, u16 baudrate)
> +{
> +	struct hexlin_baudrate_req *req;
> +	int ret;
> +
> +	if (baudrate < LIN_MIN_BAUDRATE || baudrate > LIN_MAX_BAUDRATE)
> +		return -EINVAL;
> +
> +	req = kmalloc(sizeof(*req), GFP_KERNEL);

Hmm... Why do you alloc this small structure (3 bytes?) with kmalloc() and 
not just have it in stack as a local variable?

> +	if (!req)
> +		return -ENOMEM;
> +
> +	req->cmd = HEXLIN_SET_BAUDRATE;
> +	req->baudrate = cpu_to_le16(baudrate);
> +
> +	ret = hexlin_tx_req_status(priv, req, sizeof(*req));
> +	if (ret)
> +		hid_err(priv->hid_dev, "set baudrate failed: %d\n", ret);
> +
> +	kfree(req);
> +	return ret;
> +}
> +
> +static int hexlin_get_responder_answer_id(struct hexlin_priv_data *priv, u8 id,
> +					  struct hexlin_responder_answer *ransw)
> +{
> +	struct hexlin_val8_req *req;
> +	int ret;
> +
> +	if (id > LIN_ID_MASK)
> +		return -EINVAL;
> +
> +	req = kmalloc(sizeof(*req), GFP_KERNEL);

Use a stack variable instead?

> +	if (!req)
> +		return -ENOMEM;
> +
> +	req->cmd = HEXLIN_GET_RESPONDER_ANSWER_ID;
> +	req->v = id;
> +
> +	ret = hexlin_tx_req_status(priv, req, sizeof(*req));
> +	if (ret) {
> +		hid_err(priv->hid_dev, "get respond answer failed: %d\n", ret);
> +		kfree(req);
> +		return ret;
> +	}
> +
> +	memcpy(ransw, &priv->answ, sizeof(priv->answ));
> +
> +	kfree(req);
> +	return 0;
> +}
> +
> +static int hexlin_set_responder_answer_id(struct hexlin_priv_data *priv,
> +					  const struct lin_responder_answer *answ)
> +{
> +	struct hexlin_responder_answer_req *req;
> +	int ret;
> +
> +	if (answ->lf.lin_id > LIN_ID_MASK ||
> +	    answ->event_associated_id > LIN_ID_MASK)
> +		return -EINVAL;
> +
> +	req = kmalloc(sizeof(*req), GFP_KERNEL);
> +	if (!req)
> +		return -ENOMEM;
> +
> +	req->cmd = HEXLIN_SET_RESPONDER_ANSWER_ID;
> +	req->answ.is_active = answ->is_active;
> +	req->answ.is_event_frame = answ->is_event_frame;
> +	req->answ.event_associated_id = answ->event_associated_id;
> +	req->answ.frm.len = answ->lf.len;
> +	req->answ.frm.lin_id = answ->lf.lin_id;
> +	memcpy(req->answ.frm.data, answ->lf.data, LIN_MAX_DLEN);
> +	req->answ.frm.checksum = answ->lf.checksum;
> +	req->answ.frm.checksum_mode = answ->lf.checksum_mode;
> +
> +	ret = hexlin_tx_req_status(priv, req, sizeof(*req));
> +	if (ret)
> +		hid_err(priv->hid_dev, "set respond answer failed: %d\n", ret);
> +
> +	kfree(req);
> +	return ret;
> +}
> +
> +static int hexlin_open(struct lin_device *ldev)
> +{
> +	struct hid_device *hdev = to_hid_device(ldev->dev);
> +
> +	return hid_hw_open(hdev);
> +}
> +
> +static int hexlin_stop(struct lin_device *ldev)
> +{
> +	struct hid_device *hdev = to_hid_device(ldev->dev);
> +	struct hexlin_priv_data *priv = hid_get_drvdata(hdev);
> +
> +	hid_hw_close(hdev);
> +
> +	priv->is_error = true;
> +	complete(&priv->wait_in_report);
> +
> +	return 0;
> +}
> +
> +static int hexlin_ldo_tx(struct lin_device *ldev,
> +			 const struct lin_frame *lf)
> +{
> +	struct hid_device *hdev = to_hid_device(ldev->dev);
> +	struct hexlin_priv_data *priv = hid_get_drvdata(hdev);
> +	int ret = -EINVAL;
> +
> +	hid_dbg(hdev, "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");
> +
> +	if (lf->lin_id && lf->len == 0) {
> +		ret = hexlin_send_break(priv, lf->lin_id);
> +	} else if (lf->len <= LIN_MAX_DLEN) {
> +		struct hexlin_frame hxf;
> +
> +		hxf.len = lf->len;
> +		hxf.lin_id = lf->lin_id;
> +		memcpy(&hxf.data, lf->data, LIN_MAX_DLEN);
> +		hxf.checksum = lf->checksum;
> +		hxf.checksum_mode = lf->checksum_mode;
> +		ret = hexlin_send_unconditional(priv, &hxf);
> +	} else {
> +		hid_err(hdev, "unknown format\n");
> +	}
> +
> +	return ret;
> +}
> +
> +static int hexlin_update_bitrate(struct lin_device *ldev, u16 bitrate)
> +{
> +	struct hid_device *hdev = to_hid_device(ldev->dev);
> +	struct hexlin_priv_data *priv = hid_get_drvdata(hdev);
> +	int ret;
> +
> +	hid_dbg(hdev, "update bitrate to: %u\n", bitrate);
> +
> +	ret = hexlin_open(ldev);
> +	if (ret)
> +		return ret;
> +
> +	ret = hexlin_set_baudrate(priv, bitrate);
> +	if (ret)
> +		return ret;
> +
> +	ret = hexlin_get_baudrate(priv);
> +	if (ret)
> +		return ret;
> +
> +	if (priv->baudrate != bitrate) {
> +		hid_err(hdev, "update bitrate failed\n");
> +		return -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
> +static int hexlin_get_responder_answer(struct lin_device *ldev, u8 id,
> +				       struct lin_responder_answer *answ)
> +{
> +	struct hid_device *hdev = to_hid_device(ldev->dev);
> +	struct hexlin_priv_data *priv = hid_get_drvdata(hdev);
> +	struct hexlin_responder_answer ransw;
> +	int ret;
> +
> +	if (answ == NULL)
> +		return -EINVAL;
> +
> +	ret = hexlin_get_responder_answer_id(priv, id, &ransw);
> +	if (ret)
> +		return ret;
> +
> +	answ->is_active = ransw.is_active;
> +	answ->is_event_frame = ransw.is_event_frame;
> +	answ->event_associated_id = ransw.event_associated_id;
> +	answ->lf.len = ransw.frm.len;
> +	answ->lf.lin_id = ransw.frm.lin_id;
> +	memcpy(answ->lf.data, ransw.frm.data, LIN_MAX_DLEN);
> +	answ->lf.checksum = ransw.frm.checksum;
> +	answ->lf.checksum_mode = ransw.frm.checksum_mode;
> +
> +	return 0;
> +}
> +
> +static int hexlin_update_resp_answer(struct lin_device *ldev,
> +				     const struct lin_responder_answer *answ)
> +{
> +	struct hid_device *hdev = to_hid_device(ldev->dev);
> +	struct hexlin_priv_data *priv = hid_get_drvdata(hdev);
> +
> +	if (answ == NULL)
> +		return -EINVAL;
> +
> +	return hexlin_set_responder_answer_id(priv, answ);
> +}
> +
> +static const struct lin_device_ops hexlin_ldo = {
> +	.ldo_open = hexlin_open,
> +	.ldo_stop = hexlin_stop,
> +	.ldo_tx = hexlin_ldo_tx,
> +	.update_bitrate = hexlin_update_bitrate,
> +	.get_responder_answer = hexlin_get_responder_answer,
> +	.update_responder_answer = hexlin_update_resp_answer,
> +};
> +
> +static int hexlin_queue_frames_insert(struct hexlin_priv_data *priv,
> +				      const struct hexlin_frame *hxf)
> +{
> +	struct hid_device *hdev = priv->hid_dev;
> +	struct lin_frame lf;
> +
> +	lf.len = hxf->len;
> +	lf.lin_id = hxf->lin_id;
> +	memcpy(lf.data, hxf->data, LIN_MAX_DLEN);
> +	lf.checksum = hxf->checksum;
> +	lf.checksum_mode = hxf->checksum_mode;
> +
> +	hid_dbg(hdev, "id:%02x, len:%u, data:%*ph, chk:%02x (%s), flg:%08x\n",
> +		lf.lin_id, lf.len, lf.len, lf.data, lf.checksum,
> +		lf.checksum_mode ? "enhanced" : "classic", hxf->flags);
> +
> +	lin_rx(priv->ldev, &lf);
> +
> +	return 0;
> +}
> +
> +static int hexlin_raw_event(struct hid_device *hdev,
> +			    struct hid_report *report, u8 *data, int sz)
> +{
> +	struct hexlin_priv_data *priv;
> +	struct hexlin_baudrate_req *br;
> +	struct hexlin_responder_answer_req *rar;
> +	struct hexlin_unconditional_req *hfr;
> +	struct hexlin_val8_req *vr;
> +
> +	if (sz < 1 || sz > HEXLIN_PKGLEN_MAX)
> +		return -EREMOTEIO;
> +
> +	priv = hid_get_drvdata(hdev);
> +
> +	hid_dbg(hdev, "%s, size:%i, data[0]: 0x%02x\n", __func__, sz, data[0]);
> +
> +	priv->is_error = false;
> +
> +	switch (data[0]) {
> +	case HEXLIN_SUCCESS:
> +		if (sz != HEXLIN_LEN_RETCODE)
> +			return -EREMOTEIO;
> +		hid_dbg(hdev, "HEXLIN_SUCCESS: 0x%02x\n", data[0]);
> +		complete(&priv->wait_in_report);
> +		break;
> +	case HEXLIN_FAIL:
> +		if (sz != HEXLIN_LEN_RETCODE)
> +			return -EREMOTEIO;
> +		hid_err(hdev, "HEXLIN_FAIL: 0x%02x\n", data[0]);
> +		priv->is_error = true;
> +		complete(&priv->wait_in_report);
> +		break;
> +	case HEXLIN_GET_VERSION:
> +		if (sz != sizeof(*vr))
> +			return -EREMOTEIO;
> +		vr = (struct hexlin_val8_req *) data;
> +		priv->fw_version = vr->v;
> +		complete(&priv->wait_in_report);
> +		break;
> +	case HEXLIN_GET_RESPONDER_ANSWER_ID:
> +		if (sz != sizeof(*rar))
> +			return -EREMOTEIO;
> +		rar = (struct hexlin_responder_answer_req *) data;
> +		memcpy(&priv->answ, &rar->answ, sizeof(priv->answ));
> +		complete(&priv->wait_in_report);
> +		break;
> +	case HEXLIN_GET_BAUDRATE:
> +		if (sz != sizeof(*br))
> +			return -EREMOTEIO;
> +		br = (struct hexlin_baudrate_req *) data;
> +		le16_to_cpus(br->baudrate);
> +		priv->baudrate = br->baudrate;
> +		complete(&priv->wait_in_report);
> +		break;
> +	/* following cases not initiated by us, so no complete() */
> +	case HEXLIN_FRAME:
> +		if (sz != sizeof(*hfr)) {
> +			hid_err_once(hdev, "frame size mismatch: %i\n", sz);
> +			return -EREMOTEIO;
> +		}
> +		hfr = (struct hexlin_unconditional_req *) data;
> +		le32_to_cpus(hfr->frm.flags);

I'm bit worried about this from endianness perspective. Perhaps there's 
some struct reusing that shouldn't be happening because the same field 
cannot be __le32 and u32 at the same time.

> +		hexlin_queue_frames_insert(priv, &hfr->frm);
> +		break;
> +	case HEXLIN_ERROR:
> +		hid_err(hdev, "error from adapter\n");
> +		break;
> +	default:
> +		hid_err(hdev, "unknown event: 0x%02x\n", data[0]);
> +	}
> +
> +	return 0;
> +}
> +
> +static int init_hw(struct hexlin_priv_data *priv)
> +{
> +	int ret;
> +
> +	ret = hexlin_reset_dev(priv);
> +	if (ret) {
> +		/* if first reset fails, try one more time */
> +		ret = hexlin_reset_dev(priv);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = hexlin_get_version(priv);
> +	if (ret)
> +		return ret;
> +
> +	priv->baudrate = LIN_DEFAULT_BAUDRATE;
> +	ret = hexlin_set_baudrate(priv, priv->baudrate);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int hexlin_probe(struct hid_device *hdev,
> +			const struct hid_device_id *id)
> +{
> +	struct hexlin_priv_data *priv;
> +	int ret;
> +
> +	priv = devm_kzalloc(&hdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->hid_dev = hdev;
> +	hid_set_drvdata(hdev, priv);
> +
> +	mutex_init(&priv->tx_lock);
> +
> +	ret = hid_parse(hdev);
> +	if (ret) {
> +		hid_err(hdev, "hid parse failed with %d\n", ret);
> +		goto fail_and_free;
> +	}
> +
> +	ret = hid_hw_start(hdev, HID_CONNECT_DRIVER);
> +	if (ret) {
> +		hid_err(hdev, "hid hw start failed with %d\n", ret);
> +		goto fail_and_stop;
> +	}
> +
> +	ret = hid_hw_open(hdev);
> +	if (ret) {
> +		hid_err(hdev, "hid hw open failed with %d\n", ret);
> +		goto fail_and_close;
> +	}
> +
> +	init_completion(&priv->wait_in_report);
> +
> +	hid_device_io_start(hdev);
> +
> +	ret = init_hw(priv);
> +	if (ret)
> +		goto fail_and_close;
> +
> +	priv->ldev = register_lin(&hdev->dev, &hexlin_ldo);
> +	if (IS_ERR(priv->ldev)) {
> +		ret = PTR_ERR(priv->ldev);
> +		goto fail_and_close;
> +	}
> +
> +	hid_hw_close(hdev);
> +
> +	hid_info(hdev, "hexLIN (fw-version: %u) probed\n", priv->fw_version);

By custom, the success path in probe should not print anything so make 
this dbg level message if the fw version information is valuable in some 
debugging scenario.

> +	return 0;
> +
> +fail_and_close:
> +	hid_hw_close(hdev);
> +fail_and_stop:
> +	hid_hw_stop(hdev);
> +fail_and_free:
> +	mutex_destroy(&priv->tx_lock);
> +	return ret;
> +}
> +
> +static void hexlin_remove(struct hid_device *hdev)
> +{
> +	struct hexlin_priv_data *priv = hid_get_drvdata(hdev);
> +
> +	unregister_lin(priv->ldev);
> +	hid_hw_stop(hdev);
> +}
> +
> +static const struct hid_device_id hexlin_table[] = {
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_MCS, USB_DEVICE_ID_MCS_HEXDEV_HEXLIN) },
> +	{ }
> +};
> +
> +MODULE_DEVICE_TABLE(hid, hexlin_table);
> +
> +static struct hid_driver hexlin_driver = {
> +	.name = "hexLIN",
> +	.id_table = hexlin_table,
> +	.probe = hexlin_probe,
> +	.remove = hexlin_remove,
> +	.raw_event = hexlin_raw_event,
> +};
> +
> +module_hid_driver(hexlin_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Christoph Fritz <christoph.fritz@hexdev.de>");
> +MODULE_DESCRIPTION("LIN bus driver for hexLIN USB adapter");
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 61d2a21affa26..c6fe6f99a0e80 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -907,6 +907,7 @@
>  
>  #define USB_VENDOR_ID_MCS		0x16d0
>  #define USB_DEVICE_ID_MCS_GAMEPADBLOCK	0x0bcc
> +#define USB_DEVICE_ID_MCS_HEXDEV_HEXLIN	0x0648
>  
>  #define USB_VENDOR_MEGAWORLD		0x07b5
>  #define USB_DEVICE_ID_MEGAWORLD_GAMEPAD	0x0312
> diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
> index e0bbf0c6345d6..d721110d0889b 100644
> --- a/drivers/hid/hid-quirks.c
> +++ b/drivers/hid/hid-quirks.c
> @@ -436,6 +436,9 @@ static const struct hid_device_id hid_have_special_driver[] = {
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_GYRATION, USB_DEVICE_ID_GYRATION_REMOTE_2) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_GYRATION, USB_DEVICE_ID_GYRATION_REMOTE_3) },
>  #endif
> +#if IS_ENABLED(CONFIG_HID_MCS_HEXDEV)
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_MCS, USB_DEVICE_ID_MCS_HEXDEV_HEXLIN) },
> +#endif
>  #if IS_ENABLED(CONFIG_HID_HOLTEK)
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_HOLTEK, USB_DEVICE_ID_HOLTEK_ON_LINE_GRIP) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_HOLTEK_ALT, USB_DEVICE_ID_HOLTEK_ALT_KEYBOARD) },
>
Ilpo Järvinen May 10, 2024, 2:39 p.m. UTC | #3
On Thu, 9 May 2024, Christoph Fritz wrote:

> A LIN node can work as commander or responder, so introduce a new
> control mode (CAN_CTRLMODE_LIN_COMMANDER) for configuration.
> 
> This enables e.g. the userland tool ip from iproute2 to turn on
> commander mode when the device is being brought up.
> 
> Signed-off-by: Christoph Fritz <christoph.fritz@hexdev.de>
> ---
>  drivers/net/can/lin.c            | 40 +++++++++++++++++++++++++++++++-
>  include/net/lin.h                |  7 ++++++
>  include/uapi/linux/can/netlink.h |  1 +
>  3 files changed, 47 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/lin.c b/drivers/net/can/lin.c
> index f77abd7d7d21c..03ddf5d5a31b8 100644
> --- a/drivers/net/can/lin.c
> +++ b/drivers/net/can/lin.c
> @@ -262,11 +262,40 @@ static netdev_tx_t lin_start_xmit(struct sk_buff *skb,
>  	return NETDEV_TX_OK;
>  }
>  
> +static int lin_update_mode(struct net_device *ndev)
> +{
> +	struct lin_device *ldev = netdev_priv(ndev);
> +	u32 ctrlmode = ldev->can.ctrlmode;
> +	enum lin_mode lm;
> +	int ret = 0;
> +
> +	lm = (ctrlmode & CAN_CTRLMODE_LIN_COMMANDER) ? LINBUS_COMMANDER :
> +						       LINBUS_RESPONDER;
> +	if (ldev->lmode != lm) {
> +		if (!ldev->ldev_ops->update_lin_mode) {
> +			netdev_err(ndev, "setting lin mode unsupported\n");

In user visible messages, it would be best to use the expected 
capitalization, which I suppose is LIN given you use capitals in the 
commit message yourself?

> +			return -EINVAL;
> +		}
> +		ret = ldev->ldev_ops->update_lin_mode(ldev, lm);
> +		if (ret) {
> +			netdev_err(ndev, "Failed to set lin mode: %d\n", ret);

Ditto.

There might be other cases in any of the patches, please check.

> +			return ret;
> +		}
> +		ldev->lmode = lm;
> +	}
> +
> +	return ret;
> +}
> +
>  static int lin_open(struct net_device *ndev)
>  {
>  	struct lin_device *ldev = netdev_priv(ndev);
>  	int ret;
>  
> +	ret = lin_update_mode(ndev);
> +	if (ret)
> +		return ret;
> +
>  	ldev->tx_busy = false;
>  
>  	ret = open_candev(ndev);
> @@ -443,7 +472,7 @@ struct lin_device *register_lin(struct device *dev,
>  	ndev->sysfs_groups[0] = &lin_sysfs_group;
>  	ldev->can.bittiming.bitrate = LIN_DEFAULT_BAUDRATE;
>  	ldev->can.ctrlmode = CAN_CTRLMODE_LIN;
> -	ldev->can.ctrlmode_supported = 0;
> +	ldev->can.ctrlmode_supported = CAN_CTRLMODE_LIN_COMMANDER;
>  	ldev->can.bitrate_const = lin_bitrate;
>  	ldev->can.bitrate_const_cnt = ARRAY_SIZE(lin_bitrate);
>  	ldev->can.do_set_bittiming = lin_set_bittiming;
> @@ -458,6 +487,15 @@ struct lin_device *register_lin(struct device *dev,
>  		goto exit_candev;
>  	}
>  
> +	ldev->lmode = LINBUS_RESPONDER;
> +	if (ldev->ldev_ops->update_lin_mode) {
> +		ret = ldev->ldev_ops->update_lin_mode(ldev, ldev->lmode);
> +		if (ret) {
> +			netdev_err(ndev, "updating lin mode failed\n");

Ditto.

> +			goto exit_candev;
> +		}
> +	}
> +
>  	ret = register_candev(ndev);
>  	if (ret)
>  		goto exit_candev;
> diff --git a/include/net/lin.h b/include/net/lin.h
> index 31bb0feefd188..63ac870a0ab6f 100644
> --- a/include/net/lin.h
> +++ b/include/net/lin.h
> @@ -36,6 +36,11 @@ struct lin_attr {
>  	struct lin_device *ldev;
>  };
>  
> +enum lin_mode {
> +	LINBUS_RESPONDER = 0,
> +	LINBUS_COMMANDER,
> +};
> +
>  struct lin_device {
>  	struct can_priv can;  /* must be the first member */
>  	struct net_device *ndev;
> @@ -45,6 +50,7 @@ struct lin_device {
>  	struct work_struct tx_work;
>  	bool tx_busy;
>  	struct sk_buff *tx_skb;
> +	enum lin_mode lmode;
>  };
>  
>  enum lin_checksum_mode {
> @@ -71,6 +77,7 @@ 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_lin_mode)(struct lin_device *ldev, enum lin_mode lm);
>  	int (*update_bitrate)(struct lin_device *ldev, u16 bitrate);
>  	int (*update_responder_answer)(struct lin_device *ldev,
>  				       const struct lin_responder_answer *answ);
> diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
> index a37f56d86c5f2..cc390f6444d59 100644
> --- a/include/uapi/linux/can/netlink.h
> +++ b/include/uapi/linux/can/netlink.h
> @@ -104,6 +104,7 @@ struct can_ctrlmode {
>  #define CAN_CTRLMODE_TDC_AUTO		0x200	/* CAN transiver automatically calculates TDCV */
>  #define CAN_CTRLMODE_TDC_MANUAL		0x400	/* TDCV is manually set up by user */
>  #define CAN_CTRLMODE_LIN		BIT(11)	/* LIN bus mode */
> +#define CAN_CTRLMODE_LIN_COMMANDER	BIT(12)	/* LIN bus specific commander mode */
>  
>  /*
>   * CAN device statistics
>
Christoph Fritz May 11, 2024, 8:14 a.m. UTC | #4
...
> > diff --git a/drivers/hid/hid-hexdev-hexlin.c b/drivers/hid/hid-hexdev-hexlin.c
> > new file mode 100644
> > index 0000000000000..a9ed080b3e33e
> > --- /dev/null
> > +++ b/drivers/hid/hid-hexdev-hexlin.c
> > 
...
> 
> > +}
> > +
> > +#define HEXLIN_GET_CMD(name, enum_cmd)					\
> > +	static int hexlin_##name(struct hexlin_priv_data *p)		\
> > +	{								\
> > +		u8 *req;						\
> > +		int ret;						\
> > +									\
> > +		req = kmalloc(sizeof(*req), GFP_KERNEL)	;		\
> 
> Extra space.
> 
> Use:
> 
> u8 *req __free(kfree) = kmalloc(sizeof(*req), GFP_KERNEL);
> 
> > +		if (!req)						\
> > +			return -ENOMEM;					\
> > +									\
> > +		*req = enum_cmd;					\
> > +									\
> > +		ret = hexlin_tx_req_status(p, req, sizeof(*req));	\
> > +		if (ret)						\
> > +			hid_err(p->hid_dev, "%s failed, error: %d\n",	\
> > +				#name, ret);				\
> > +									\
> > +		kfree(req);						\
> 
> Not needed after using __free().
> 
> > +		return ret;						\
> > +	}
> > +
> > +HEXLIN_GET_CMD(get_version, HEXLIN_GET_VERSION)
> > +HEXLIN_GET_CMD(reset_dev, HEXLIN_RESET)
> > +HEXLIN_GET_CMD(get_baudrate, HEXLIN_GET_BAUDRATE)
> 
> Could you convert the function in the macro into a helper function which 
> is just called by a simple function with the relevant parameters for 
> these 3 cases?

The device actually has a lot more features that I'm using in my debug
version and which might end up here in the future. So I would like to
keep it. Any objections?

...
> > +
> > +static int hexlin_set_baudrate(struct hexlin_priv_data *priv, u16 baudrate)
> > +{
> > +	struct hexlin_baudrate_req *req;
> > +	int ret;
> > +
> > +	if (baudrate < LIN_MIN_BAUDRATE || baudrate > LIN_MAX_BAUDRATE)
> > +		return -EINVAL;
> > +
> > +	req = kmalloc(sizeof(*req), GFP_KERNEL);
> 
> Hmm... Why do you alloc this small structure (3 bytes?) with kmalloc() and 
> not just have it in stack as a local variable?


This buffer must be suitable for DMA (see docu for struct urb).

So with a stack variable we would need to use kmemdup() before the
actual sending call, but that's what you did not like since v3 so I
changed it to this which now you also don't like.

Let's dial it back to the original kmemdup() usage, ok?

...
> > +static int hexlin_queue_frames_insert(struct hexlin_priv_data *priv,
> > +				      const struct hexlin_frame *hxf)
> > +{
> > +	struct hid_device *hdev = priv->hid_dev;
> > +	struct lin_frame lf;
> > +
> > +	lf.len = hxf->len;
> > +	lf.lin_id = hxf->lin_id;
> > +	memcpy(lf.data, hxf->data, LIN_MAX_DLEN);
> > +	lf.checksum = hxf->checksum;
> > +	lf.checksum_mode = hxf->checksum_mode;
> > +
> > +	hid_dbg(hdev, "id:%02x, len:%u, data:%*ph, chk:%02x (%s), flg:%08x\n",
> > +		lf.lin_id, lf.len, lf.len, lf.data, lf.checksum,
> > +		lf.checksum_mode ? "enhanced" : "classic", hxf->flags);
> > +
> > +	lin_rx(priv->ldev, &lf);
> > +
> > +	return 0;
> > +}
> > +
> > +static int hexlin_raw_event(struct hid_device *hdev,
> > +			    struct hid_report *report, u8 *data, int sz)
> > +{
> > +	struct hexlin_priv_data *priv;
> > +	struct hexlin_baudrate_req *br;
> > +	struct hexlin_responder_answer_req *rar;
> > +	struct hexlin_unconditional_req *hfr;
> > +	struct hexlin_val8_req *vr;
> > +
> > +	if (sz < 1 || sz > HEXLIN_PKGLEN_MAX)
> > +		return -EREMOTEIO;
> > +
> > +	priv = hid_get_drvdata(hdev);
> > +
> > +	hid_dbg(hdev, "%s, size:%i, data[0]: 0x%02x\n", __func__, sz, data[0]);
> > +
> > +	priv->is_error = false;
> > +
> > +	switch (data[0]) {
> > +	case HEXLIN_SUCCESS:
> > +		if (sz != HEXLIN_LEN_RETCODE)
> > +			return -EREMOTEIO;
> > +		hid_dbg(hdev, "HEXLIN_SUCCESS: 0x%02x\n", data[0]);
> > +		complete(&priv->wait_in_report);
> > +		break;
> > +	case HEXLIN_FAIL:
> > +		if (sz != HEXLIN_LEN_RETCODE)
> > +			return -EREMOTEIO;
> > +		hid_err(hdev, "HEXLIN_FAIL: 0x%02x\n", data[0]);
> > +		priv->is_error = true;
> > +		complete(&priv->wait_in_report);
> > +		break;
> > +	case HEXLIN_GET_VERSION:
> > +		if (sz != sizeof(*vr))
> > +			return -EREMOTEIO;
> > +		vr = (struct hexlin_val8_req *) data;
> > +		priv->fw_version = vr->v;
> > +		complete(&priv->wait_in_report);
> > +		break;
> > +	case HEXLIN_GET_RESPONDER_ANSWER_ID:
> > +		if (sz != sizeof(*rar))
> > +			return -EREMOTEIO;
> > +		rar = (struct hexlin_responder_answer_req *) data;
> > +		memcpy(&priv->answ, &rar->answ, sizeof(priv->answ));
> > +		complete(&priv->wait_in_report);
> > +		break;
> > +	case HEXLIN_GET_BAUDRATE:
> > +		if (sz != sizeof(*br))
> > +			return -EREMOTEIO;
> > +		br = (struct hexlin_baudrate_req *) data;
> > +		le16_to_cpus(br->baudrate);
> > +		priv->baudrate = br->baudrate;
> > +		complete(&priv->wait_in_report);
> > +		break;
> > +	/* following cases not initiated by us, so no complete() */
> > +	case HEXLIN_FRAME:
> > +		if (sz != sizeof(*hfr)) {
> > +			hid_err_once(hdev, "frame size mismatch: %i\n", sz);
> > +			return -EREMOTEIO;
> > +		}
> > +		hfr = (struct hexlin_unconditional_req *) data;
> > +		le32_to_cpus(hfr->frm.flags);
> 
> I'm bit worried about this from endianness perspective. Perhaps there's 
> some struct reusing that shouldn't be happening because the same field 
> cannot be __le32 and u32 at the same time.

Can you propose a solution?

> 
...

thanks
  -- Christoph
Jiri Slaby May 13, 2024, 5:26 a.m. UTC | #5
On 10. 05. 24, 15:46, Ilpo Järvinen wrote:
>> +	reinit_completion(&priv->wait_in_report);
>> +
>> +	n = hid_hw_output_report(priv->hid_dev, (__u8 *) out_report, len);
> 
> The usual is to not leave space between cast and what is being cast. I
> know hid functions seem to use __u8 but that's intended for uapi and in
> kernel, u8 should be used (somebody should eventually cleanup the hid
> function types too).

Apart from that, you are attached to USB, so this goes down to usbhid 
(the ll driver). Are you sure the put-const-away cast is right thing to 
do here? (usbhid passes it to usb_interrupt_msg().)

That is the only reason why you have the cast in there in the first place…

regards,
Ilpo Järvinen May 13, 2024, 12:27 p.m. UTC | #6
On Sat, 11 May 2024, Christoph Fritz wrote:

> ...
> > > diff --git a/drivers/hid/hid-hexdev-hexlin.c b/drivers/hid/hid-hexdev-hexlin.c
> > > new file mode 100644
> > > index 0000000000000..a9ed080b3e33e
> > > --- /dev/null
> > > +++ b/drivers/hid/hid-hexdev-hexlin.c
> > > 
> ...
> > 
> > > +}
> > > +
> > > +#define HEXLIN_GET_CMD(name, enum_cmd)					\
> > > +	static int hexlin_##name(struct hexlin_priv_data *p)		\
> > > +	{								\
> > > +		u8 *req;						\
> > > +		int ret;						\
> > > +									\
> > > +		req = kmalloc(sizeof(*req), GFP_KERNEL)	;		\
> > 
> > Extra space.
> > 
> > Use:
> > 
> > u8 *req __free(kfree) = kmalloc(sizeof(*req), GFP_KERNEL);
> > 
> > > +		if (!req)						\
> > > +			return -ENOMEM;					\
> > > +									\
> > > +		*req = enum_cmd;					\
> > > +									\
> > > +		ret = hexlin_tx_req_status(p, req, sizeof(*req));	\
> > > +		if (ret)						\
> > > +			hid_err(p->hid_dev, "%s failed, error: %d\n",	\
> > > +				#name, ret);				\
> > > +									\
> > > +		kfree(req);						\
> > 
> > Not needed after using __free().
> > 
> > > +		return ret;						\
> > > +	}
> > > +
> > > +HEXLIN_GET_CMD(get_version, HEXLIN_GET_VERSION)
> > > +HEXLIN_GET_CMD(reset_dev, HEXLIN_RESET)
> > > +HEXLIN_GET_CMD(get_baudrate, HEXLIN_GET_BAUDRATE)
> > 
> > Could you convert the function in the macro into a helper function which 
> > is just called by a simple function with the relevant parameters for 
> > these 3 cases?
> 
> The device actually has a lot more features that I'm using in my debug
> version and which might end up here in the future. So I would like to
> keep it. Any objections?

I don't follow, HEXLIN_GET_CMD() will always produce a copy of that same 
function even if you use it 1000 times?? (Except for the enum and string 
which can easily be passed as arguments to a common function).

You can still keep HEXLIN_GET_CMD() which just calls that common function
if you want to avoid giving those two parameters directly.

> > > +static int hexlin_set_baudrate(struct hexlin_priv_data *priv, u16 baudrate)
> > > +{
> > > +	struct hexlin_baudrate_req *req;
> > > +	int ret;
> > > +
> > > +	if (baudrate < LIN_MIN_BAUDRATE || baudrate > LIN_MAX_BAUDRATE)
> > > +		return -EINVAL;
> > > +
> > > +	req = kmalloc(sizeof(*req), GFP_KERNEL);
> > 
> > Hmm... Why do you alloc this small structure (3 bytes?) with kmalloc() and 
> > not just have it in stack as a local variable?
> 
> This buffer must be suitable for DMA (see docu for struct urb).
> 
> So with a stack variable we would need to use kmemdup() before the
> actual sending call, but that's what you did not like since v3 so I
> changed it to this which now you also don't like.
>
> Let's dial it back to the original kmemdup() usage, ok?

I asked a question. Since you have a good reason for alloc, just keep the 
alloc then.

Now I notice it's kmalloc(), why not kzalloc()?

> > > +static int hexlin_queue_frames_insert(struct hexlin_priv_data *priv,
> > > +				      const struct hexlin_frame *hxf)
> > > +{
> > > +	struct hid_device *hdev = priv->hid_dev;
> > > +	struct lin_frame lf;
> > > +
> > > +	lf.len = hxf->len;
> > > +	lf.lin_id = hxf->lin_id;
> > > +	memcpy(lf.data, hxf->data, LIN_MAX_DLEN);
> > > +	lf.checksum = hxf->checksum;
> > > +	lf.checksum_mode = hxf->checksum_mode;
> > > +
> > > +	hid_dbg(hdev, "id:%02x, len:%u, data:%*ph, chk:%02x (%s), flg:%08x\n",
> > > +		lf.lin_id, lf.len, lf.len, lf.data, lf.checksum,
> > > +		lf.checksum_mode ? "enhanced" : "classic", hxf->flags);
> > > +
> > > +	lin_rx(priv->ldev, &lf);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int hexlin_raw_event(struct hid_device *hdev,
> > > +			    struct hid_report *report, u8 *data, int sz)
> > > +{
> > > +	struct hexlin_priv_data *priv;
> > > +	struct hexlin_baudrate_req *br;
> > > +	struct hexlin_responder_answer_req *rar;
> > > +	struct hexlin_unconditional_req *hfr;
> > > +	struct hexlin_val8_req *vr;
> > > +
> > > +	if (sz < 1 || sz > HEXLIN_PKGLEN_MAX)
> > > +		return -EREMOTEIO;
> > > +
> > > +	priv = hid_get_drvdata(hdev);
> > > +
> > > +	hid_dbg(hdev, "%s, size:%i, data[0]: 0x%02x\n", __func__, sz, data[0]);
> > > +
> > > +	priv->is_error = false;
> > > +
> > > +	switch (data[0]) {
> > > +	case HEXLIN_SUCCESS:
> > > +		if (sz != HEXLIN_LEN_RETCODE)
> > > +			return -EREMOTEIO;
> > > +		hid_dbg(hdev, "HEXLIN_SUCCESS: 0x%02x\n", data[0]);
> > > +		complete(&priv->wait_in_report);
> > > +		break;
> > > +	case HEXLIN_FAIL:
> > > +		if (sz != HEXLIN_LEN_RETCODE)
> > > +			return -EREMOTEIO;
> > > +		hid_err(hdev, "HEXLIN_FAIL: 0x%02x\n", data[0]);
> > > +		priv->is_error = true;
> > > +		complete(&priv->wait_in_report);
> > > +		break;
> > > +	case HEXLIN_GET_VERSION:
> > > +		if (sz != sizeof(*vr))
> > > +			return -EREMOTEIO;
> > > +		vr = (struct hexlin_val8_req *) data;
> > > +		priv->fw_version = vr->v;
> > > +		complete(&priv->wait_in_report);
> > > +		break;
> > > +	case HEXLIN_GET_RESPONDER_ANSWER_ID:
> > > +		if (sz != sizeof(*rar))
> > > +			return -EREMOTEIO;
> > > +		rar = (struct hexlin_responder_answer_req *) data;
> > > +		memcpy(&priv->answ, &rar->answ, sizeof(priv->answ));
> > > +		complete(&priv->wait_in_report);
> > > +		break;
> > > +	case HEXLIN_GET_BAUDRATE:
> > > +		if (sz != sizeof(*br))
> > > +			return -EREMOTEIO;
> > > +		br = (struct hexlin_baudrate_req *) data;
> > > +		le16_to_cpus(br->baudrate);
> > > +		priv->baudrate = br->baudrate;
> > > +		complete(&priv->wait_in_report);
> > > +		break;
> > > +	/* following cases not initiated by us, so no complete() */
> > > +	case HEXLIN_FRAME:
> > > +		if (sz != sizeof(*hfr)) {
> > > +			hid_err_once(hdev, "frame size mismatch: %i\n", sz);
> > > +			return -EREMOTEIO;
> > > +		}
> > > +		hfr = (struct hexlin_unconditional_req *) data;
> > > +		le32_to_cpus(hfr->frm.flags);
> > 
> > I'm bit worried about this from endianness perspective. Perhaps there's 
> > some struct reusing that shouldn't be happening because the same field 
> > cannot be __le32 and u32 at the same time.
> 
> Can you propose a solution?

Just do a le32_to_cpu(hfr->frm.flags) in the debug print's arguments?
Christoph Fritz May 18, 2024, 6:29 p.m. UTC | #7
> This series is introducing basic Local Interconnect Network (LIN)
> (ISO 17987) [0] support

I just wanted to let you know that I'm unfortunately personally busy at
the moment. I will resume addressing the remaining issues with v5
sometime after the current merge window.

thanks
  -- Christoph