On 07/25/2010 04:23 PM, Wolfgang Grandegger wrote:
> On 07/25/2010 03:35 PM, Marc Kleine-Budde wrote:
>> Wolfgang Grandegger wrote:
>>> I found some time to work on this issue.
>>>
>>> So far we only report CAN error state changes to the worse:
>>> "error-active -> error-warning -> error-passive -> bus-off", but not
>>> "bus-off -> error-active" or "error-passive -> error-warning ->
>>> error-active". There have been request from other users to correct
>>> this behavior.
>>>
>>> This patch reports any state change reported by the SJA1000 and
>>> MSCAN-MPC5200 controller. It introduces "CAN_ERR_CRTL_ACTIVE" to
>>> signal state changes to error active. Furthermore, the SJA1000
>>> triggers now the bus-off recovery procedure when restarted. So far,
>>> the controller was *stopped* and then restarted. I did two tests to
>>> analyze the state changes behavior:
>>>
>>> 1. First I forced the controller into the error-passive by sending
>>> a message (with cansend) without cable connected (*). Then I
>>> reconnected the cable (**) and continued to send messages (with
>>> cangen) (***) until the error active state is reached:
>>>
>>> On SJA1000:
>>>
>>> # candump any,0:0,#FFFFFFFF
>>> (*)
>>> can0 20000004 [8] 00 08 00 00 00 00 60 00 ERROR-WARNING
>>> can0 20000004 [8] 00 20 00 00 00 00 80 00 ERROR-PASSIVE
>>> (**)
>>> can0 123 [4] DE AD BE EF
>>> (***)
>>> can0 42A [1] 00
>>> ...
>>> can0 42A [1] 07
>>> can0 20000004 [8] 00 08 00 00 00 00 7F 00 ERROR-WARNING
>>
>> Is it possible to tell apart whether the driver goes from passive to
>> warning or from active to warning? Without the need to have a state
>> machine in userspace, too?
>
> Yes, I realized that side effect as well. The user must monitor state
> changes to understand if the state gets worse or better. We could use an
> error bit to indicate state improvements.
>
>> Looking at the tx/rx error level might be an option, but I've seen that
>> the level can rise very quickly.
>
> Be aware that the tx/rx error counters are *not* readable on all CAN
> controllers.
>
>>> can0 42A [1] 08
>>> ...
>>> can0 42A [1] 27
>>> can0 20000004 [8] 00 40 00 00 00 00 5F 00 ERROR-ACTIVE
>>> \-- tx-err !!!
>>> On MSCAN:
>>>
>>> # candump any,0:0,#FFFFFFFF
>>> (*)
>>> can2 20000004 [8] 00 08 00 00 00 00 00 00 ERROR-WARNING
>>> can2 20000004 [8] 00 20 00 00 00 00 00 00 ERROR-PASSIVE
>>> (**)
>>> can2 123 [4] DE AD BE EF
>>> can2 20000004 [8] 00 08 00 00 00 00 00 00 ERROR-WARNING
>>> (***)
>>> can2 42A [1] 00
>>> can2 42A [1] 01
>>> ...
>>> can2 42A [1] 1E
>>> can2 42A [1] 1F
>>> can2 20000004 [8] 00 40 00 00 00 00 00 00 ERROR-ACTIVE
>>>
>>> As you can see, the state change to error-active is signaled
>>> differently. The MSCAN seems not to respect the specs :-(.
>>
>> Do you mean the fact that error active is signalled with tx err == 0?
>
> Yes, the SJA1000 triggers an interrupt to signal error active when the
> error counter goes below 96.
>
>> From the patch below you only changed the part of the driver that runs
>> if a change of state is detected "(old_state != priv->can.state)".
>>
>> Maybe it's a problem in the driver, that calculated the new state wrong.
>> Maybe due to a problem with the hardware. (I don't know the driver nor
>> the hardware).
>
> I monitored the interrupt generation as well:
>
> [ 1234.089446] mpc5xxx_can f0000900.can: error interrupt (canrflg=0x44)
> [ 1234.092228] mpc5xxx_can f0000900.can: error interrupt (canrflg=0x48)
> [ 1240.960487] mpc5xxx_can f0000900.can: error interrupt (canrflg=0x44)
> [ 1352.336647] mpc5xxx_can f0000900.can: error interrupt (canrflg=0x40)
>
> The last interrupt comes that late, even if the state is misinterpreted.
> Note also that the MSCAN has a dedicated state field for TX and RX.
> Therefore the state is defined without a state machine in software.
>
>> Looking at you patches below I want to rise my old proposal again. I
>> think we should move the state machine that assembles and sends the
>> error frames into a common function. Each driver just needs to have a
>> proper function that calculates the current state and then call the
>> state handling state machine.
>
> The error handling code is already quite large and complex, especially
> for the purpose it serves. Any common function improving that situation
> would be nice. But unfortunately, we rely on the hardware :-(.
>
>>> When time permits, I will also try the MSCAN of the MPC5121.
>>
>>> 2. I forced the controller into bus-off by short-circuiting
>>> the low and hi lines and sending a message (*), Then I
>>> restarted the controller manually with
>>> "ip link set can0 type can restart" (**):
>>>
>>> On SJA1000:
>>>
>>> # ./candump -t d any,0:0,#FFFFFFFF
>>> (*)
>>> can0 20000004 [8] 00 08 00 00 00 00 88 00 ERROR-WARNING
>>> can0 20000004 [8] 00 20 00 00 00 00 88 00 ERROR-PASSIVE
>>> can0 20000044 [8] 00 00 00 00 00 00 7F 00 BUS-OFF
>>> (**)
>>> can0 20000100 [8] 00 00 00 00 00 00 00 00 RESTARTED
>>> can0 20000004 [8] 00 40 00 00 00 00 00 00 ERROR-ACTIVE
>>> \-- tx-err !!!
>>>
>>> On MSCAN:
>>>
>>> # ./candump -t d any,0:0,#FFFFFFFF
>>> (*)
>>> can2 20000004 [8] 00 08 00 00 00 00 00 00 ERROR-WARNING
>>> can2 20000040 [8] 00 00 00 00 00 00 00 00 BUS-OFF
>>> (**)
>>> can2 20000100 [8] 00 00 00 00 00 00 00 00 RESTARTED
>>> can2 20000004 [8] 00 40 00 00 00 00 00 00 ERROR-ACTIVE
>>>
>>> The MSCAN seems not to signal the ERROR-PASSIVE state before
>>> going bus-off.
>>
>> I think the signalling of ERROR-PASSIVE depends on multiple things:
>> - the current bitrate.
>> I think this doesn't count so much in this short circuit test.
>> - the CAN hardware itself: does it signal this "in-the-middle-passive"
>> interrupt
>> - overall speed and load of your system:
>> reaction time on interrupts,
>> is your system fast enough to handle these two back-to-back interrupts
>> as two single events
>> - driver design, i.e.
>> - is the state change signalled from the hard-irq or from napi:
>> the second interrupt may occur somewhere between hardirq and napi,
>> so napi just sees bus off
>> - design of the error state machine:
>> in the at91 and flexcan I always send a warning interrupt if going
>> from active to passive.
>>
>> The last point is another argument for a common state machine.
>
> Well, yes, a common state handling function would be nice, but as the
> hardware is not really smart, a simple state machine is not enough. It's
> not just reading the current state and check if it has changed. Have a
> look to the SJA1000 and MSCAN driver, e.g.:
>
> if (isrc & IRQ_EPI) {
> /* error passive interrupt */
> dev_dbg(dev->dev.parent, "error passive interrupt\n",
> status);
> if (status & SR_ES) {
> if (priv->can.state == CAN_STATE_ERROR_WARNING)
> state = CAN_STATE_ERROR_PASSIVE;
> else
> state = CAN_STATE_ERROR_WARNING;
> } else {
> state = CAN_STATE_ERROR_ACTIVE;
> }
> }
>
> No need for another state machine. But a common function is still useful
> to fill an error frame for the state change, of course. I will try to
> come up with a proposal soon.
int can_handle_state_change(struct net_device *dev,
struct can_frame *cf,
enum can_state new_state,
u16 tx_err, u16 rx_err)
If "tx_err" or "rx_err == -1", they are ignored. If "new_state ==
CAN_STATE_UNKNOWN", the state could be derived from tx_err or rx_err.
What do you think?
Wolfgang.
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
