Message ID | 20240502182804.145926-1-christoph.fritz@hexdev.de |
---|---|
Headers | show |
Series | LIN Bus support for Linux | expand |
On Thu, May 02, 2024 at 08:27:56PM +0200, Christoph Fritz wrote: > This patch introduces an additional receive buffer callback variation > besides the already existing receive_buf(). This new callback function > also passes the flag buffer (TTY_NORMAL, TTY_BREAK, and friends). > > If defined, this function gets prioritized and called instead of the > standard receive_buf(). > > An alternative approach could have been to enhance the receive_buf() > function and update all drivers that use it. Please, let's do that instead of adding random letters at the end of a function pointer :) > Signed-off-by: Christoph Fritz <christoph.fritz@hexdev.de> > --- > drivers/tty/serdev/serdev-ttyport.c | 2 +- > include/linux/serdev.h | 17 ++++++++++++++--- > 2 files changed, 15 insertions(+), 4 deletions(-) > > diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c > index 3d7ae7fa50186..bb47691afdb21 100644 > --- a/drivers/tty/serdev/serdev-ttyport.c > +++ b/drivers/tty/serdev/serdev-ttyport.c > @@ -32,7 +32,7 @@ static size_t ttyport_receive_buf(struct tty_port *port, const u8 *cp, > if (!test_bit(SERPORT_ACTIVE, &serport->flags)) > return 0; > > - ret = serdev_controller_receive_buf(ctrl, cp, count); > + ret = serdev_controller_receive_buf(ctrl, cp, fp, count); > > dev_WARN_ONCE(&ctrl->dev, ret > count, > "receive_buf returns %zu (count = %zu)\n", > diff --git a/include/linux/serdev.h b/include/linux/serdev.h > index ff78efc1f60df..c6ef5a8988e07 100644 > --- a/include/linux/serdev.h > +++ b/include/linux/serdev.h > @@ -23,11 +23,17 @@ struct serdev_device; > * struct serdev_device_ops - Callback operations for a serdev device > * @receive_buf: Function called with data received from device; > * returns number of bytes accepted; may sleep. > + * @receive_buf_fp: Function called with data and flag buffer received > + * from device; If defined, this function gets called > + * instead of @receive_buf; > + * returns number of bytes accepted; may sleep. I don't remember waht "fp" means here, and you don't document it, so let's just have one recieve_buf() callback please. thanks, greg k-h
On Thu, 2 May 2024, 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> > --- > drivers/net/can/Kconfig | 10 + > drivers/net/can/Makefile | 1 + > drivers/net/can/lin.c | 495 +++++++++++++++++++++++++++++++ > include/net/lin.h | 92 ++++++ > include/uapi/linux/can/netlink.h | 1 + > 5 files changed, 599 insertions(+) > create mode 100644 drivers/net/can/lin.c > create mode 100644 include/net/lin.h > > diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig > index 2e31db55d9278..0934bbf8d03b2 100644 > --- a/drivers/net/can/Kconfig > +++ b/drivers/net/can/Kconfig > @@ -171,6 +171,16 @@ config CAN_KVASER_PCIEFD > Kvaser M.2 PCIe 4xCAN > Kvaser PCIe 8xCAN > > +config CAN_LIN > + tristate "LIN mode support" > + help > + This is a glue driver for LIN-BUS support. > + > + The local interconnect (LIN) bus is a simple bus with a feature > + subset of CAN. It is often combined with CAN for simple controls. > + > + Actual device drivers need to be enabled too. > + > config CAN_SLCAN > tristate "Serial / USB serial CAN Adaptors (slcan)" > depends on TTY > diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile > index 4669cd51e7bf5..0093ee9219ca8 100644 > --- a/drivers/net/can/Makefile > +++ b/drivers/net/can/Makefile > @@ -25,6 +25,7 @@ obj-$(CONFIG_CAN_GRCAN) += grcan.o > obj-$(CONFIG_CAN_IFI_CANFD) += ifi_canfd/ > obj-$(CONFIG_CAN_JANZ_ICAN3) += janz-ican3.o > obj-$(CONFIG_CAN_KVASER_PCIEFD) += kvaser_pciefd.o > +obj-$(CONFIG_CAN_LIN) += lin.o > obj-$(CONFIG_CAN_MSCAN) += mscan/ > obj-$(CONFIG_CAN_M_CAN) += m_can/ > obj-$(CONFIG_CAN_PEAK_PCIEFD) += peak_canfd/ > diff --git a/drivers/net/can/lin.c b/drivers/net/can/lin.c > new file mode 100644 > index 0000000000000..95906003666fb > --- /dev/null > +++ b/drivers/net/can/lin.c > @@ -0,0 +1,495 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* Copyright (C) 2024 hexDEV GmbH - https://hexdev.de */ > + > +#include <linux/can/core.h> > +#include <linux/can/dev.h> > +#include <linux/can/error.h> > +#include <linux/module.h> > +#include <linux/device.h> > +#include <linux/netdevice.h> > +#include <linux/init.h> > +#include <linux/kernel.h> > +#include <net/lin.h> > + > +static const u8 lin_id_parity_tbl[] = { > + 0x80, 0xC0, 0x40, 0x00, 0xC0, 0x80, 0x00, 0x40, > + 0x00, 0x40, 0xC0, 0x80, 0x40, 0x00, 0x80, 0xC0, > + 0x40, 0x00, 0x80, 0xC0, 0x00, 0x40, 0xC0, 0x80, > + 0xC0, 0x80, 0x00, 0x40, 0x80, 0xC0, 0x40, 0x00, > + 0x00, 0x40, 0xC0, 0x80, 0x40, 0x00, 0x80, 0xC0, > + 0x80, 0xC0, 0x40, 0x00, 0xC0, 0x80, 0x00, 0x40, > + 0xC0, 0x80, 0x00, 0x40, 0x80, 0xC0, 0x40, 0x00, > + 0x40, 0x00, 0x80, 0xC0, 0x00, 0x40, 0xC0, 0x80, > +}; > + > +u8 lin_get_id_parity(u8 id) > +{ > + return lin_id_parity_tbl[id]; > +} > +EXPORT_SYMBOL(lin_get_id_parity); > + > +static ssize_t lin_identifier_show(struct kobject *kobj, > + struct kobj_attribute *attr, char *buf) > +{ > + struct lin_attr *lin_attr = container_of(attr, struct lin_attr, attr); > + struct lin_device *ldev = lin_attr->ldev; > + ssize_t count = 0; > + struct lin_responder_answer answ; > + int k, ret; > + long id; > + > + if (!ldev->ldev_ops->get_responder_answer) > + return -EOPNOTSUPP; > + > + ret = kstrtol(attr->attr.name, 16, &id); > + if (ret) > + return ret; > + if (id < 0 || id >= LIN_NUM_IDS) > + return -EINVAL; > + > + count += scnprintf(buf + count, PAGE_SIZE - count, > + "%-6s %-11s %-9s %-9s %-2s %-24s %-6s\n", > + "state", "cksum-mode", "is_event", "event_id", > + "n", "data", "cksum"); Onl use sysfs_emit() and sysfs_emit_at() in *_show functions. > + > + ret = ldev->ldev_ops->get_responder_answer(ldev, id, &answ); > + if (ret) > + return ret; > + > + count += scnprintf(buf + count, PAGE_SIZE - count, > + "%-6s %-11s %-9s %-9u %-2u ", > + answ.is_active ? "active" : "off", > + answ.lf.checksum_mode ? "enhanced" : "classic", > + answ.is_event_frame ? "yes" : "no", > + answ.event_associated_id, > + answ.lf.len); > + > + for (k = 0; k < answ.lf.len; k++) > + count += scnprintf(buf + count, PAGE_SIZE - count, > + "%02x ", answ.lf.data[k]); > + for (; k < 8; k++) > + count += scnprintf(buf + count, PAGE_SIZE - count, > + " "); > + if (answ.lf.len) > + count += scnprintf(buf + count, PAGE_SIZE - count, > + " %02x", answ.lf.checksum); > + > + count += scnprintf(buf + count, PAGE_SIZE - count, "\n"); > + > + return count; > +} > + > +static const char *parse_and_advance(const char *buf, long *result, > + unsigned int base) > +{ > + char num_str[5] = {0}; > + int num_len = 0; > + > + while (*buf && isspace(*buf)) > + buf++; > + while (*buf && isalnum(*buf) && num_len < sizeof(num_str) - 1) > + num_str[num_len++] = *buf++; > + if (kstrtol(num_str, base, result)) > + return NULL; > + > + return buf; > +} > + > +static ssize_t lin_identifier_store(struct kobject *kobj, > + struct kobj_attribute *attr, > + const char *buf, size_t count) > +{ > + struct lin_attr *lin_attr = container_of(attr, struct lin_attr, attr); > + struct lin_device *ldev = lin_attr->ldev; > + struct lin_responder_answer answ = { 0 }; > + const char *ptr = buf; > + int ret; > + long v; > + > + if (!ldev->ldev_ops->update_responder_answer) > + return -EOPNOTSUPP; > + > + ret = kstrtol(attr->attr.name, 16, &v); > + if (ret) > + return ret; > + if (v < 0 || v >= LIN_NUM_IDS) > + return -EINVAL; > + answ.lf.lin_id = v; > + > + ptr = parse_and_advance(ptr, &v, 2); > + if (!ptr) > + return -EINVAL; > + answ.is_active = v != 0; > + > + ptr = parse_and_advance(ptr, &v, 2); > + if (!ptr) > + return -EINVAL; > + answ.lf.checksum_mode = v != 0; > + > + ptr = parse_and_advance(ptr, &v, 2); > + if (!ptr) > + return -EINVAL; > + answ.is_event_frame = v != 0; > + > + ptr = parse_and_advance(ptr, &v, 16); > + if (!ptr || v > LIN_ID_MASK) > + return -EINVAL; > + answ.event_associated_id = v; > + > + ptr = parse_and_advance(ptr, &v, 16); > + if (!ptr || v > LIN_MAX_DLEN) > + return -EINVAL; > + answ.lf.len = v; > + > + for (int i = 0; i < answ.lf.len; i++) { > + ptr = parse_and_advance(ptr, &v, 16); > + if (!ptr) > + return -EINVAL; > + answ.lf.data[i] = v; > + } > + > + ret = ldev->ldev_ops->update_responder_answer(ldev, &answ); > + if (ret) > + return ret; > + > + return count; > +} > + > +static int lin_create_sysfs_id_files(struct net_device *ndev) > +{ > + struct lin_device *ldev = netdev_priv(ndev); > + struct kobj_attribute *attr; > + int ret; > + > + for (int id = 0; id < LIN_NUM_IDS; id++) { > + ldev->sysfs_entries[id].ldev = ldev; > + attr = &ldev->sysfs_entries[id].attr; > + attr->attr.name = kasprintf(GFP_KERNEL, "%02x", id); > + if (!attr->attr.name) > + return -ENOMEM; > + attr->attr.mode = 0644; > + attr->show = lin_identifier_show; > + attr->store = lin_identifier_store; > + > + sysfs_attr_init(&attr->attr); > + ret = sysfs_create_file(ldev->lin_ids_kobj, &attr->attr); > + if (ret) { > + kfree(attr->attr.name); > + return -ENOMEM; > + } > + } > + > + return 0; > +} Can you use .dev_groups instead ? FWIW, this function doesn't do rollback when error occurs. > + > +static void lin_remove_sysfs_id_files(struct net_device *ndev) > +{ > + struct lin_device *ldev = netdev_priv(ndev); > + struct kobj_attribute *attr; > + > + for (int id = 0; id < LIN_NUM_IDS; id++) { > + attr = &ldev->sysfs_entries[id].attr; > + sysfs_remove_file(ldev->lin_ids_kobj, &attr->attr); > + kfree(attr->attr.name); > + } > +} > + > +static void lin_tx_work_handler(struct work_struct *ws) > +{ > + struct lin_device *ldev = container_of(ws, struct lin_device, > + tx_work); > + struct net_device *ndev = ldev->ndev; > + struct canfd_frame *cfd; > + struct lin_frame lf; > + int ret; > + > + ldev->tx_busy = true; > + > + cfd = (struct canfd_frame *)ldev->tx_skb->data; > + lf.checksum_mode = (cfd->can_id & LIN_ENHANCED_CKSUM_FLAG) ? > + LINBUS_ENHANCED : LINBUS_CLASSIC; > + lf.lin_id = cfd->can_id & LIN_ID_MASK; > + lf.len = min(cfd->len, LIN_MAX_DLEN); > + memcpy(lf.data, cfd->data, lf.len); > + > + ret = ldev->ldev_ops->ldo_tx(ldev, &lf); > + if (ret) { > + DEV_STATS_INC(ndev, tx_dropped); > + netdev_err_once(ndev, "transmission failure %d\n", ret); > + goto lin_tx_out; > + } > + > + DEV_STATS_INC(ndev, tx_packets); > + DEV_STATS_ADD(ndev, tx_bytes, lf.len); > + > +lin_tx_out: > + ldev->tx_busy = false; > + netif_wake_queue(ndev); > +} > + > +static netdev_tx_t lin_start_xmit(struct sk_buff *skb, > + struct net_device *ndev) > +{ > + struct lin_device *ldev = netdev_priv(ndev); > + > + if (ldev->tx_busy) > + return NETDEV_TX_BUSY; > + > + netif_stop_queue(ndev); > + ldev->tx_skb = skb; > + queue_work(ldev->wq, &ldev->tx_work); > + > + return NETDEV_TX_OK; > +} > + > +static int lin_open(struct net_device *ndev) > +{ > + struct lin_device *ldev = netdev_priv(ndev); > + int ret; > + > + ldev->tx_busy = false; > + > + ret = open_candev(ndev); > + if (ret) > + return ret; > + > + netif_wake_queue(ndev); > + > + ldev->can.state = CAN_STATE_ERROR_ACTIVE; > + ndev->mtu = CANFD_MTU; > + > + return ldev->ldev_ops->ldo_open(ldev); > +} > + > +static int lin_stop(struct net_device *ndev) > +{ > + struct lin_device *ldev = netdev_priv(ndev); > + > + close_candev(ndev); > + > + flush_work(&ldev->tx_work); > + > + ldev->can.state = CAN_STATE_STOPPED; > + > + return ldev->ldev_ops->ldo_stop(ldev); > +} > + > +static const struct net_device_ops lin_netdev_ops = { > + .ndo_open = lin_open, > + .ndo_stop = lin_stop, > + .ndo_start_xmit = lin_start_xmit, > + .ndo_change_mtu = can_change_mtu, > +}; > + > +u8 lin_get_checksum(u8 pid, u8 n_of_bytes, const u8 *bytes, > + enum lin_checksum_mode cm) > +{ > + unsigned int csum = 0; > + int i; > + > + if (cm == LINBUS_ENHANCED) > + csum += pid; > + > + for (i = 0; i < n_of_bytes; i++) { > + csum += bytes[i]; > + if (csum > 255) > + csum -= 255; > + } > + > + return (~csum & 0xff); > +} > +EXPORT_SYMBOL_GPL(lin_get_checksum); > + > +static int lin_bump_rx_err(struct lin_device *ldev, const struct lin_frame *lf) > +{ > + struct net_device *ndev = ldev->ndev; > + struct can_frame cf = {0 }; > + > + if (lf->lin_id > LIN_ID_MASK) { > + netdev_dbg(ndev, "id exceeds LIN max id\n"); > + cf.can_id = CAN_ERR_FLAG | CAN_ERR_PROT; > + cf.data[3] = CAN_ERR_PROT_LOC_ID12_05; > + } > + > + if (lf->len > LIN_MAX_DLEN) { > + netdev_dbg(ndev, "frame exceeds number of bytes\n"); > + cf.can_id = CAN_ERR_FLAG | CAN_ERR_PROT; > + cf.data[3] = CAN_ERR_PROT_LOC_DLC; > + } > + > + if (lf->len) { > + u8 checksum = lin_get_checksum(LIN_FORM_PID(lf->lin_id), > + lf->len, lf->data, > + lf->checksum_mode); > + > + if (checksum != lf->checksum) { > + netdev_dbg(ndev, "expected cksm: 0x%02x got: 0x%02x\n", > + checksum, lf->checksum); > + cf.can_id = CAN_ERR_FLAG | CAN_ERR_PROT; > + cf.data[2] = CAN_ERR_PROT_FORM; > + } > + } > + > + if (cf.can_id & CAN_ERR_FLAG) { > + struct can_frame *err_cf; > + struct sk_buff *skb = alloc_can_err_skb(ndev, &err_cf); > + > + if (unlikely(!skb)) > + return -ENOMEM; > + > + err_cf->can_id |= cf.can_id; > + memcpy(err_cf->data, cf.data, CAN_MAX_DLEN); > + > + netif_rx(skb); > + > + return -EREMOTEIO; > + } > + > + return 0; > +} > + > +int lin_rx(struct lin_device *ldev, const struct lin_frame *lf) > +{ > + struct net_device *ndev = ldev->ndev; > + struct can_frame *cf; > + struct sk_buff *skb; > + int ret; > + > + if (ldev->can.state == CAN_STATE_STOPPED) > + return 0; > + > + netdev_dbg(ndev, "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"); > + > + ret = lin_bump_rx_err(ldev, lf); > + if (ret) { > + DEV_STATS_INC(ndev, rx_dropped); > + return ret; > + } > + > + skb = alloc_can_skb(ndev, &cf); > + if (unlikely(!skb)) { > + DEV_STATS_INC(ndev, rx_dropped); > + return -ENOMEM; > + } > + > + cf->can_id = lf->lin_id; > + cf->len = min(lf->len, LIN_MAX_DLEN); > + memcpy(cf->data, lf->data, cf->len); > + > + DEV_STATS_INC(ndev, rx_packets); > + DEV_STATS_ADD(ndev, rx_bytes, cf->len); > + > + netif_receive_skb(skb); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(lin_rx); > + > +static int lin_set_bittiming(struct net_device *ndev) > +{ > + struct lin_device *ldev = netdev_priv(ndev); > + unsigned int bitrate = ldev->can.bittiming.bitrate; > + > + return ldev->ldev_ops->update_bitrate(ldev, bitrate); > +} > + > +static const u32 lin_bitrate[] = { 1200, 2400, 4800, 9600, 19200 }; > + > +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"); > + 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; > + > + 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); > + > +void unregister_lin(struct lin_device *ldev) > +{ > + struct net_device *ndev = ldev->ndev; > + > + lin_remove_sysfs_id_files(ndev); > + kobject_put(ldev->lin_ids_kobj); > + unregister_candev(ndev); > + destroy_workqueue(ldev->wq); > + free_candev(ndev); > +} > +EXPORT_SYMBOL_GPL(unregister_lin); > + > +MODULE_LICENSE("GPL"); > +MODULE_AUTHOR("Christoph Fritz <christoph.fritz@hexdev.de>"); > +MODULE_DESCRIPTION("LIN bus to CAN glue driver"); > diff --git a/include/net/lin.h b/include/net/lin.h > new file mode 100644 > index 0000000000000..e7c7c820a6e18 > --- /dev/null > +++ b/include/net/lin.h > @@ -0,0 +1,92 @@ > +/* SPDX-License-Identifier: GPL-2.0+ */ > +/* Copyright (C) 2024 hexDEV GmbH - https://hexdev.de */ > + > +#ifndef _NET_LIN_H_ > +#define _NET_LIN_H_ > + > +#include <linux/bitfield.h> > +#include <linux/can/dev.h> > +#include <linux/device.h> > + > +#define LIN_NUM_IDS 64 > +#define LIN_HEADER_SIZE 3 > +#define LIN_MAX_DLEN 8 > + > +#define LIN_MAX_BAUDRATE 20000 > +#define LIN_MIN_BAUDRATE 1000 > +#define LIN_DEFAULT_BAUDRATE 9600 > +#define LIN_SYNC_BYTE 0x55 > + > +#define LIN_ID_MASK GENMASK(5, 0) > +/* special ID descriptions for LIN */ > +#define LIN_RXOFFLOAD_DATA_FLAG 0x00000200U > +#define LIN_ENHANCED_CKSUM_FLAG 0x00000100U BIT(x) x 2 > + > +extern u8 lin_get_id_parity(u8 id); > + > +#define LIN_GET_ID(PID) FIELD_GET(LIN_ID_MASK, PID) > +#define LIN_FORM_PID(ID) (LIN_GET_ID(ID) | \ > + lin_get_id_parity(LIN_GET_ID(ID))) > +#define LIN_GET_PARITY(PID) ((PID) & ~LIN_ID_MASK) > +#define LIN_CHECK_PID(PID) (LIN_GET_PARITY(PID) == \ > + LIN_GET_PARITY(LIN_FORM_PID(PID))) > + > +struct lin_attr { > + struct kobj_attribute attr; > + struct lin_device *ldev; > +}; > + > +struct lin_device { > + struct can_priv can; /* must be the first member */ > + struct net_device *ndev; > + struct device *dev; > + const struct lin_device_ops *ldev_ops; > + struct workqueue_struct *wq; > + struct work_struct tx_work; > + bool tx_busy; > + struct sk_buff *tx_skb; > + struct kobject *lin_ids_kobj; > + struct lin_attr sysfs_entries[LIN_NUM_IDS]; > +}; > + > +enum lin_checksum_mode { > + LINBUS_CLASSIC = 0, > + LINBUS_ENHANCED, > +}; > + > +struct lin_frame { > + u8 lin_id; > + u8 len; > + u8 data[LIN_MAX_DLEN]; > + u8 checksum; > + enum lin_checksum_mode checksum_mode; > +}; > + > +struct lin_responder_answer { > + bool is_active; > + bool is_event_frame; > + u8 event_associated_id; > + struct lin_frame lf; > +}; > + > +struct lin_device_ops { > + int (*ldo_open)(struct lin_device *ldev); > + int (*ldo_stop)(struct lin_device *ldev); > + int (*ldo_tx)(struct lin_device *ldev, const struct lin_frame *frame); > + int (*update_bitrate)(struct lin_device *ldev, u16 bitrate); > + int (*update_responder_answer)(struct lin_device *ldev, > + const struct lin_responder_answer *answ); > + int (*get_responder_answer)(struct lin_device *ldev, u8 id, > + struct lin_responder_answer *answ); > +}; > + > +int lin_rx(struct lin_device *ldev, const struct lin_frame *lf); > + > +u8 lin_get_checksum(u8 pid, u8 n_of_bytes, const u8 *bytes, > + enum lin_checksum_mode cm); > + > +struct lin_device *register_lin(struct device *dev, > + const struct lin_device_ops *ldops); > +void unregister_lin(struct lin_device *ldev); > + > +#endif /* _NET_LIN_H_ */ > diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h > index 02ec32d694742..51b0e2a7624e4 100644 > --- a/include/uapi/linux/can/netlink.h > +++ b/include/uapi/linux/can/netlink.h > @@ -103,6 +103,7 @@ struct can_ctrlmode { > #define CAN_CTRLMODE_CC_LEN8_DLC 0x100 /* Classic CAN DLC option */ > #define CAN_CTRLMODE_TDC_AUTO 0x200 /* CAN transiver automatically calculates TDCV */ > #define CAN_CTRLMODE_TDC_MANUAL 0x400 /* TDCV is manually set up by user */ BIT(x) is these days available also for uapi I think. > +#define CAN_CTRLMODE_LIN 0x800 /* LIN bus mode */ > > /* > * CAN device statistics >
On Thu, 2 May 2024, 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> > --- > drivers/hid/Kconfig | 19 + > drivers/hid/Makefile | 1 + > drivers/hid/hid-hexdev-hexlin.c | 611 ++++++++++++++++++++++++++++++++ > drivers/hid/hid-ids.h | 1 + > drivers/hid/hid-quirks.c | 3 + > 5 files changed, 635 insertions(+) > create mode 100644 drivers/hid/hid-hexdev-hexlin.c > > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig > index 4c682c6507040..d2fb35d83c640 100644 > --- a/drivers/hid/Kconfig > +++ b/drivers/hid/Kconfig > @@ -496,6 +496,25 @@ config HID_GYRATION > help > Support for Gyration remote control. > > +config HID_MCS_HEXDEV > + tristate "hexDEV LIN-BUS adapter support" > + depends on HID && CAN_NETLINK && CAN_DEV > + select CAN_LIN > + help > + Support for hexDEV its hexLIN USB LIN bus adapter. > + > + Local Interconnect Network (LIN) to USB adapter for controller and > + responder usage. > + This device driver is using CAN_LIN for a userland connection on > + one side and USB HID for the actual hardware adapter on the other > + side. > + > + If you have such an adapter, say Y here and see > + <https://hexdev.de/hexlin>. > + > + To compile this driver as a module, choose M here: the > + module will be called hid-hexlin. > + > config HID_ICADE > tristate "ION iCade arcade controller" > help > diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile > index 082a728eac600..f9b13e6117e60 100644 > --- a/drivers/hid/Makefile > +++ b/drivers/hid/Makefile > @@ -59,6 +59,7 @@ obj-$(CONFIG_HID_GOOGLE_STADIA_FF) += hid-google-stadiaff.o > obj-$(CONFIG_HID_VIVALDI) += hid-vivaldi.o > obj-$(CONFIG_HID_GT683R) += hid-gt683r.o > obj-$(CONFIG_HID_GYRATION) += hid-gyration.o > +obj-$(CONFIG_HID_MCS_HEXDEV) += hid-hexdev-hexlin.o > obj-$(CONFIG_HID_HOLTEK) += hid-holtek-kbd.o > obj-$(CONFIG_HID_HOLTEK) += hid-holtek-mouse.o > obj-$(CONFIG_HID_HOLTEK) += hid-holtekff.o > diff --git a/drivers/hid/hid-hexdev-hexlin.c b/drivers/hid/hid-hexdev-hexlin.c > new file mode 100644 > index 0000000000000..1ddc1e00ab2da > --- /dev/null > +++ b/drivers/hid/hid-hexdev-hexlin.c > @@ -0,0 +1,611 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * LIN bus USB adapter driver https://hexdev.de/hexlin > + * > + * Copyright (C) 2024 hexDEV GmbH > + */ > + > +#include <linux/completion.h> > +#include <linux/hid.h> > +#include <linux/module.h> > +#include <linux/wait.h> > +#include <net/lin.h> > +#include "hid-ids.h" > + > +enum { > + /* answers */ > + HEXLIN_SUCCESS = 0x01, > + HEXLIN_FRAME = 0x02, > + HEXLIN_ERROR = 0x03, > + HEXLIN_FAIL = 0x0F, > + > + /* lin-responder */ > + HEXLIN_SET_MODE_RESPONDER = 0x10, > + HEXLIN_SET_RESPONDER_ANSWER_ID = 0x11, > + HEXLIN_GET_RESPONDER_ANSWER_ID = 0x12, > + > + /* lin-controller */ > + HEXLIN_SET_MODE_CONTROLLER = 0x20, > + HEXLIN_SEND_BREAK = 0x21, > + HEXLIN_SEND_UNCONDITIONAL_FRAME = 0x22, > + > + /* lin-div */ > + HEXLIN_SET_BAUDRATE = 0x34, > + HEXLIN_GET_BAUDRATE = 0x35, > + > + /* div */ > + HEXLIN_RESET = 0xF0, > + HEXLIN_GET_VERSION = 0xF1, Could you align the values? > +}; > + > +#define HEXLIN_SUCCESS_SZ 1 > +#define HEXLIN_FRAME_SZ 17 > +#define HEXLIN_FAIL_SZ 1 > +#define HEXLIN_GET_RESPONDER_ANSWER_ID_SZ 20 > +#define HEXLIN_GET_BAUDRATE_SZ 3 Is this sizeof(hexlin_baudrate_req)? If so, don't add define for it. This probably applies to other defines here too. > +#define HEXLIN_BAUDRATE_SZ 2 > +#define HEXLIN_GET_VERSION_SZ 2 > +#define HEXLIN_PKGLEN_MAX_SZ 64 > + > +struct hexlin_val8_req { > + u8 cmd; > + u8 v; > +} __packed; > + > +struct hexlin_baudrate_req { > + u8 cmd; > + u16 baudrate; > +} __packed; > + > +struct hexlin_frame { > + u32 flags; > + u8 len; > + u8 lin_id; > + u8 data[LIN_MAX_DLEN]; > + u8 checksum; > + u8 checksum_mode; > +} __packed; > + > +struct hexlin_unconditional_req { > + u8 cmd; > + struct hexlin_frame frm; > +} __packed; > + > +struct hexlin_responder_answer { > + u8 is_active; > + u8 is_event_frame; > + u8 event_associated_id; > + struct hexlin_frame frm; > +} __packed; > + > +struct hexlin_responder_answer_req { > + u8 cmd; > + struct hexlin_responder_answer answ; > +} __packed; > + > +struct hexlin_priv_data { > + struct hid_device *hid_dev; > + struct lin_device *ldev; > + u16 baudrate; > + struct completion wait_in_report; > + bool is_error; > + struct mutex tx_lock; /* protects hexlin_tx_report() */ > + struct hexlin_responder_answer_req rar; > + u8 fw_version; > +}; > + > +static int hexlin_tx_report(struct hexlin_priv_data *priv, > + const void *out_report, size_t len) > +{ > + u8 *buf; > + int ret; > + > + buf = kmemdup(out_report, len, GFP_KERNEL); > + if (!buf) > + return -ENOMEM; > + > + ret = hid_hw_output_report(priv->hid_dev, buf, len); > + kfree(buf); Is duplicatign the buffer necessary? > + if (ret < 0) > + return ret; > + if (ret != len) > + return -EIO; > + > + return 0; > +} > + > +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); > + > + 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)); HZ? > + if (!t) > + ret = -ETIMEDOUT; > + > + if (priv->is_error) > + ret = -EINVAL; > + > +tx_exit: > + mutex_unlock(&priv->tx_lock); > + > + return ret; > +} > + > +#define HEXLIN_GET_CMD(name, enum_cmd) \ > + static int hexlin_##name(struct hexlin_priv_data *priv) \ > + { \ > + u8 cmd = enum_cmd; \ > + int ret; \ > + \ > + ret = hexlin_tx_req_status(priv, &cmd, sizeof(u8)); \ Take sizeof() of the relevant variable instead, so: sizeof(cmd) > + if (ret) \ > + hid_err(priv->hid_dev, "%s failed with %d\n", \ > + __func__, ret); \ > + \ > + return ret; \ > + } > + > +HEXLIN_GET_CMD(get_version, HEXLIN_GET_VERSION) > +HEXLIN_GET_CMD(reset_dev, HEXLIN_RESET) > +HEXLIN_GET_CMD(get_baudrate, HEXLIN_GET_BAUDRATE) > + > +#define HEXLIN_VAL_CMD(name, enum_cmd, struct_type, vtype) \ > + static int hexlin_##name(struct hexlin_priv_data *p, vtype val) \ > + { \ > + struct struct_type req; \ > + int ret; \ > + \ > + req.cmd = enum_cmd; \ > + req.v = val; \ > + \ > + ret = hexlin_tx_req_status(p, &req, \ > + sizeof(struct struct_type)); \ sizeof(req) > + if (ret) \ > + hid_err(p->hid_dev, "%s failed with %d\n", \ > + __func__, ret); \ > + \ > + return ret; \ > + } > + > +HEXLIN_VAL_CMD(send_break, HEXLIN_SEND_BREAK, hexlin_val8_req, u8) > + > +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); Why you cannot just cast the pointer to correct type? > + le32_to_cpus(hxf.flags); You must use correct endianess typing. The struct hexlin_frame should have __le32 flags so sparse's endianness check is happy. But .flags are not used at all so why is this required in the first place? > + 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"); > + > + lin_rx(priv->ldev, &lf); > + > + return 0; > +} > + > +static int hexlin_send_unconditional(struct hexlin_priv_data *priv, > + const struct hexlin_frame *hxf) > +{ > + struct hexlin_unconditional_req req; > + int ret; > + > + if (hxf->lin_id > LIN_ID_MASK) > + return -EINVAL; > + > + req.cmd = HEXLIN_SEND_UNCONDITIONAL_FRAME; > + memcpy(&req.frm, hxf, sizeof(struct hexlin_frame)); > + > + ret = hexlin_tx_req_status(priv, &req, > + sizeof(struct hexlin_unconditional_req)); sizeof(req) > + > + if (ret) > + hid_err(priv->hid_dev, "%s failed with %d\n", __func__, ret); > + > + return ret; > +} > + > +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 struct should have __le16 baudrate. > + > + 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_get_responder_answer_id(struct hexlin_priv_data *priv, u8 id, > + struct hexlin_responder_answer_req *rar) > +{ > + u8 req[2] = { HEXLIN_GET_RESPONDER_ANSWER_ID, id }; > + int ret; > + > + if (id > LIN_ID_MASK) > + return -EINVAL; > + > + ret = hexlin_tx_req_status(priv, &req, sizeof(req)); > + if (ret) { > + hid_err(priv->hid_dev, "%s failed with %d\n", __func__, ret); Try to write error message that is meaningful to user, printing __func__ is not very helpful for user. > + return ret; > + } > + > + memcpy(rar, &priv->rar, sizeof(struct hexlin_responder_answer_req)); sizeof(*rar) > + return 0; > +} > + > +static int hexlin_set_responder_answer_id(struct hexlin_priv_data *priv, > + const struct lin_responder_answer *answ) > +{ > + struct hexlin_responder_answer_req rar; > + int ret; > + > + if (answ->lf.lin_id > LIN_ID_MASK || > + answ->event_associated_id > LIN_ID_MASK) > + return -EINVAL; > + > + rar.cmd = HEXLIN_SET_RESPONDER_ANSWER_ID; > + rar.answ.is_active = answ->is_active; > + rar.answ.is_event_frame = answ->is_event_frame; > + rar.answ.event_associated_id = answ->event_associated_id; > + rar.answ.frm.len = answ->lf.len; > + rar.answ.frm.lin_id = answ->lf.lin_id; > + memcpy(rar.answ.frm.data, answ->lf.data, LIN_MAX_DLEN); > + rar.answ.frm.checksum = answ->lf.checksum; > + rar.answ.frm.checksum_mode = answ->lf.checksum_mode; > + > + ret = hexlin_tx_req_status(priv, &rar, > + sizeof(struct hexlin_responder_answer_req)); Ditto.
On Thu, 2 May 2024, 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> > --- > drivers/net/can/Kconfig | 16 ++ > drivers/net/can/Makefile | 1 + > drivers/net/can/lin-serdev.c | 500 +++++++++++++++++++++++++++++++++++ > 3 files changed, 517 insertions(+) > create mode 100644 drivers/net/can/lin-serdev.c > > diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig > index 0934bbf8d03b2..91c6cdeefe440 100644 > --- a/drivers/net/can/Kconfig > +++ b/drivers/net/can/Kconfig > @@ -181,6 +181,22 @@ config CAN_LIN > > Actual device drivers need to be enabled too. > > +config CAN_LIN_SERDEV > + tristate "Serial LIN Adaptors" > + depends on CAN_LIN && SERIAL_DEV_BUS && OF > + help > + LIN-Bus support for serial devices equipped with LIN transceievers. > + This device driver is using CAN_LIN for a userland connection on > + one side and the kernel its serial device bus (serdev) interface > + on the other side. > + > + If you have a hexlin tty adapter, say Y here and see > + <https://hexdev.de/hexlin#tty>. > + > + This driver can also be built as a module. If so, the module will be > + called lin-serdev.ko. > + > + > config CAN_SLCAN > tristate "Serial / USB serial CAN Adaptors (slcan)" > depends on TTY > diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile > index 0093ee9219ca8..21ca514a42439 100644 > --- a/drivers/net/can/Makefile > +++ b/drivers/net/can/Makefile > @@ -26,6 +26,7 @@ obj-$(CONFIG_CAN_IFI_CANFD) += ifi_canfd/ > obj-$(CONFIG_CAN_JANZ_ICAN3) += janz-ican3.o > obj-$(CONFIG_CAN_KVASER_PCIEFD) += kvaser_pciefd.o > obj-$(CONFIG_CAN_LIN) += lin.o > +obj-$(CONFIG_CAN_LIN_SERDEV) += lin-serdev.o > obj-$(CONFIG_CAN_MSCAN) += mscan/ > obj-$(CONFIG_CAN_M_CAN) += m_can/ > obj-$(CONFIG_CAN_PEAK_PCIEFD) += peak_canfd/ > diff --git a/drivers/net/can/lin-serdev.c b/drivers/net/can/lin-serdev.c > new file mode 100644 > index 0000000000000..13fd9a54c6c93 > --- /dev/null > +++ b/drivers/net/can/lin-serdev.c > @@ -0,0 +1,500 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* Copyright (C) 2024 hexDEV GmbH - https://hexdev.de */ > + > +#include <linux/delay.h> > +#include <linux/init.h> > +#include <linux/kfifo.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/serdev.h> > +#include <linux/slab.h> > +#include <linux/tty.h> > +#include <net/lin.h> > + > +#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; > + unsigned long break_usleep_min; > + unsigned long break_usleep_max; > + unsigned long post_break_usleep_min; > + unsigned long post_break_usleep_max; > + unsigned long force_timeout_jfs; > + struct lin_responder_answer respond_answ[LIN_NUM_IDS]; > + struct mutex resp_lock; /* protects respond_answ */ > + bool is_stopped; > +}; > + > +static int linser_open(struct lin_device *ldev) > +{ > + struct serdev_device *serdev = to_serdev_device(ldev->dev); > + struct linser_priv *priv = serdev_device_get_drvdata(serdev); > + int ret; > + > + if (priv->is_stopped) { > + ret = serdev_device_open(serdev); > + if (ret) { > + dev_err(&serdev->dev, "Unable to open device\n"); > + return ret; > + } > + > + serdev_device_set_flow_control(serdev, false); > + serdev_device_set_break_detection(serdev, true); > + > + priv->is_stopped = false; > + } > + > + return 0; > +} > + > +static int linser_stop(struct lin_device *ldev) > +{ > + struct serdev_device *serdev = to_serdev_device(ldev->dev); > + struct linser_priv *priv = serdev_device_get_drvdata(serdev); > + > + if (priv->is_stopped) > + return 0; > + > + serdev_device_close(serdev); > + priv->is_stopped = true; > + > + return 0; > +} > + > +static int linser_send_break(struct linser_priv *priv) > +{ > + struct serdev_device *serdev = priv->serdev; > + int ret; > + > + ret = serdev_device_break_ctl(serdev, -1); > + if (ret) > + return ret; > + > + usleep_range(priv->break_usleep_min, priv->break_usleep_max); > + > + ret = serdev_device_break_ctl(serdev, 0); > + if (ret) > + return ret; > + > + usleep_range(priv->post_break_usleep_min, priv->post_break_usleep_max); > + > + return 0; > +} > + > +static int linser_ldo_tx(struct lin_device *ldev, const struct lin_frame *lf) > +{ > + struct serdev_device *serdev = to_serdev_device(ldev->dev); > + struct linser_priv *priv = serdev_device_get_drvdata(serdev); > + u8 pid = LIN_FORM_PID(lf->lin_id); > + u8 buf[LINSER_TX_BUFFER_SIZE]; > + ssize_t written_len, total_len; > + u8 checksum; > + int ret; > + > + if (lf->len + 3 > LINSER_TX_BUFFER_SIZE) { > + dev_err(&serdev->dev, "Frame length %u exceeds buffer size\n", lf->len); > + return -EINVAL; > + } > + > + buf[0] = LIN_SYNC_BYTE; > + buf[1] = pid; > + total_len = 2; > + > + if (lf->len) { > + memcpy(&buf[2], lf->data, lf->len); > + checksum = lin_get_checksum(pid, lf->len, lf->data, > + lf->checksum_mode); > + buf[lf->len + 2] = checksum; > + total_len += lf->len + 1; > + } > + > + ret = linser_send_break(priv); > + if (ret) > + return ret; > + > + written_len = serdev_device_write(serdev, buf, total_len, 0); > + if (written_len < total_len) > + return written_len < 0 ? (int)written_len : -EIO; > + > + dev_dbg(&serdev->dev, "sent out: %*ph\n", (int)total_len, buf); > + > + serdev_device_wait_until_sent(serdev, 0); > + > + return 0; > +} > + > +static void linser_derive_timings(struct linser_priv *priv, u16 bitrate) > +{ > + unsigned long break_baud = (bitrate * 2) / 3; > + unsigned long timeout_us; > + > + priv->break_usleep_min = (USEC_PER_SEC * LINSER_SAMPLES_PER_CHAR) / > + break_baud; > + priv->break_usleep_max = priv->break_usleep_min + 50; > + priv->post_break_usleep_min = (USEC_PER_SEC * 1 /* 1 bit */) / break_baud; Don't mix comments and code like this. > + priv->post_break_usleep_max = priv->post_break_usleep_min + 30; > + > + timeout_us = DIV_ROUND_CLOSEST(USEC_PER_SEC * 256 /* bit */, bitrate); Ditto. > + priv->force_timeout_jfs = usecs_to_jiffies(timeout_us); > +} > + > +static int linser_update_bitrate(struct lin_device *ldev, u16 bitrate) > +{ > + struct serdev_device *serdev = to_serdev_device(ldev->dev); > + struct linser_priv *priv = serdev_device_get_drvdata(serdev); > + unsigned int speed; > + int ret; > + > + ret = linser_open(ldev); > + if (ret) > + return ret; > + > + speed = serdev_device_set_baudrate(serdev, bitrate); > + if (!bitrate || speed != bitrate) > + return -EINVAL; > + > + linser_derive_timings(priv, bitrate); > + > + return 0; > +} > + > +static int linser_get_responder_answer(struct lin_device *ldev, u8 id, > + struct lin_responder_answer *answ) > +{ > + struct serdev_device *serdev = to_serdev_device(ldev->dev); > + struct linser_priv *priv = serdev_device_get_drvdata(serdev); > + struct lin_responder_answer *r = &priv->respond_answ[id]; > + > + if (!answ) > + return -EINVAL; > + > + mutex_lock(&priv->resp_lock); guard(mutex)(&priv->resp_lock); > + memcpy(answ, r, sizeof(struct lin_responder_answer)); sizeof(*answ); > + mutex_unlock(&priv->resp_lock); > + > + return 0; > +} > + > +static int linser_update_resp_answer(struct lin_device *ldev, > + const struct lin_responder_answer *answ) > +{ > + struct serdev_device *serdev = to_serdev_device(ldev->dev); > + struct linser_priv *priv = serdev_device_get_drvdata(serdev); > + struct lin_responder_answer *r = &priv->respond_answ[answ->lf.lin_id]; > + > + if (!answ) > + return -EINVAL; > + > + mutex_lock(&priv->resp_lock); > + memcpy(r, answ, sizeof(struct lin_responder_answer)); Ditto. > + r->lf.checksum = lin_get_checksum(LIN_FORM_PID(answ->lf.lin_id), > + answ->lf.len, > + answ->lf.data, > + answ->lf.checksum_mode); > + mutex_unlock(&priv->resp_lock); > + > + return 0; > +} > + > +static struct lin_device_ops linser_lindev_ops = { > + .ldo_open = linser_open, > + .ldo_stop = linser_stop, > + .ldo_tx = linser_ldo_tx, > + .update_bitrate = linser_update_bitrate, > + .get_responder_answer = linser_get_responder_answer, > + .update_responder_answer = linser_update_resp_answer, > +}; > + > +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); scoped_guard() can be used here. > + > + if (!answ->is_active) { > + mutex_unlock(&priv->resp_lock); > + return 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 { > + mutex_unlock(&priv->resp_lock); > + return false; Use reverse logic so you can reduce indent. > + } > + } 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; > +} > + > +static void linser_pop_fifo(struct linser_priv *priv, size_t n) > +{ > + for (size_t i = 0; i < n; i++) > + kfifo_skip(&priv->rx_fifo); > +} > + > +static int linser_fill_frame(struct linser_priv *priv, struct lin_frame *lf) > +{ > + struct serdev_device *serdev = priv->serdev; > + struct linser_rx buf[LINSER_PARSE_BUFFER]; > + unsigned int count, i, brk = 0; > + > + count = kfifo_out_peek(&priv->rx_fifo, buf, LINSER_PARSE_BUFFER); > + > + memset(lf, 0, sizeof(struct lin_frame)); > + > + for (i = 0; i < count; i++) { > + dev_dbg(&serdev->dev, "buf[%d]: data=%02x, flag=%02x\n", > + i, buf[i].data, buf[i].flag); > + } > + > + if (count < 3) > + return NEED_MORE; > + > + if (buf[0].flag != TTY_BREAK || buf[1].data != LIN_SYNC_BYTE) { > + linser_pop_fifo(priv, 1); /* pop incorrect start */ > + return NEED_MORE; > + } else if (!LIN_CHECK_PID(buf[2].data)) { > + linser_pop_fifo(priv, 3); /* pop incorrect header */ > + return NEED_MORE; > + } > + > + lf->lin_id = LIN_GET_ID(buf[2].data); > + > + /* from here on we do have a correct LIN header */ > + > + if (count == 3) > + return linser_tx_frame_as_responder(priv, lf->lin_id) ? > + NEED_MORE : NEED_FORCE; > + > + for (i = 3; i < count && i < LINSER_PARSE_BUFFER && i < 12; i++) { > + if (buf[i].flag == TTY_BREAK) { > + brk = i; > + break; > + } > + lf->len++; > + } > + if (lf->len) > + lf->len -= 1; /* account for checksum */ > + > + if (brk == 3) > + return MODE_OK; > + > + if (brk == 4) { > + /* suppress wrong answer data-byte in between PID and break > + * because checksum is missing > + */ > + return MODE_OK; > + } > + > + for (i = 0; i < lf->len; i++) > + lf->data[i] = buf[3 + i].data; > + lf->checksum = buf[2 + lf->len + 1].data; > + mutex_lock(&priv->resp_lock); > + lf->checksum_mode = priv->respond_answ[lf->lin_id].lf.checksum_mode; > + mutex_unlock(&priv->resp_lock); > + > + dev_dbg(&serdev->dev, "brk:%i, len:%u, data:%*ph, checksum:%x (%s)\n", > + brk, lf->len, lf->len, lf->data, lf->checksum, > + lf->checksum_mode ? "enhanced" : "classic"); > + > + if (brk > 4) > + return MODE_OK; /* frame in between two breaks: so complete */ > + > + if (lf->len == 8) > + return MODE_OK; > + > + return NEED_FORCE; > +} > + > +static int linser_process_frame(struct linser_priv *priv, bool force) > +{ > + struct serdev_device *serdev = priv->serdev; > + struct lin_frame lf; > + size_t bytes_to_pop; > + int ret = NEED_MORE; > + > + while (kfifo_len(&priv->rx_fifo) >= LIN_HEADER_SIZE) { > + ret = linser_fill_frame(priv, &lf); > + > + if (ret == MODE_OK || (ret == NEED_FORCE && force)) { > + dev_dbg(&serdev->dev, "lin_rx: %s\n", > + force ? "force" : "normal"); > + lin_rx(priv->lin_dev, &lf); > + bytes_to_pop = LIN_HEADER_SIZE + lf.len + > + (lf.len ? 1 : 0); > + linser_pop_fifo(priv, bytes_to_pop); > + force = false; > + ret = MODE_OK; > + } else { > + return ret; Reverse logic & deindent the main block. > + } > + } > + > + return ret; > +} > + > +static void linser_process_delayed(struct work_struct *work) > +{ > + struct linser_priv *priv = container_of(work, struct linser_priv, > + rx_work.work); > + > + linser_process_frame(priv, true); > +} > + > +static ssize_t linser_receive_buf_fp(struct serdev_device *serdev, > + const u8 *data, const u8 *fp, > + size_t count) > +{ > + struct linser_priv *priv = serdev_device_get_drvdata(serdev); > + enum linser_rx_status rx_status; > + ssize_t n = 0; > + int i; > + > + cancel_delayed_work_sync(&priv->rx_work); > + > + for (i = 0; i < count; i++) { > + struct linser_rx rx; > + > + rx.data = data[i]; > + rx.flag = (fp ? fp[i] : 0); > + n += kfifo_in(&priv->rx_fifo, &rx, 1); > + dev_dbg(&serdev->dev, "%s: n:%zd, flag:0x%02x, data:0x%02x\n", > + __func__, n, rx.flag, data[i]); > + } > + > + rx_status = linser_process_frame(priv, false); > + > + if (rx_status == NEED_FORCE) > + schedule_delayed_work(&priv->rx_work, priv->force_timeout_jfs); > + > + return n; > +} > + > +static const struct serdev_device_ops linser_ops = { > + .receive_buf_fp = linser_receive_buf_fp, > + .write_wakeup = serdev_device_write_wakeup, > +}; > + > +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); > + 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; > +} > + > +static void linser_remove(struct serdev_device *serdev) > +{ > + struct linser_priv *priv = serdev_device_get_drvdata(serdev); > + > + unregister_lin(priv->lin_dev); > +} > + > +static const struct of_device_id linser_of_match[] = { > + { > + .compatible = "hexdev,lin-serdev", > + }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, linser_of_match); > + > +static struct serdev_device_driver linser_driver = { > + .probe = linser_probe, > + .remove = linser_remove, > + .driver = { > + .name = KBUILD_MODNAME, > + .of_match_table = linser_of_match, > + } > +}; > + > +module_serdev_device_driver(linser_driver); > + > +MODULE_LICENSE("GPL"); > +MODULE_AUTHOR("Christoph Fritz <christoph.fritz@hexdev.de>"); > +MODULE_DESCRIPTION("LIN-Bus serdev driver"); >
On Thu, 2 May 2024, Christoph Fritz wrote: > Enhance CAN broadcast manager with RX_LIN_SETUP and RX_LIN_DELETE > operations to setup automatic LIN frame responses in responder mode. > > Additionally, the patch introduces the LIN_EVENT_FRAME flag to > setup event-triggered LIN frames. > > Signed-off-by: Christoph Fritz <christoph.fritz@hexdev.de> > --- > include/uapi/linux/can/bcm.h | 5 ++- > net/can/bcm.c | 74 +++++++++++++++++++++++++++++++++++- > 2 files changed, 77 insertions(+), 2 deletions(-) > > diff --git a/include/uapi/linux/can/bcm.h b/include/uapi/linux/can/bcm.h > index f1e45f533a72c..c46268a114078 100644 > --- a/include/uapi/linux/can/bcm.h > +++ b/include/uapi/linux/can/bcm.h > @@ -86,7 +86,9 @@ enum { > TX_EXPIRED, /* notification on performed transmissions (count=0) */ > RX_STATUS, /* reply to RX_READ request */ > RX_TIMEOUT, /* cyclic message is absent */ > - RX_CHANGED /* updated CAN frame (detected content change) */ > + RX_CHANGED, /* updated CAN frame (detected content change) */ > + RX_LIN_SETUP, /* create auto-response for LIN frame */ > + RX_LIN_DELETE, /* remove auto-response for LIN frame */ > }; > > #define SETTIMER 0x0001 > @@ -101,5 +103,6 @@ enum { > #define TX_RESET_MULTI_IDX 0x0200 > #define RX_RTR_FRAME 0x0400 > #define CAN_FD_FRAME 0x0800 > +#define LIN_EVENT_FRAME 0x1000 > > #endif /* !_UAPI_CAN_BCM_H */ > diff --git a/net/can/bcm.c b/net/can/bcm.c > index 27d5fcf0eac9d..a717e594234d1 100644 > --- a/net/can/bcm.c > +++ b/net/can/bcm.c > @@ -59,6 +59,7 @@ > #include <linux/can/bcm.h> > #include <linux/slab.h> > #include <net/sock.h> > +#include <net/lin.h> > #include <net/net_namespace.h> > > /* > @@ -1330,6 +1331,59 @@ static int bcm_tx_send(struct msghdr *msg, int ifindex, struct sock *sk, > return cfsiz + MHSIZ; > } > > +static int bcm_lin_setup(struct bcm_msg_head *msg_head, struct msghdr *msg, > + int ifindex, struct sock *sk, int cfsiz, int is_active) > +{ > + struct lin_responder_answer answ; > + struct net_device *dev; > + struct sk_buff *skb; > + struct canfd_frame cf; > + netdevice_tracker tracker; > + size_t sz; > + int ret; > + > + if (msg_head->nframes > 1) > + return -EINVAL; > + > + if (!(msg_head->flags & CAN_FD_FRAME)) > + return -EINVAL; > + > + ret = memcpy_from_msg(&cf, msg, cfsiz); > + if (ret < 0) > + return ret; > + > + answ.lf.lin_id = cf.can_id & LIN_ID_MASK; > + answ.is_active = is_active; > + answ.is_event_frame = !!(msg_head->flags & LIN_EVENT_FRAME); > + answ.event_associated_id = msg_head->can_id; > + answ.lf.len = min(cf.len, LIN_MAX_DLEN); > + memcpy(answ.lf.data, cf.data, answ.lf.len); > + sz = min(sizeof(struct lin_responder_answer), sizeof(cf.data)); > + cf.can_id |= LIN_RXOFFLOAD_DATA_FLAG; > + memcpy(cf.data, &answ, sz); > + > + dev = netdev_get_by_index(sock_net(sk), ifindex, &tracker, GFP_KERNEL); > + if (!dev) > + return -ENODEV; > + > + skb = alloc_skb(cfsiz + sizeof(struct can_skb_priv), gfp_any()); You just called the other function with GFP_KERNEL and you now need gfp_any(). Which is correct?? > + if (!skb) > + goto lin_out; > + > + can_skb_reserve(skb); > + can_skb_prv(skb)->ifindex = dev->ifindex; > + can_skb_prv(skb)->skbcnt = 0; > + skb_put_data(skb, &cf, cfsiz); > + > + skb->dev = dev; > + can_skb_set_owner(skb, sk); > + ret = can_send(skb, 1); /* send with loopback */ > + > +lin_out: > + netdev_put(dev, &tracker); > + return ret; > +} > + > /* > * bcm_sendmsg - process BCM commands (opcodes) from the userspace > */ > @@ -1429,12 +1483,30 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) > > case TX_SEND: > /* we need exactly one CAN frame behind the msg head */ > - if ((msg_head.nframes != 1) || (size != cfsiz + MHSIZ)) > + if (msg_head.nframes != 1 || size != cfsiz + MHSIZ) Unrelated style fix, doesn't belong to this patch. > ret = -EINVAL; > else > ret = bcm_tx_send(msg, ifindex, sk, cfsiz); > break; > > + case RX_LIN_SETUP: > + /* we need exactly one CAN frame behind the msg head */ > + if (msg_head.nframes != 1 || size != cfsiz + MHSIZ) > + ret = -EINVAL; > + else > + ret = bcm_lin_setup(&msg_head, msg, ifindex, sk, cfsiz, > + 1); > + break; > + > + case RX_LIN_DELETE: > + /* we need exactly one CAN frame behind the msg head */ > + if (msg_head.nframes != 1 || size != cfsiz + MHSIZ) > + ret = -EINVAL; > + else > + ret = bcm_lin_setup(&msg_head, msg, ifindex, sk, cfsiz, > + 0); > + break; > + > default: > ret = -EINVAL; > break; >
On Thu, 2 May 2024, Christoph Fritz wrote: > The CAN Broadcast Manager now has the capability to dispatch CANFD > frames marked with the id LINBUS_RXOFFLOAD_ID. This patch introduces > functionality to interpret these specific frames, enabling the > configuration of RX offloading within the LIN driver. > > Signed-off-by: Christoph Fritz <christoph.fritz@hexdev.de> > --- > drivers/net/can/lin.c | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/drivers/net/can/lin.c b/drivers/net/can/lin.c > index 95906003666fb..ee2ebea2c865f 100644 > --- a/drivers/net/can/lin.c > +++ b/drivers/net/can/lin.c > @@ -194,6 +194,27 @@ static void lin_remove_sysfs_id_files(struct net_device *ndev) > } > } > > +static int lin_setup_rxoffload(struct lin_device *ldev, > + struct canfd_frame *cfd) > +{ > + struct lin_responder_answer answ; > + > + if (!(cfd->flags & CANFD_FDF)) > + return -EMSGSIZE; This seems a bit odd error code. > + BUILD_BUG_ON(sizeof(struct lin_responder_answer) > sizeof(cfd->data)); > + memcpy(&answ, cfd->data, sizeof(struct lin_responder_answer)); 2x sizeof(answ) > + > + answ.lf.checksum_mode = (cfd->can_id & LIN_ENHANCED_CKSUM_FLAG) ? > + LINBUS_ENHANCED : LINBUS_CLASSIC; > + > + if (answ.lf.lin_id > LIN_ID_MASK || > + answ.event_associated_id > LIN_ID_MASK) > + return -EINVAL; > + > + return ldev->ldev_ops->update_responder_answer(ldev, &answ); > +} > + > static void lin_tx_work_handler(struct work_struct *ws) > { > struct lin_device *ldev = container_of(ws, struct lin_device, > @@ -206,6 +227,14 @@ static void lin_tx_work_handler(struct work_struct *ws) > ldev->tx_busy = true; > > cfd = (struct canfd_frame *)ldev->tx_skb->data; > + > + if (cfd->can_id & LIN_RXOFFLOAD_DATA_FLAG) { > + ret = lin_setup_rxoffload(ldev, cfd); > + if (ret < 0) > + netdev_err(ndev, "setting up rx failed %d\n", ret); > + goto lin_tx_out; > + } > + > lf.checksum_mode = (cfd->can_id & LIN_ENHANCED_CKSUM_FLAG) ? > LINBUS_ENHANCED : LINBUS_CLASSIC; > lf.lin_id = cfd->can_id & LIN_ID_MASK; >
On Sat, 2024-05-04 at 18:00 +0200, Greg Kroah-Hartman wrote: > On Thu, May 02, 2024 at 08:27:56PM +0200, Christoph Fritz wrote: > > This patch introduces an additional receive buffer callback variation > > besides the already existing receive_buf(). This new callback function > > also passes the flag buffer (TTY_NORMAL, TTY_BREAK, and friends). > > > > If defined, this function gets prioritized and called instead of the > > standard receive_buf(). > > > > An alternative approach could have been to enhance the receive_buf() > > function and update all drivers that use it. > > Please, let's do that instead of adding random letters at the end of a > function pointer :) :) sure > > > Signed-off-by: Christoph Fritz <christoph.fritz@hexdev.de> > > --- > > drivers/tty/serdev/serdev-ttyport.c | 2 +- > > include/linux/serdev.h | 17 ++++++++++++++--- > > 2 files changed, 15 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c > > index 3d7ae7fa50186..bb47691afdb21 100644 > > --- a/drivers/tty/serdev/serdev-ttyport.c > > +++ b/drivers/tty/serdev/serdev-ttyport.c > > @@ -32,7 +32,7 @@ static size_t ttyport_receive_buf(struct tty_port *port, const u8 *cp, > > if (!test_bit(SERPORT_ACTIVE, &serport->flags)) > > return 0; > > > > - ret = serdev_controller_receive_buf(ctrl, cp, count); > > + ret = serdev_controller_receive_buf(ctrl, cp, fp, count); > > > > dev_WARN_ONCE(&ctrl->dev, ret > count, > > "receive_buf returns %zu (count = %zu)\n", > > diff --git a/include/linux/serdev.h b/include/linux/serdev.h > > index ff78efc1f60df..c6ef5a8988e07 100644 > > --- a/include/linux/serdev.h > > +++ b/include/linux/serdev.h > > @@ -23,11 +23,17 @@ struct serdev_device; > > * struct serdev_device_ops - Callback operations for a serdev device > > * @receive_buf: Function called with data received from device; > > * returns number of bytes accepted; may sleep. > > + * @receive_buf_fp: Function called with data and flag buffer received > > + * from device; If defined, this function gets called > > + * instead of @receive_buf; > > + * returns number of bytes accepted; may sleep. > > I don't remember waht "fp" means here, and you don't document it, so > let's just have one recieve_buf() callback please. OK, that is a great opportunity for me to use Coccinelle. In the upcoming revision v4 I'll add the "flag buffer pointer" treewide, then named "flags". thanks -- Christoph
On Mon, 2024-05-06 at 19:24 +0300, Ilpo Järvinen wrote: > On Thu, 2 May 2024, 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. ... > > +static ssize_t lin_identifier_show(struct kobject *kobj, > > + struct kobj_attribute *attr, char *buf) > > +{ > > + struct lin_attr *lin_attr = container_of(attr, struct lin_attr, attr); > > + struct lin_device *ldev = lin_attr->ldev; > > + ssize_t count = 0; > > + struct lin_responder_answer answ; > > + int k, ret; > > + long id; > > + > > + if (!ldev->ldev_ops->get_responder_answer) > > + return -EOPNOTSUPP; > > + > > + ret = kstrtol(attr->attr.name, 16, &id); > > + if (ret) > > + return ret; > > + if (id < 0 || id >= LIN_NUM_IDS) > > + return -EINVAL; > > + > > + count += scnprintf(buf + count, PAGE_SIZE - count, > > + "%-6s %-11s %-9s %-9s %-2s %-24s %-6s\n", > > + "state", "cksum-mode", "is_event", "event_id", > > + "n", "data", "cksum"); > > Onl use sysfs_emit() and sysfs_emit_at() in *_show functions. OK > > + > > + ret = ldev->ldev_ops->get_responder_answer(ldev, id, &answ); > > + if (ret) > > + return ret; > > + > > + count += scnprintf(buf + count, PAGE_SIZE - count, > > + "%-6s %-11s %-9s %-9u %-2u ", > > + answ.is_active ? "active" : "off", > > + answ.lf.checksum_mode ? "enhanced" : "classic", > > + answ.is_event_frame ? "yes" : "no", > > + answ.event_associated_id, > > + answ.lf.len); > > + > > + for (k = 0; k < answ.lf.len; k++) > > + count += scnprintf(buf + count, PAGE_SIZE - count, > > + "%02x ", answ.lf.data[k]); > > + for (; k < 8; k++) > > + count += scnprintf(buf + count, PAGE_SIZE - count, > > + " "); > > + if (answ.lf.len) > > + count += scnprintf(buf + count, PAGE_SIZE - count, > > + " %02x", answ.lf.checksum); > > + > > + count += scnprintf(buf + count, PAGE_SIZE - count, "\n"); > > + > > + return count; > > +} ... > > + > > +static int lin_create_sysfs_id_files(struct net_device *ndev) > > +{ > > + struct lin_device *ldev = netdev_priv(ndev); > > + struct kobj_attribute *attr; > > + int ret; > > + > > + for (int id = 0; id < LIN_NUM_IDS; id++) { > > + ldev->sysfs_entries[id].ldev = ldev; > > + attr = &ldev->sysfs_entries[id].attr; > > + attr->attr.name = kasprintf(GFP_KERNEL, "%02x", id); > > + if (!attr->attr.name) > > + return -ENOMEM; > > + attr->attr.mode = 0644; > > + attr->show = lin_identifier_show; > > + attr->store = lin_identifier_store; > > + > > + sysfs_attr_init(&attr->attr); > > + ret = sysfs_create_file(ldev->lin_ids_kobj, &attr->attr); > > + if (ret) { > > + kfree(attr->attr.name); > > + return -ENOMEM; > > + } > > + } > > + > > + return 0; > > +} > > Can you use .dev_groups instead ? I'm not sure where to attach this in this glue code here. Should I do a class_register() and add the .dev_groups there? > FWIW, this function doesn't do rollback when error occurs. OK, this issue can be fixed in revision v4. ... > > diff --git a/include/net/lin.h b/include/net/lin.h > > new file mode 100644 > > index 0000000000000..e7c7c820a6e18 > > --- /dev/null > > +++ b/include/net/lin.h > > @@ -0,0 +1,92 @@ > > +/* SPDX-License-Identifier: GPL-2.0+ */ > > +/* Copyright (C) 2024 hexDEV GmbH - https://hexdev.de */ > > + > > +#ifndef _NET_LIN_H_ > > +#define _NET_LIN_H_ > > + > > +#include <linux/bitfield.h> > > +#include <linux/can/dev.h> > > +#include <linux/device.h> > > + > > +#define LIN_NUM_IDS 64 > > +#define LIN_HEADER_SIZE 3 > > +#define LIN_MAX_DLEN 8 > > + > > +#define LIN_MAX_BAUDRATE 20000 > > +#define LIN_MIN_BAUDRATE 1000 > > +#define LIN_DEFAULT_BAUDRATE 9600 > > +#define LIN_SYNC_BYTE 0x55 > > + > > +#define LIN_ID_MASK GENMASK(5, 0) > > +/* special ID descriptions for LIN */ > > +#define LIN_RXOFFLOAD_DATA_FLAG 0x00000200U > > +#define LIN_ENHANCED_CKSUM_FLAG 0x00000100U > > BIT(x) x 2 OK > > > + > > +extern u8 lin_get_id_parity(u8 id); > > + > > +#define LIN_GET_ID(PID) FIELD_GET(LIN_ID_MASK, PID) > > +#define LIN_FORM_PID(ID) (LIN_GET_ID(ID) | \ > > + lin_get_id_parity(LIN_GET_ID(ID))) > > +#define LIN_GET_PARITY(PID) ((PID) & ~LIN_ID_MASK) > > +#define LIN_CHECK_PID(PID) (LIN_GET_PARITY(PID) == \ > > + LIN_GET_PARITY(LIN_FORM_PID(PID))) > > + > > +struct lin_attr { > > + struct kobj_attribute attr; > > + struct lin_device *ldev; > > +}; > > + > > +struct lin_device { > > + struct can_priv can; /* must be the first member */ > > + struct net_device *ndev; > > + struct device *dev; > > + const struct lin_device_ops *ldev_ops; > > + struct workqueue_struct *wq; > > + struct work_struct tx_work; > > + bool tx_busy; > > + struct sk_buff *tx_skb; > > + struct kobject *lin_ids_kobj; > > + struct lin_attr sysfs_entries[LIN_NUM_IDS]; > > +}; > > + > > +enum lin_checksum_mode { > > + LINBUS_CLASSIC = 0, > > + LINBUS_ENHANCED, > > +}; > > + > > +struct lin_frame { > > + u8 lin_id; > > + u8 len; > > + u8 data[LIN_MAX_DLEN]; > > + u8 checksum; > > + enum lin_checksum_mode checksum_mode; > > +}; > > + > > +struct lin_responder_answer { > > + bool is_active; > > + bool is_event_frame; > > + u8 event_associated_id; > > + struct lin_frame lf; > > +}; > > + > > +struct lin_device_ops { > > + int (*ldo_open)(struct lin_device *ldev); > > + int (*ldo_stop)(struct lin_device *ldev); > > + int (*ldo_tx)(struct lin_device *ldev, const struct lin_frame *frame); > > + int (*update_bitrate)(struct lin_device *ldev, u16 bitrate); > > + int (*update_responder_answer)(struct lin_device *ldev, > > + const struct lin_responder_answer *answ); > > + int (*get_responder_answer)(struct lin_device *ldev, u8 id, > > + struct lin_responder_answer *answ); > > +}; > > + > > +int lin_rx(struct lin_device *ldev, const struct lin_frame *lf); > > + > > +u8 lin_get_checksum(u8 pid, u8 n_of_bytes, const u8 *bytes, > > + enum lin_checksum_mode cm); > > + > > +struct lin_device *register_lin(struct device *dev, > > + const struct lin_device_ops *ldops); > > +void unregister_lin(struct lin_device *ldev); > > + > > +#endif /* _NET_LIN_H_ */ > > diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h > > index 02ec32d694742..51b0e2a7624e4 100644 > > --- a/include/uapi/linux/can/netlink.h > > +++ b/include/uapi/linux/can/netlink.h > > @@ -103,6 +103,7 @@ struct can_ctrlmode { > > #define CAN_CTRLMODE_CC_LEN8_DLC 0x100 /* Classic CAN DLC option */ > > #define CAN_CTRLMODE_TDC_AUTO 0x200 /* CAN transiver automatically calculates TDCV */ > > #define CAN_CTRLMODE_TDC_MANUAL 0x400 /* TDCV is manually set up by user */ > > BIT(x) is these days available also for uapi I think. > > > +#define CAN_CTRLMODE_LIN 0x800 /* LIN bus mode */ So, should I use just BIT(11) for the new define, or should I also refactor the whole list while at it? Thanks -- Christoph
On Wed, 8 May 2024, Christoph Fritz wrote: > On Mon, 2024-05-06 at 19:24 +0300, Ilpo Järvinen wrote: > > On Thu, 2 May 2024, 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. > > > +static int lin_create_sysfs_id_files(struct net_device *ndev) > > > +{ > > > + struct lin_device *ldev = netdev_priv(ndev); > > > + struct kobj_attribute *attr; > > > + int ret; > > > + > > > + for (int id = 0; id < LIN_NUM_IDS; id++) { > > > + ldev->sysfs_entries[id].ldev = ldev; > > > + attr = &ldev->sysfs_entries[id].attr; > > > + attr->attr.name = kasprintf(GFP_KERNEL, "%02x", id); > > > + if (!attr->attr.name) > > > + return -ENOMEM; > > > + attr->attr.mode = 0644; > > > + attr->show = lin_identifier_show; > > > + attr->store = lin_identifier_store; > > > + > > > + sysfs_attr_init(&attr->attr); > > > + ret = sysfs_create_file(ldev->lin_ids_kobj, &attr->attr); > > > + if (ret) { > > > + kfree(attr->attr.name); > > > + return -ENOMEM; > > > + } > > > + } > > > + > > > + return 0; > > > +} > > > > Can you use .dev_groups instead ? > > I'm not sure where to attach this in this glue code here. Should I do a > class_register() and add the .dev_groups there? I guess struct class would be correct direction but I'm not sure if it's viable in this case. It would avoid the need for custom sysfs setup code if it's workable. > > FWIW, this function doesn't do rollback when error occurs. > > OK, this issue can be fixed in revision v4. > > ... > > > diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h > > > index 02ec32d694742..51b0e2a7624e4 100644 > > > --- a/include/uapi/linux/can/netlink.h > > > +++ b/include/uapi/linux/can/netlink.h > > > @@ -103,6 +103,7 @@ struct can_ctrlmode { > > > #define CAN_CTRLMODE_CC_LEN8_DLC 0x100 /* Classic CAN DLC option */ > > > #define CAN_CTRLMODE_TDC_AUTO 0x200 /* CAN transiver automatically calculates TDCV */ > > > #define CAN_CTRLMODE_TDC_MANUAL 0x400 /* TDCV is manually set up by user */ > > > > BIT(x) is these days available also for uapi I think. > > > > > +#define CAN_CTRLMODE_LIN 0x800 /* LIN bus mode */ > > So, should I use just BIT(11) for the new define, or should I also > refactor the whole list while at it? Either is fine for me.
... > ... > > > > +static int lin_create_sysfs_id_files(struct net_device *ndev) > > > > +{ > > > > + struct lin_device *ldev = netdev_priv(ndev); > > > > + struct kobj_attribute *attr; > > > > + int ret; > > > > + > > > > + for (int id = 0; id < LIN_NUM_IDS; id++) { > > > > + ldev->sysfs_entries[id].ldev = ldev; > > > > + attr = &ldev->sysfs_entries[id].attr; > > > > + attr->attr.name = kasprintf(GFP_KERNEL, "%02x", id); > > > > + if (!attr->attr.name) > > > > + return -ENOMEM; > > > > + attr->attr.mode = 0644; > > > > + attr->show = lin_identifier_show; > > > > + attr->store = lin_identifier_store; > > > > + > > > > + sysfs_attr_init(&attr->attr); > > > > + ret = sysfs_create_file(ldev->lin_ids_kobj, &attr->attr); > > > > + if (ret) { > > > > + kfree(attr->attr.name); > > > > + return -ENOMEM; > > > > + } > > > > + } > > > > + > > > > + return 0; > > > > +} > > > > > > Can you use .dev_groups instead ? > > > > I'm not sure where to attach this in this glue code here. Should I do a > > class_register() and add the .dev_groups there? > > I guess struct class would be correct direction but I'm not sure if it's > viable in this case. It would avoid the need for custom sysfs setup code > if it's workable. I just tried to find a way, but these are 64 sysfs files and declaring them all static looks a bit odd to me. I might be missing something here. For v4 I would stick to the dynamic setup and fix the rollback. Any objections? Thanks -- Christoph
On Wed, May 08, 2024 at 08:20:51PM +0200, Christoph Fritz wrote: > ... > > ... > > > > > +static int lin_create_sysfs_id_files(struct net_device *ndev) > > > > > +{ > > > > > + struct lin_device *ldev = netdev_priv(ndev); > > > > > + struct kobj_attribute *attr; > > > > > + int ret; > > > > > + > > > > > + for (int id = 0; id < LIN_NUM_IDS; id++) { > > > > > + ldev->sysfs_entries[id].ldev = ldev; > > > > > + attr = &ldev->sysfs_entries[id].attr; > > > > > + attr->attr.name = kasprintf(GFP_KERNEL, "%02x", id); > > > > > + if (!attr->attr.name) > > > > > + return -ENOMEM; > > > > > + attr->attr.mode = 0644; > > > > > + attr->show = lin_identifier_show; > > > > > + attr->store = lin_identifier_store; > > > > > + > > > > > + sysfs_attr_init(&attr->attr); > > > > > + ret = sysfs_create_file(ldev->lin_ids_kobj, &attr->attr); > > > > > + if (ret) { > > > > > + kfree(attr->attr.name); > > > > > + return -ENOMEM; > > > > > + } > > > > > + } > > > > > + > > > > > + return 0; > > > > > +} > > > > > > > > Can you use .dev_groups instead ? > > > > > > I'm not sure where to attach this in this glue code here. Should I do a > > > class_register() and add the .dev_groups there? > > > > I guess struct class would be correct direction but I'm not sure if it's > > viable in this case. It would avoid the need for custom sysfs setup code > > if it's workable. > > I just tried to find a way, but these are 64 sysfs files and declaring > them all static looks a bit odd to me. I might be missing something > here. > > For v4 I would stick to the dynamic setup and fix the rollback. > > Any objections? Yes, you race with userspace and loose by trying to do this "by hand". Make this static please. thanks, greg k-h
On Wed, 2024-05-08 at 19:48 +0100, Greg Kroah-Hartman wrote: > On Wed, May 08, 2024 at 08:20:51PM +0200, Christoph Fritz wrote: > > ... > > > ... > > > > > > +static int lin_create_sysfs_id_files(struct net_device *ndev) > > > > > > +{ > > > > > > + struct lin_device *ldev = netdev_priv(ndev); > > > > > > + struct kobj_attribute *attr; > > > > > > + int ret; > > > > > > + > > > > > > + for (int id = 0; id < LIN_NUM_IDS; id++) { > > > > > > + ldev->sysfs_entries[id].ldev = ldev; > > > > > > + attr = &ldev->sysfs_entries[id].attr; > > > > > > + attr->attr.name = kasprintf(GFP_KERNEL, "%02x", id); > > > > > > + if (!attr->attr.name) > > > > > > + return -ENOMEM; > > > > > > + attr->attr.mode = 0644; > > > > > > + attr->show = lin_identifier_show; > > > > > > + attr->store = lin_identifier_store; > > > > > > + > > > > > > + sysfs_attr_init(&attr->attr); > > > > > > + ret = sysfs_create_file(ldev->lin_ids_kobj, &attr->attr); > > > > > > + if (ret) { > > > > > > + kfree(attr->attr.name); > > > > > > + return -ENOMEM; > > > > > > + } > > > > > > + } > > > > > > + > > > > > > + return 0; > > > > > > +} > > > > > > > > > > Can you use .dev_groups instead ? > > > > > > > > I'm not sure where to attach this in this glue code here. Should I do a > > > > class_register() and add the .dev_groups there? > > > > > > I guess struct class would be correct direction but I'm not sure if it's > > > viable in this case. It would avoid the need for custom sysfs setup code > > > if it's workable. > > > > I just tried to find a way, but these are 64 sysfs files and declaring > > them all static looks a bit odd to me. I might be missing something > > here. > > > > For v4 I would stick to the dynamic setup and fix the rollback. > > > > Any objections? > > Yes, you race with userspace and loose by trying to do this "by hand". > Make this static please. OK, static init coming up in v4 thanks -- Christoph
On Mon, 2024-05-06 at 19:53 +0300, Ilpo Järvinen wrote: > On Thu, 2 May 2024, 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> > > --- > > drivers/hid/Kconfig | 19 + > > drivers/hid/Makefile | 1 + > > drivers/hid/hid-hexdev-hexlin.c | 611 ++++++++++++++++++++++++++++++++ > > drivers/hid/hid-ids.h | 1 + > > drivers/hid/hid-quirks.c | 3 + > > 5 files changed, 635 insertions(+) > > create mode 100644 drivers/hid/hid-hexdev-hexlin.c > > > > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig > > index 4c682c6507040..d2fb35d83c640 100644 > > --- a/drivers/hid/Kconfig > > +++ b/drivers/hid/Kconfig > > @@ -496,6 +496,25 @@ config HID_GYRATION > > help > > Support for Gyration remote control. > > > > +config HID_MCS_HEXDEV > > + tristate "hexDEV LIN-BUS adapter support" > > + depends on HID && CAN_NETLINK && CAN_DEV > > + select CAN_LIN > > + help > > + Support for hexDEV its hexLIN USB LIN bus adapter. > > + > > + Local Interconnect Network (LIN) to USB adapter for controller and > > + responder usage. > > + This device driver is using CAN_LIN for a userland connection on > > + one side and USB HID for the actual hardware adapter on the other > > + side. > > + > > + If you have such an adapter, say Y here and see > > + <https://hexdev.de/hexlin>. > > + > > + To compile this driver as a module, choose M here: the > > + module will be called hid-hexlin. > > + > > config HID_ICADE > > tristate "ION iCade arcade controller" > > help > > diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile > > index 082a728eac600..f9b13e6117e60 100644 > > --- a/drivers/hid/Makefile > > +++ b/drivers/hid/Makefile > > @@ -59,6 +59,7 @@ obj-$(CONFIG_HID_GOOGLE_STADIA_FF) += hid-google-stadiaff.o > > obj-$(CONFIG_HID_VIVALDI) += hid-vivaldi.o > > obj-$(CONFIG_HID_GT683R) += hid-gt683r.o > > obj-$(CONFIG_HID_GYRATION) += hid-gyration.o > > +obj-$(CONFIG_HID_MCS_HEXDEV) += hid-hexdev-hexlin.o > > obj-$(CONFIG_HID_HOLTEK) += hid-holtek-kbd.o > > obj-$(CONFIG_HID_HOLTEK) += hid-holtek-mouse.o > > obj-$(CONFIG_HID_HOLTEK) += hid-holtekff.o > > diff --git a/drivers/hid/hid-hexdev-hexlin.c b/drivers/hid/hid-hexdev-hexlin.c > > new file mode 100644 > > index 0000000000000..1ddc1e00ab2da > > --- /dev/null > > +++ b/drivers/hid/hid-hexdev-hexlin.c > > @@ -0,0 +1,611 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * LIN bus USB adapter driver https://hexdev.de/hexlin > > + * > > + * Copyright (C) 2024 hexDEV GmbH > > + */ > > + > > +#include <linux/completion.h> > > +#include <linux/hid.h> > > +#include <linux/module.h> > > +#include <linux/wait.h> > > +#include <net/lin.h> > > +#include "hid-ids.h" > > + > > +enum { > > + /* answers */ > > + HEXLIN_SUCCESS = 0x01, > > + HEXLIN_FRAME = 0x02, > > + HEXLIN_ERROR = 0x03, > > + HEXLIN_FAIL = 0x0F, > > + > > + /* lin-responder */ > > + HEXLIN_SET_MODE_RESPONDER = 0x10, > > + HEXLIN_SET_RESPONDER_ANSWER_ID = 0x11, > > + HEXLIN_GET_RESPONDER_ANSWER_ID = 0x12, > > + > > + /* lin-controller */ > > + HEXLIN_SET_MODE_CONTROLLER = 0x20, > > + HEXLIN_SEND_BREAK = 0x21, > > + HEXLIN_SEND_UNCONDITIONAL_FRAME = 0x22, > > + > > + /* lin-div */ > > + HEXLIN_SET_BAUDRATE = 0x34, > > + HEXLIN_GET_BAUDRATE = 0x35, > > + > > + /* div */ > > + HEXLIN_RESET = 0xF0, > > + HEXLIN_GET_VERSION = 0xF1, > > Could you align the values? OK > > > +}; > > + > > +#define HEXLIN_SUCCESS_SZ 1 > > +#define HEXLIN_FRAME_SZ 17 > > +#define HEXLIN_FAIL_SZ 1 > > +#define HEXLIN_GET_RESPONDER_ANSWER_ID_SZ 20 > > +#define HEXLIN_GET_BAUDRATE_SZ 3 > > Is this sizeof(hexlin_baudrate_req)? No > If so, don't add define for it. > This probably applies to other defines here too. I'll refactor these defines in upcoming v4 to a minimum. > > > +#define HEXLIN_BAUDRATE_SZ 2 > > +#define HEXLIN_GET_VERSION_SZ 2 > > +#define HEXLIN_PKGLEN_MAX_SZ 64 > > + > > +struct hexlin_val8_req { > > + u8 cmd; > > + u8 v; > > +} __packed; > > + > > +struct hexlin_baudrate_req { > > + u8 cmd; > > + u16 baudrate; > > +} __packed; > > + > > +struct hexlin_frame { > > + u32 flags; > > + u8 len; > > + u8 lin_id; > > + u8 data[LIN_MAX_DLEN]; > > + u8 checksum; > > + u8 checksum_mode; > > +} __packed; > > + > > +struct hexlin_unconditional_req { > > + u8 cmd; > > + struct hexlin_frame frm; > > +} __packed; > > + > > +struct hexlin_responder_answer { > > + u8 is_active; > > + u8 is_event_frame; > > + u8 event_associated_id; > > + struct hexlin_frame frm; > > +} __packed; > > + > > +struct hexlin_responder_answer_req { > > + u8 cmd; > > + struct hexlin_responder_answer answ; > > +} __packed; > > + > > +struct hexlin_priv_data { > > + struct hid_device *hid_dev; > > + struct lin_device *ldev; > > + u16 baudrate; > > + struct completion wait_in_report; > > + bool is_error; > > + struct mutex tx_lock; /* protects hexlin_tx_report() */ > > + struct hexlin_responder_answer_req rar; > > + u8 fw_version; > > +}; > > + > > +static int hexlin_tx_report(struct hexlin_priv_data *priv, > > + const void *out_report, size_t len) > > +{ > > + u8 *buf; > > + int ret; > > + > > + buf = kmemdup(out_report, len, GFP_KERNEL); > > + if (!buf) > > + return -ENOMEM; > > + > > + ret = hid_hw_output_report(priv->hid_dev, buf, len); > > + kfree(buf); > > Is duplicatign the buffer necessary? No, my intention was to keep the functions calling hexlin_tx_report() simpler and let the compiler optimize this. In the upcoming v4, I merged this into hexlin_tx_req_status() and dropped the kmemdup(). > > > + if (ret < 0) > > + return ret; > > + if (ret != len) > > + return -EIO; > > + > > + return 0; > > +} > > + > > +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); > > + > > + 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)); > > HZ? OK > > > + if (!t) > > + ret = -ETIMEDOUT; > > + > > + if (priv->is_error) > > + ret = -EINVAL; > > + > > +tx_exit: > > + mutex_unlock(&priv->tx_lock); > > + > > + return ret; > > +} > > + > > +#define HEXLIN_GET_CMD(name, enum_cmd) \ > > + static int hexlin_##name(struct hexlin_priv_data *priv) \ > > + { \ > > + u8 cmd = enum_cmd; \ > > + int ret; \ > > + \ > > + ret = hexlin_tx_req_status(priv, &cmd, sizeof(u8)); \ > > Take sizeof() of the relevant variable instead, so: > > sizeof(cmd) OK > > > + if (ret) \ > > + hid_err(priv->hid_dev, "%s failed with %d\n", \ > > + __func__, ret); \ > > + \ > > + return ret; \ > > + } > > + > > +HEXLIN_GET_CMD(get_version, HEXLIN_GET_VERSION) > > +HEXLIN_GET_CMD(reset_dev, HEXLIN_RESET) > > +HEXLIN_GET_CMD(get_baudrate, HEXLIN_GET_BAUDRATE) > > + > > +#define HEXLIN_VAL_CMD(name, enum_cmd, struct_type, vtype) \ > > + static int hexlin_##name(struct hexlin_priv_data *p, vtype val) \ > > + { \ > > + struct struct_type req; \ > > + int ret; \ > > + \ > > + req.cmd = enum_cmd; \ > > + req.v = val; \ > > + \ > > + ret = hexlin_tx_req_status(p, &req, \ > > + sizeof(struct struct_type)); \ > > sizeof(req) OK > > > + if (ret) \ > > + hid_err(p->hid_dev, "%s failed with %d\n", \ > > + __func__, ret); \ > > + \ > > + return ret; \ > > + } > > + > > +HEXLIN_VAL_CMD(send_break, HEXLIN_SEND_BREAK, hexlin_val8_req, u8) > > + > > +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); > > Why you cannot just cast the pointer to correct type? I can, will be refactored in v4. > > > + le32_to_cpus(hxf.flags); > > You must use correct endianess typing. The struct hexlin_frame should have > __le32 flags so sparse's endianness check is happy. OK > > But .flags are not used at all so why is this required in the first place? Was necessary in the development process and will be used in hid_dbg() below in v4. > > > + 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"); > > + > > + lin_rx(priv->ldev, &lf); > > + > > + return 0; > > +} > > + > > +static int hexlin_send_unconditional(struct hexlin_priv_data *priv, > > + const struct hexlin_frame *hxf) > > +{ > > + struct hexlin_unconditional_req req; > > + int ret; > > + > > + if (hxf->lin_id > LIN_ID_MASK) > > + return -EINVAL; > > + > > + req.cmd = HEXLIN_SEND_UNCONDITIONAL_FRAME; > > + memcpy(&req.frm, hxf, sizeof(struct hexlin_frame)); > > + > > + ret = hexlin_tx_req_status(priv, &req, > > + sizeof(struct hexlin_unconditional_req)); > > sizeof(req) OK > > > + > > + if (ret) > > + hid_err(priv->hid_dev, "%s failed with %d\n", __func__, ret); > > + > > + return ret; > > +} > > + > > +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 struct should have __le16 baudrate. OK > > > + > > + 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_get_responder_answer_id(struct hexlin_priv_data *priv, u8 id, > > + struct hexlin_responder_answer_req *rar) > > +{ > > + u8 req[2] = { HEXLIN_GET_RESPONDER_ANSWER_ID, id }; > > + int ret; > > + > > + if (id > LIN_ID_MASK) > > + return -EINVAL; > > + > > + ret = hexlin_tx_req_status(priv, &req, sizeof(req)); > > + if (ret) { > > + hid_err(priv->hid_dev, "%s failed with %d\n", __func__, ret); > > Try to write error message that is meaningful to user, printing __func__ > is not very helpful for user. OK, will be refactored in v4. > > > + return ret; > > + } > > + > > + memcpy(rar, &priv->rar, sizeof(struct hexlin_responder_answer_req)); > > sizeof(*rar) OK > > > + return 0; > > +} > > + > > +static int hexlin_set_responder_answer_id(struct hexlin_priv_data *priv, > > + const struct lin_responder_answer *answ) > > +{ > > + struct hexlin_responder_answer_req rar; > > + int ret; > > + > > + if (answ->lf.lin_id > LIN_ID_MASK || > > + answ->event_associated_id > LIN_ID_MASK) > > + return -EINVAL; > > + > > + rar.cmd = HEXLIN_SET_RESPONDER_ANSWER_ID; > > + rar.answ.is_active = answ->is_active; > > + rar.answ.is_event_frame = answ->is_event_frame; > > + rar.answ.event_associated_id = answ->event_associated_id; > > + rar.answ.frm.len = answ->lf.len; > > + rar.answ.frm.lin_id = answ->lf.lin_id; > > + memcpy(rar.answ.frm.data, answ->lf.data, LIN_MAX_DLEN); > > + rar.answ.frm.checksum = answ->lf.checksum; > > + rar.answ.frm.checksum_mode = answ->lf.checksum_mode; > > + > > + ret = hexlin_tx_req_status(priv, &rar, > > + sizeof(struct hexlin_responder_answer_req)); > > Ditto. OK thanks -- Christoph
On Mon, 2024-05-06 at 20:03 +0300, Ilpo Järvinen wrote: > On Thu, 2 May 2024, 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> > > --- > > drivers/net/can/Kconfig | 16 ++ > > drivers/net/can/Makefile | 1 + > > drivers/net/can/lin-serdev.c | 500 +++++++++++++++++++++++++++++++++++ > > 3 files changed, 517 insertions(+) > > create mode 100644 drivers/net/can/lin-serdev.c > > > > diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig > > index 0934bbf8d03b2..91c6cdeefe440 100644 > > --- a/drivers/net/can/Kconfig > > +++ b/drivers/net/can/Kconfig > > @@ -181,6 +181,22 @@ config CAN_LIN > > > > Actual device drivers need to be enabled too. > > > > +config CAN_LIN_SERDEV > > + tristate "Serial LIN Adaptors" > > + depends on CAN_LIN && SERIAL_DEV_BUS && OF > > + help > > + LIN-Bus support for serial devices equipped with LIN transceievers. > > + This device driver is using CAN_LIN for a userland connection on > > + one side and the kernel its serial device bus (serdev) interface > > + on the other side. > > + > > + If you have a hexlin tty adapter, say Y here and see > > + <https://hexdev.de/hexlin#tty>. > > + > > + This driver can also be built as a module. If so, the module will be > > + called lin-serdev.ko. > > + > > + > > config CAN_SLCAN > > tristate "Serial / USB serial CAN Adaptors (slcan)" > > depends on TTY > > diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile > > index 0093ee9219ca8..21ca514a42439 100644 > > --- a/drivers/net/can/Makefile > > +++ b/drivers/net/can/Makefile > > @@ -26,6 +26,7 @@ obj-$(CONFIG_CAN_IFI_CANFD) += ifi_canfd/ > > obj-$(CONFIG_CAN_JANZ_ICAN3) += janz-ican3.o > > obj-$(CONFIG_CAN_KVASER_PCIEFD) += kvaser_pciefd.o > > obj-$(CONFIG_CAN_LIN) += lin.o > > +obj-$(CONFIG_CAN_LIN_SERDEV) += lin-serdev.o > > obj-$(CONFIG_CAN_MSCAN) += mscan/ > > obj-$(CONFIG_CAN_M_CAN) += m_can/ > > obj-$(CONFIG_CAN_PEAK_PCIEFD) += peak_canfd/ > > diff --git a/drivers/net/can/lin-serdev.c b/drivers/net/can/lin-serdev.c > > new file mode 100644 > > index 0000000000000..13fd9a54c6c93 > > --- /dev/null > > +++ b/drivers/net/can/lin-serdev.c > > @@ -0,0 +1,500 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* Copyright (C) 2024 hexDEV GmbH - https://hexdev.de */ > > + > > +#include <linux/delay.h> > > +#include <linux/init.h> > > +#include <linux/kfifo.h> > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/serdev.h> > > +#include <linux/slab.h> > > +#include <linux/tty.h> > > +#include <net/lin.h> > > + > > +#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; > > + unsigned long break_usleep_min; > > + unsigned long break_usleep_max; > > + unsigned long post_break_usleep_min; > > + unsigned long post_break_usleep_max; > > + unsigned long force_timeout_jfs; > > + struct lin_responder_answer respond_answ[LIN_NUM_IDS]; > > + struct mutex resp_lock; /* protects respond_answ */ > > + bool is_stopped; > > +}; > > + > > +static int linser_open(struct lin_device *ldev) > > +{ > > + struct serdev_device *serdev = to_serdev_device(ldev->dev); > > + struct linser_priv *priv = serdev_device_get_drvdata(serdev); > > + int ret; > > + > > + if (priv->is_stopped) { > > + ret = serdev_device_open(serdev); > > + if (ret) { > > + dev_err(&serdev->dev, "Unable to open device\n"); > > + return ret; > > + } > > + > > + serdev_device_set_flow_control(serdev, false); > > + serdev_device_set_break_detection(serdev, true); > > + > > + priv->is_stopped = false; > > + } > > + > > + return 0; > > +} > > + > > +static int linser_stop(struct lin_device *ldev) > > +{ > > + struct serdev_device *serdev = to_serdev_device(ldev->dev); > > + struct linser_priv *priv = serdev_device_get_drvdata(serdev); > > + > > + if (priv->is_stopped) > > + return 0; > > + > > + serdev_device_close(serdev); > > + priv->is_stopped = true; > > + > > + return 0; > > +} > > + > > +static int linser_send_break(struct linser_priv *priv) > > +{ > > + struct serdev_device *serdev = priv->serdev; > > + int ret; > > + > > + ret = serdev_device_break_ctl(serdev, -1); > > + if (ret) > > + return ret; > > + > > + usleep_range(priv->break_usleep_min, priv->break_usleep_max); > > + > > + ret = serdev_device_break_ctl(serdev, 0); > > + if (ret) > > + return ret; > > + > > + usleep_range(priv->post_break_usleep_min, priv->post_break_usleep_max); > > + > > + return 0; > > +} > > + > > +static int linser_ldo_tx(struct lin_device *ldev, const struct lin_frame *lf) > > +{ > > + struct serdev_device *serdev = to_serdev_device(ldev->dev); > > + struct linser_priv *priv = serdev_device_get_drvdata(serdev); > > + u8 pid = LIN_FORM_PID(lf->lin_id); > > + u8 buf[LINSER_TX_BUFFER_SIZE]; > > + ssize_t written_len, total_len; > > + u8 checksum; > > + int ret; > > + > > + if (lf->len + 3 > LINSER_TX_BUFFER_SIZE) { > > + dev_err(&serdev->dev, "Frame length %u exceeds buffer size\n", lf->len); > > + return -EINVAL; > > + } > > + > > + buf[0] = LIN_SYNC_BYTE; > > + buf[1] = pid; > > + total_len = 2; > > + > > + if (lf->len) { > > + memcpy(&buf[2], lf->data, lf->len); > > + checksum = lin_get_checksum(pid, lf->len, lf->data, > > + lf->checksum_mode); > > + buf[lf->len + 2] = checksum; > > + total_len += lf->len + 1; > > + } > > + > > + ret = linser_send_break(priv); > > + if (ret) > > + return ret; > > + > > + written_len = serdev_device_write(serdev, buf, total_len, 0); > > + if (written_len < total_len) > > + return written_len < 0 ? (int)written_len : -EIO; > > + > > + dev_dbg(&serdev->dev, "sent out: %*ph\n", (int)total_len, buf); > > + > > + serdev_device_wait_until_sent(serdev, 0); > > + > > + return 0; > > +} > > + > > +static void linser_derive_timings(struct linser_priv *priv, u16 bitrate) > > +{ > > + unsigned long break_baud = (bitrate * 2) / 3; > > + unsigned long timeout_us; > > + > > + priv->break_usleep_min = (USEC_PER_SEC * LINSER_SAMPLES_PER_CHAR) / > > + break_baud; > > + priv->break_usleep_max = priv->break_usleep_min + 50; > > + priv->post_break_usleep_min = (USEC_PER_SEC * 1 /* 1 bit */) / break_baud; > > Don't mix comments and code like this. OK > > > + priv->post_break_usleep_max = priv->post_break_usleep_min + 30; > > + > > + timeout_us = DIV_ROUND_CLOSEST(USEC_PER_SEC * 256 /* bit */, bitrate); > > Ditto. OK > > > + priv->force_timeout_jfs = usecs_to_jiffies(timeout_us); > > +} > > + > > +static int linser_update_bitrate(struct lin_device *ldev, u16 bitrate) > > +{ > > + struct serdev_device *serdev = to_serdev_device(ldev->dev); > > + struct linser_priv *priv = serdev_device_get_drvdata(serdev); > > + unsigned int speed; > > + int ret; > > + > > + ret = linser_open(ldev); > > + if (ret) > > + return ret; > > + > > + speed = serdev_device_set_baudrate(serdev, bitrate); > > + if (!bitrate || speed != bitrate) > > + return -EINVAL; > > + > > + linser_derive_timings(priv, bitrate); > > + > > + return 0; > > +} > > + > > +static int linser_get_responder_answer(struct lin_device *ldev, u8 id, > > + struct lin_responder_answer *answ) > > +{ > > + struct serdev_device *serdev = to_serdev_device(ldev->dev); > > + struct linser_priv *priv = serdev_device_get_drvdata(serdev); > > + struct lin_responder_answer *r = &priv->respond_answ[id]; > > + > > + if (!answ) > > + return -EINVAL; > > + > > + mutex_lock(&priv->resp_lock); > > guard(mutex)(&priv->resp_lock); OK > > > + memcpy(answ, r, sizeof(struct lin_responder_answer)); > > sizeof(*answ); OK > > > + mutex_unlock(&priv->resp_lock); > > + > > + return 0; > > +} > > + > > +static int linser_update_resp_answer(struct lin_device *ldev, > > + const struct lin_responder_answer *answ) > > +{ > > + struct serdev_device *serdev = to_serdev_device(ldev->dev); > > + struct linser_priv *priv = serdev_device_get_drvdata(serdev); > > + struct lin_responder_answer *r = &priv->respond_answ[answ->lf.lin_id]; > > + > > + if (!answ) > > + return -EINVAL; > > + > > + mutex_lock(&priv->resp_lock); > > + memcpy(r, answ, sizeof(struct lin_responder_answer)); > > Ditto. OK > > > + r->lf.checksum = lin_get_checksum(LIN_FORM_PID(answ->lf.lin_id), > > + answ->lf.len, > > + answ->lf.data, > > + answ->lf.checksum_mode); > > + mutex_unlock(&priv->resp_lock); > > + > > + return 0; > > +} > > + > > +static struct lin_device_ops linser_lindev_ops = { > > + .ldo_open = linser_open, > > + .ldo_stop = linser_stop, > > + .ldo_tx = linser_ldo_tx, > > + .update_bitrate = linser_update_bitrate, > > + .get_responder_answer = linser_get_responder_answer, > > + .update_responder_answer = linser_update_resp_answer, > > +}; > > + > > +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); > > scoped_guard() can be used here. OK > > > + > > + if (!answ->is_active) { > > + mutex_unlock(&priv->resp_lock); > > + return 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 { > > + mutex_unlock(&priv->resp_lock); > > + return false; > > Use reverse logic so you can reduce indent. 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; > > +} > > + > > +static void linser_pop_fifo(struct linser_priv *priv, size_t n) > > +{ > > + for (size_t i = 0; i < n; i++) > > + kfifo_skip(&priv->rx_fifo); > > +} > > + > > +static int linser_fill_frame(struct linser_priv *priv, struct lin_frame *lf) > > +{ > > + struct serdev_device *serdev = priv->serdev; > > + struct linser_rx buf[LINSER_PARSE_BUFFER]; > > + unsigned int count, i, brk = 0; > > + > > + count = kfifo_out_peek(&priv->rx_fifo, buf, LINSER_PARSE_BUFFER); > > + > > + memset(lf, 0, sizeof(struct lin_frame)); > > + > > + for (i = 0; i < count; i++) { > > + dev_dbg(&serdev->dev, "buf[%d]: data=%02x, flag=%02x\n", > > + i, buf[i].data, buf[i].flag); > > + } > > + > > + if (count < 3) > > + return NEED_MORE; > > + > > + if (buf[0].flag != TTY_BREAK || buf[1].data != LIN_SYNC_BYTE) { > > + linser_pop_fifo(priv, 1); /* pop incorrect start */ > > + return NEED_MORE; > > + } else if (!LIN_CHECK_PID(buf[2].data)) { > > + linser_pop_fifo(priv, 3); /* pop incorrect header */ > > + return NEED_MORE; > > + } > > + > > + lf->lin_id = LIN_GET_ID(buf[2].data); > > + > > + /* from here on we do have a correct LIN header */ > > + > > + if (count == 3) > > + return linser_tx_frame_as_responder(priv, lf->lin_id) ? > > + NEED_MORE : NEED_FORCE; > > + > > + for (i = 3; i < count && i < LINSER_PARSE_BUFFER && i < 12; i++) { > > + if (buf[i].flag == TTY_BREAK) { > > + brk = i; > > + break; > > + } > > + lf->len++; > > + } > > + if (lf->len) > > + lf->len -= 1; /* account for checksum */ > > + > > + if (brk == 3) > > + return MODE_OK; > > + > > + if (brk == 4) { > > + /* suppress wrong answer data-byte in between PID and break > > + * because checksum is missing > > + */ > > + return MODE_OK; > > + } > > + > > + for (i = 0; i < lf->len; i++) > > + lf->data[i] = buf[3 + i].data; > > + lf->checksum = buf[2 + lf->len + 1].data; > > + mutex_lock(&priv->resp_lock); > > + lf->checksum_mode = priv->respond_answ[lf->lin_id].lf.checksum_mode; > > + mutex_unlock(&priv->resp_lock); > > + > > + dev_dbg(&serdev->dev, "brk:%i, len:%u, data:%*ph, checksum:%x (%s)\n", > > + brk, lf->len, lf->len, lf->data, lf->checksum, > > + lf->checksum_mode ? "enhanced" : "classic"); > > + > > + if (brk > 4) > > + return MODE_OK; /* frame in between two breaks: so complete */ > > + > > + if (lf->len == 8) > > + return MODE_OK; > > + > > + return NEED_FORCE; > > +} > > + > > +static int linser_process_frame(struct linser_priv *priv, bool force) > > +{ > > + struct serdev_device *serdev = priv->serdev; > > + struct lin_frame lf; > > + size_t bytes_to_pop; > > + int ret = NEED_MORE; > > + > > + while (kfifo_len(&priv->rx_fifo) >= LIN_HEADER_SIZE) { > > + ret = linser_fill_frame(priv, &lf); > > + > > + if (ret == MODE_OK || (ret == NEED_FORCE && force)) { > > + dev_dbg(&serdev->dev, "lin_rx: %s\n", > > + force ? "force" : "normal"); > > + lin_rx(priv->lin_dev, &lf); > > + bytes_to_pop = LIN_HEADER_SIZE + lf.len + > > + (lf.len ? 1 : 0); > > + linser_pop_fifo(priv, bytes_to_pop); > > + force = false; > > + ret = MODE_OK; > > + } else { > > + return ret; > > Reverse logic & deindent the main block. OK ... thanks -- Christoph
On Mon, 2024-05-06 at 20:08 +0300, Ilpo Järvinen wrote: > On Thu, 2 May 2024, Christoph Fritz wrote: > > > Enhance CAN broadcast manager with RX_LIN_SETUP and RX_LIN_DELETE > > operations to setup automatic LIN frame responses in responder mode. > > > > Additionally, the patch introduces the LIN_EVENT_FRAME flag to > > setup event-triggered LIN frames. > > > > Signed-off-by: Christoph Fritz <christoph.fritz@hexdev.de> > > --- > > include/uapi/linux/can/bcm.h | 5 ++- > > net/can/bcm.c | 74 +++++++++++++++++++++++++++++++++++- > > 2 files changed, 77 insertions(+), 2 deletions(-) > > > > diff --git a/include/uapi/linux/can/bcm.h b/include/uapi/linux/can/bcm.h > > index f1e45f533a72c..c46268a114078 100644 > > --- a/include/uapi/linux/can/bcm.h > > +++ b/include/uapi/linux/can/bcm.h > > @@ -86,7 +86,9 @@ enum { > > TX_EXPIRED, /* notification on performed transmissions (count=0) */ > > RX_STATUS, /* reply to RX_READ request */ > > RX_TIMEOUT, /* cyclic message is absent */ > > - RX_CHANGED /* updated CAN frame (detected content change) */ > > + RX_CHANGED, /* updated CAN frame (detected content change) */ > > + RX_LIN_SETUP, /* create auto-response for LIN frame */ > > + RX_LIN_DELETE, /* remove auto-response for LIN frame */ > > }; > > > > #define SETTIMER 0x0001 > > @@ -101,5 +103,6 @@ enum { > > #define TX_RESET_MULTI_IDX 0x0200 > > #define RX_RTR_FRAME 0x0400 > > #define CAN_FD_FRAME 0x0800 > > +#define LIN_EVENT_FRAME 0x1000 > > > > #endif /* !_UAPI_CAN_BCM_H */ > > diff --git a/net/can/bcm.c b/net/can/bcm.c > > index 27d5fcf0eac9d..a717e594234d1 100644 > > --- a/net/can/bcm.c > > +++ b/net/can/bcm.c > > @@ -59,6 +59,7 @@ > > #include <linux/can/bcm.h> > > #include <linux/slab.h> > > #include <net/sock.h> > > +#include <net/lin.h> > > #include <net/net_namespace.h> > > > > /* > > @@ -1330,6 +1331,59 @@ static int bcm_tx_send(struct msghdr *msg, int ifindex, struct sock *sk, > > return cfsiz + MHSIZ; > > } > > > > +static int bcm_lin_setup(struct bcm_msg_head *msg_head, struct msghdr *msg, > > + int ifindex, struct sock *sk, int cfsiz, int is_active) > > +{ > > + struct lin_responder_answer answ; > > + struct net_device *dev; > > + struct sk_buff *skb; > > + struct canfd_frame cf; > > + netdevice_tracker tracker; > > + size_t sz; > > + int ret; > > + > > + if (msg_head->nframes > 1) > > + return -EINVAL; > > + > > + if (!(msg_head->flags & CAN_FD_FRAME)) > > + return -EINVAL; > > + > > + ret = memcpy_from_msg(&cf, msg, cfsiz); > > + if (ret < 0) > > + return ret; > > + > > + answ.lf.lin_id = cf.can_id & LIN_ID_MASK; > > + answ.is_active = is_active; > > + answ.is_event_frame = !!(msg_head->flags & LIN_EVENT_FRAME); > > + answ.event_associated_id = msg_head->can_id; > > + answ.lf.len = min(cf.len, LIN_MAX_DLEN); > > + memcpy(answ.lf.data, cf.data, answ.lf.len); > > + sz = min(sizeof(struct lin_responder_answer), sizeof(cf.data)); > > + cf.can_id |= LIN_RXOFFLOAD_DATA_FLAG; > > + memcpy(cf.data, &answ, sz); > > + > > + dev = netdev_get_by_index(sock_net(sk), ifindex, &tracker, GFP_KERNEL); > > + if (!dev) > > + return -ENODEV; > > + > > + skb = alloc_skb(cfsiz + sizeof(struct can_skb_priv), gfp_any()); > > You just called the other function with GFP_KERNEL and you now need > gfp_any(). Which is correct?? I guess GFP_KERNEL but I'm not sure so I'll let gfp_any() decide for netdev_get_by_index() too. > > > + if (!skb) > > + goto lin_out; > > + > > + can_skb_reserve(skb); > > + can_skb_prv(skb)->ifindex = dev->ifindex; > > + can_skb_prv(skb)->skbcnt = 0; > > + skb_put_data(skb, &cf, cfsiz); > > + > > + skb->dev = dev; > > + can_skb_set_owner(skb, sk); > > + ret = can_send(skb, 1); /* send with loopback */ > > + > > +lin_out: > > + netdev_put(dev, &tracker); > > + return ret; > > +} > > + > > /* > > * bcm_sendmsg - process BCM commands (opcodes) from the userspace > > */ > > @@ -1429,12 +1483,30 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) > > > > case TX_SEND: > > /* we need exactly one CAN frame behind the msg head */ > > - if ((msg_head.nframes != 1) || (size != cfsiz + MHSIZ)) > > + if (msg_head.nframes != 1 || size != cfsiz + MHSIZ) > > Unrelated style fix, doesn't belong to this patch. OK ... thanks -- Christoph
On Mon, 2024-05-06 at 20:11 +0300, Ilpo Järvinen wrote: > On Thu, 2 May 2024, Christoph Fritz wrote: > > > The CAN Broadcast Manager now has the capability to dispatch CANFD > > frames marked with the id LINBUS_RXOFFLOAD_ID. This patch introduces > > functionality to interpret these specific frames, enabling the > > configuration of RX offloading within the LIN driver. > > > > Signed-off-by: Christoph Fritz <christoph.fritz@hexdev.de> > > --- > > drivers/net/can/lin.c | 29 +++++++++++++++++++++++++++++ > > 1 file changed, 29 insertions(+) > > > > diff --git a/drivers/net/can/lin.c b/drivers/net/can/lin.c > > index 95906003666fb..ee2ebea2c865f 100644 > > --- a/drivers/net/can/lin.c > > +++ b/drivers/net/can/lin.c > > @@ -194,6 +194,27 @@ static void lin_remove_sysfs_id_files(struct net_device *ndev) > > } > > } > > > > +static int lin_setup_rxoffload(struct lin_device *ldev, > > + struct canfd_frame *cfd) > > +{ > > + struct lin_responder_answer answ; > > + > > + if (!(cfd->flags & CANFD_FDF)) > > + return -EMSGSIZE; > > This seems a bit odd error code. OK, will be changed in v4 to more common -EINVAL. > > > + BUILD_BUG_ON(sizeof(struct lin_responder_answer) > sizeof(cfd->data)); > > + memcpy(&answ, cfd->data, sizeof(struct lin_responder_answer)); > > 2x sizeof(answ) OK ... thanks -- Christoph
On Thu, 9 May 2024, Christoph Fritz wrote: > On Mon, 2024-05-06 at 19:53 +0300, Ilpo Järvinen wrote: > > On Thu, 2 May 2024, 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> > > > --- > > > + le32_to_cpus(hxf.flags); > > > > You must use correct endianess typing. The struct hexlin_frame should have > > __le32 flags so sparse's endianness check is happy. > > OK > > > > > But .flags are not used at all so why is this required in the first place? > > Was necessary in the development process and will be used in hid_dbg() > below in v4. Ok, I was expecting you'd print it out there but since it wasn't, I made the unused comment. BTW, you don't need to reply "OK" to me for the review comments which you're going to do in the next version. I trust you'll address those comments which are not replied into. It saves us both time :-). > > > + 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"); > > > + > > > + lin_rx(priv->ldev, &lf); > > > + > > > + return 0; > > > +}