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

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

Hi Benjamin,

 thanks for your review, please see my answers below.

...
> > +
> > +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);
> 
> AFAICT, any operation using the device will use this function and
> therefore this is enforcing a single user at the same time.
> 
> Is this a bus or a hw limitation?

It's a hw limitation.

> > +
> > +	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));
> > +	if (!t)
> > +		ret = -ETIMEDOUT;
> > +
> > +	if (priv->is_error)
> > +		ret = -EINVAL;
> > +
> > +tx_exit:
> > +	mutex_unlock(&priv->tx_lock);
> > +
> > +	return ret;
> > +}
...
> > +static int hexlin_raw_event(struct hid_device *hdev,
> > +			    struct hid_report *report, u8 *data, int sz)
> > +{
> > +	struct hexlin_priv_data *priv;
> > +	int ret;
> > +
> > +	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 != 1)
> > +			return -EREMOTEIO;
> 
> Could we have some #define for all of these sizes (here and in all of
> the other branches)?

OK

> 
> > +		hid_dbg(hdev, "HEXLIN_SUCCESS: 0x%02x\n", data[0]);
> > +		complete(&priv->wait_in_report);
> 
> Shouldn't you ensure that you currently have a request pending?
> This works as long as no-one opens the hidraw node (see my remark
> below).

Thanks for the heads up, there is no need for hidraw.

> > +		break;
> > +	case HEXLIN_FAIL:
> > +		if (sz != 1)
> > +			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 != 2)
> > +			return -EREMOTEIO;
> > +		priv->fw_version = data[1];
> > +		complete(&priv->wait_in_report);
> > +		break;
> > +	case HEXLIN_GET_RESPONDER_ANSWER_ID:
> > +		if (sz != 20)
> > +			return -EREMOTEIO;
> > +		BUILD_BUG_ON(sizeof(priv->rar) != 20);
> 
> magical constants again

OK

> 
> > +		memcpy(&priv->rar, data, sizeof(priv->rar));
> > +		complete(&priv->wait_in_report);
> > +		break;
> > +	case HEXLIN_GET_BAUDRATE:
> > +		if (sz != 3)
> > +			return -EREMOTEIO;
> > +		BUILD_BUG_ON(sizeof(priv->baudrate) != 2);
> > +		memcpy(&priv->baudrate, &data[1], sizeof(priv->baudrate));
> > +		le16_to_cpus(priv->baudrate);
> > +		complete(&priv->wait_in_report);
> > +		break;
> > +	/* following cases not initiated by us, so no complete() */
> > +	case HEXLIN_FRAME:
> > +		if (sz != 17) {
> > +			hid_err_once(hdev, "frame size mismatch: %i\n", sz);
> > +			return -EREMOTEIO;
> > +		}
> > +		ret = hexlin_queue_frames_insert(priv, &data[1], sz-1);
> > +		if (ret) {
> > +			hid_err(hdev, "failed to add frame: %i\n", ret);
> > +			return ret;
> > +		}
> > +		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)
> > +		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_HIDRAW);
> 
> Are you sure you want HID_CONNECT_HIDRAW?
> 
> Given that your whole driver relies on the assumption that any command
> sent to the device is guarded by the mutex, if one client opens the
> hidraw node and starts sending commands behind your back you are
> screwed...
> 
> Maybe use HID_CONNECT_DRIVER instead.

HID_CONNECT_DRIVER it is

> 
> > +	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_info(hdev, "hexLIN (fw-version: %u) probed\n", priv->fw_version);
> 
> you are not calling hid_hw_close(hdev) here (on purpose I guess).
> 
> However, this prevents the device to enter any sleep mode as the kernel
> will always consider it to be in use.
> Is there some open/close mechanism in LIN or in CAN that can tell the
> device that it needs to be opened or do we assume that the device needs
> to be powered on all the time?

One can bring the LIN device up and down, just like any other Ethernet
or CAN device. So, for revision 2 of this patchset, I added open/stop
handling. This allows for hid_hw_close(hdev) here and also makes
remove() handling way easier. Thanks for the heads up.

> 
> > +
> > +	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);
> > +
> > +	complete(&priv->wait_in_report);
> 
> what if you get one LIN request just now, between those 2 calls?
> 
> You should probably disable the ability to take the mutex before sending
> the complete call above or you might still have the mutex taken here.
> 
> Also shouldn't you set priv->is_error = true before the complete?
> 
> > +	unregister_lin(priv->ldev);
> > +	hid_hw_close(hdev);
> > +	hid_hw_stop(hdev);
> > +	mutex_destroy(&priv->tx_lock);
> 
> Given how the device works, I think it would be safer to do this in the
> following order:
> 
> // prevent any incoming event (assuming hidraw is not available)
> hid_hw_close(hdev);
> // ensure the device is powered off
> hid_hw_stop(hdev);
> // mark any pending request as failed
> priv->is_error = true;
> // mark the device as unusable
> priv->removed = true;
> complete(&priv->wait_in_report);
> // unregister
> unregister_lin(priv->ldev);
> // mutex is not used anymore
> mutex_destroy(&priv->tx_lock);
> 
> (I might be wrong but this seems more sensible to me).
> 
> Actually, instead of having a priv->removed boolean, you could also take
> and release the mutex before releasing it, this way you are sure to not
> be in the critical code section. This should work because you are using
> wait_for_completion_killable_timeout() and so after 1 s you are
> guaranteed to exit the mutex.

Thanks for the great explanation.

> > +}
> > +
> > +static const struct hid_device_id hexlin_table[] = {
> > +	{ HID_USB_DEVICE(USB_VENDOR_ID_MCS, USB_DEVICE_ID_MCS_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,
> > +};
> > +
> > +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);
> > +module_exit(hexlin_exit);
> > +
> > +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 8376fb5e2d0b4..157d234e1d400 100644
> > --- a/drivers/hid/hid-ids.h
> > +++ b/drivers/hid/hid-ids.h
> > @@ -903,6 +903,7 @@
> >  #define USB_DEVICE_ID_MCC_PMD1208LS	0x007a
> >  
> >  #define USB_VENDOR_ID_MCS		0x16d0
> > +#define USB_DEVICE_ID_MCS_HEXLIN	0x0648
> >  #define USB_DEVICE_ID_MCS_GAMEPADBLOCK	0x0bcc
> >  
> >  #define USB_VENDOR_MEGAWORLD		0x07b5
> > diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
> > index e0bbf0c6345d6..328fcc61303f3 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_HEXLIN)
> > +	{ HID_USB_DEVICE(USB_VENDOR_ID_MCS, USB_DEVICE_ID_MCS_HEXLIN) },
> 
> Generally, the pattern for drivers in the HID subsystem is to rely on
> the vendor name, not the product, in order to be able to extend it to
> more than one product.
> 
> Is your vendor name MCS? Or Hexdev?
> 
> If so, the driver should likely be hid-hexdev.c...

We got the PID from MCS online shop here:

https://www.mcselec.com/index.php?page=shop.product_details&product_id=92&option=com_phpshop

Our vendor name is hexDEV, but the USB VID is MCS, and the product name
is hexLIN...

So is 'hid-hexlin.c' or 'hid-hexdev-hexlin.c' okay or does it need to
be named 'hid-mcs-hexlin.c' in spite MCS has nearly nothing to do with
it?


Cheers
  -- Christoph