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: <acf0251e-41b9-410d-a663-ff6c34d2bc3e@kernel.org>
Date: Thu, 2 May 2024 10:30:29 +0200
From: Jiri Slaby <jirislaby@...nel.org>
To: Christoph Fritz <christoph.fritz@...dev.de>,
 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>
Cc: 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 02. 05. 24, 9:55, 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>
...
> --- /dev/null
> +++ b/drivers/hid/hid-hexdev-hexlin.c
> @@ -0,0 +1,630 @@
...
> +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);
> +
> +	mutex_lock(&priv->tx_lock);
> +	mutex_unlock(&priv->tx_lock);

This is a weird way to implement a completion. It looks like you need 
another one.

> +	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_OR_NULL(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);
> +
> +	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);
> +	mutex_destroy(&priv->tx_lock);

It is unusual to destroy a mutex. Why do you do that?

> +}
...
> +static int __init hexlin_init(void)
> +{
> +	return hid_register_driver(&hexlin_driver);
> +}
> +
> +static void __exit hexlin_exit(void)
> +{
> +	hid_unregister_driver(&hexlin_driver);
> +}

> +
> +/*
> + * When compiled into the kernel, initialize after the hid bus.
> + */
> +late_initcall(hexlin_init);

Hmm, why not module_init() then? (And module_hid_driver().)

> +module_exit(hexlin_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Christoph Fritz <christoph.fritz@...dev.de>");
> +MODULE_DESCRIPTION("LIN bus driver for hexLIN USB adapter");

thanks,
-- 
js
suse labs