netdev - Re: [PATCH v2 02/12] HID: hexLIN: Add support for USB LIN bus adapter

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <45ed69b00f64607c70f1090941f36b71202807f2.camel@hexdev.de>
Date: Wed, 08 May 2024 10:37:42 +0200
From: Christoph Fritz <christoph.fritz@...dev.de>
To: Simon Horman <horms@...nel.org>
Cc: 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>, Jiri Slaby <jirislaby@...nel.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@...r.kernel.org,
 devicetree@...r.kernel.org, linux-input@...r.kernel.org, 
 linux-serial@...r.kernel.org
Subject: Re: [PATCH v2 02/12] HID: hexLIN: Add support for USB LIN bus
 adapter

On Sat, 2024-05-04 at 13:58 +0100, Simon Horman wrote:
> On Thu, May 02, 2024 at 09:55:24AM +0200, 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>
> 
> Hi Christoph,
> 
> Some minor feedback from my side.
> 
> ...
> 
> > diff --git a/drivers/hid/hid-hexdev-hexlin.c b/drivers/hid/hid-hexdev-hexlin.c
> 
> ...
> 
> > +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);
> > +	le32_to_cpus(hxf.flags);
> > +
> > +	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");
> 
> nit: The indentation above is not quite right.
>      It should align to inside the opening '(' like this.
> 
> 	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");
> 
> Flagged by checkpatch.

OK

> 
> ...
> 
> > +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 type of req.baudrate is u16, a host byte-order type.
> But it is being assigned a little endian value.
> This does not seem right.
> 
> Flagged by Smatch.

In upcoming v4 I'll use __le16

> 
> > +
> > +	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_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_OR_NULL(priv->ldev)) {
> > +		ret = PTR_ERR(priv->ldev);
> > +		goto fail_and_close;
> 
> Is it intentional that when priv->ldev is NULL here,
> the function will return 0?
> 
> Flagged by Smatch.

IS_ERR_OR_NULL() gets IS_ERR() in upcoming v4

> 
> > +	}
> > +
> > +	hid_hw_close(hdev);
> > +
> > +	hid_info(hdev, "hexLIN (fw-version: %u) probed\n", priv->fw_version);
> > +
> > +	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;
> > +}
> 
> ...

Thanks
  -- Christoph