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@xxxxxxxxx>");
> > +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
