[PATCH v5 3/5] can: mscan: Consolidate and unify state change handling

[PATCH v5 3/5] can: mscan: Consolidate and unify state change handling

[Andri Yngvason]

Replacing error state change handling with the new mechanism.

Signed-off-by: Andri Yngvason <andri.yngvason@marel.com>
---
Changes made since last proposal: None.

 drivers/net/can/mscan/mscan.c | 48 +++++++++++++------------------------------
 1 file changed, 14 insertions(+), 34 deletions(-)

diff --git a/drivers/net/can/mscan/mscan.c b/drivers/net/can/mscan/mscan.c
index e0c9be5..e36b740 100644
--- a/drivers/net/can/mscan/mscan.c
+++ b/drivers/net/can/mscan/mscan.c
@@ -289,18 +289,15 @@ static netdev_tx_t mscan_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	return NETDEV_TX_OK;
 }
 
-/* This function returns the old state to see where we came from */
-static enum can_state check_set_state(struct net_device *dev, u8 canrflg)
+static enum can_state get_new_state(struct net_device *dev, u8 canrflg)
 {
 	struct mscan_priv *priv = netdev_priv(dev);
-	enum can_state state, old_state = priv->can.state;
 
-	if (canrflg & MSCAN_CSCIF && old_state <= CAN_STATE_BUS_OFF) {
-		state = state_map[max(MSCAN_STATE_RX(canrflg),
-				      MSCAN_STATE_TX(canrflg))];
-		priv->can.state = state;
-	}
-	return old_state;
+	if (unlikely(canrflg & MSCAN_CSCIF))
+		return state_map[max(MSCAN_STATE_RX(canrflg),
+				 MSCAN_STATE_TX(canrflg))];
+
+	return priv->can.state;
 }
 
 static void mscan_get_rx_frame(struct net_device *dev, struct can_frame *frame)
