Re: [PATCH v3 02/11] HID: hexLIN: Add support for USB LIN bus adapter

[Christoph Fritz <christoph.fritz@hexdev.de>]

On Mon, 2024-05-06 at 19:53 +0300, Ilpo Järvinen wrote:
> On Thu, 2 May 2024, Christoph Fritz wrote:
> 
> > This patch introduces driver support for the hexLIN USB LIN bus 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 | 611 ++++++++++++++++++++++++++++++++
> >  drivers/hid/hid-ids.h           |   1 +
> >  drivers/hid/hid-quirks.c        |   3 +
> >  5 files changed, 635 insertions(+)
> >  create mode 100644 drivers/hid/hid-hexdev-hexlin.c
> > 
> > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > index 4c682c6507040..d2fb35d83c640 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 082a728eac600..f9b13e6117e60 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..1ddc1e00ab2da
> > --- /dev/null
> > +++ b/drivers/hid/hid-hexdev-hexlin.c
> > @@ -0,0 +1,611 @@
> > +// 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"
> > +
> > +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,
> 
> Could you align the values?

OK

> 
> > +};
> > +
> > +#define HEXLIN_SUCCESS_SZ			1
> > +#define HEXLIN_FRAME_SZ				17
> > +#define HEXLIN_FAIL_SZ				1
> > +#define HEXLIN_GET_RESPONDER_ANSWER_ID_SZ	20
> > +#define HEXLIN_GET_BAUDRATE_SZ			3
> 
> Is this sizeof(hexlin_baudrate_req)?

No

> If so, don't add define for it.
> This probably applies to other defines here too.

I'll refactor these defines in upcoming v4 to a minimum.

> 
> > +#define HEXLIN_BAUDRATE_SZ			2
> > +#define HEXLIN_GET_VERSION_SZ			2
> > +#define HEXLIN_PKGLEN_MAX_SZ			64
> > +
> > +struct hexlin_val8_req {
> > +	u8 cmd;
> > +	u8 v;
> > +} __packed;
> > +
> > +struct hexlin_baudrate_req {
> > +	u8 cmd;
> > +	u16 baudrate;
> > +} __packed;
> > +
> > +struct hexlin_frame {
> > +	u32 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_req rar;
> > +	u8 fw_version;
> > +};
> > +
> > +static int hexlin_tx_report(struct hexlin_priv_data *priv,
> > +			    const void *out_report, size_t len)
> > +{
> > +	u8 *buf;
> > +	int ret;
> > +
> > +	buf = kmemdup(out_report, len, GFP_KERNEL);
> > +	if (!buf)
> > +		return -ENOMEM;
> > +
> > +	ret = hid_hw_output_report(priv->hid_dev, buf, len);
> > +	kfree(buf);
> 
> Is duplicatign the buffer necessary?

No, my intention was to keep the functions calling hexlin_tx_report()
simpler and let the compiler optimize this.

In the upcoming v4, I merged this into hexlin_tx_req_status() and
dropped the kmemdup().

> 
> > +	if (ret < 0)
> > +		return ret;
> > +	if (ret != len)
> > +		return -EIO;
> > +
> > +	return 0;
> > +}
> > +
> > +static int hexlin_tx_req_status(struct hexlin_priv_data *priv,
> > +				const void *out_report, int len)
> > +{
> > +	int ret;
> > +	unsigned long t;
> > +
> > +	mutex_lock(&priv->tx_lock);
> > +
> > +	reinit_completion(&priv->wait_in_report);
> > +
> > +	ret = hexlin_tx_report(priv, out_report, len);
> > +	if (ret)
> > +		goto tx_exit;
> > +
> > +	t = wait_for_completion_killable_timeout(&priv->wait_in_report,
> > +						 msecs_to_jiffies(1000));
> 
> HZ?

OK

> 
> > +	if (!t)
> > +		ret = -ETIMEDOUT;
> > +
> > +	if (priv->is_error)
> > +		ret = -EINVAL;
> > +
> > +tx_exit:
> > +	mutex_unlock(&priv->tx_lock);
> > +
> > +	return ret;
> > +}
> > +
> > +#define HEXLIN_GET_CMD(name, enum_cmd)					\
> > +	static int hexlin_##name(struct hexlin_priv_data *priv)		\
> > +	{								\
> > +		u8 cmd = enum_cmd;					\
> > +		int ret;						\
> > +									\
> > +		ret = hexlin_tx_req_status(priv, &cmd, sizeof(u8));	\
> 
> Take sizeof() of the relevant variable instead, so:
> 
> sizeof(cmd)

OK

> 
> > +		if (ret)						\
> > +			hid_err(priv->hid_dev, "%s failed with %d\n",	\
> > +				__func__, ret);				\
> > +									\
> > +		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)
> > +
> > +#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.cmd = enum_cmd;					\
> > +		req.v = val;						\
> > +									\
> > +		ret = hexlin_tx_req_status(p, &req,			\
> > +					   sizeof(struct struct_type));	\
> 
> sizeof(req)

