Date: Mon, 22 Apr 2024 10:16:07 +0200
From: Jiri Slaby <jirislaby@...nel.org>
To: Christoph Fritz <christoph.fritz@...dev.de>,
 Oliver Hartkopp <socketcan@...tkopp.net>,
 Marc Kleine-Budde <mkl@...gutronix.de>,
 Vincent Mailhol <mailhol.vincent@...adoo.fr>,
 "David S . Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Jiri Kosina <jikos@...nel.org>,
 Benjamin Tissoires <bentiss@...nel.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Andreas Lauser <andreas.lauser@...cedes-benz.com>,
 Jonathan Corbet <corbet@....net>, linux-can@...r.kernel.org,
 netdev@...r.kernel.org, devicetree@...r.kernel.org,
 linux-input@...r.kernel.org, linux-serial@...r.kernel.org
Subject: Re: [PATCH 01/11] can: Add LIN bus as CAN abstraction

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@...dev.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..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

Powered by blists - more mailing lists