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

On 08/02/2012 12:53 PM, Olivier Sobrie wrote:

quoted

quoted

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.

quoted

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.

quoted

quoted

quoted

quoted

quoted

quoted

+		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.

quoted

quoted

quoted

quoted

quoted

quoted

+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   |