[PATCH 1/7] can: berr_limit support

[PATCH 1/7] can: berr_limit support

[Marc Kleine-Budde]

Hello,

this series does first some cleanups in can/dev.c, the implementation of three
functions is sorted by IFLA_CAN_*, that we can see at the first look, that all
needed IFLA_CAN_* are handled and that new code can be added at the end. 

Then the bus error limiting (short: berr_limit) infrastructure + netlink
configuration interface is added. Last patch is an example implementation on
the flexcan hardware.

The CAN bus, like the old 10BASE2 Ethernet, needs bus termination. An open CAN
bus doesn't work and will produce lots of CAN bus errors.

If the user wants to detect an open CAN bus, the CAN bus error interrupts have
to be enabled. This is represented by the control mode
CAN_CTRLMODE_BERR_REPORTING.

On an unterminated CAN bus at 500 kbit/s, this can lead to more then 8000
interrupts/s on some SoCs with integrated CAN cores. These interrupts and the
associated processing in software lead to a significant load and may reader the
system unresponsive and even unusable at CAN bus speeds of 1000 kbit/s.

This patch adds the infrastructure to limit these interrupts. The driver has to
implement the do_berr_restart() callback, which re-enables the bus error
interrupts. The idea is to delay the re-enabling of the interrupts after they
have been served. The delay is configured by berr_limit_delay. A value of 0
means interrupts are restarted immediately, any other other value will start a
timer and call do_berr_restart() when the timer fires.

regards,
Marc

[PATCH 1/7] can: dev: fix nlmsg size calculation in can_get_size()

[Marc Kleine-Budde]

This patch fixes the calculation of the nlmsg size, by adding the missing
nla_total_size().

Acked-by: Wolfgang Grandegger <wg@grandegger.com>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index f9cba41..1870c47 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -705,14 +705,14 @@ static size_t can_get_size(const struct net_device *dev)
 	size_t size;
 
 	size = nla_total_size(sizeof(u32));   /* IFLA_CAN_STATE */
-	size += sizeof(struct can_ctrlmode);  /* IFLA_CAN_CTRLMODE */
+	size += nla_total_size(sizeof(struct can_ctrlmode));  /* IFLA_CAN_CTRLMODE */
 	size += nla_total_size(sizeof(u32));  /* IFLA_CAN_RESTART_MS */
-	size += sizeof(struct can_bittiming); /* IFLA_CAN_BITTIMING */
-	size += sizeof(struct can_clock);     /* IFLA_CAN_CLOCK */
+	size += nla_total_size(sizeof(struct can_bittiming)); /* IFLA_CAN_BITTIMING */
+	size += nla_total_size(sizeof(struct can_clock));     /* IFLA_CAN_CLOCK */
 	if (priv->do_get_berr_counter)        /* IFLA_CAN_BERR_COUNTER */
-		size += sizeof(struct can_berr_counter);
+		size += nla_total_size(sizeof(struct can_berr_counter));
 	if (priv->bittiming_const)	      /* IFLA_CAN_BITTIMING_CONST */
-		size += sizeof(struct can_bittiming_const);
+		size += nla_total_size(sizeof(struct can_bittiming_const));
 
 	return size;
 }
-- 
1.8.4.rc3

[PATCH 2/7] can: dev: sort can_get_size() by IFLA_CAN_*

[Marc Kleine-Budde]

