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