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:

quoted

quoted

quoted

quoted

quoted

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 .

quoted

quoted

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

quoted

Well, s/driver/hardware/ ! Local loopback is the preferred mechanism.

Sure...

quoted

quoted

I'm currently ok with this fallback mechanism.

/me not.

quoted

quoted

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.

quoted

quoted

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?