This patch sorts the individual addends of the sum by IFLA_CAN_*.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 1870c47..19ebab0 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -702,17 +702,17 @@ static int can_changelink(struct net_device *dev,
 static size_t can_get_size(const struct net_device *dev)
 {
 	struct can_priv *priv = netdev_priv(dev);
-	size_t size;
-
-	size = nla_total_size(sizeof(u32));   /* IFLA_CAN_STATE */
-	size += nla_total_size(sizeof(struct can_ctrlmode));  /* IFLA_CAN_CTRLMODE */
-	size += nla_total_size(sizeof(u32));  /* IFLA_CAN_RESTART_MS */
-	size += nla_total_size(sizeof(struct can_bittiming)); /* IFLA_CAN_BITTIMING */
-	size += nla_total_size(sizeof(struct can_clock));     /* IFLA_CAN_CLOCK */
-	if (priv->do_get_berr_counter)        /* IFLA_CAN_BERR_COUNTER */
-		size += nla_total_size(sizeof(struct can_berr_counter));
-	if (priv->bittiming_const)	      /* IFLA_CAN_BITTIMING_CONST */
+	size_t size = 0;
+
+	size += nla_total_size(sizeof(struct can_bittiming));	/* IFLA_CAN_BITTIMING */
+	if (priv->bittiming_const)				/* IFLA_CAN_BITTIMING_CONST */
 		size += nla_total_size(sizeof(struct can_bittiming_const));
+	size += nla_total_size(sizeof(struct can_clock));	/* IFLA_CAN_CLOCK */
+	size += nla_total_size(sizeof(u32));			/* IFLA_CAN_STATE */
+	size += nla_total_size(sizeof(struct can_ctrlmode));	/* IFLA_CAN_CTRLMODE */
+	size += nla_total_size(sizeof(u32));			/* IFLA_CAN_RESTART_MS */
+	if (priv->do_get_berr_counter)				/* IFLA_CAN_BERR_COUNTER */
+		size += nla_total_size(sizeof(struct can_berr_counter));
 
 	return size;
 }
-- 
1.8.4.rc3

[PATCH 3/7] can: dev: sort can_fill_info() by IFLA_CAN_*

[Marc Kleine-Budde]

This patch sorts the call to nla_put() by IFLA_CAN_*.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 19ebab0..8d6f721 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -726,23 +726,20 @@ static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
 
 	if (priv->do_get_state)
 		priv->do_get_state(dev, &state);
-	if (nla_put_u32(skb, IFLA_CAN_STATE, state) ||
-	    nla_put(skb, IFLA_CAN_CTRLMODE, sizeof(cm), &cm) ||
-	    nla_put_u32(skb, IFLA_CAN_RESTART_MS, priv->restart_ms) ||
-	    nla_put(skb, IFLA_CAN_BITTIMING,
+	if (nla_put(skb, IFLA_CAN_BITTIMING,
 		    sizeof(priv->bittiming), &priv->bittiming) ||
+	    (priv->bittiming_const &&
+	     nla_put(skb, IFLA_CAN_BITTIMING_CONST,
+		     sizeof(*priv->bittiming_const), priv->bittiming_const)) ||
 	    nla_put(skb, IFLA_CAN_CLOCK, sizeof(cm), &priv->clock) ||
+	    nla_put_u32(skb, IFLA_CAN_STATE, state) ||
+	    nla_put(skb, IFLA_CAN_CTRLMODE, sizeof(cm), &cm) ||
+	    nla_put_u32(skb, IFLA_CAN_RESTART_MS, priv->restart_ms) ||
 	    (priv->do_get_berr_counter &&
 	     !priv->do_get_berr_counter(dev, &bec) &&
-	     nla_put(skb, IFLA_CAN_BERR_COUNTER, sizeof(bec), &bec)) ||
-	    (priv->bittiming_const &&
-	     nla_put(skb, IFLA_CAN_BITTIMING_CONST,
-		     sizeof(*priv->bittiming_const), priv->bittiming_const)))
-		goto nla_put_failure;
+	     nla_put(skb, IFLA_CAN_BERR_COUNTER, sizeof(bec), &bec)))
+		return -EMSGSIZE;
 	return 0;
-
-nla_put_failure:
-	return -EMSGSIZE;
 }
 
 static size_t can_get_xstats_size(const struct net_device *dev)
-- 
1.8.4.rc3

[PATCH 4/7] can: dev: sort can_changelink() by IFLA_CAN_*

[Marc Kleine-Budde]

