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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20120802105358.GA23787@hposo>
Date:	Thu, 2 Aug 2012 12:53:58 +0200
From:	Olivier Sobrie <olivier@...rie.be>
To:	Marc Kleine-Budde <mkl@...gutronix.de>
Cc:	Wolfgang Grandegger <wg@...ndegger.com>, linux-can@...r.kernel.org,
	netdev@...r.kernel.org
Subject: Re: [PATCH] can: kvaser_usb: Add support for Kvaser CAN/USB devices

Hello,

On Tue, Jul 31, 2012 at 03:27:55PM +0200, Marc Kleine-Budde wrote:
> We can continue the review process, this problem has to be sorted out
> before I can apply this patch to linux-can-next tree.

Ok.

> > 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 :-(

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

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

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

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

Thanks a lot for your help,

-- 
Olivier
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html