[3/8] can: CAN Network device driver and SYSFS interface

The CAN network device driver interface provides a generic interface to
setup, configure and monitor CAN network devices. It exports a set of
common data structures and functions, which all real CAN network device
drivers should use. Please have a look to the SJA1000 or MSCAN driver
to understand how to use them. The name of the module is can-dev.ko.

Furthermore adds a SYSFS interface to set and get CAN device properties.
When the CAN device is registered, a set of SYSFS files is created in
"/sys/class/net/canX/". These files allow to set and get device
properties like bit-timing parameters, state, controller mode and CAN
statistics.

For further information please check "Documentation/networking/can.txt"

Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
Signed-off-by: Oliver Hartkopp <oliver.hartkopp@volkswagen.de>
---
 drivers/net/can/Kconfig  |   23 ++
 drivers/net/can/Makefile |    5 +
 drivers/net/can/dev.c    |  528 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/net/can/sysfs.c  |  509 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/net/can/sysfs.h  |   24 ++
 include/linux/can/dev.h  |  136 ++++++++++++
 6 files changed, 1225 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/can/dev.c
 create mode 100644 drivers/net/can/sysfs.c
 create mode 100644 drivers/net/can/sysfs.h
 create mode 100644 include/linux/can/dev.h

On Thu, 19 Feb 2009 20:01:17 +0100
Wolfgang Grandegger <wg@grandegger.com> wrote:

> +/*
> + * Bit-timing calculation derived from:
> + *
> + * Code based on LinCAN sources and H8S2638 project
> + * Copyright 2004-2006 Pavel Pisa - DCE FELK CVUT cz
> + * Copyright 2005      Stanislav Marek
> + * email: pisa@cmp.felk.cvut.cz
> + */
> +static int can_update_spt(const struct can_bittiming_const *btc,
> +			  int sampl_pt, int tseg, int *tseg1, int *tseg2)
> +{
> +	*tseg2 = tseg + 1 - (sampl_pt * (tseg + 1)) / 1000;
> +	if (*tseg2 < btc->tseg2_min)
> +		*tseg2 = btc->tseg2_min;
> +	if (*tseg2 > btc->tseg2_max)
> +		*tseg2 = btc->tseg2_max;
> +	*tseg1 = tseg - *tseg2;
> +	if (*tseg1 > btc->tseg1_max) {
> +		*tseg1 = btc->tseg1_max;
> +		*tseg2 = tseg - *tseg1;
> +	}
> +	return 1000 * (tseg + 1 - *tseg2) / (tseg + 1);
> +}

I can only assume that this calculation means something to somebody.  I
guess there's no hope for those of use too lazy to go read the Bosch spec,
where, I assume, this kind of stuff is described.

> +static int can_calc_bittiming(struct net_device *dev)

This function, too, is pretty well impenetrable.  I couldn't possibly try
to tell you if it's even remotely correct.  Some comments might be nice.

> +/*
> + * Allocate and setup space for the CAN network device
> + */
> +struct net_device *alloc_candev(int sizeof_priv)
> +{
> +	struct net_device *dev;
> +	struct can_priv *priv;
> +
> +	dev = alloc_netdev(sizeof_priv, "can%d", can_setup);
> +	if (!dev)
> +		return NULL;
> +
> +	priv = netdev_priv(dev);
> +
> +	priv->state = CAN_STATE_STOPPED;
> +	spin_lock_init(&priv->irq_lock);

This is the first mention I see of a lock in this file.  What are your
locking rules?  There doesn't seem to be a lot of locking going on...  In
particular, nothing in this file uses irq_lock.

> +	init_timer(&priv->timer);
> +	priv->timer.expires = 0;
> +
> +	return dev;
> +}
> +EXPORT_SYMBOL_GPL(alloc_candev);

> +/*
> + * Allocate space of the CAN network device
> + */
> +void free_candev(struct net_device *dev)
> +{
> +	free_netdev(dev);
> +}
> +EXPORT_SYMBOL_GPL(free_candev);

I do believe that function is *freeing* space...?  

The allocation function initializes a timer, but here we do nothing to
ensure that the timer is not running.  Is there a rule somewhere that I
missed?

> +/*
> + * Local echo of CAN messages
> + *
> + * CAN network devices *should* support a local echo functionality
> + * (see Documentation/networking/can.txt). To test the handling of CAN
> + * interfaces that do not support the local echo both driver types are
> + * implemented. In the case that the driver does not support the echo
> + * the IFF_ECHO remains clear in dev->flags. This causes the PF_CAN core
> + * to perform the echo as a fallback solution.
> + */
> +
> +static void can_flush_echo_skb(struct net_device *dev)
> +{
> +	struct can_priv *priv = netdev_priv(dev);
> +	struct net_device_stats *stats = &dev->stats;
> +	int i;
> +
> +	for (i = 0; i < CAN_ECHO_SKB_MAX; i++) {
> +		if (priv->echo_skb[i]) {
> +			kfree_skb(priv->echo_skb[i]);
> +			priv->echo_skb[i] = NULL;
> +			stats->tx_dropped++;
> +			stats->tx_aborted_errors++;
> +		}
> +	}
> +}