This patch sorts the handling of data[IFLA_CAN_*] by IFLA_CAN_*.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 8d6f721..bda1888 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -645,19 +645,6 @@ static int can_changelink(struct net_device *dev,
 	/* We need synchronization with dev->stop() */
 	ASSERT_RTNL();
 
-	if (data[IFLA_CAN_CTRLMODE]) {
-		struct can_ctrlmode *cm;
-
-		/* Do not allow changing controller mode while running */
-		if (dev->flags & IFF_UP)
-			return -EBUSY;
-		cm = nla_data(data[IFLA_CAN_CTRLMODE]);
-		if (cm->flags & ~priv->ctrlmode_supported)
-			return -EOPNOTSUPP;
-		priv->ctrlmode &= ~cm->mask;
-		priv->ctrlmode |= cm->flags;
-	}
-
 	if (data[IFLA_CAN_BITTIMING]) {
 		struct can_bittiming bt;
 
@@ -680,6 +667,19 @@ static int can_changelink(struct net_device *dev,
 		}
 	}
 
+	if (data[IFLA_CAN_CTRLMODE]) {
+		struct can_ctrlmode *cm;
+
+		/* Do not allow changing controller mode while running */
+		if (dev->flags & IFF_UP)
+			return -EBUSY;
+		cm = nla_data(data[IFLA_CAN_CTRLMODE]);
+		if (cm->flags & ~priv->ctrlmode_supported)
+			return -EOPNOTSUPP;
+		priv->ctrlmode &= ~cm->mask;
+		priv->ctrlmode |= cm->flags;
+	}
+
 	if (data[IFLA_CAN_RESTART_MS]) {
 		/* Do not allow changing restart delay while running */
 		if (dev->flags & IFF_UP)
-- 
1.8.4.rc3

[PATCH 5/7] can: dev: add berr_limit infrastrucutre

[Marc Kleine-Budde]

The CAN bus, like the old 10BASE2 Ethernet, needs bus termination. An open CAN
bus doesn't work and will produce lots of CAN bus errors.

If the user wants to detect an open CAN bus, the CAN bus error interrupts have
to be enabled. This is represented by the control mode
CAN_CTRLMODE_BERR_REPORTING.

On an unterminated CAN bus at 500 kbit/s, this can lead to more then 8000
interrupts/s on some SoCs with integrated CAN cores. These interrupts and the
associated processing in software lead to a significant load and may reader the
system unresponsive and even unusable at CAN bus speeds of 1000 kbit/s.

This patch adds the infrastructure to limit these interrupts. The driver has to
implement the do_berr_restart() callback, which re-enables the bus error
interrupts. The idea is to delay the re-enabling of the interrupts after they
have been served. The delay is configured by berr_limit_delay. A value of 0
means interrupts are restarted immediately, any other other value will start a
timer and call do_berr_restart() when the timer fires.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev.c   | 26 ++++++++++++++++++++++++++
 include/linux/can/dev.h |  5 +++++
 2 files changed, 31 insertions(+)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index bda1888..969d3cd 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -486,6 +486,29 @@ void can_bus_off(struct net_device *dev)
 }
 EXPORT_SYMBOL_GPL(can_bus_off);
 
+static void can_berr_restart(unsigned long data)
+{
+	struct net_device *dev = (struct net_device *)data;
+	struct can_priv *priv = netdev_priv(dev);
+
+	netdev_dbg(dev, "berr-restart\n");
+	priv->do_berr_restart(dev);
+}
+
+void can_berr_limit(struct net_device *dev)
+{
+	struct can_priv *priv = netdev_priv(dev);
+
+	if (priv->berr_limit_delay) {
+		netdev_dbg(dev, "berr-limit\n");
+		mod_timer(&priv->berr_limit_timer,
+			  jiffies + priv->berr_limit_delay);
+	} else {
+		priv->do_berr_restart(dev);
+	}
+}
+EXPORT_SYMBOL_GPL(can_berr_limit);
+
 static void can_setup(struct net_device *dev)
 {
 	dev->type = ARPHRD_CAN;
@@ -567,6 +590,7 @@ struct net_device *alloc_candev(int sizeof_priv, unsigned int echo_skb_max)
 	priv->state = CAN_STATE_STOPPED;
 
 	init_timer(&priv->restart_timer);
+	init_timer(&priv->berr_limit_timer);
 
 	return dev;
 }
@@ -601,6 +625,7 @@ int open_candev(struct net_device *dev)
 		netif_carrier_on(dev);
 
 	setup_timer(&priv->restart_timer, can_restart, (unsigned long)dev);
+	setup_timer(&priv->berr_limit_timer, can_berr_restart, (unsigned long)dev);
 
 	return 0;
 }
