| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <02d9e48619cff27fb846b5d8f367bc033bbf91e6.camel@hexdev.de>
Date: Thu, 09 May 2024 19:06:35 +0200
From: Christoph Fritz <christoph.fritz@...dev.de>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Jiri Slaby <jirislaby@...nel.org>, Oliver Hartkopp
<socketcan@...tkopp.net>, Marc Kleine-Budde <mkl@...gutronix.de>, Vincent
Mailhol <mailhol.vincent@...adoo.fr>, "David S . Miller"
<davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
<kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Rob Herring
<robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Jiri Kosina <jikos@...nel.org>, Benjamin Tissoires
<bentiss@...nel.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Sebastian Reichel <sre@...nel.org>, Linus Walleij
<linus.walleij@...aro.org>, Andreas Lauser
<andreas.lauser@...cedes-benz.com>, Jonathan Corbet <corbet@....net>,
Pavel Pisa <pisa@....felk.cvut.cz>, linux-can@...r.kernel.org, Netdev
<netdev@...r.kernel.org>, devicetree@...r.kernel.org,
linux-input@...r.kernel.org, linux-serial <linux-serial@...r.kernel.org>
Subject: Re: [PATCH v3 02/11] HID: hexLIN: Add support for USB LIN bus
adapter
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@...cedes-benz.com>
> > Signed-off-by: Christoph Fritz <christoph.fritz@...dev.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
Powered by blists - more mailing lists