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

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

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