@@ -616,6 +641,7 @@ void close_candev(struct net_device *dev)
 {
 	struct can_priv *priv = netdev_priv(dev);
 
+	del_timer_sync(&priv->berr_limit_timer);
 	del_timer_sync(&priv->restart_timer);
 	can_flush_echo_skb(dev);
 }
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index fb0ab65..9162baa 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -44,12 +44,16 @@ struct can_priv {
 	int restart_ms;
 	struct timer_list restart_timer;
 
+	unsigned long berr_limit_delay;		/* in jiffies */
+	struct timer_list berr_limit_timer;
+
 	int (*do_set_bittiming)(struct net_device *dev);
 	int (*do_set_mode)(struct net_device *dev, enum can_mode mode);
 	int (*do_get_state)(const struct net_device *dev,
 			    enum can_state *state);
 	int (*do_get_berr_counter)(const struct net_device *dev,
 				   struct can_berr_counter *bec);
+	void (*do_berr_restart)(const struct net_device *dev);
 
 	unsigned int echo_skb_max;
 	struct sk_buff **echo_skb;
@@ -117,6 +121,7 @@ void unregister_candev(struct net_device *dev);
 
 int can_restart_now(struct net_device *dev);
 void can_bus_off(struct net_device *dev);
+void can_berr_limit(struct net_device *dev);
 
 void can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
 		      unsigned int idx);
-- 
1.8.4.rc3

Re: [PATCH 5/7] can: dev: add berr_limit infrastrucutre

[Alexander Stein]

Hello Marc,

On Monday 07 October 2013 16:40:38, Marc Kleine-Budde wrote:
> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> index bda1888..969d3cd 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
> @@ -486,6 +486,29 @@ void can_bus_off(struct net_device *dev)
>  }
>  EXPORT_SYMBOL_GPL(can_bus_off);
>  
> +static void can_berr_restart(unsigned long data)
> +{
> +	struct net_device *dev = (struct net_device *)data;
> +	struct can_priv *priv = netdev_priv(dev);
> +
> +	netdev_dbg(dev, "berr-restart\n");
> +	priv->do_berr_restart(dev);
     ^^^^^^^^^^^^^^^^^^^^^^^^
see below!

> +}
> +
> +void can_berr_limit(struct net_device *dev)
> +{
> +	struct can_priv *priv = netdev_priv(dev);
> +
> +	if (priv->berr_limit_delay) {
> +		netdev_dbg(dev, "berr-limit\n");
> +		mod_timer(&priv->berr_limit_timer,
> +			  jiffies + priv->berr_limit_delay);
> +	} else {
> +		priv->do_berr_restart(dev);
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^
Here you are silently requiring that do_berr_restart is non-NULL. I know that the driver has to properly set this when using can_berr_limit, but it might seem confusing.

Regards,
Alexander

Re: [PATCH 5/7] can: dev: add berr_limit infrastrucutre

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2128 bytes --]

On 10/07/2013 05:39 PM, Alexander Stein wrote:
> Hello Marc,
> 
> On Monday 07 October 2013 16:40:38, Marc Kleine-Budde wrote:
>> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
>> index bda1888..969d3cd 100644
>> --- a/drivers/net/can/dev.c
>> +++ b/drivers/net/can/dev.c
>> @@ -486,6 +486,29 @@ void can_bus_off(struct net_device *dev)
>>  }
>>  EXPORT_SYMBOL_GPL(can_bus_off);
>>  
>> +static void can_berr_restart(unsigned long data)
>> +{
>> +	struct net_device *dev = (struct net_device *)data;
>> +	struct can_priv *priv = netdev_priv(dev);
>> +
>> +	netdev_dbg(dev, "berr-restart\n");
>> +	priv->do_berr_restart(dev);
>      ^^^^^^^^^^^^^^^^^^^^^^^^
> see below!
> 
>> +}
>> +
>> +void can_berr_limit(struct net_device *dev)
>> +{
>> +	struct can_priv *priv = netdev_priv(dev);
>> +
>> +	if (priv->berr_limit_delay) {
>> +		netdev_dbg(dev, "berr-limit\n");
>> +		mod_timer(&priv->berr_limit_timer,
>> +			  jiffies + priv->berr_limit_delay);
>> +	} else {
>> +		priv->do_berr_restart(dev);
>          ^^^^^^^^^^^^^^^^^^^^^^^^^^^

> Here you are silently requiring that do_berr_restart is non-NULL. I
> know that the driver has to properly set this when using
> can_berr_limit, but it might seem confusing.

If the driver wants to make use of this feature, it has to setup this
callback. If I add NULL pointer checks, the kernel wouldn't blow up, but
the driver doesn't work any more.

If this callback isn't setup, the userspace cannot activate the feature
via netlink.

Maybe we can define a control mode that the driver supports this feature
and during can_open() it's checked if the callback is != NULL. Than
"ifconfig up" will fail, the driver doesn't work, same as a NULL pointer
deref later, but coated in sugar :) Maybe we find a better way.

thanks for the review,
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [PATCH 5/7] can: dev: add berr_limit infrastrucutre

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2128 bytes --]

