netdev - Re: [PATCH net-next-2.6 v5 1/1] can: c_can: Added support for Bosch C

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <4D47B4D8.1010304@grandegger.com>
Date:	Tue, 01 Feb 2011 08:23:04 +0100
From:	Wolfgang Grandegger <wg@...ndegger.com>
To:	Bhupesh SHARMA <bhupesh.sharma@...com>
CC:	"Socketcan-core@...ts.berlios.de" <Socketcan-core@...ts.berlios.de>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	Marc Kleine-Budde <mkl@...gutronix.de>,
	David Miller <davem@...emloft.net>
Subject: Re: [PATCH net-next-2.6 v5 1/1] can: c_can: Added support for Bosch
 C_CAN controller

Hi Bhupesh,

On 02/01/2011 05:29 AM, Bhupesh SHARMA wrote:
> Hello Wolfgang,
> 
>> ...
>>>>> +		/* handle error on the bus */
>>>>> +		lec_type = c_can_has_and_handle_berr(priv);
>>>>> +		if (lec_type && (error_type != C_CAN_NO_ERROR))
>>>>> +			work_done += c_can_err(dev, error_type, lec_type);
>>>>
>>>> State changes are only reported if berr_reporting is enabled and a
>> bus
>>>> error occured. This needs to be fixed.
>>>
>>> As I mentioned earlier in a response to a review comment, the Bus
>> Error
>>> reporting for C_CAN seems different from sja1000 and flexcan
>> approaches.
>>> Do you think it will be useful to drop CAN_CTRLMODE_BERR_REPORTING
>> from
>>> priv->can.ctrlmode_supported as done by *pch* driver? Or do you have
>>> a better idea..
>>
>> You bus error reporting is OK. The problem is that it does not only
>> affect bus errors but also state changes. State change messages should
>> alway be send independent of priv->can.ctrlmode. It's just a matter of
>> moving code to the right location. E.g. the code snippet above inside
>> c_can_err() before you check for bus errors.
>>
>>>> Feel free to send the output of "candump any,0:0,#FFFFFFFF" when
>>>> sending
>>>> messages without cable connected and with a bus error provocuted.
>>>
>>> OK. I will try to cross-compile candump for my arm-v7 architecture
>>> and will send the output.
>>
> 
> I did some changes to the code to ensure that the state change and lec
> handling are handled separately and properly.

Great.

> Please find the candump any,0:0,#FFFFFFFF output below:
> 
> 1. With No-Cable connected, I keep getting:
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME

The SJA1000 reports:

---- Error-active -> error-warning -> error-passive
---- cable disconnected
-bash-3.2# ./candump -t d any,0:0,#FFFFFFFF

 (0.000000)  can0  20000004  [8] 00 08 00 00 00 00 60 00   ERROR-WARNING
 (0.002588)  can0  20000004  [8] 00 20 00 00 00 00 80 00   ERROR-PASSIVE

I wonder why you get data[1]=28 immediately. Also it's nice to have the
txerr and rxerr in data[6..7].

> 2. With Tx and Rx shorted to simulate bus-error, I get:
>   can0  20000044  [8] 00 20 00 00 00 00 00 00   ERRORFRAME

Also here, you should see:

---- Error-active -> error-warning -> error-passive -> bus-off

-bash-3.2# ./candump -t d any,0:0,#FFFFFFFF
 (0.000000)  can0  20000004  [8] 00 08 00 00 00 00 88 00   ERROR-WARNING
 (0.001362)  can0  20000004  [8] 00 20 00 00 00 00 88 00   ERROR-PASSIVE
 (0.001560)  can0  20000044  [8] 00 00 00 00 00 00 7F 00   BUS-OFF

> In case 2, when I enable debug messages I get the correct state change sequence:
> entered error warning state
> entered error passive state
> entered bus off state

As the log confirms.

> Does this result seem fine to you?

Not yet. Also the output with "berr-reporting on" should be checked. And
the output when you recover from bus-off manually (using "ip link set
type can restart")

Thanks,

Wolfgang.


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