Re: [PATCH 01/11] can: Add LIN bus as CAN abstraction

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Automotive Discussions]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]     [CAN Bus]

  Powered by Linux