On 10/07/2013 05:56 PM, Marc Kleine-Budde wrote:
> On 10/07/2013 05:39 PM, Alexander Stein wrote:
>> Hello Marc,
>>
>> On Monday 07 October 2013 16:40:38, Marc Kleine-Budde wrote:
>>> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
>>> index bda1888..969d3cd 100644
>>> --- a/drivers/net/can/dev.c
>>> +++ b/drivers/net/can/dev.c
>>> @@ -486,6 +486,29 @@ void can_bus_off(struct net_device *dev)
>>>  }
>>>  EXPORT_SYMBOL_GPL(can_bus_off);
>>>  
>>> +static void can_berr_restart(unsigned long data)
>>> +{
>>> +	struct net_device *dev = (struct net_device *)data;
>>> +	struct can_priv *priv = netdev_priv(dev);
>>> +
>>> +	netdev_dbg(dev, "berr-restart\n");
>>> +	priv->do_berr_restart(dev);
>>      ^^^^^^^^^^^^^^^^^^^^^^^^
>> see below!
>>
>>> +}
>>> +
>>> +void can_berr_limit(struct net_device *dev)
>>> +{
>>> +	struct can_priv *priv = netdev_priv(dev);
>>> +
>>> +	if (priv->berr_limit_delay) {
>>> +		netdev_dbg(dev, "berr-limit\n");
>>> +		mod_timer(&priv->berr_limit_timer,
>>> +			  jiffies + priv->berr_limit_delay);
>>> +	} else {
>>> +		priv->do_berr_restart(dev);
>>          ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
>> Here you are silently requiring that do_berr_restart is non-NULL. I
>> know that the driver has to properly set this when using
>> can_berr_limit, but it might seem confusing.
> 
> If the driver wants to make use of this feature, it has to setup this
> callback. If I add NULL pointer checks, the kernel wouldn't blow up, but
> the driver doesn't work any more.

I mean, if the driver doesn't setup the callback and there is a NULL
pointer checks in both functions, the driver doesn't work properly,
because the bus error interrupts will stay disabled and you might not
notice it, because there isn't any big sign of failure (like the NULL
pointer deref).

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [PATCH 5/7] can: dev: add berr_limit infrastrucutre

[Alexander Stein]

On Monday 07 October 2013 18:00:45, Marc Kleine-Budde wrote:
> >>> +void can_berr_limit(struct net_device *dev)
> >>> +{
> >>> +	struct can_priv *priv = netdev_priv(dev);
> >>> +
> >>> +	if (priv->berr_limit_delay) {
> >>> +		netdev_dbg(dev, "berr-limit\n");
> >>> +		mod_timer(&priv->berr_limit_timer,
> >>> +			  jiffies + priv->berr_limit_delay);
> >>> +	} else {
> >>> +		priv->do_berr_restart(dev);
> >>          ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> >> Here you are silently requiring that do_berr_restart is non-NULL. I
> >> know that the driver has to properly set this when using
> >> can_berr_limit, but it might seem confusing.
> > 
> > If the driver wants to make use of this feature, it has to setup this
> > callback. If I add NULL pointer checks, the kernel wouldn't blow up, but
> > the driver doesn't work any more.
> 
> I mean, if the driver doesn't setup the callback and there is a NULL
> pointer checks in both functions, the driver doesn't work properly,
> because the bus error interrupts will stay disabled and you might not
> notice it, because there isn't any big sign of failure (like the NULL
> pointer deref).

Mh, what would be the effect if a driver doesn't setup the callback and still uses can_berr_limit? The NULL dereference raises and OOPS and what then? The corresponding application gets killed? Well if the system as a whole still works and even better then faulty driver can be unloaded again, this might be suitable.

Best regards,
Alexander

Re: [PATCH 5/7] can: dev: add berr_limit infrastrucutre

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1991 bytes --]

On 10/08/2013 08:03 AM, Alexander Stein wrote:
> On Monday 07 October 2013 18:00:45, Marc Kleine-Budde wrote:
>>>>> +void can_berr_limit(struct net_device *dev)
>>>>> +{
>>>>> +	struct can_priv *priv = netdev_priv(dev);
>>>>> +
>>>>> +	if (priv->berr_limit_delay) {
>>>>> +		netdev_dbg(dev, "berr-limit\n");
>>>>> +		mod_timer(&priv->berr_limit_timer,
>>>>> +			  jiffies + priv->berr_limit_delay);
>>>>> +	} else {
>>>>> +		priv->do_berr_restart(dev);
>>>>          ^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>
>>>> Here you are silently requiring that do_berr_restart is non-NULL. I
>>>> know that the driver has to properly set this when using
>>>> can_berr_limit, but it might seem confusing.
>>>
>>> If the driver wants to make use of this feature, it has to setup this
>>> callback. If I add NULL pointer checks, the kernel wouldn't blow up, but
>>> the driver doesn't work any more.
>>
>> I mean, if the driver doesn't setup the callback and there is a NULL
>> pointer checks in both functions, the driver doesn't work properly,
>> because the bus error interrupts will stay disabled and you might not
>> notice it, because there isn't any big sign of failure (like the NULL
>> pointer deref).
> 
> Mh, what would be the effect if a driver doesn't setup the callback
> and still uses can_berr_limit? The NULL dereference raises and OOPS
> and what then? The corresponding application gets killed? Well if the
> system as a whole still works and even better then faulty driver can
> be unloaded again, this might be suitable.

It makes no sense to use can_berr_limit() without implementing the
callback. So it makes no sense from my point of view to add a NULL
pointer check.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

[PATCH 6/7] can: dev: berr_limit netlink support for configuration

[Marc Kleine-Budde]

This patch adds add netlink support to configure the berr_limit delay via
netlink.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev.c            | 19 ++++++++++++++++++-
 include/uapi/linux/can/netlink.h |  1 +
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 969d3cd..3119052 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -660,6 +660,7 @@ static const struct nla_policy can_policy[IFLA_CAN_MAX + 1] = {
 				= { .len = sizeof(struct can_bittiming_const) },
 	[IFLA_CAN_CLOCK]	= { .len = sizeof(struct can_clock) },
 	[IFLA_CAN_BERR_COUNTER]	= { .len = sizeof(struct can_berr_counter) },
+	[IFLA_CAN_BERR_LIMIT_MS]= { .type = NLA_MSECS },
 };
 
 static int can_changelink(struct net_device *dev,
@@ -722,6 +723,17 @@ static int can_changelink(struct net_device *dev,
 			return err;
 	}
 
+	if (data[IFLA_CAN_BERR_LIMIT_MS]) {
+		/* Do not allow changing berr limit delay while running */
+		if (!priv->do_berr_restart)
+			return -EOPNOTSUPP;
+		if (dev->flags & IFF_UP)
+			return -EBUSY;
+
+		priv->berr_limit_delay =
+			nla_get_msecs(data[IFLA_CAN_BERR_LIMIT_MS]);
+	}
+
 	return 0;
 }
 
@@ -739,6 +751,8 @@ static size_t can_get_size(const struct net_device *dev)
 	size += nla_total_size(sizeof(u32));			/* IFLA_CAN_RESTART_MS */
 	if (priv->do_get_berr_counter)				/* IFLA_CAN_BERR_COUNTER */
 		size += nla_total_size(sizeof(struct can_berr_counter));
+	if (priv->do_berr_restart)				/* IFLA_CAN_BERR_LIMIT_MS */
+		size += nla_total_size(sizeof(u64));
 
 	return size;
 }
@@ -763,7 +777,10 @@ static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
 	    nla_put_u32(skb, IFLA_CAN_RESTART_MS, priv->restart_ms) ||
 	    (priv->do_get_berr_counter &&
 	     !priv->do_get_berr_counter(dev, &bec) &&
-	     nla_put(skb, IFLA_CAN_BERR_COUNTER, sizeof(bec), &bec)))
+	     nla_put(skb, IFLA_CAN_BERR_COUNTER, sizeof(bec), &bec)) ||
+	    (priv->do_berr_restart &&
+	     nla_put_msecs(skb, IFLA_CAN_BERR_LIMIT_MS,
+			   priv->berr_limit_delay)))
 		return -EMSGSIZE;
 	return 0;
 }
diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
index df944ed..71f750e 100644
--- a/include/uapi/linux/can/netlink.h
+++ b/include/uapi/linux/can/netlink.h
@@ -122,6 +122,7 @@ enum {
 	IFLA_CAN_RESTART_MS,
 	IFLA_CAN_RESTART,
 	IFLA_CAN_BERR_COUNTER,
+	IFLA_CAN_BERR_LIMIT_MS,
 	__IFLA_CAN_MAX
 };
 
-- 
1.8.4.rc3

[PATCH 7/7] can: flexcan: add berr_limit support

[Marc Kleine-Budde]

This patch adds berr_limit support to the flexcan driver.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/flexcan.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index df010d6..0a4a7a2 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -301,6 +301,14 @@ static int flexcan_get_berr_counter(const struct net_device *dev,
 	return 0;
 }
 
+static void flexcan_berr_restart(const struct net_device *dev)
+{
+	const struct flexcan_priv *priv = netdev_priv(dev);
+	struct flexcan_regs __iomem *regs = priv->base;
+
+	flexcan_write(priv->reg_ctrl_default, &regs->ctrl);
+}
+
 static int flexcan_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	const struct flexcan_priv *priv = netdev_priv(dev);
@@ -594,7 +602,7 @@ static int flexcan_poll(struct napi_struct *napi, int quota)
 		napi_complete(napi);
 		/* enable IRQs */
 		flexcan_write(FLEXCAN_IFLAG_DEFAULT, &regs->imask1);
