| Message ID | 20240502075534.882628-1-christoph.fritz@hexdev.de |
|---|---|
| Headers | show |
| Series | LIN Bus support for Linux | expand |
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@mercedes-benz.com> > Signed-off-by: Christoph Fritz <christoph.fritz@hexdev.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@hexdev.de>"); > +MODULE_DESCRIPTION("LIN bus driver for hexLIN USB adapter"); thanks,
On 02. 05. 24, 9:55, Christoph Fritz wrote: > This commit introduces LIN-Bus support for UART devices equipped with > LIN transceivers, utilizing the Serial Device Bus (serdev) interface. > > For more details on an adapter, visit: https://hexdev.de/hexlin#tty ... > --- /dev/null > +++ b/drivers/net/can/lin-serdev.c > @@ -0,0 +1,514 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* Copyright (C) 2024 hexDEV GmbH - https://hexdev.de */ > + > +#include <linux/module.h> > +#include <linux/wait.h> > +#include <linux/init.h> > +#include <linux/errno.h> > +#include <linux/string.h> > +#include <linux/kernel.h> What do you need kernel.h for? You should explicitly require what you need (you apparently do), so kernel.h should not be needed. > +#include <net/lin.h> > +#include <linux/of.h> > +#include <linux/serdev.h> > +#include <linux/slab.h> > +#include <linux/kfifo.h> > +#include <linux/workqueue.h> > +#include <linux/tty.h> Might be eaier to maintain if you sort them. > +#define LINSER_SAMPLES_PER_CHAR 10 > +#define LINSER_TX_BUFFER_SIZE 11 > +#define LINSER_RX_FIFO_SIZE 256 > +#define LINSER_PARSE_BUFFER 24 > + > +struct linser_rx { > + u8 data; > + u8 flag; > +}; > + > +enum linser_rx_status { > + NEED_MORE = -1, > + MODE_OK = 0, > + NEED_FORCE, > +}; > + > +struct linser_priv { > + struct lin_device *lin_dev; > + struct serdev_device *serdev; > + DECLARE_KFIFO_PTR(rx_fifo, struct linser_rx); > + struct delayed_work rx_work; > + ulong break_usleep_min; > + ulong break_usleep_max; > + ulong post_break_usleep_min; > + ulong post_break_usleep_max; > + ulong force_timeout_jfs; The same as for uint :) > + struct lin_responder_answer respond_answ[LIN_NUM_IDS]; > + struct mutex resp_lock; /* protects respond_answ */ > + bool is_stopped; > +}; ... > +static void linser_derive_timings(struct linser_priv *priv, u16 bitrate) > +{ > + unsigned long break_baud = (bitrate * 2) / 3; > + unsigned long timeout_us; > + Are those 1000000UL USEC_PER_SEC? > + priv->break_usleep_min = (1000000UL * LINSER_SAMPLES_PER_CHAR) / > + break_baud; > + priv->break_usleep_max = priv->break_usleep_min + 50; > + priv->post_break_usleep_min = (1000000UL * 1 /* 1 bit */) / break_baud; > + priv->post_break_usleep_max = priv->post_break_usleep_min + 30; > + > + timeout_us = DIV_ROUND_CLOSEST(1000000UL * 256 /* bit */, bitrate); > + priv->force_timeout_jfs = usecs_to_jiffies(timeout_us); > +} ... > +static bool linser_tx_frame_as_responder(struct linser_priv *priv, u8 id) > +{ > + struct lin_responder_answer *answ = &priv->respond_answ[id]; > + struct serdev_device *serdev = priv->serdev; > + u8 buf[LINSER_TX_BUFFER_SIZE]; > + u8 checksum, count, n; > + ssize_t write_len; > + > + mutex_lock(&priv->resp_lock); > + > + if (!answ->is_active) > + goto unlock_and_exit_false; > + > + if (answ->is_event_frame) { > + struct lin_responder_answer *e_answ; > + > + e_answ = &priv->respond_answ[answ->event_associated_id]; > + n = min(e_answ->lf.len, LIN_MAX_DLEN); > + if (memcmp(answ->lf.data, e_answ->lf.data, n) != 0) { > + memcpy(answ->lf.data, e_answ->lf.data, n); > + checksum = lin_get_checksum(LIN_FORM_PID(answ->lf.lin_id), > + n, e_answ->lf.data, > + answ->lf.checksum_mode); > + answ = e_answ; > + } else { > + goto unlock_and_exit_false; Can't you simply use guard(mutex) above and avoid the error-prone gotos/cleanup completely? > + } > + } else { > + checksum = answ->lf.checksum; > + } > + > + count = min(answ->lf.len, LIN_MAX_DLEN); > + memcpy(&buf[0], answ->lf.data, count); > + buf[count] = checksum; > + > + mutex_unlock(&priv->resp_lock); > + > + write_len = serdev_device_write(serdev, buf, count + 1, 0); > + if (write_len < count + 1) > + return false; > + > + serdev_device_wait_until_sent(serdev, 0); > + > + return true; > + > +unlock_and_exit_false: > + mutex_unlock(&priv->resp_lock); > + return false; > +} > + > +static void linser_pop_fifo(struct linser_priv *priv, size_t n) > +{ > + struct serdev_device *serdev = priv->serdev; > + struct linser_rx dummy; > + size_t ret, i; > + > + for (i = 0; i < n; i++) { > + ret = kfifo_out(&priv->rx_fifo, &dummy, 1); Does kfifo_skip() not work for records? (I added it recently for serial.) > + if (ret != 1) { > + dev_err(&serdev->dev, "Failed to pop from FIFO\n"); > + break; > + } > + } > +} thanks,
On 02/05/2024 09:55, Christoph Fritz wrote: > A LIN bus supports up to 64 identifiers in one byte. This commit adds a > special frame ID, beyond the actual LIN identifiers, for signaling RX > offload configuration requests. This ID will be utilized in future LIN > enhancements to the CAN broadcast manager. > > Signed-off-by: Christoph Fritz <christoph.fritz@hexdev.de> > --- > include/net/lin.h | 1 + You just added this file in other patch. What is the point of splitting line-per-line additions? There is no user of this in this patch. Squash it with the patch adding the file. Best regards, Krzysztof
On Thu, 2024-05-02 at 10:30 +0200, Jiri Slaby wrote: > 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@mercedes-benz.com> > > Signed-off-by: Christoph Fritz <christoph.fritz@hexdev.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. They are not necessary, even more so when I can drop the mutex_destroy() below. > > > + 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? > Just for code clarity and it should help if someone wants to use CONFIG_DEBUG_MUTEXES. To be able to drop the lock/unlock from above, I could add the lock/unlock here or just drop the mutex_destroy() completely. I'll just drop it in upcoming v3. > > +} > ... > > +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().) Looking at the other hid drivers and testing with just module_hid_driver(hexlin_driver) works here fine for compiled into the kernel and as a module. > > > +module_exit(hexlin_exit); > > + > > +MODULE_LICENSE("GPL"); > > +MODULE_AUTHOR("Christoph Fritz <christoph.fritz@hexdev.de>"); > > +MODULE_DESCRIPTION("LIN bus driver for hexLIN USB adapter"); Thanks -- Christoph
On Thu, 2024-05-02 at 12:27 +0200, Krzysztof Kozlowski wrote: > On 02/05/2024 09:55, Christoph Fritz wrote: > > A LIN bus supports up to 64 identifiers in one byte. This commit adds a > > special frame ID, beyond the actual LIN identifiers, for signaling RX > > offload configuration requests. This ID will be utilized in future LIN > > enhancements to the CAN broadcast manager. > > > > Signed-off-by: Christoph Fritz <christoph.fritz@hexdev.de> > > --- > > include/net/lin.h | 1 + > > You just added this file in other patch. What is the point of splitting > line-per-line additions? My intention was to make the review process easier by separating the BCM (Broadcast Manager) logic from the basic driver implementation. > There is no user of this in this patch. Squash it with the patch adding > the file. OK v3 is coming up Thanks -- Christoph
On Thu, 2024-05-02 at 10:44 +0200, Jiri Slaby wrote: > On 02. 05. 24, 9:55, Christoph Fritz wrote: > > This commit introduces LIN-Bus support for UART devices equipped with > > LIN transceivers, utilizing the Serial Device Bus (serdev) interface. > > > > For more details on an adapter, visit: https://hexdev.de/hexlin#tty > ... > > --- /dev/null > > +++ b/drivers/net/can/lin-serdev.c > > @@ -0,0 +1,514 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* Copyright (C) 2024 hexDEV GmbH - https://hexdev.de */ > > + > > +#include <linux/module.h> > > +#include <linux/wait.h> > > +#include <linux/init.h> > > +#include <linux/errno.h> > > +#include <linux/string.h> > > +#include <linux/kernel.h> > > What do you need kernel.h for? You should explicitly require what you > need (you apparently do), so kernel.h should not be needed. OK > > > +#include <net/lin.h> > > +#include <linux/of.h> > > +#include <linux/serdev.h> > > +#include <linux/slab.h> > > +#include <linux/kfifo.h> > > +#include <linux/workqueue.h> > > +#include <linux/tty.h> > > Might be eaier to maintain if you sort them. OK, hid driver gets also sorted > > > +#define LINSER_SAMPLES_PER_CHAR 10 > > +#define LINSER_TX_BUFFER_SIZE 11 > > +#define LINSER_RX_FIFO_SIZE 256 > > +#define LINSER_PARSE_BUFFER 24 > > + > > +struct linser_rx { > > + u8 data; > > + u8 flag; > > +}; > > + > > +enum linser_rx_status { > > + NEED_MORE = -1, > > + MODE_OK = 0, > > + NEED_FORCE, > > +}; > > + > > +struct linser_priv { > > + struct lin_device *lin_dev; > > + struct serdev_device *serdev; > > + DECLARE_KFIFO_PTR(rx_fifo, struct linser_rx); > > + struct delayed_work rx_work; > > + ulong break_usleep_min; > > + ulong break_usleep_max; > > + ulong post_break_usleep_min; > > + ulong post_break_usleep_max; > > + ulong force_timeout_jfs; > > The same as for uint :) OK > > > + struct lin_responder_answer respond_answ[LIN_NUM_IDS]; > > + struct mutex resp_lock; /* protects respond_answ */ > > + bool is_stopped; > > +}; > ... > > +static void linser_derive_timings(struct linser_priv *priv, u16 bitrate) > > +{ > > + unsigned long break_baud = (bitrate * 2) / 3; > > + unsigned long timeout_us; > > + > > Are those 1000000UL USEC_PER_SEC? yes > > > + priv->break_usleep_min = (1000000UL * LINSER_SAMPLES_PER_CHAR) / > > + break_baud; > > + priv->break_usleep_max = priv->break_usleep_min + 50; > > + priv->post_break_usleep_min = (1000000UL * 1 /* 1 bit */) / break_baud; > > + priv->post_break_usleep_max = priv->post_break_usleep_min + 30; > > + > > + timeout_us = DIV_ROUND_CLOSEST(1000000UL * 256 /* bit */, bitrate); > > + priv->force_timeout_jfs = usecs_to_jiffies(timeout_us); > > +} > ... > > +static bool linser_tx_frame_as_responder(struct linser_priv *priv, u8 id) > > +{ > > + struct lin_responder_answer *answ = &priv->respond_answ[id]; > > + struct serdev_device *serdev = priv->serdev; > > + u8 buf[LINSER_TX_BUFFER_SIZE]; > > + u8 checksum, count, n; > > + ssize_t write_len; > > + > > + mutex_lock(&priv->resp_lock); > > + > > + if (!answ->is_active) > > + goto unlock_and_exit_false; > > + > > + if (answ->is_event_frame) { > > + struct lin_responder_answer *e_answ; > > + > > + e_answ = &priv->respond_answ[answ->event_associated_id]; > > + n = min(e_answ->lf.len, LIN_MAX_DLEN); > > + if (memcmp(answ->lf.data, e_answ->lf.data, n) != 0) { > > + memcpy(answ->lf.data, e_answ->lf.data, n); > > + checksum = lin_get_checksum(LIN_FORM_PID(answ->lf.lin_id), > > + n, e_answ->lf.data, > > + answ->lf.checksum_mode); > > + answ = e_answ; > > + } else { > > + goto unlock_and_exit_false; > > Can't you simply use guard(mutex) above and avoid the error-prone > gotos/cleanup completely? OK > > > + } > > + } else { > > + checksum = answ->lf.checksum; > > + } > > + > > + count = min(answ->lf.len, LIN_MAX_DLEN); > > + memcpy(&buf[0], answ->lf.data, count); > > + buf[count] = checksum; > > + > > + mutex_unlock(&priv->resp_lock); > > + > > + write_len = serdev_device_write(serdev, buf, count + 1, 0); > > + if (write_len < count + 1) > > + return false; > > + > > + serdev_device_wait_until_sent(serdev, 0); > > + > > + return true; > > + > > +unlock_and_exit_false: > > + mutex_unlock(&priv->resp_lock); > > + return false; > > +} > > + > > +static void linser_pop_fifo(struct linser_priv *priv, size_t n) > > +{ > > + struct serdev_device *serdev = priv->serdev; > > + struct linser_rx dummy; > > + size_t ret, i; > > + > > + for (i = 0; i < n; i++) { > > + ret = kfifo_out(&priv->rx_fifo, &dummy, 1); > > Does kfifo_skip() not work for records? (I added it recently for serial.) Using kfifo_skip() greatly simplifies this function and it works for records (uses __kfifo_skip_r), tests are successful. Maybe the comment in kfifo.h could be made more clear from: "kfifo_skip - skip output data" to "kfifo_skip - skip the next fifo record" > > > + if (ret != 1) { > > + dev_err(&serdev->dev, "Failed to pop from FIFO\n"); > > + break; > > + } > > + } > > +} > Let me address these points and reroll in v3. Thanks -- Christoph
On Thu, May 02, 2024 at 09:55:23AM +0200, Christoph Fritz wrote: > This patch adds a LIN (local interconnect network) bus abstraction on > top of CAN. It is a glue driver adapting CAN on one side while offering > LIN abstraction on the other side. So that upcoming LIN device drivers > can make use of it. > > Tested-by: Andreas Lauser <andreas.lauser@mercedes-benz.com> > Signed-off-by: Christoph Fritz <christoph.fritz@hexdev.de> ... > diff --git a/drivers/net/can/lin.c b/drivers/net/can/lin.c ... > +struct lin_device *register_lin(struct device *dev, > + const struct lin_device_ops *ldops) > +{ > + struct net_device *ndev; > + struct lin_device *ldev; > + int ret; > + > + if (!ldops || !ldops->ldo_tx || !ldops->update_bitrate || > + !ldops->ldo_open || !ldops->ldo_stop) { > + netdev_err(ndev, "missing mandatory lin_device_ops\n"); Hi Christoph, The line above uses ndev, but ndev is not initialised until a few lines further down. Flagged by Smatch. > + return ERR_PTR(-EINVAL); > + } > + > + ndev = alloc_candev(sizeof(struct lin_device), 1); > + if (!ndev) > + return ERR_PTR(-ENOMEM); > + > + ldev = netdev_priv(ndev); > + > + ldev->ldev_ops = ldops; > + ndev->netdev_ops = &lin_netdev_ops; > + ndev->flags |= IFF_ECHO; > + ndev->mtu = CANFD_MTU; > + ldev->can.bittiming.bitrate = LIN_DEFAULT_BAUDRATE; > + ldev->can.ctrlmode = CAN_CTRLMODE_LIN; > + ldev->can.ctrlmode_supported = 0; > + ldev->can.bitrate_const = lin_bitrate; > + ldev->can.bitrate_const_cnt = ARRAY_SIZE(lin_bitrate); > + ldev->can.do_set_bittiming = lin_set_bittiming; > + ldev->ndev = ndev; > + ldev->dev = dev; > + > + SET_NETDEV_DEV(ndev, dev); > + > + ret = lin_set_bittiming(ndev); > + if (ret) { > + netdev_err(ndev, "set bittiming failed\n"); > + goto exit_candev; > + } > + > + ret = register_candev(ndev); > + if (ret) > + goto exit_candev; > + > + ldev->lin_ids_kobj = kobject_create_and_add("lin_ids", &ndev->dev.kobj); > + if (!ldev->lin_ids_kobj) { > + netdev_err(ndev, "Failed to create sysfs directory\n"); > + ret = -ENOMEM; > + goto exit_unreg; > + } > + > + ret = lin_create_sysfs_id_files(ndev); > + if (ret) { > + netdev_err(ndev, "Failed to create sysfs entry: %d\n", ret); > + goto exit_kobj_put; > + } > + > + /* Using workqueue as tx over USB/SPI/... may sleep */ > + ldev->wq = alloc_workqueue(dev_name(dev), WQ_FREEZABLE | WQ_MEM_RECLAIM, > + 0); > + if (!ldev->wq) > + goto exit_rm_files; The goto above will result in: return ERR_PTR(ret) But ret is 0 here. Should it be set to a negative error value? Also flagged by Smatch. > + > + INIT_WORK(&ldev->tx_work, lin_tx_work_handler); > + > + netdev_info(ndev, "LIN initialized.\n"); > + > + return ldev; > + > +exit_rm_files: > + lin_remove_sysfs_id_files(ndev); > +exit_kobj_put: > + kobject_put(ldev->lin_ids_kobj); > +exit_unreg: > + unregister_candev(ndev); > +exit_candev: > + free_candev(ndev); > + return ERR_PTR(ret); > +} > +EXPORT_SYMBOL_GPL(register_lin); ...
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@mercedes-benz.com> > Signed-off-by: Christoph Fritz <christoph.fritz@hexdev.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. ... > +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. > + > + 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. > + } > + > + 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; > +} ...
On Thu, May 02, 2024 at 09:55:29AM +0200, Christoph Fritz wrote: > This commit introduces LIN-Bus support for UART devices equipped with > LIN transceivers, utilizing the Serial Device Bus (serdev) interface. > > For more details on an adapter, visit: https://hexdev.de/hexlin#tty > > Signed-off-by: Christoph Fritz <christoph.fritz@hexdev.de> ... > diff --git a/drivers/net/can/lin-serdev.c b/drivers/net/can/lin-serdev.c ... > +static int linser_probe(struct serdev_device *serdev) > +{ > + struct linser_priv *priv; > + int ret; > + > + priv = devm_kzalloc(&serdev->dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + ret = kfifo_alloc(&priv->rx_fifo, LINSER_RX_FIFO_SIZE, GFP_KERNEL); > + if (ret) > + return ret; > + > + INIT_DELAYED_WORK(&priv->rx_work, linser_process_delayed); > + > + priv->serdev = serdev; > + serdev_device_set_drvdata(serdev, priv); > + serdev_device_set_client_ops(serdev, &linser_ops); > + > + ret = serdev_device_open(serdev); > + if (ret) { > + dev_err(&serdev->dev, "Unable to open device\n"); > + goto err_open; > + } > + > + serdev_device_set_flow_control(serdev, false); > + serdev_device_set_break_detection(serdev, true); > + linser_derive_timings(priv, LIN_DEFAULT_BAUDRATE); > + > + mutex_init(&priv->resp_lock); > + > + priv->lin_dev = register_lin(&serdev->dev, &linser_lindev_ops); > + if (IS_ERR_OR_NULL(priv->lin_dev)) { > + ret = PTR_ERR(priv->lin_dev); As per my feedback on an earlier patch in the series, in the case where priv->lin_dev is NULL, this will result in the function returning 0. Is that ok? Flagged by Smatch > + goto err_register_lin; > + } > + > + serdev_device_close(serdev); > + priv->is_stopped = true; > + > + return 0; > + > +err_register_lin: > + serdev_device_close(serdev); > +err_open: > + kfifo_free(&priv->rx_fifo); > + return ret; > +} ...
On Sat, 2024-05-04 at 13:49 +0100, Simon Horman wrote: > On Thu, May 02, 2024 at 09:55:23AM +0200, Christoph Fritz wrote: > > This patch adds a LIN (local interconnect network) bus abstraction on > > top of CAN. It is a glue driver adapting CAN on one side while offering > > LIN abstraction on the other side. So that upcoming LIN device drivers > > can make use of it. > > > > Tested-by: Andreas Lauser <andreas.lauser@mercedes-benz.com> > > Signed-off-by: Christoph Fritz <christoph.fritz@hexdev.de> > > ... > > > diff --git a/drivers/net/can/lin.c b/drivers/net/can/lin.c > > ... > > > +struct lin_device *register_lin(struct device *dev, > > + const struct lin_device_ops *ldops) > > +{ > > + struct net_device *ndev; > > + struct lin_device *ldev; > > + int ret; > > + > > + if (!ldops || !ldops->ldo_tx || !ldops->update_bitrate || > > + !ldops->ldo_open || !ldops->ldo_stop) { > > + netdev_err(ndev, "missing mandatory lin_device_ops\n"); > > Hi Christoph, Hi Simon > The line above uses ndev, but ndev is not initialised > until a few lines further down. > > Flagged by Smatch. Despite netdev_err() checks validity of ndev, I agree with Smatch: In upcoming v4 I'll use dev_err() here instead. > > + return ERR_PTR(-EINVAL); > > + } > > + > > + ndev = alloc_candev(sizeof(struct lin_device), 1); > > + if (!ndev) > > + return ERR_PTR(-ENOMEM); > > + > > + ldev = netdev_priv(ndev); > > + > > + ldev->ldev_ops = ldops; > > + ndev->netdev_ops = &lin_netdev_ops; > > + ndev->flags |= IFF_ECHO; > > + ndev->mtu = CANFD_MTU; > > + ldev->can.bittiming.bitrate = LIN_DEFAULT_BAUDRATE; > > + ldev->can.ctrlmode = CAN_CTRLMODE_LIN; > > + ldev->can.ctrlmode_supported = 0; > > + ldev->can.bitrate_const = lin_bitrate; > > + ldev->can.bitrate_const_cnt = ARRAY_SIZE(lin_bitrate); > > + ldev->can.do_set_bittiming = lin_set_bittiming; > > + ldev->ndev = ndev; > > + ldev->dev = dev; > > + > > + SET_NETDEV_DEV(ndev, dev); > > + > > + ret = lin_set_bittiming(ndev); > > + if (ret) { > > + netdev_err(ndev, "set bittiming failed\n"); > > + goto exit_candev; > > + } > > + > > + ret = register_candev(ndev); > > + if (ret) > > + goto exit_candev; > > + > > + ldev->lin_ids_kobj = kobject_create_and_add("lin_ids", &ndev->dev.kobj); > > + if (!ldev->lin_ids_kobj) { > > + netdev_err(ndev, "Failed to create sysfs directory\n"); > > + ret = -ENOMEM; > > + goto exit_unreg; > > + } > > + > > + ret = lin_create_sysfs_id_files(ndev); > > + if (ret) { > > + netdev_err(ndev, "Failed to create sysfs entry: %d\n", ret); > > + goto exit_kobj_put; > > + } > > + > > + /* Using workqueue as tx over USB/SPI/... may sleep */ > > + ldev->wq = alloc_workqueue(dev_name(dev), WQ_FREEZABLE | WQ_MEM_RECLAIM, > > + 0); > > + if (!ldev->wq) > > + goto exit_rm_files; > > The goto above will result in: return ERR_PTR(ret) > But ret is 0 here. Should it be set to a negative error value? > > Also flagged by Smatch. OK, will get an ret = -ENOMEM; > > > + > > + INIT_WORK(&ldev->tx_work, lin_tx_work_handler); > > + > > + netdev_info(ndev, "LIN initialized.\n"); > > + > > + return ldev; > > + > > +exit_rm_files: > > + lin_remove_sysfs_id_files(ndev); > > +exit_kobj_put: > > + kobject_put(ldev->lin_ids_kobj); > > +exit_unreg: > > + unregister_candev(ndev); > > +exit_candev: > > + free_candev(ndev); > > + return ERR_PTR(ret); > > +} > > +EXPORT_SYMBOL_GPL(register_lin); > > ... Thanks -- Christoph
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@mercedes-benz.com> > > Signed-off-by: Christoph Fritz <christoph.fritz@hexdev.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
On Sat, 2024-05-04 at 14:13 +0100, Simon Horman wrote: > On Thu, May 02, 2024 at 09:55:29AM +0200, Christoph Fritz wrote: > > This commit introduces LIN-Bus support for UART devices equipped with > > LIN transceivers, utilizing the Serial Device Bus (serdev) interface. > > > > For more details on an adapter, visit: https://hexdev.de/hexlin#tty > > > > Signed-off-by: Christoph Fritz <christoph.fritz@hexdev.de> > > ... > > > diff --git a/drivers/net/can/lin-serdev.c b/drivers/net/can/lin-serdev.c > > ... > > > +static int linser_probe(struct serdev_device *serdev) > > +{ > > + struct linser_priv *priv; > > + int ret; > > + > > + priv = devm_kzalloc(&serdev->dev, sizeof(*priv), GFP_KERNEL); > > + if (!priv) > > + return -ENOMEM; > > + > > + ret = kfifo_alloc(&priv->rx_fifo, LINSER_RX_FIFO_SIZE, GFP_KERNEL); > > + if (ret) > > + return ret; > > + > > + INIT_DELAYED_WORK(&priv->rx_work, linser_process_delayed); > > + > > + priv->serdev = serdev; > > + serdev_device_set_drvdata(serdev, priv); > > + serdev_device_set_client_ops(serdev, &linser_ops); > > + > > + ret = serdev_device_open(serdev); > > + if (ret) { > > + dev_err(&serdev->dev, "Unable to open device\n"); > > + goto err_open; > > + } > > + > > + serdev_device_set_flow_control(serdev, false); > > + serdev_device_set_break_detection(serdev, true); > > + linser_derive_timings(priv, LIN_DEFAULT_BAUDRATE); > > + > > + mutex_init(&priv->resp_lock); > > + > > + priv->lin_dev = register_lin(&serdev->dev, &linser_lindev_ops); > > + if (IS_ERR_OR_NULL(priv->lin_dev)) { > > + ret = PTR_ERR(priv->lin_dev); > > As per my feedback on an earlier patch in the series, > in the case where priv->lin_dev is NULL, > this will result in the function returning 0. > Is that ok? > > Flagged by Smatch IS_ERR_OR_NULL() gets IS_ERR() in upcoming v4 Thanks -- Christoph > > > + goto err_register_lin; > > + } > > + > > + serdev_device_close(serdev); > > + priv->is_stopped = true; > > + > > + return 0; > > + > > +err_register_lin: > > + serdev_device_close(serdev); > > +err_open: > > + kfifo_free(&priv->rx_fifo); > > + return ret; > > +} > > ...