@@ -349,7 +346,7 @@ static void mscan_get_err_frame(struct net_device *dev, struct can_frame *frame,
 	struct mscan_priv *priv = netdev_priv(dev);
 	struct mscan_regs __iomem *regs = priv->reg_base;
 	struct net_device_stats *stats = &dev->stats;
-	enum can_state old_state;
+	enum can_state new_state;
 
 	netdev_dbg(dev, "error interrupt (canrflg=%#x)\n", canrflg);
 	frame->can_id = CAN_ERR_FLAG;
@@ -363,27 +360,13 @@ static void mscan_get_err_frame(struct net_device *dev, struct can_frame *frame,
 		frame->data[1] = 0;
 	}
 
-	old_state = check_set_state(dev, canrflg);
-	/* State changed */
-	if (old_state != priv->can.state) {
-		switch (priv->can.state) {
-		case CAN_STATE_ERROR_WARNING:
-			frame->can_id |= CAN_ERR_CRTL;
-			priv->can.can_stats.error_warning++;
-			if ((priv->shadow_statflg & MSCAN_RSTAT_MSK) <
-			    (canrflg & MSCAN_RSTAT_MSK))
-				frame->data[1] |= CAN_ERR_CRTL_RX_WARNING;
-			if ((priv->shadow_statflg & MSCAN_TSTAT_MSK) <
-			    (canrflg & MSCAN_TSTAT_MSK))
-				frame->data[1] |= CAN_ERR_CRTL_TX_WARNING;
-			break;
-		case CAN_STATE_ERROR_PASSIVE:
-			frame->can_id |= CAN_ERR_CRTL;
-			priv->can.can_stats.error_passive++;
-			frame->data[1] |= CAN_ERR_CRTL_RX_PASSIVE;
-			break;
-		case CAN_STATE_BUS_OFF:
-			frame->can_id |= CAN_ERR_BUSOFF;
+	new_state = get_new_state(dev, canrflg);
+	if (new_state != priv->can.state) {
+		can_change_state(dev, frame,
+				 state_map[MSCAN_STATE_TX(canrflg)],
+				 state_map[MSCAN_STATE_RX(canrflg)]);
+
+		if (priv->can.state == CAN_STATE_BUS_OFF) {
 			/*
 			 * The MSCAN on the MPC5200 does recover from bus-off
 			 * automatically. To avoid that we stop the chip doing
@@ -396,9 +379,6 @@ static void mscan_get_err_frame(struct net_device *dev, struct can_frame *frame,
 					 MSCAN_SLPRQ | MSCAN_INITRQ);
 			}
 			can_bus_off(dev);
-			break;
-		default:
-			break;
 		}
 	}
 	priv->shadow_statflg = canrflg & MSCAN_STAT_MSK;
-- 
1.9.1

c_can: (newbie) high system load when frame not acked?

[Viktor Babrian]

Hi,

I am new to linux with can though I have had some experience with both 
separately.

I have here an am3359 board (having DCAN CAN controller). CAN transciever 
is terminated by a 60R resistor, no other party connected (no cables etc). 
I set up a can transmission using cansend:
# ip link set can1 type can bitrate 500000 triple-sampling on
# ip link set can1 up
# cansend can1 5A1#11.2233.44556677.88

Oscilloscope probe is on canh.

Everything works just fine, except:
- Frame is retransmitted for ever (seen on bus)
- user experience drops (serial terminal hardly echoes)
- load is up
- top shows sirq level of 75-80%

after bringing down can1 with
# ip link set can1 down,
I see that load is fine again (=0), however frame retransmission is going 
on still (observed by oscilloscope).
Transmission stops only after enabling can1 again:
# ip link set can1 up

Is this expected? Has anybody seen similar behavior?

I am using linux kernel 3.14.26. I have also tried c_can driver from 
3.19rc3 ("backported"=copied), same results.

Thanks in advance,
Viktor babrian

Re: c_can: (newbie) high system load when frame not acked?

[Viktor Babrian]

Hi there,

thanks for the quick answer
> please check http://www.can-wiki.info/doku.php?id=can_faq:problem_solving_questions
checks out, thanks

> Yes, retransmission is what the CAN controller always does if no other node is connected and gives a ACK to a correct received message

my main concern is the high system load it causes

Regards,
Viktor

Re: c_can: (newbie) high system load when frame not acked?

[Tom Evans]

On 13/01/15 05:50, Viktor Babrian wrote:
> Hi there,
>
> thanks for the quick answer
>> please check
>> http://www.can-wiki.info/doku.php?id=can_faq:problem_solving_questions
> checks out, thanks
>
>> Yes, retransmission is what the CAN controller always does if no other node
>> is connected and gives a ACK to a correct received message
>
> my main concern is the high system load it causes

You're using a "am3359 board (having DCAN CAN controller)". I'm not familiar 
with that chip.

 > # ip link set can1 type can bitrate 500000 triple-sampling on

Does it have a "berr-reporting" option, can you turn it off and does it make 
it any better?

Searching ... you're probably using:

http://lxr.free-electrons.com/source/drivers/net/can/c_can/c_can.c

It has "CAN_CTRLMODE_BERR_REPORTING", but turning that off doesn't prevent the 
interrupts. It just prevents it from calling "netif_receive_skb(skb)" with the 
error information from the poll. It configures the interrupts this way:

     /* enable status change, error and module interrupts */
     #define CONTROL_IRQMSK  (CONTROL_EIE | CONTROL_IE | CONTROL_SIE)

Maybe you can turn the error interrupts off and still have it function. Maybe 
it won't (may be required for bus off recovery).

I'm familiar with the problems with Freescale FlexCAN controllers. They can't 
generate interrupts on CAN state-changes, so the driver writers enable *ALL* 
error interrupts so as to track the state changes. Not many people use or need 
this facility.

The consequence (depending on the CPU and Linux build options) is 10%-100% CPU 
utilisation when there's nothing to ACK the packet, and a higher figure again 
when the bus isn't terminated.

CAN is meant to be used in cars or factories. Running with only one node or a 
disconnected bus is not a "normal" condition, so the hardware and drivers 
don't handle those conditions well.

I'd recommend running a shell script that periodically monitors the CAN 
interrupt rate (or opens an error socket and monitors the error rate) and if 
it detects this situation to just disables the CAN bus for a while or until 
the user fixes the problem.

If you read through the following it might give you enough understanding to 
change your driver, or find another solution.

https://community.freescale.com/thread/333839

Read my final post in that thread.

Search the linux-can list for the series of posts titled "[PATCH] Consolidate 
and unify state change handling" starting 2014-10-20:

http://article.gmane.org/gmane.linux.can/7165

There have also been proposed patches to recognise when there's a "flood" of 
error interrupts and to limit the rate, but possibly not for your driver.

Tom

Re: c_can: (newbie) high system load when frame not acked?

[Viktor Babrian]

> Maybe you can turn the error interrupts off and still have it function. Maybe 
> it won't (may be required for bus off recovery).

Thanks for the detailed answer. I read the pages and took a look into the 
code.

Regarding state-change behavior, DCAN controller is similar to the one in 
i.MX6. (It has explicit interrupt source for Act->Warn and Pass->BOFF 
transitions).
Although as I understand CAN, you have to have a successful RX or TX event 
to have the counters decrease. That  means that you will have an immediate 
report (i.e. interrupt) on Warn->Active, Pass->Warn and BOFF->Active. As 
far as I see, this latter is explicitly handled by upper layers so driver 
will have to know it anyway.
This means that if I turn off bus error monitoring, the only event I will 
not get immediate report of is Warn->Pass. (Pass->Warn can be reported on 
successful message events if the driver is implemented accordingly. 
Currently it isn't). However it could be reported later upon the first 
successful message event. I beleive missing an accurate report of 
active->passive state change is far better than having a non-responding 
system in the field in scenarios that do happen.

Anyway I modified the code so that state change interrupt is not 
enabled when user sets berr-reporting off (as you kindly suggested). It 
solves the load issue. Bus-off recovery behavior did not change (I made a 
few tests only). Btw the driver disables all interrupts in bus off so I 
guess it does not matter what exact flags were set.

> The consequence (depending on the CPU and Linux build options) is 10%-100% 
> CPU utilisation when there's nothing to ACK the packet, and a higher figure 
> again when the bus isn't terminated.
>
> CAN is meant to be used in cars or factories. Running with only one node or a 
> disconnected bus is not a "normal" condition, so the hardware and drivers 
> don't handle those conditions well.
Well error states are defined by the spec so they ARE normal. What is not 
normal is to bring down system performace when they happen.


I alose added a line to put the controller in init mode in 
c_can_stop() so that bringing the bus down will actually cause the 
controller stop transmitting. I hope it's the way to do this, it would be 
nice if some expert could verify this.

Thanks,
Best Regards,
Viktor

Re: c_can: (newbie) high system load when frame not acked?

[Marc Kleine-Budde]

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

On 01/13/2015 04:10 PM, Viktor Babrian wrote:
>> Maybe you can turn the error interrupts off and still have it
>> function. Maybe it won't (may be required for bus off recovery).
> 
> Thanks for the detailed answer. I read the pages and took a look into
> the code.
> 
> Regarding state-change behavior, DCAN controller is similar to the one
> in i.MX6. (It has explicit interrupt source for Act->Warn and Pass->BOFF
> transitions).
> Although as I understand CAN, you have to have a successful RX or TX
> event to have the counters decrease. That  means that you will have an
> immediate report (i.e. interrupt) on Warn->Active, Pass->Warn and
> BOFF->Active. As far as I see, this latter is explicitly handled by
> upper layers so driver will have to know it anyway.
> This means that if I turn off bus error monitoring, the only event I
> will not get immediate report of is Warn->Pass. (Pass->Warn can be
> reported on successful message events if the driver is implemented
> accordingly. Currently it isn't). However it could be reported later
> upon the first successful message event. I beleive missing an accurate
> report of active->passive state change is far better than having a
> non-responding system in the field in scenarios that do happen.
> 
> Anyway I modified the code so that state change interrupt is not enabled
> when user sets berr-reporting off (as you kindly suggested). It solves
> the load issue. Bus-off recovery behavior did not change (I made a few
> tests only). Btw the driver disables all interrupts in bus off so I
> guess it does not matter what exact flags were set.
> 
>> The consequence (depending on the CPU and Linux build options) is
>> 10%-100% CPU utilisation when there's nothing to ACK the packet, and a
>> higher figure again when the bus isn't terminated.
>>
>> CAN is meant to be used in cars or factories. Running with only one
>> node or a disconnected bus is not a "normal" condition, so the
>> hardware and drivers don't handle those conditions well.
> Well error states are defined by the spec so they ARE normal. What is
> not normal is to bring down system performace when they happen.
> 
> 
> I alose added a line to put the controller in init mode in c_can_stop()
> so that bringing the bus down will actually cause the controller stop
> transmitting. I hope it's the way to do this, it would be nice if some
> expert could verify this.

Can you send patches?

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: 819 bytes --]

Re: c_can: (newbie) high system load when frame not acked?

[Viktor Babrian]

> Can you send patches?

Sure, which version shall I diff it to? (where do I get them?) I currently 
work with the one that comes in 3.19-rc3. The changes are 3 lines though.

Viktor

Re: c_can: (newbie) high system load when frame not acked?

[Marc Kleine-Budde]

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

On 01/13/2015 04:54 PM, Viktor Babrian wrote:
>> Can you send patches?
> 
> Sure, which version shall I diff it to? (where do I get them?) I
> currently work with the one that comes in 3.19-rc3. The changes are 3
> lines though.

3.19-rc3 is probably okay...

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: 819 bytes --]

Re: c_can: (newbie) high system load when frame not acked?

[Andri Yngvason]

Quoting Viktor Babrian (2015-01-13 15:10:29)
> > Maybe you can turn the error interrupts off and still have it function. Maybe 
> > it won't (may be required for bus off recovery).
> 
> Thanks for the detailed answer. I read the pages and took a look into the 
> code.
> 
> Regarding state-change behavior, DCAN controller is similar to the one in 
> i.MX6. (It has explicit interrupt source for Act->Warn and Pass->BOFF 
> transitions).
> Although as I understand CAN, you have to have a successful RX or TX event 
> to have the counters decrease. That  means that you will have an immediate 
> report (i.e. interrupt) on Warn->Active, Pass->Warn and BOFF->Active. As 
> far as I see, this latter is explicitly handled by upper layers so driver 
> will have to know it anyway.
Correct.
> This means that if I turn off bus error monitoring, the only event I will 
> not get immediate report of is Warn->Pass. (Pass->Warn can be reported on 
> successful message events if the driver is implemented accordingly. 
Also correct.
> Currently it isn't). However it could be reported later upon the first 
> successful message event. I beleive missing an accurate report of 
> active->passive state change is far better than having a non-responding 
> system in the field in scenarios that do happen.
I agree.

Note: The FlexCAN driver does actually respect berr-reporting for i.MX6.
However, it does not respect it for some older chips.

> 
> Anyway I modified the code so that state change interrupt is not 
> enabled when user sets berr-reporting off (as you kindly suggested). It 
> solves the load issue. Bus-off recovery behavior did not change (I made a 
> few tests only). Btw the driver disables all interrupts in bus off so I 
> guess it does not matter what exact flags were set.
Perhaps, you would like to post your patch? Turning off the interrupts when
berr-reporting is off is definitely correct behaviour. Not turning them off
seems rather pointless to me. AFAIK the whole point of this flag is to suppress
ack-error flood.
> 
> > The consequence (depending on the CPU and Linux build options) is 10%-100% 
> > CPU utilisation when there's nothing to ACK the packet, and a higher figure 
> > again when the bus isn't terminated.
> >
> > CAN is meant to be used in cars or factories. Running with only one node or a 
> > disconnected bus is not a "normal" condition, so the hardware and drivers 
> > don't handle those conditions well.
> Well error states are defined by the spec so they ARE normal. What is not 
> normal is to bring down system performace when they happen.
> 
> 
> I alose added a line to put the controller in init mode in 
> c_can_stop() so that bringing the bus down will actually cause the 
> controller stop transmitting. I hope it's the way to do this, it would be 
> nice if some expert could verify this.
> 
Put this in a separate patch if you post this change.

Best regards,
Andri Yngvason

Re: c_can: (newbie) high system load when frame not acked?

[Tom Evans]

On 14/01/15 02:32, Andri Yngvason wrote:
> Quoting Viktor Babrian (2015-01-13 15:10:29)
>
> Note: The FlexCAN driver does actually respect berr-reporting for i.MX6.
> However, it does not respect it for some older chips.

The flexcan.c driver behaves differently depending on whether it has 
"FLEXCAN_HAS_BROKEN_ERR_STATE" set (on for MX25, MX35, MX53 and off for MX28 
and MX6). I don't think this distinction actually helps, or improves matters 
enough to be worth the CPU loading this causes in these disconnected/solo CAN 
states.

On the "unbroken" chips, the only difference is that you get is interrupts on 
the WARNING transitions. As far as I can see, all that gets you is an 
interrupt when the WARNING state goes away which will trigger a poll that can 
then see that the ERROR_PASSIVE state has cleared in the absence of Receive or 
Transmit interrupts. You get interrupts when the WARNING state sets as well, 
but that happens before the PASSIVE state, so you'll miss that one.

The TXECTR can only decrement on successful transmission which causes a 
transmit interrupt, so the warning interrupt doesn't help here.

The RXECTR only decrements on successful reception, which will cause a receive 
interrupt UNLESS it doesn't match on a filter. But since the driver doesn't 
USE the filters (it accepts everything) that's a moot point too.

Does "FLEXCAN_HAS_BROKEN_ERR_STATE" do anything useful that I've missed? If 
not then that code should be removed. I've done that in the old (Linux 3.4) 
version of the driver I'm using.

> Perhaps, you would like to post your patch? Turning off the
 > interrupts when berr-reporting is off is definitely correct
 > behaviour. Not turning them off seems rather pointless to me.
 > AFAIK the whole point of this flag is to suppress ack-error flood.

There's the load caused by the interrupts, and the additional load caused by 
the call to netif_receive_skb(skb) to report the error up through the stack. 
Getting rid of the latter overhead might have been the intention, or might 
have been "this is the best we can do on this chip".

Tom Evans

Re: c_can: (newbie) high system load when frame not acked?

[Andri Yngvason]

Quoting Tom Evans (2015-01-14 01:35:46)
> On 14/01/15 02:32, Andri Yngvason wrote:
> > Quoting Viktor Babrian (2015-01-13 15:10:29)
> >
> > Note: The FlexCAN driver does actually respect berr-reporting for i.MX6.
> > However, it does not respect it for some older chips.
> 
> The flexcan.c driver behaves differently depending on whether it has 
> "FLEXCAN_HAS_BROKEN_ERR_STATE" set (on for MX25, MX35, MX53 and off for MX28 
> and MX6). I don't think this distinction actually helps, or improves matters 
> enough to be worth the CPU loading this causes in these disconnected/solo CAN 
> states.
> 
I agree.

> ... 
> Does "FLEXCAN_HAS_BROKEN_ERR_STATE" do anything useful that I've missed? If 
> not then that code should be removed. I've done that in the old (Linux 3.4) 
> version of the driver I'm using.
>
The first step towards making that change is posting patches. ;)

--
Andri

Re: c_can: (newbie) high system load when frame not acked?

[Tom Evans]

On 14/01/15 20:55, Andri Yngvason wrote:
> Quoting Tom Evans (2015-01-14 01:35:46)
>> On 14/01/15 02:32, Andri Yngvason wrote:
>>> Quoting Viktor Babrian (2015-01-13 15:10:29)
>>>
>>> Note: The FlexCAN driver does actually respect berr-reporting for i.MX6.
>>> However, it does not respect it for some older chips.
>>
>> The flexcan.c driver behaves differently depending on whether it has
>> "FLEXCAN_HAS_BROKEN_ERR_STATE" set (on for MX25, MX35, MX53 and off for MX28
>> and MX6). I don't think this distinction actually helps, or improves matters
>> enough to be worth the CPU loading this causes in these disconnected/solo CAN
>> states.
>>
> I agree.
>
>> ...
>> Does "FLEXCAN_HAS_BROKEN_ERR_STATE" do anything useful that I've missed? If
>> not then that code should be removed. I've done that in the old (Linux 3.4)
>> version of the driver I'm using.
>>
> The first step towards making that change is posting patches. ;)

Of course. But I'm working on a Linux 3.4 code base that was "frozen" about 15 
linux versions ago. I can only generate AND TEST patches for that version. 
There have been at least 19 mainstream releases/changes to that code since 
that time.

I could generate an untested patch for a later version, but I'd rather not. 
I'd rather someone who can test the code do this on the current codebase.

I would propose one of:

1 - Remove ".features = FLEXCAN_HAS_BROKEN_ERR_STATE," from all structures 
referencing all chips, like "fsl_p1010_devtype_data". That would be misleading 
though and would make the code more confusing than it is (and it is bad enough 
at the moment).

2 - Leave "FLEXCAN_HAS_BROKEN_ERR_STATE" for the chips where the WARNING 
interrupt doesn't work, but don't DO anything as a consequence. That means 
changing the following to ONLY set the error mask on 
"CAN_CTRLMODE_BERR_REPORTING" - removing the condition at line 942:

937         /*
938          * enable the "error interrupt" (FLEXCAN_CTRL_ERR_MSK),
939          * on most Flexcan cores, too. Otherwise we don't get
940          * any error warning or passive interrupts.
941          */
942         if (priv->devtype_data->features & FLEXCAN_HAS_BROKEN_ERR_STATE ||
943             priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)
944                 reg_ctrl |= FLEXCAN_CTRL_ERR_MSK;
945         else
946                 reg_ctrl &= ~FLEXCAN_CTRL_ERR_MSK;

The comment in the above is very misleading. "Error Interrupts" have nothing 
to do with "FLEXCAN_HAS_BROKEN_ERR_STATE". Neither does "passive" as no 
FlexCAN cores can generate "passive interrupts". Some cores can generate 
"warning", but that's a useless interrupt as it doesn't do anything useful.

It should only say the error interrupts are enabled for 
CAN_CTRLMODE_BERR_REPORTING. For "historical interest" it could say that 
"FLEXCAN_HAS_BROKEN_ERR_STATE" used to set the error interrupts, but it causes 
interrupt flooding and can't fix the "state tracking" anyway.

Tom Evans

[PATCH 3.19-rc3] c_can: SIE disabled when berr-reporting is off to reduce irq flood

[Viktor Babrian]

Status interrupt is to be disabled if bus error reporting is off in order 
to reduce interrupt flood when e.g. a frame is not acked. Side effect is 
that when berr-reporting off, warn->pass and pass-warn state transitions 
may be reported later (upon a successful rx/tx event) or even go 
undetected. With proper implementation, these transitions could always be 
detected (but still later). It is considered better to have a non-accurate 
report of these transitions than having a non-responding system in 
possible scenarios.

Signed-off-by: Viktor Babrian <babrian.viktor@renyi.mta.hu>
---
--- linux-3.19-rc3/drivers/net/can/c_can/c_can.c	2015-01-06 
02:05:20.000000000 +0100
+++ linux/drivers/net/can/c_can/c_can.c	2015-01-18 17:46:09.000000000 
+0100
@@ -242,8 +242,11 @@ static void c_can_irq_control(struct c_c
  {
  	u32 ctrl = priv->read_reg(priv,	C_CAN_CTRL_REG) & ~CONTROL_IRQMSK;

+	/* do not enable status irq if bus error reporting is not needed 
*/
  	if (enable)
-		ctrl |= CONTROL_IRQMSK;
+		ctrl |= (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING) 
?
+			CONTROL_IRQMSK :
+			(CONTROL_IRQMSK & ~CONTROL_SIE);

  	priv->write_reg(priv, C_CAN_CTRL_REG, ctrl);
  }

Re: [PATCH 3.19-rc3] c_can: SIE disabled when berr-reporting is off to reduce irq flood

[Marc Kleine-Budde]

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

On 01/18/2015 07:30 PM, Viktor Babrian wrote:
> 
> Status interrupt is to be disabled if bus error reporting is off in order 
> to reduce interrupt flood when e.g. a frame is not acked. Side effect is 
> that when berr-reporting off, warn->pass and pass-warn state transitions 
> may be reported later (upon a successful rx/tx event) or even go 
> undetected. With proper implementation, these transitions could always be 
> detected (but still later). It is considered better to have a non-accurate 
> report of these transitions than having a non-responding system in 
> possible scenarios.
> 
> Signed-off-by: Viktor Babrian <babrian.viktor@renyi.mta.hu>

NACK. The transitions are important for some applications. I've an not
yet mainlined series that limits the bus errors. I'm going to port them
to the current kernel tomorrow. Can you test it?

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: 819 bytes --]

Re: [PATCH 3.19-rc3] c_can: SIE disabled when berr-reporting is off to reduce irq flood

[Viktor Babrian]

> NACK. The transitions are important for some applications. I've an not
> yet mainlined series that limits the bus errors. I'm going to port them
> to the current kernel tomorrow. Can you test it?

This controller simply does not have a warn->pass state transition 
interrupt. The only way to accurately (right on time) report this 
transition is to switch on all error interrupts (ie the status interrupt) 
that cause interrupt flood in some scenarios e.g. when frame is not acked 
by other parties.
If we don't turn on the status interrupt, the warn->pass state transition 
can be detected later on a successful rx/tx event (or when entering bus 
off etc).

I have learned that the warn->pass transition interrupt is also missing 
from other controllers as well (like Flexcan). Interrupt flood caused by 
enabling error interrupts regardless of the berr-reporting state is an 
issue there too.

Also note that when the system is flooded by these error interrupts, 
system performace degrades so much that serial terminal echo becomes 
annoyingly long. When this happens, I wonder how usable the "accurate" 
state transition reports are anyway.

Regards,
Viktor

Re: [PATCH 3.19-rc3] c_can: SIE disabled when berr-reporting is off to reduce irq flood

[Marc Kleine-Budde]

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

On 01/18/2015 07:52 PM, Viktor Babrian wrote:
> 
>> NACK. The transitions are important for some applications. I've an not
>> yet mainlined series that limits the bus errors. I'm going to port them
>> to the current kernel tomorrow. Can you test it?
> 
> This controller simply does not have a warn->pass state transition 
> interrupt. The only way to accurately (right on time) report this 
> transition is to switch on all error interrupts (ie the status interrupt) 
> that cause interrupt flood in some scenarios e.g. when frame is not acked 
> by other parties.
> If we don't turn on the status interrupt, the warn->pass state transition 
> can be detected later on a successful rx/tx event (or when entering bus 
> off etc).
> 
> I have learned that the warn->pass transition interrupt is also missing 
> from other controllers as well (like Flexcan). Interrupt flood caused by 
> enabling error interrupts regardless of the berr-reporting state is an 
> issue there too.
> 
> Also note that when the system is flooded by these error interrupts, 
> system performace degrades so much that serial terminal echo becomes 
> annoyingly long. When this happens, I wonder how usable the "accurate" 
> state transition reports are anyway.

As I said, they are vital to some applications. The bus error limiting
series will follow.

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: 819 bytes --]

Re: [PATCH 3.19-rc3] c_can: SIE disabled when berr-reporting is off to reduce irq flood

[Viktor Babrian]

>> Also note that when the system is flooded by these error interrupts,
>> system performace degrades so much that serial terminal echo becomes
>> annoyingly long. When this happens, I wonder how usable the "accurate"
>> state transition reports are anyway.
>
> As I said, they are vital to some applications. The bus error limiting
> series will follow.

OK. possible compromise is to enable buserr interrupts in Warning state 
only. This way Warning->Passive transition is detected for sure.
Interrupt flood can only occur if error counter could stay between 96 and 
128 which can hardly happen if bus is full of error.

Re: [PATCH 3.19-rc3] c_can: SIE disabled when berr-reporting is off to reduce irq flood

[Tom Evans]

On 19/01/15 05:34, Marc Kleine-Budde wrote:
> On 01/18/2015 07:30 PM, Viktor Babrian wrote:
>>
>> Status interrupt is to be disabled if bus error reporting is off in order
>> to reduce interrupt flood when e.g. a frame is not acked. Side effect is
>> that when berr-reporting off, warn->pass and pass-warn state transitions
>> may be reported later (upon a successful rx/tx event) or even go
>> undetected. With proper implementation, these transitions could always be
>> detected (but still later). It is considered better to have a non-accurate
>> report of these transitions than having a non-responding system in
>> possible scenarios.
>>
>> Signed-off-by: Viktor Babrian <babrian.viktor@renyi.mta.hu>
>
> NACK. The transitions are important for some applications. I've an not
> yet mainlined series that limits the bus errors. I'm going to port them
> to the current kernel tomorrow. Can you test it?

Those "some applications" can enable "CAN_CTRLMODE_BERR_REPORTING" and get 
error reporting and prompt (and complete) state reporting.

Doesn't that answer your concern?

That approach was implemented in flexcan.c as per this patch:

 > Author: Wolfgang Grandegger <wg@grandegger.com>  2012-10-11 06:10:42
 > Committer: Marc Kleine-Budde <mkl@pengutronix.de>  2012-10-24 03:43:17

     can: flexcan: disable bus error interrupts for the i.MX6q

     This patch adds some Flexcan version info and removes the feature flag
     FLEXCAN_HAS_BROKEN_ERR_STATE for the i.MX6Q. It also has the line
     [TR]WRN_INT properly connected.

You approved this approach then (although possibly not knowing that that was 
what the patch was doing then).

Tom Evans

Re: [PATCH 3.19-rc3] c_can: end transmission on network stop

[Viktor Babrian]

Controller put into init mode in betwork stop to end pending 
transmission. The issue is observed in cases when transmitted frame is not 
acked.

Signed-off-by: Viktor Babrian <babrian.viktor@renyi.mta.hu>
---
--- linux-3.19-rc3/drivers/net/can/c_can/c_can.c	2015-01-06 
02:05:20.000000000 +0100
+++ linux/drivers/net/can/c_can/c_can.c	2015-01-18 18:07:09.000000000 
+0100
@@ -615,6 +615,9 @@ static void c_can_stop(struct net_device

  	c_can_irq_control(priv, false);

+	/* put ctrl to init on stop to end ongoing transmission */
+	priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_INIT);
+
  	/* deactivate pins */
  	pinctrl_pm_select_sleep_state(dev->dev.parent);
  	priv->can.state = CAN_STATE_STOPPED;

Re: [PATCH 3.19-rc3] c_can: end transmission on network stop

[Marc Kleine-Budde]

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

On 01/18/2015 08:01 PM, Viktor Babrian wrote:
> 
> Controller put into init mode in betwork stop to end pending 
> transmission. The issue is observed in cases when transmitted frame is not 
> acked.
> 
> Signed-off-by: Viktor Babrian <babrian.viktor@renyi.mta.hu>
> ---
> --- linux-3.19-rc3/drivers/net/can/c_can/c_can.c	2015-01-06 
> 02:05:20.000000000 +0100
> +++ linux/drivers/net/can/c_can/c_can.c	2015-01-18 18:07:09.000000000 
> +0100
> @@ -615,6 +615,9 @@ static void c_can_stop(struct net_device
> 
>   	c_can_irq_control(priv, false);
> 
> +	/* put ctrl to init on stop to end ongoing transmission */
> +	priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_INIT);
> +
>   	/* deactivate pins */
>   	pinctrl_pm_select_sleep_state(dev->dev.parent);
>   	priv->can.state = CAN_STATE_STOPPED;

Your mailer has word-wrapped the patch. I've applied it by hand to
can/master. Please consider using git send-email in the future.

Thanks,
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: 819 bytes --]