-		flexcan_write(priv->reg_ctrl_default, &regs->ctrl);
+		can_berr_limit(dev);
 	}
 
 	return work_done;
@@ -1055,6 +1063,7 @@ static int flexcan_probe(struct platform_device *pdev)
 	priv->can.bittiming_const = &flexcan_bittiming_const;
 	priv->can.do_set_mode = flexcan_set_mode;
 	priv->can.do_get_berr_counter = flexcan_get_berr_counter;
+	priv->can.do_berr_restart = flexcan_berr_restart;
 	priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
 		CAN_CTRLMODE_LISTENONLY	| CAN_CTRLMODE_3_SAMPLES |
 		CAN_CTRLMODE_BERR_REPORTING;
-- 
1.8.4.rc3

Re: [PATCH 1/7] can: berr_limit support

[Wolfgang Grandegger]

Hi Marc,

On 10/07/2013 04:40 PM, Marc Kleine-Budde wrote:
> Hello,
> 
> this series does first some cleanups in can/dev.c, the implementation of three
> functions is sorted by IFLA_CAN_*, that we can see at the first look, that all
> needed IFLA_CAN_* are handled and that new code can be added at the end. 
> 
> Then the bus error limiting (short: berr_limit) infrastructure + netlink
> configuration interface is added. Last patch is an example implementation on
> the flexcan hardware.
> 
> The CAN bus, like the old 10BASE2 Ethernet, needs bus termination. An open CAN
> bus doesn't work and will produce lots of CAN bus errors.
> 
> If the user wants to detect an open CAN bus, the CAN bus error interrupts have
> to be enabled. This is represented by the control mode
> CAN_CTRLMODE_BERR_REPORTING.
> 
> On an unterminated CAN bus at 500 kbit/s, this can lead to more then 8000
> interrupts/s on some SoCs with integrated CAN cores. These interrupts and the
> associated processing in software lead to a significant load and may reader the
> system unresponsive and even unusable at CAN bus speeds of 1000 kbit/s.

