| 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
| ||
|
Date: Tue, 31 Jul 2012 15:27:55 +0200
From: Marc Kleine-Budde <mkl@...gutronix.de>
To: Olivier Sobrie <olivier@...rie.be>
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
On 07/31/2012 03:06 PM, Olivier Sobrie wrote:
[...]
>>>> Please check if your driver survives hot-unplugging while sending and
>>>> receiving CAN frames at maximum laod.
>>>
>>> I tested this with two Kvaser sending frames with "cangen can0 -g 0 -i"
>>> never saw a crash.
>>
>> Please test send sending and receiving at the same time.
>
> Yes that's what I did; "cangen can0 -g 0 -i" on both sides.
Fine.
[...]
>>>>> --- /dev/null
>>>>> +++ b/drivers/net/can/usb/kvaser_usb.c
>>>>> @@ -0,0 +1,1062 @@
>>>>> +/*
>>>>
>>>> Please add a license statement and probably your copyright:
>>>>
>>>> * This program is free software; you can redistribute it and/or
>>>> * modify it under the terms of the GNU General Public License as
>>>> * published by the Free Software Foundation version 2.
>>>>
>>>> You also should copy the copyright from the drivers you used:
>>>>
>>>>> + * Parts of this driver are based on the following:
>>>>> + * - Kvaser linux leaf driver (version 4.78)
>>>>
>>>> I just downloaded their driver and noticed that it's quite sparse on
>>>> stating the license the code is released under.
>>>> "doc/HTMLhelp/copyright.htx" is quite restrictive, the word GPL occurs 3
>>>> times, all in MODULE_LICENSE("GPL"). Running modinfo on the usbcan.ko
>>>> shows "license: GPL"
>>>
>>> I'll add the license statement.
>>> In fact it's the leaf.ko which is used for this device and it is under
>>> GPL as modinfo said.
>>
>> I just talked to my boss and we're the same opinion, that
>> MODULE_LICENSE("GPL") is a technical term and not relevant if the
>> included license doesn't say a word about GPL. If the kvaser tarball
>> violates the GPL, however is written on different sheet of paper (as we
>> say in Germany).
>>
>> So I cannot put my S-o-b under this driver as long as we haven't talked
>> to kvaser.
>
> Ok I thought it was sufficient enough to have MODULE_LICENSE("GPL") in
> the code to indicate it is a GPL driver. I'll ask Kvaser before sending
> any new version of the patch.
We can continue the review process, this problem has to be sorted out
before I can apply this patch to linux-can-next tree.
[...]
>>>>> +static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
>>>>> + const struct kvaser_msg *msg)
>>>>> +{
>>>>> + struct can_frame *cf;
>>>>> + struct sk_buff *skb;
>>>>> + struct net_device_stats *stats;
>>>>> + struct kvaser_usb_net_priv *priv;
>>>>> + u8 channel, status, txerr, rxerr;
>>>>> +
>>>>> + if (msg->id == CMD_CAN_ERROR_EVENT) {
>>>>> + channel = msg->u.error_event.channel;
>>>>> + status = msg->u.error_event.status;
>>>>> + txerr = msg->u.error_event.tx_errors_count;
>>>>> + rxerr = msg->u.error_event.rx_errors_count;
>>>>> + } else {
>>>>> + channel = msg->u.chip_state_event.channel;
>>>>> + status = msg->u.chip_state_event.status;
>>>>> + txerr = msg->u.chip_state_event.tx_errors_count;
>>>>> + rxerr = msg->u.chip_state_event.rx_errors_count;
>>>>> + }
>>>>> +
>>>>> + if (channel >= dev->nchannels) {
>>>>> + dev_err(dev->udev->dev.parent,
>>>>> + "Invalid channel number (%d)\n", channel);
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> + priv = dev->nets[channel];
>>>>> + stats = &priv->netdev->stats;
>>>>> +
>>>>> + skb = alloc_can_err_skb(priv->netdev, &cf);
>>>>> + if (!skb) {
>>>>> + stats->rx_dropped++;
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> + if ((status & M16C_STATE_BUS_OFF) ||
>>>>> + (status & M16C_STATE_BUS_RESET)) {
>>>>> + priv->can.state = CAN_STATE_BUS_OFF;
>>>>> + cf->can_id |= CAN_ERR_BUSOFF;
>>>>> + can_bus_off(priv->netdev);
>>
>> you should increment priv->can.can_stats.bus_off
>> What does the firmware do in this state? Does it automatically try to
>> recover and try to send the outstanding frames?
>
> Yes that's what I observed.
It's behaves like the at91_can.
>> If so, you should turn of the CAN interface, it possible. See:
>> http://lxr.free-electrons.com/source/drivers/net/can/at91_can.c#L986
>
> Ok I'll have a look at this.
>
>>
>> Please test Bus-Off behaviour:
>> - setup working CAN network
>> - short circuit CAN-H and CAN-L wires
>> - start "candump any,0:0,#FFFFFFFF" on one shell
>> - send one can frame on the other
>>
>> then
>>
>> - remove the short circuit
>> - see if the can frame is transmitted to the other side
>> - it should show up as an echo'ed CAN frame on the sender side
>>
>> Repeat the same test with disconnecting CAN-H and CAN-L from the other
>> CAN station instead of short circuit.
>>
>> Please send the output from candump.
>
> 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?
> ...
> can1 200000C8 [8] 00 00 90 00 00 00 00 00 ERRORFRAME <-- bus off
> 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 123 [2] 11 22 <-- short circuit removed
>
> I see the echo and on the other end I see the frame coming in.
> By the way I see that the txerr and rxerr fields of the structure
> kvaser_msg_error_event stay at 0.
>
> 2) With CAN-H and CAN-L disconnected:
>
> I never see the bus going in OFF state. It stays in PASSIVE mode.
>
> can1 20000088 [8] 00 10 80 1B 00 00 00 00 ERRORFRAME
> can1 20000088 [8] 00 10 80 1B 00 00 00 00 ERRORFRAME
> can1 20000088 [8] 00 10 80 1B 00 00 00 00 ERRORFRAME
> can1 20000088 [8] 00 10 80 1B 00 00 00 00 ERRORFRAME
> can1 20000088 [8] 00 10 80 1B 00 00 00 00 ERRORFRAME
> can1 20000088 [8] 00 10 80 1B 00 00 00 00 ERRORFRAME
> can1 20000088 [8] 00 10 80 1B 00 00 00 00 ERRORFRAME
> can1 20000088 [8] 00 10 80 1B 00 00 00 00 ERRORFRAME
> can1 20000088 [8] 00 10 80 1B 00 00 00 00 ERRORFRAME
> can1 20000088 [8] 00 10 80 1B 00 00 00 00 ERRORFRAME
> can1 20000088 [8] 00 10 80 1B 00 00 00 00 ERRORFRAME
> can1 20000088 [8] 00 10 80 1B 00 00 00 00 ERRORFRAME
> can1 20000088 [8] 00 10 80 1B 00 00 00 00 ERRORFRAME
> can1 20000088 [8] 00 10 80 1B 00 00 00 00 ERRORFRAME
> can1 20000088 [8] 00 10 80 1B 00 00 00 00 ERRORFRAME
> can1 20000088 [8] 00 10 80 1B 00 00 00 00 ERRORFRAME
> ...
> can1 20000088 [8] 00 10 80 1B 00 00 00 00 ERRORFRAME
> can1 20000088 [8] 00 10 80 1B 00 00 00 00 ERRORFRAME
> can1 20000088 [8] 00 10 80 1B 00 00 00 00 ERRORFRAME
> can1 123 [2] 11 22 <-- other end connected
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.
>>>>> + } else if (status & M16C_STATE_BUS_ERROR) {
>>>>> + priv->can.state = CAN_STATE_ERROR_WARNING;
>>>>> + priv->can.can_stats.error_warning++;
>>>>> + } else if (status & M16C_STATE_BUS_PASSIVE) {
>>>>> + priv->can.state = CAN_STATE_ERROR_PASSIVE;
>>>>> + priv->can.can_stats.error_passive++;
>>>>> + } else {
>>>>> + priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>>>> + cf->can_id |= CAN_ERR_PROT;
>>>>> + cf->data[2] = CAN_ERR_PROT_ACTIVE;
>>>>> + }
>>>>> +
>>>>> + if (msg->id == CMD_CAN_ERROR_EVENT) {
>>>>> + u8 error_factor = msg->u.error_event.error_factor;
>>>>> +
>>>>> + priv->can.can_stats.bus_error++;
>>>>> + stats->rx_errors++;
>>>>> +
>>>>> + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
>>>>> +
>>>>> + 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.
>>>>> + }
>>>>> +
>>>>> + if (error_factor & M16C_EF_ACKE)
>>>>> + cf->data[3] |= (CAN_ERR_PROT_LOC_ACK |
>>>>> + CAN_ERR_PROT_LOC_ACK_DEL);
>>>>> + if (error_factor & M16C_EF_CRCE)
>>>>> + cf->data[3] |= (CAN_ERR_PROT_LOC_CRC_SEQ |
>>>>> + CAN_ERR_PROT_LOC_CRC_DEL);
>>>>> + if (error_factor & M16C_EF_FORME)
>>>>> + cf->data[2] |= CAN_ERR_PROT_FORM;
>>>>> + if (error_factor & M16C_EF_STFE)
>>>>> + cf->data[2] |= CAN_ERR_PROT_STUFF;
>>>>> + if (error_factor & M16C_EF_BITE0)
>>>>> + cf->data[2] |= CAN_ERR_PROT_BIT0;
>>>>> + if (error_factor & M16C_EF_BITE1)
>>>>> + cf->data[2] |= CAN_ERR_PROT_BIT1;
>>>>> + if (error_factor & M16C_EF_TRE)
>>>>> + cf->data[2] |= CAN_ERR_PROT_TX;
>>>>> + }
>>>>> +
>>>>> + cf->data[6] = txerr;
>>>>> + cf->data[7] = rxerr;
>>>>> +
>>>>> + netif_rx(skb);
>>>>> +
>>>>> + priv->bec.txerr = txerr;
>>>>> + priv->bec.rxerr = rxerr;
>>>>> +
>>>>> + stats->rx_packets++;
>>>>> + stats->rx_bytes += cf->can_dlc;
>>>>> +}
>>>>> +
[...]
>>>>> +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;
>>>>> +
>>>>> + return 0;
>>>>> +}
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 |
Download attachment "signature.asc" of type "application/pgp-signature" (263 bytes)
Powered by blists - more mailing lists