Re: [PATCH] can: kvaser_usb: Add support for Kvaser CAN/USB devices

[Marc Kleine-Budde <mkl@pengutronix.de>]

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

On 08/02/2012 12:53 PM, Olivier Sobrie wrote:
>>> 1) With the short circuit:
>>>
>>> I perform the test you described. It showed that the Kvaser passes from
>>> ERROR-WARNING to ERROR-PASSIVE and then BUS-OFF. But after going to the
>>> state BUS-OFF it comes back to ERROR-WARNING.
>>>
>>>   can1  20000088  [8] 00 10 90 00 00 00 00 00   ERRORFRAME
>>>   can1  20000088  [8] 00 10 90 00 00 00 00 00   ERRORFRAME
>>>   can1  20000088  [8] 00 10 90 00 00 00 00 00   ERRORFRAME
>>>   can1  20000088  [8] 00 10 90 00 00 00 00 00   ERRORFRAME
>>>   can1  20000088  [8] 00 10 90 00 00 00 00 00   ERRORFRAME
>>>   can1  20000088  [8] 00 10 90 00 00 00 00 00   ERRORFRAME
>>>   can1  20000088  [8] 00 10 90 00 00 00 00 00   ERRORFRAME
>>>   can1  20000088  [8] 00 10 90 00 00 00 00 00   ERRORFRAME
>>>   can1  20000088  [8] 00 10 90 00 00 00 00 00   ERRORFRAME
>>
>> Why don't we have any rx/tx numbers in the error frame?
> 
> Because the hardware seems to not update the tx/rx_errors_count
> fields :-(

Okay.

>> From the hardware point of view the short circuit and open end tests
>> look good. Please adjust the driver to turn off the CAN interface in
>> case of a bus off if restart_ms is 0.
> 
> And in the case where restart_ms is not 0? Don't I've to put it off so
> and drop the frame?

No, don't drop the frame. restart-ms != 0 means the controller is
automatically restarted after the specified time (if the controller
supports). Or in your and the at91 case, automatically.

> I actually implemeted it as you said and here is what I observed in
> candump output with restart_ms set to 100 ms:
> 
> t0: Short circuit between CAN-H and CAN-L + cansend can1 123#1122
>   can1  2000008C  [8] 00 04 90 00 00 00 00 00   ERRORFRAME
> 	controller-problem{rx-error-warning}
> 	protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
> 	bus-error
>   can1  2000008C  [8] 00 10 90 00 00 00 00 00   ERRORFRAME
> 	controller-problem{rx-error-passive}
> 	protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
> 	bus-error
>   can1  200000C8  [8] 00 00 90 00 00 00 00 00   ERRORFRAME
> 	protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
> 	bus-off
> 	bus-error
> ...
>   can1  2000008C  [8] 00 04 90 00 00 00 00 00   ERRORFRAME
> 	controller-problem{rx-error-warning}
> 	protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
> 	bus-error
>   can1  2000008C  [8] 00 10 90 00 00 00 00 00   ERRORFRAME
> 	controller-problem{rx-error-passive}
> 	protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
> 	bus-error
>   can1  200000C8  [8] 00 00 90 00 00 00 00 00   ERRORFRAME
> 	protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
> 	bus-off
> 	bus-error
> 
> t1: short circuit removed
>   can1  123  [2] 11 22
>   can1  20000100  [8] 00 00 00 00 00 00 00 00   ERRORFRAME
> 	restarted-after-bus-of
> 
> The echo coming before the restart looks weird? No?
> Shouldn't we drop the frame once BUF-OFF is reached?

No, I don't think so. But wait for Wolfgang, here's more into error
handling then me.

> 
>>>>>>> +		if ((priv->can.state == CAN_STATE_ERROR_WARNING) ||
>>>>>>> +		    (priv->can.state == CAN_STATE_ERROR_PASSIVE)) {
>>>>>>> +			cf->data[1] = (txerr > rxerr) ?
>>>>>>> +				CAN_ERR_CRTL_TX_PASSIVE
>>>>>>> +				: CAN_ERR_CRTL_RX_PASSIVE;
>>
>> Please use CAN_ERR_CRTL_RX_WARNING, CAN_ERR_CRTL_TX_WARNING where
>> appropriate.

> Ok. As the hardware doesn't report good values for txerr and rxerr, I'll
> also remove the tests on txerr and rxerr.
> I observed the same behavior with the original driver.
> I asked Kvaser for this problem. I've to wait before their developer is
> back (same for the GPL issue).

Okay.

>>>>>>> +static int kvaser_usb_get_berr_counter(const struct net_device *netdev,
>>>>>>> +				       struct can_berr_counter *bec)
>>>>>>> +{
>>>>>>> +	struct kvaser_usb_net_priv *priv = netdev_priv(netdev);
>>>>>>> +
>>>>>>> +	bec->txerr = priv->bec.txerr;
>>>>>>> +	bec->rxerr = priv->bec.rxerr;
>>
>> I think you can copy the struct like this:
>>
>> 	*bec = priv->bec;
> 
> Thanks. I'll remove the function kvaser_usb_get_berr_counter as the
> hardware seems to never report txerr and rxerr.

Sounds reasonable.

BTW: is it possible to update the firmware on these devices?

> I'll look deeper at this driver during the week-end if possible...

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