And there might be more than just one interface.

> This patch adds the infrastructure to limit these interrupts. The driver has to
> implement the do_berr_restart() callback, which re-enables the bus error
> interrupts. The idea is to delay the re-enabling of the interrupts after they
> have been served. The delay is configured by berr_limit_delay. A value of 0
> means interrupts are restarted immediately, any other other value will start a
> timer and call do_berr_restart() when the timer fires.

IIRC, some versions of the Flexcan controller need bus error interupts
enabled otherwise state changes are not realized. This makes an
efficient support for CAN_CTRLMODE_BERR_REPORTING impossible, which can
normally be achieved by simply masking the interrupt source. Does state
change reporting still work properly with this kind of interrupt
throttling? Apart from that the approach looks good.

Wolfgang.

Re: [PATCH 1/7] can: berr_limit support

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2619 bytes --]

On 10/07/2013 09:38 PM, Wolfgang Grandegger wrote:
> Hi Marc,
> 
> On 10/07/2013 04:40 PM, Marc Kleine-Budde wrote:
>> Hello,
>>
>> this series does first some cleanups in can/dev.c, the implementation of three
>> functions is sorted by IFLA_CAN_*, that we can see at the first look, that all
>> needed IFLA_CAN_* are handled and that new code can be added at the end. 
>>
>> Then the bus error limiting (short: berr_limit) infrastructure + netlink
>> configuration interface is added. Last patch is an example implementation on
>> the flexcan hardware.
>>
>> The CAN bus, like the old 10BASE2 Ethernet, needs bus termination. An open CAN
>> bus doesn't work and will produce lots of CAN bus errors.
>>
>> If the user wants to detect an open CAN bus, the CAN bus error interrupts have
>> to be enabled. This is represented by the control mode
>> CAN_CTRLMODE_BERR_REPORTING.
>>
>> On an unterminated CAN bus at 500 kbit/s, this can lead to more then 8000
>> interrupts/s on some SoCs with integrated CAN cores. These interrupts and the
>> associated processing in software lead to a significant load and may reader the
>> system unresponsive and even unusable at CAN bus speeds of 1000 kbit/s.
> 
> And there might be more than just one interface.

Which makes it even worse.

>> This patch adds the infrastructure to limit these interrupts. The driver has to
>> implement the do_berr_restart() callback, which re-enables the bus error
>> interrupts. The idea is to delay the re-enabling of the interrupts after they
>> have been served. The delay is configured by berr_limit_delay. A value of 0
>> means interrupts are restarted immediately, any other other value will start a
>> timer and call do_berr_restart() when the timer fires.
> 
> IIRC, some versions of the Flexcan controller need bus error interupts
> enabled otherwise state changes are not realized. This makes an
> efficient support for CAN_CTRLMODE_BERR_REPORTING impossible, which can
> normally be achieved by simply masking the interrupt source. Does state
> change reporting still work properly with this kind of interrupt
> throttling? Apart from that the approach looks good.

I'll do some tests with another imx system. I think the state change
will be delayed until the bus error interrupts will be enabled again.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]