On 22. 04. 24, 8:51, 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.
...
--- /dev/null
+++ b/drivers/net/can/lin.c
...> +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);
+ kfree(attr);
Is the latter kfree() right? It appears not.
+ return -ENOMEM;
+ }
+ }
+
+ return 0;
+}
...
+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;
+
+ ldev->tx_busy = true;
How is this store protected against reordering/race conditions?
+
+ 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 = (u8)(cfd->can_id & LIN_ID_MASK);
Why is that cast necessary?
+ 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;
Where is this label?
+ }
+
+ DEV_STATS_INC(ndev, tx_packets);
+ DEV_STATS_ADD(ndev, tx_bytes, lf.len);
+ ldev->tx_busy = false;
The same as above.
+ 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;
And here too.
+
+ netif_stop_queue(ndev);
+ ldev->tx_skb = skb;
+ queue_work(ldev->wq, &ldev->tx_work);
+
+ return NETDEV_TX_OK;
+}
...
+u8 lin_get_checksum(u8 pid, u8 n_of_bytes, const u8 *bytes,
+ enum lin_checksum_mode cm)
+{
+ uint csum = 0;
Is "uint" of the preffered types in the kernel?
+ 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 (u8)(~csum & 0xff);
Unnecessary cast?
+}
+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 (!ndev)
+ return -ENODEV;
Is this racy or is this only a sanity check?
+ 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;
+ int ret;
+
+ bitrate = ldev->can.bittiming.bitrate;
+ ret = ldev->ldev_ops->update_bitrate(ldev, bitrate);
+
+ return ret;
No need for ret then.
+}
+
+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) {
+ netdev_err(ndev, "missing mandatory lin_device_ops\n");
+ return ERR_PTR(-EOPNOTSUPP);
Would EINVAL fit better?
+ }
+
+ 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;
+ }
+
+ ldev->wq = alloc_workqueue(dev_name(dev), WQ_FREEZABLE | WQ_MEM_RECLAIM,
+ 0);
It would be worth noting why you need your own WQ.
+ 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@xxxxxxxxx>");
+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..2fe16e142db96
--- /dev/null
+++ b/include/net/lin.h
@@ -0,0 +1,97 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/* Copyright (C) 2024 hexDEV GmbH - https://hexdev.de */
+
+#ifndef _NET_LIN_H_
+#define _NET_LIN_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 0x0000003FU
GEN_MASK() ?
+/* special ID descriptions for LIN */
+#define LIN_ENHANCED_CKSUM_FLAG 0x00000100U
+
+static const unsigned char lin_id_parity_tbl[] = {
This ends up in every translation unit you include lin.h into. Bad.
And perhaps u8?
+ 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,
+};
+
+#define LIN_GET_ID(PID) ((PID) & LIN_ID_MASK)
FIELD_GET() ?
+#define LIN_FORM_PID(ID) (LIN_GET_ID(ID) | \
+ lin_id_parity_tbl[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)))
thanks,
--
js
suse labs