OK

> 
> > +		if (ret)						\
> > +			hid_err(p->hid_dev, "%s failed with %d\n",	\
> > +				__func__, ret);				\
> > +									\
> > +		return ret;						\
> > +	}
> > +
> > +HEXLIN_VAL_CMD(send_break, HEXLIN_SEND_BREAK, hexlin_val8_req, u8)
> > +
> > +static int hexlin_queue_frames_insert(struct hexlin_priv_data *priv,
> > +				      const u8 *raw_data, int sz)
> > +{
> > +	struct hid_device *hdev = priv->hid_dev;
> > +	struct hexlin_frame hxf;
> > +	struct lin_frame lf;
> > +
> > +	if (sz != sizeof(struct hexlin_frame))
> > +		return -EREMOTEIO;
> > +
> > +	memcpy(&hxf, raw_data, sz);
> 
> Why you cannot just cast the pointer to correct type?

I can, will be refactored in v4.

> 
> > +	le32_to_cpus(hxf.flags);
> 
> You must use correct endianess typing. The struct hexlin_frame should have 
> __le32 flags so sparse's endianness check is happy.

OK

> 
> But .flags are not used at all so why is this required in the first place?

Was necessary in the development process and will be used in hid_dbg()
below in v4.

> 
> > +	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, checksum:%02x (%s)\n",
> > +		   lf.lin_id, lf.len, lf.len, lf.data, lf.checksum,
> > +		   lf.checksum_mode ? "enhanced" : "classic");
> > +
> > +	lin_rx(priv->ldev, &lf);
> > +
> > +	return 0;
> > +}
> > +
> > +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.cmd = HEXLIN_SEND_UNCONDITIONAL_FRAME;
> > +	memcpy(&req.frm, hxf, sizeof(struct hexlin_frame));
> > +
> > +	ret = hexlin_tx_req_status(priv, &req,
> > +				   sizeof(struct hexlin_unconditional_req));
> 
> sizeof(req)

OK

> 
> > +
> > +	if (ret)
> > +		hid_err(priv->hid_dev, "%s failed with %d\n", __func__, ret);
> > +
> > +	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.cmd = HEXLIN_SET_BAUDRATE;
> > +	req.baudrate = cpu_to_le16(baudrate);
> 
> The struct should have __le16 baudrate.

OK

> 
> > +
> > +	ret = hexlin_tx_req_status(priv, &req,
> > +				   sizeof(struct hexlin_baudrate_req));
> > +	if (ret)
> > +		hid_err(priv->hid_dev, "%s failed with %d\n", __func__, ret);
> > +
> > +	return ret;
> > +}
> > +
> > +static int hexlin_get_responder_answer_id(struct hexlin_priv_data *priv, u8 id,
> > +					  struct hexlin_responder_answer_req *rar)
> > +{
> > +	u8 req[2] = { HEXLIN_GET_RESPONDER_ANSWER_ID, id };
> > +	int ret;
> > +
> > +	if (id > LIN_ID_MASK)
> > +		return -EINVAL;
> > +
> > +	ret = hexlin_tx_req_status(priv, &req, sizeof(req));
> > +	if (ret) {
> > +		hid_err(priv->hid_dev, "%s failed with %d\n", __func__, ret);
> 
> Try to write error message that is meaningful to user, printing __func__ 
> is not very helpful for user.

OK, will be refactored in v4.

> 
> > +		return ret;
> > +	}
> > +
> > +	memcpy(rar, &priv->rar, sizeof(struct hexlin_responder_answer_req));
> 
> sizeof(*rar)

OK

> 
> > +	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 rar;
> > +	int ret;
> > +
> > +	if (answ->lf.lin_id > LIN_ID_MASK ||
> > +	    answ->event_associated_id > LIN_ID_MASK)
> > +		return -EINVAL;
> > +
> > +	rar.cmd = HEXLIN_SET_RESPONDER_ANSWER_ID;
> > +	rar.answ.is_active = answ->is_active;
> > +	rar.answ.is_event_frame = answ->is_event_frame;
> > +	rar.answ.event_associated_id = answ->event_associated_id;
> > +	rar.answ.frm.len = answ->lf.len;
> > +	rar.answ.frm.lin_id = answ->lf.lin_id;
> > +	memcpy(rar.answ.frm.data, answ->lf.data, LIN_MAX_DLEN);
> > +	rar.answ.frm.checksum = answ->lf.checksum;
> > +	rar.answ.frm.checksum_mode = answ->lf.checksum_mode;
> > +
> > +	ret = hexlin_tx_req_status(priv, &rar,
> > +				   sizeof(struct hexlin_responder_answer_req));
> 
> Ditto.

OK

thanks
  -- Christoph