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

[Christoph Fritz <christoph.fritz@hexdev.de>]

...
> > --- /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.

Thanks for the catch, it's wrong and will be removed in v2.

> 
> > +			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?

Falsely it is not, like in mcp251x.c I'll add a mutex.

> 
> > +
> > +	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?

It's not.

> 
> > +	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?

In a later patch, let me fix the patchset accordingly.

> 
> > +	}
> > +
> > +	DEV_STATS_INC(ndev, tx_packets);
> > +	DEV_STATS_ADD(ndev, tx_bytes, lf.len);
> > +	ldev->tx_busy = false;
> 
> The same as above.

OK

> 
> > +	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.

OK

> 
> > +
> > +	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?

OK, no sysv 'uint', will be changed in another patch too

> 
> > +	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?

Yes

> 
> > +}
> 
> 
> > +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?

Just beeing cautious, I guess it can be removed

> 
> > +	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.

Compact code, sure, will get adapted

> 
> > +}
> > +
> > +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?

no preference over the other, will use 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;
> > +	}
> > +
> > +	ldev->wq = alloc_workqueue(dev_name(dev), WQ_FREEZABLE | WQ_MEM_RECLAIM,
> > +				   0);
> 
> It would be worth noting why you need your own WQ.

I'll add a comment stating:

/* Use workqueue as tx over USB/SPI/... may sleep */

> 
> > +	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..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() ?

In the upcoming v2 I'll use:

#define LIN_ID_MASK   GENMASK(5, 0)

> 
> > +/* 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.

This is also being used by a serial lin driver. But I guess most of the drivers have no need for this. Mhm, ... any ideas?

> And perhaps u8?

OK

> 
> > +	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() ?

In the upcoming v2 I'll use:

#define LIN_GET_ID(PID)		FIELD_GET(LIN_ID_MASK, PID)

> 
> > +#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 for the feedback
  -- Christoph