What lock is protecting priv->echo_skb?

> +/*
> + * Put the skb on the stack to be looped backed locally lateron
> + *
> + * The function is typically called in the start_xmit function
> + * of the device driver.
> + */
> +void can_put_echo_skb(struct sk_buff *skb, struct net_device *dev, int idx)
> +{
> +	struct can_priv *priv = netdev_priv(dev);
> +
> +	/* set flag whether this packet has to be looped back */
> +	if (!(dev->flags & IFF_ECHO) || skb->pkt_type != PACKET_LOOPBACK) {
> +		kfree_skb(skb);
> +		return;
> +	}

The comment says "set flag", but the code potentially frees the skb and aborts.

[...]

> +/*
> + * CAN device restart for bus-off recovery
> + */
> +int can_restart_now(struct net_device *dev)
> +{
> +	struct can_priv *priv = netdev_priv(dev);
> +	struct net_device_stats *stats = &dev->stats;
> +	struct sk_buff *skb;
> +	struct can_frame *cf;
> +	int err;
> +
> +	if (netif_carrier_ok(dev))
> +		netif_carrier_off(dev);
> +
> +	/* Cancel restart in progress */
> +	if (priv->timer.expires) {
> +		del_timer(&priv->timer);
> +		priv->timer.expires = 0; /* mark inactive timer */
> +	}

Are you sure you don't want del_timer_sync() there?  What keeps you from
racing with the timer?

[...]

> +/*
> + * Cleanup function before the device gets closed.
> + *
> + * This functions should be called in the close function of the device
> + * driver.
> + */
> +void can_close_cleanup(struct net_device *dev)
> +{
> +	struct can_priv *priv = netdev_priv(dev);
> +
> +	if (priv->timer.expires) {
> +		del_timer(&priv->timer);
> +		priv->timer.expires = 0;
> +	}
> +
> +	can_flush_echo_skb(dev);
> +}
> +EXPORT_SYMBOL_GPL(can_close_cleanup);

You definitely want del_timer_sync() here.  You could potentially return
with the timer still running.

> diff --git a/drivers/net/can/sysfs.c b/drivers/net/can/sysfs.c
> new file mode 100644
> index 0000000..746f641
> --- /dev/null
> +++ b/drivers/net/can/sysfs.c

Most of this file looks like typical sysfs cruft.  Again, though, I wonder
about your locking...

> +/* use same locking rules as GIF* ioctl's */
> +static ssize_t can_dev_show(struct device *d,
> +			    struct device_attribute *attr, char *buf,
> +			    ssize_t (*fmt)(struct net_device *, char *))
> +{
> +	struct net_device *dev = to_net_dev(d);
> +	ssize_t ret = -EINVAL;
> +
> +	read_lock(&dev_base_lock);
> +	if (dev_isalive(dev))
> +		ret = (*fmt)(dev, buf);
> +	read_unlock(&dev_base_lock);
> +
> +	return ret;
> +}

I'm not sure what the GIF* ioctl() locking rules are, but do they really
give you properly-serialized access to your device?  dev_base_lock seems
like an unlikely choice, somehow, but maybe I'm missing something.

[...]

> +/* use same locking and permission rules as SIF* ioctl's */
> +static ssize_t can_dev_store(struct device *d, struct device_attribute *attr,
> +			     const char *buf, size_t len,
> +			     int (*set)(struct net_device *, unsigned long))
> +{
> +	struct net_device *dev = to_net_dev(d);
> +	unsigned long new;
> +	int ret = -EINVAL;
> +
> +	if (!capable(CAP_NET_ADMIN))
> +		return -EPERM;
> +
> +	ret = strict_strtoul(buf, 0, &new);
> +	if (ret)
> +		goto out;
> +
> +	rtnl_lock();
> +	if (dev_isalive(dev)) {
> +		ret = (*set)(dev, new);
> +		if (!ret)
> +			ret = len;
> +	}
> +	rtnl_unlock();
> +out:
> +	return ret;
> +}

Here we're using a different (global) lock?  Me confused...

[...]

> diff --git a/drivers/net/can/sysfs.h b/drivers/net/can/sysfs.h
> new file mode 100644
> index 0000000..e21f2fa
> --- /dev/null
> +++ b/drivers/net/can/sysfs.h
> @@ -0,0 +1,24 @@
> +/*
> + * Copyright (C) 2007 Wolfgang Grandegger <wg@grandegger.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the version 2 of the GNU General Public License
> + * as published by the Free Software Foundation
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + */
> +
> +#ifndef CAN_SYSFS_H
> +#define CAN_SYSFS_H
> +
> +void can_create_sysfs(struct net_device *dev);
> +void can_remove_sysfs(struct net_device *dev);
> +
> +#endif /* CAN_SYSFS_H */
> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
> new file mode 100644
> index 0000000..078ac03
> --- /dev/null
> +++ b/include/linux/can/dev.h
> @@ -0,0 +1,136 @@
> +/*
> + * linux/can/dev.h
> + *
> + * Definitions for the CAN network device driver interface
> + *
> + * Copyright (C) 2006 Andrey Volkov <avolkov@varma-el.com>
> + *               Varma Electronics Oy
> + *
> + * Copyright (C) 2008 Wolfgang Grandegger <wg@grandegger.com>

Nothing in 2009? :)

> + *
> + * Send feedback to <socketcan-users@lists.berlios.de>
> + */
> +
> +#ifndef CAN_DEV_H
> +#define CAN_DEV_H
> +
> +#include <linux/can/error.h>
> +
> +/*
> + * CAN bitrate and bit-timing
> + */
> +struct can_bittiming {
> +	u32 bitrate;
> +	u32 sample_point;
> +	u32 tq;
> +	u32 prop_seg;
> +	u32 phase_seg1;
> +	u32 phase_seg2;
> +	u32 sjw;
> +	u32 clock;
> +	u32 brp;
> +};

It sure would be nice (again) to know what all these parameters are.

> +struct can_bittiming_const {
> +	u32 tseg1_min;
> +	u32 tseg1_max;
> +	u32 tseg2_min;
> +	u32 tseg2_max;
> +	u32 sjw_max;
> +	u32 brp_min;
> +	u32 brp_max;
> +	u32 brp_inc;
> +};

Ditto.

[...]

> +/*
> + * CAN common private data
> + */
> +#define CAN_ECHO_SKB_MAX  4
> +
> +struct can_priv {
> +	struct can_device_stats can_stats;
> +
> +	struct can_bittiming bittiming;
> +	struct can_bittiming_const *bittiming_const;
> +
> +	spinlock_t irq_lock;

...protecting what...?

> +	enum can_state state;
> +	u32 ctrlmode;
> +
> +	int restart_ms;
> +	struct timer_list timer;
> +
> +	struct sk_buff *echo_skb[CAN_ECHO_SKB_MAX];
> +
> +	int (*do_set_bittiming)(struct net_device *dev);
> +	int (*do_get_state)(struct net_device *dev, enum can_state *state);
> +	int (*do_set_mode)(struct net_device *dev, enum can_mode mode);
> +	int (*do_set_ctrlmode)(struct net_device *dev, u32 ctrlmode);
> +	int (*do_get_ctrlmode)(struct net_device *dev, u32 *ctrlmode);
> +};
> +
> +#define ND2D(_ndev)	(_ndev->dev.parent)

Does this macro really improve the clarity of the code?

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jonathan Corbet wrote:
> On Thu, 19 Feb 2009 20:01:17 +0100
> Wolfgang Grandegger <wg@grandegger.com> wrote:
> 
>> +/*
>> + * Bit-timing calculation derived from:
>> + *
>> + * Code based on LinCAN sources and H8S2638 project
>> + * Copyright 2004-2006 Pavel Pisa - DCE FELK CVUT cz
>> + * Copyright 2005      Stanislav Marek
>> + * email: pisa@cmp.felk.cvut.cz
>> + */
>> +static int can_update_spt(const struct can_bittiming_const *btc,
>> +			  int sampl_pt, int tseg, int *tseg1, int *tseg2)
>> +{
>> +	*tseg2 = tseg + 1 - (sampl_pt * (tseg + 1)) / 1000;
>> +	if (*tseg2 < btc->tseg2_min)
>> +		*tseg2 = btc->tseg2_min;
>> +	if (*tseg2 > btc->tseg2_max)
>> +		*tseg2 = btc->tseg2_max;
>> +	*tseg1 = tseg - *tseg2;
>> +	if (*tseg1 > btc->tseg1_max) {
>> +		*tseg1 = btc->tseg1_max;
>> +		*tseg2 = tseg - *tseg1;
>> +	}
>> +	return 1000 * (tseg + 1 - *tseg2) / (tseg + 1);
>> +}
> 
> I can only assume that this calculation means something to somebody.  I
> guess there's no hope for those of use too lazy to go read the Bosch spec,
> where, I assume, this kind of stuff is described.

The purpose and quirks of this bit-timing calculation are described
briefly in the Kconfig entry for CONFIG_CAN_CALC_BITTIMING and in
Documentation/networking/can.txt which I quote below:

 "6.5.2 Setting the CAN bit-timing

  The CAN bit-timing parameters can always be specified in a hardware
  independent format as proposed in the Bosch CAN 2.0 specification
  using the SYSFS files "tq", "prop_seg", "phase_seg1", "phase_seg2"
  and "sjw". The SYSFS files are described in the previous chapter.

  If the kernel option CONFIG_CAN_CALC_BITTIMING is enabled, CIA
  recommended CAN bit-timing parameters will be calculated for the bit-
  rate written to the SYSFS file "bitrate" when the device gets started.
  Note that this works fine for the most common CAN controllers with
  standard bit-rates but may *fail* for exotic bit-rates or CAN source
  clock frequencies. Disabling CONFIG_CAN_CALC_BITTIMING saves some
  space and allows user space tools to solely determine and set the
  bit-timing parameters. The CAN controller specific bit-timing
  constants can be used for that purpose. They are available in the
  SYSFS directory "can_bittiming" as well with the name prefix "hw_"."

The user just sets the bit-rate and this function will determine proper
bit-timing parameters.

>> +static int can_calc_bittiming(struct net_device *dev)
> 
> This function, too, is pretty well impenetrable.  I couldn't possibly try
> to tell you if it's even remotely correct.  Some comments might be nice.

Unfortunately, there are no simple speed settings like 100 bps FD for
Ethernet. Setting proper CAN bit-timing might be tricky and in rare
cases you even may need the help of a real CAN expert, including myself
:-(. Furthermore, setting the hardware bit-timing is very hardware
*dependent* and we tried to address this issue using a hardware
*independent* format as proposed by the Bosch specification. Well, maybe
I should document our approach in more detail.

>> +/*
>> + * Allocate and setup space for the CAN network device
>> + */
>> +struct net_device *alloc_candev(int sizeof_priv)
>> +{
>> +	struct net_device *dev;
>> +	struct can_priv *priv;
>> +
>> +	dev = alloc_netdev(sizeof_priv, "can%d", can_setup);
>> +	if (!dev)
>> +		return NULL;
>> +
>> +	priv = netdev_priv(dev);
>> +
>> +	priv->state = CAN_STATE_STOPPED;
>> +	spin_lock_init(&priv->irq_lock);
> 
> This is the first mention I see of a lock in this file.  What are your
> locking rules?  There doesn't seem to be a lot of locking going on...  In
> particular, nothing in this file uses irq_lock.

I see, the locking needs some more careful analysis. Thanks for pointing
that out.

> 
>> +	init_timer(&priv->timer);
>> +	priv->timer.expires = 0;
>> +
>> +	return dev;
>> +}
>> +EXPORT_SYMBOL_GPL(alloc_candev);
> 
>> +/*
>> + * Allocate space of the CAN network device
>> + */
>> +void free_candev(struct net_device *dev)
>> +{
>> +	free_netdev(dev);
>> +}
>> +EXPORT_SYMBOL_GPL(free_candev);
> 
> I do believe that function is *freeing* space...?  

Of course, will fix.

> The allocation function initializes a timer, but here we do nothing to
> ensure that the timer is not running.  Is there a rule somewhere that I
> missed?

If the device is freed when its still open, the network layer calls the
close function of the device, which will do to proper cleanup including
deleting the timer.

>> +/*
>> + * Local echo of CAN messages
>> + *
>> + * CAN network devices *should* support a local echo functionality
>> + * (see Documentation/networking/can.txt). To test the handling of CAN
>> + * interfaces that do not support the local echo both driver types are
>> + * implemented. In the case that the driver does not support the echo
>> + * the IFF_ECHO remains clear in dev->flags. This causes the PF_CAN core
>> + * to perform the echo as a fallback solution.
>> + */
>> +
>> +static void can_flush_echo_skb(struct net_device *dev)
>> +{
>> +	struct can_priv *priv = netdev_priv(dev);
>> +	struct net_device_stats *stats = &dev->stats;
>> +	int i;
>> +
>> +	for (i = 0; i < CAN_ECHO_SKB_MAX; i++) {
>> +		if (priv->echo_skb[i]) {
>> +			kfree_skb(priv->echo_skb[i]);
>> +			priv->echo_skb[i] = NULL;
>> +			stats->tx_dropped++;
>> +			stats->tx_aborted_errors++;
>> +		}
>> +	}
>> +}
> 
> What lock is protecting priv->echo_skb?

This code is called when the device is stopped, but I need to double-check.

> 
>> +/*
>> + * Put the skb on the stack to be looped backed locally lateron
>> + *
>> + * The function is typically called in the start_xmit function
>> + * of the device driver.
>> + */
>> +void can_put_echo_skb(struct sk_buff *skb, struct net_device *dev, int idx)
>> +{
>> +	struct can_priv *priv = netdev_priv(dev);
>> +
>> +	/* set flag whether this packet has to be looped back */
>> +	if (!(dev->flags & IFF_ECHO) || skb->pkt_type != PACKET_LOOPBACK) {
>> +		kfree_skb(skb);
>> +		return;
>> +	}
> 
> The comment says "set flag", but the code potentially frees the skb and aborts.

Will fix the comment.

> [...]
> 
>> +/*
>> + * CAN device restart for bus-off recovery
>> + */
>> +int can_restart_now(struct net_device *dev)
>> +{
>> +	struct can_priv *priv = netdev_priv(dev);
>> +	struct net_device_stats *stats = &dev->stats;
>> +	struct sk_buff *skb;
>> +	struct can_frame *cf;
>> +	int err;
>> +
>> +	if (netif_carrier_ok(dev))
>> +		netif_carrier_off(dev);
>> +
>> +	/* Cancel restart in progress */
>> +	if (priv->timer.expires) {
>> +		del_timer(&priv->timer);
>> +		priv->timer.expires = 0; /* mark inactive timer */
>> +	}
> 
> Are you sure you don't want del_timer_sync() there?  What keeps you from
> racing with the timer?

Right, will fix.

> [...]
> 
>> +/*
>> + * Cleanup function before the device gets closed.
>> + *
>> + * This functions should be called in the close function of the device
>> + * driver.
>> + */
>> +void can_close_cleanup(struct net_device *dev)
>> +{
>> +	struct can_priv *priv = netdev_priv(dev);
>> +
>> +	if (priv->timer.expires) {
>> +		del_timer(&priv->timer);
>> +		priv->timer.expires = 0;
>> +	}
>> +
>> +	can_flush_echo_skb(dev);
>> +}
>> +EXPORT_SYMBOL_GPL(can_close_cleanup);
> 
> You definitely want del_timer_sync() here.  You could potentially return
> with the timer still running.
> 
>> diff --git a/drivers/net/can/sysfs.c b/drivers/net/can/sysfs.c
>> new file mode 100644
>> index 0000000..746f641
>> --- /dev/null
>> +++ b/drivers/net/can/sysfs.c
> 
> Most of this file looks like typical sysfs cruft.  Again, though, I wonder
> about your locking...
> 
>> +/* use same locking rules as GIF* ioctl's */
>> +static ssize_t can_dev_show(struct device *d,
>> +			    struct device_attribute *attr, char *buf,
>> +			    ssize_t (*fmt)(struct net_device *, char *))
>> +{
>> +	struct net_device *dev = to_net_dev(d);
>> +	ssize_t ret = -EINVAL;
>> +
>> +	read_lock(&dev_base_lock);
>> +	if (dev_isalive(dev))
>> +		ret = (*fmt)(dev, buf);
>> +	read_unlock(&dev_base_lock);
>> +
>> +	return ret;
>> +}
> 
> I'm not sure what the GIF* ioctl() locking rules are, but do they really
> give you properly-serialized access to your device?  dev_base_lock seems
> like an unlikely choice, somehow, but maybe I'm missing something.

This locking code is from

http://lxr.linux.no/linux+v2.6.28.6/net/core/net-sysfs.c#L34

accessing SYSFS files for the network device in a similar way.

> [...]
> 
>> +/* use same locking and permission rules as SIF* ioctl's */
>> +static ssize_t can_dev_store(struct device *d, struct device_attribute *attr,
>> +			     const char *buf, size_t len,
>> +			     int (*set)(struct net_device *, unsigned long))
>> +{
>> +	struct net_device *dev = to_net_dev(d);
>> +	unsigned long new;
>> +	int ret = -EINVAL;
>> +
>> +	if (!capable(CAP_NET_ADMIN))
>> +		return -EPERM;
>> +
>> +	ret = strict_strtoul(buf, 0, &new);
>> +	if (ret)
>> +		goto out;
>> +
>> +	rtnl_lock();
>> +	if (dev_isalive(dev)) {
>> +		ret = (*set)(dev, new);
>> +		if (!ret)
>> +			ret = len;
>> +	}
>> +	rtnl_unlock();
>> +out:
>> +	return ret;
>> +}
> 
> Here we're using a different (global) lock?  Me confused...

See http://lxr.linux.no/linux+v2.6.28.6/net/core/net-sysfs.c#L63

> 
> [...]
> 
>> diff --git a/drivers/net/can/sysfs.h b/drivers/net/can/sysfs.h
>> new file mode 100644
>> index 0000000..e21f2fa
>> --- /dev/null
>> +++ b/drivers/net/can/sysfs.h
>> @@ -0,0 +1,24 @@
>> +/*
>> + * Copyright (C) 2007 Wolfgang Grandegger <wg@grandegger.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the version 2 of the GNU General Public License
>> + * as published by the Free Software Foundation
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
>> + */
>> +
>> +#ifndef CAN_SYSFS_H
>> +#define CAN_SYSFS_H
>> +
>> +void can_create_sysfs(struct net_device *dev);
>> +void can_remove_sysfs(struct net_device *dev);
>> +
>> +#endif /* CAN_SYSFS_H */
>> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
>> new file mode 100644
>> index 0000000..078ac03
>> --- /dev/null
>> +++ b/include/linux/can/dev.h
>> @@ -0,0 +1,136 @@
>> +/*
>> + * linux/can/dev.h
>> + *
>> + * Definitions for the CAN network device driver interface
>> + *
>> + * Copyright (C) 2006 Andrey Volkov <avolkov@varma-el.com>
>> + *               Varma Electronics Oy
>> + *
>> + * Copyright (C) 2008 Wolfgang Grandegger <wg@grandegger.com>
> 
> Nothing in 2009? :)

Will fix.

> 
>> + *
>> + * Send feedback to <socketcan-users@lists.berlios.de>
>> + */
>> +
>> +#ifndef CAN_DEV_H
>> +#define CAN_DEV_H
>> +
>> +#include <linux/can/error.h>
>> +
>> +/*
>> + * CAN bitrate and bit-timing
>> + */
>> +struct can_bittiming {
>> +	u32 bitrate;
>> +	u32 sample_point;
>> +	u32 tq;
>> +	u32 prop_seg;
>> +	u32 phase_seg1;
>> +	u32 phase_seg2;
>> +	u32 sjw;
>> +	u32 clock;
>> +	u32 brp;
>> +};
> 
> It sure would be nice (again) to know what all these parameters are.
> 
>> +struct can_bittiming_const {
>> +	u32 tseg1_min;
>> +	u32 tseg1_max;
>> +	u32 tseg2_min;
>> +	u32 tseg2_max;
>> +	u32 sjw_max;
>> +	u32 brp_min;
>> +	u32 brp_max;
>> +	u32 brp_inc;
>> +};
> 
> Ditto.

OK, will add some documentation.

> [...]
> 
>> +/*
>> + * CAN common private data
>> + */
>> +#define CAN_ECHO_SKB_MAX  4
>> +
>> +struct can_priv {
>> +	struct can_device_stats can_stats;
>> +
>> +	struct can_bittiming bittiming;
>> +	struct can_bittiming_const *bittiming_const;
>> +
>> +	spinlock_t irq_lock;
> 
> ...protecting what...?

This is not OK, indeed. It is currently just used by the  SJA1000
driver. Either it can be moved to the SJA1000 private struct or it is
required for some missing locking. I will check.

>> +	enum can_state state;
>> +	u32 ctrlmode;
>> +
>> +	int restart_ms;
>> +	struct timer_list timer;
>> +
>> +	struct sk_buff *echo_skb[CAN_ECHO_SKB_MAX];
>> +
>> +	int (*do_set_bittiming)(struct net_device *dev);
>> +	int (*do_get_state)(struct net_device *dev, enum can_state *state);
>> +	int (*do_set_mode)(struct net_device *dev, enum can_mode mode);
>> +	int (*do_set_ctrlmode)(struct net_device *dev, u32 ctrlmode);
>> +	int (*do_get_ctrlmode)(struct net_device *dev, u32 *ctrlmode);
>> +};
>> +
>> +#define ND2D(_ndev)	(_ndev->dev.parent)
> 
> Does this macro really improve the clarity of the code?

IMO, yes, but a more descriptive name should be used, e.g. NDEV2DEV.

Thanks,

Wolfgang.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Wolfgang Grandegger wrote:
> The CAN network device driver interface provides a generic interface to
> setup, configure and monitor CAN network devices. It exports a set of
> common data structures and functions, which all real CAN network device
> drivers should use. Please have a look to the SJA1000 or MSCAN driver
> to understand how to use them. The name of the module is can-dev.ko.
> 
> Furthermore adds a SYSFS interface to set and get CAN device properties.
> When the CAN device is registered, a set of SYSFS files is created in
> "/sys/class/net/canX/". These files allow to set and get device
> properties like bit-timing parameters, state, controller mode and CAN
> statistics.

Is sysfs really the best choice for this? It has the usual problems,
you can't change multiple parameters at once atomically. Now, I don't
know whether this is actually (and currently) a problem for CAN, but
given that we already have a generic netlink interface for creating
network interfaces that can easily support this, what are the reasons
for adding a new interface instead of using the existing one?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patrick McHardy wrote:
> Wolfgang Grandegger wrote:
>> The CAN network device driver interface provides a generic interface to
>> setup, configure and monitor CAN network devices. It exports a set of
>> common data structures and functions, which all real CAN network device
>> drivers should use. Please have a look to the SJA1000 or MSCAN driver
>> to understand how to use them. The name of the module is can-dev.ko.
>>
>> Furthermore adds a SYSFS interface to set and get CAN device properties.
>> When the CAN device is registered, a set of SYSFS files is created in
>> "/sys/class/net/canX/". These files allow to set and get device
>> properties like bit-timing parameters, state, controller mode and CAN
>> statistics.
> 
> Is sysfs really the best choice for this? It has the usual problems,
> you can't change multiple parameters at once atomically. Now, I don't
> know whether this is actually (and currently) a problem for CAN, but
> given that we already have a generic netlink interface for creating
> network interfaces that can easily support this, what are the reasons
> for adding a new interface instead of using the existing one?

You refer to the API described at http://lwn.net/Articles/236919/, I
assume. Well, I know that netlink is used to define routes in the kernel
or VLANs but I was not aware that it can also be used to configure real
network devices. What a pity. At a first glance, I could also be used
for CAN, of course. There are the following requirements:

1. Set and get CAN device properties like bit-rate and controller mode.
2. Show CAN bus state (active, error-passive or bus-off).
3. Get device constants like clock frequency and bit-timing parameters.
4. Dump CAN device statistics.
5. Trigger device restart if CAN bus-off state has been detected.

Not sure yet, if the interface if good for all purposes. Especially
point 5. might be better handled by an IOCTL. And we need a user space
tool, e.g. canconfig, to handle the user requests and communicate with
the kernel side. We will follow that promising route. Any further help
or link is appreciated. Thanks.

Wolfgang.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Wolfgang Grandegger wrote:
> Patrick McHardy wrote:
>> Is sysfs really the best choice for this? It has the usual problems,
>> you can't change multiple parameters at once atomically. Now, I don't
>> know whether this is actually (and currently) a problem for CAN, but
>> given that we already have a generic netlink interface for creating
>> network interfaces that can easily support this, what are the reasons
>> for adding a new interface instead of using the existing one?
> 
> You refer to the API described at http://lwn.net/Articles/236919/, I
> assume.

Exactly.

> Well, I know that netlink is used to define routes in the kernel
> or VLANs but I was not aware that it can also be used to configure real
> network devices. What a pity. At a first glance, I could also be used
> for CAN, of course. There are the following requirements:
> 
> 1. Set and get CAN device properties like bit-rate and controller mode.
> 2. Show CAN bus state (active, error-passive or bus-off).
> 3. Get device constants like clock frequency and bit-timing parameters.
> 4. Dump CAN device statistics.
> 5. Trigger device restart if CAN bus-off state has been detected.
> 
> Not sure yet, if the interface if good for all purposes. Especially
> point 5. might be better handled by an IOCTL.

The first points sound pretty regular. The last one - just triggering
it is no problem of course, but since I don't know how the detection
works, I can't really tell whether its suitable.

> And we need a user space
> tool, e.g. canconfig, to handle the user requests and communicate with
> the kernel side.

Thats pretty standard :)

> We will follow that promising route. Any further help
> or link is appreciated. Thanks.

It should be quite easy to move your interface on top. A few examples
that might be useful are vlan, ip_gre and macvlan.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patrick McHardy wrote:
> Wolfgang Grandegger wrote:
>> Patrick McHardy wrote:
>>> Is sysfs really the best choice for this? It has the usual problems,
>>> you can't change multiple parameters at once atomically. Now, I don't
>>> know whether this is actually (and currently) a problem for CAN, but
>>> given that we already have a generic netlink interface for creating
>>> network interfaces that can easily support this, what are the reasons
>>> for adding a new interface instead of using the existing one?
>>
>> You refer to the API described at http://lwn.net/Articles/236919/, I
>> assume.
> 
> Exactly.
> 
>> Well, I know that netlink is used to define routes in the kernel
>> or VLANs but I was not aware that it can also be used to configure real
>> network devices. What a pity. At a first glance, I could also be used
>> for CAN, of course. There are the following requirements:
>>
>> 1. Set and get CAN device properties like bit-rate and controller mode.
>> 2. Show CAN bus state (active, error-passive or bus-off).
>> 3. Get device constants like clock frequency and bit-timing parameters.
>> 4. Dump CAN device statistics.
>> 5. Trigger device restart if CAN bus-off state has been detected.
>>
>> Not sure yet, if the interface if good for all purposes. Especially
>> point 5. might be better handled by an IOCTL.
> 
> The first points sound pretty regular. The last one - just triggering
> it is no problem of course, but since I don't know how the detection
> works, I can't really tell whether its suitable.

CAN errors and state changes might be delivered as CAN error messages to
the receiving socket/application, like normal messages. When the
application realizes the state change to CAN bus-off, it may want to
trigger a bus-off recovery (controller restart). The CAN controller HW
enters bus-off, when too much errors occurred on the bus. No more
messages can then be sent of received and therefore the driver calls
netif_carrier_off(). Is there already an interface for the user to
restart? An appropriate IOCTL request would be a simply option, but they
are generally deprecated, AFAIK. Using netlink would be more cumbersome,
as with SYSFS.

>> And we need a user space
>> tool, e.g. canconfig, to handle the user requests and communicate with
>> the kernel side.
> 
> Thats pretty standard :)

For the iproute2 utility "ip", mainliy a netlink_can.c would be
required. But a dedicated tool for CAN seems more appropriate to me.

>> We will follow that promising route. Any further help
>> or link is appreciated. Thanks.
> 
> It should be quite easy to move your interface on top. A few examples
> that might be useful are vlan, ip_gre and macvlan.

Also the drivers dummy and ipb are quite useful. Using netlink seems
straight-forward, indeed, but it requires still some effort. Hope to
find some time soon.

Wolfgang.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Wolfgang Grandegger wrote:
>>> 1. Set and get CAN device properties like bit-rate and controller mode.
>>> 2. Show CAN bus state (active, error-passive or bus-off).
>>> 3. Get device constants like clock frequency and bit-timing parameters.
>>> 4. Dump CAN device statistics.
>>> 5. Trigger device restart if CAN bus-off state has been detected.
>>>
>>> Not sure yet, if the interface if good for all purposes. Especially
>>> point 5. might be better handled by an IOCTL.
>> The first points sound pretty regular. The last one - just triggering
>> it is no problem of course, but since I don't know how the detection
>> works, I can't really tell whether its suitable.
> 
> CAN errors and state changes might be delivered as CAN error messages to
> the receiving socket/application, like normal messages. When the
> application realizes the state change to CAN bus-off, it may want to
> trigger a bus-off recovery (controller restart). The CAN controller HW
> enters bus-off, when too much errors occurred on the bus. No more
> messages can then be sent of received and therefore the driver calls
> netif_carrier_off(). Is there already an interface for the user to
> restart? An appropriate IOCTL request would be a simply option, but they
> are generally deprecated, AFAIK. Using netlink would be more cumbersome,
> as with SYSFS.

You could consider triggering the restart automatically, similar
to the netdev watchdog. In fact, you could simply use that one.
If you really want to trigger this from userspace, setting the
device DOWN and UP again seems like the most appropriate way to
me (even though not very nice to use from an application POV).

>>> And we need a user space
>>> tool, e.g. canconfig, to handle the user requests and communicate with
>>> the kernel side.
>> Thats pretty standard :)
> 
> For the iproute2 utility "ip", mainliy a netlink_can.c would be
> required. But a dedicated tool for CAN seems more appropriate to me.

I don't know the CAN specifics. One goal of the rtnl_link API
was to reduce to amount of userspace tools for link configuration
though, so please only create a new tool if there's really a good
reason. From my POV it would be preferrable to have link configuration
in iproute for direct use by users and possibly also support in
libnl for use by other applications.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patrick McHardy wrote:
> Wolfgang Grandegger wrote:
>>>> 1. Set and get CAN device properties like bit-rate and controller mode.
>>>> 2. Show CAN bus state (active, error-passive or bus-off).
>>>> 3. Get device constants like clock frequency and bit-timing parameters.
>>>> 4. Dump CAN device statistics.
>>>> 5. Trigger device restart if CAN bus-off state has been detected.
>>>>
>>>> Not sure yet, if the interface if good for all purposes. Especially
>>>> point 5. might be better handled by an IOCTL.
>>> The first points sound pretty regular. The last one - just triggering
>>> it is no problem of course, but since I don't know how the detection
>>> works, I can't really tell whether its suitable.
>>
>> CAN errors and state changes might be delivered as CAN error messages to
>> the receiving socket/application, like normal messages. When the
>> application realizes the state change to CAN bus-off, it may want to
>> trigger a bus-off recovery (controller restart). The CAN controller HW
>> enters bus-off, when too much errors occurred on the bus. No more
>> messages can then be sent of received and therefore the driver calls
>> netif_carrier_off(). Is there already an interface for the user to
>> restart? An appropriate IOCTL request would be a simply option, but they
>> are generally deprecated, AFAIK. Using netlink would be more cumbersome,
>> as with SYSFS.
> 
> You could consider triggering the restart automatically, similar
> to the netdev watchdog. In fact, you could simply use that one.

Automatic restart is already support by this patch series but we do not
use the watchdog, because it would trigger on normal transmit timeouts
as well. But maybe we can suppress them by fiddling with the watchdog's
timeout value.

> If you really want to trigger this from userspace, setting the
> device DOWN and UP again seems like the most appropriate way to
> me (even though not very nice to use from an application POV).

That's an option, but it might be a bit heavy, though.

>>>> And we need a user space
>>>> tool, e.g. canconfig, to handle the user requests and communicate with
>>>> the kernel side.
>>> Thats pretty standard :)
>>
>> For the iproute2 utility "ip", mainliy a netlink_can.c would be
>> required. But a dedicated tool for CAN seems more appropriate to me.
> 
> I don't know the CAN specifics. One goal of the rtnl_link API
> was to reduce to amount of userspace tools for link configuration
> though, so please only create a new tool if there's really a good
> reason. From my POV it would be preferrable to have link configuration
> in iproute for direct use by users and possibly also support in
> libnl for use by other applications.

OK, it also depends on what we would like the tool finally to perform,
apart from setting and getting device properties. On idea is to move the
rather heavy calculation of bit-timing parameters out of the kernel to
the user-space configuration tool.

Wolfgang.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html