netdev - Re: [PATCH RESEND] net: can: Introduce MEN 16Z192-00 CAN controller driver

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: <a0169f54-c8c7-5cf6-b93b-acd4a308a052@hartkopp.net>
Date:	Wed, 10 Aug 2016 22:28:45 +0200
From:	Oliver Hartkopp <socketcan@...tkopp.net>
To:	Andreas Werner <andreas.werner@....de>,
	Wolfgang Grandegger <wg@...ndegger.com>
Cc:	mkl@...gutronix.de, linux-can@...r.kernel.org,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	davem@...emloft.net, jthumshirn@...e.de, andy@...nerandy.de,
	michael.miehling@....de
Subject: Re: [PATCH RESEND] net: can: Introduce MEN 16Z192-00 CAN controller
 driver

Hi Andreas,

On 08/09/2016 08:10 AM, Andreas Werner wrote:
> On Mon, Aug 08, 2016 at 04:35:34PM +0200, Wolfgang Grandegger wrote:

>>>>>> You specify here one echo_skb but it's not used anywhere. Local loopback
>>>>>> seems not to be implemented.
>>>>>>
>>>>>
>>>>> Agree with you, will set it to "0".
>>>>
>>>> No, the local loopback is mandetory!
>>>>
>>>
>>> Hm ok, but if i check alloc_candev() in drivers/net/can/dev.c
>>> it is not mandatory.

It is.

Even those drivers that show up to use zero echo skbs in alloc_candev() 
implement the echo functionality correct.

Just check 'git grep IFF_ECHO'. Even grcan.c and janz-ican3.c have 
IFF_ECHO set - but implement it in a different way without using the 
provided machanism from dev.c .

>>> In the Documentation/networking/can.txt
>>> there is also a "should" and a fallback mechnism if the driver
>>> does not support the local loopback.

But this fallback mechanism is bad - really bad!

E.g. the slcan.c driver sends a stream of CAN frames without knowing 
whether the frames ever hit the wire. The slcan driver is more less for 
hobby users. The CAN frame echo with IFF_ECHO gives a correct 
representation of the traffic on the wire - including the correct 
timestamps.

You really want to know whether a CAN frame was sent correctly on the 
bus instead of getting some shortcut info from inside the network layer.
.
>>
>> Well, s/driver/hardware/ ! Local loopback is the preferred mechanism.
>>
>
> Sure...
>
>>> I'm currently ok with this fallback mechanism.

/me not.

>>> Anyway I am not sure that the driver can handle the echo skb correctly.
>>> If i understand it correctly, the can_get_echo_skb() is normally called
>>> on a "TX done IRQ" to get the skb and loop it back.

ack.

>>> I do not have such a "TX done IRQ" and have not implemented implemented
>>> and added the local looback.

I'm not really sure how grcan.c and janz-ican3.c implemented the echo 
functionality but they must have faced a similar situation.

A local loopback inside the CAN controller which is generated after 
successful transmit is an excellent implementation with excellent 
timestamps. The only problem for you is to detect the looped CAN frames 
and match them to the skb pointer of the outgoing frame to 'receive' the 
correct echo skb.

When you send CAN frames to an unconnected CAN bus it can't be sent out 
due to the missing acknowledge from other nodes. So when you send frames 
and you get echo frames due to the fallback mode your cool CAN 
controller degrades to slcan level.

Regards,
Oliver

ps. Do you have any URL where one can get the MEN 16Z192 spec?