Re: [BULK]Re: [PATCH] can: fix loss of frames due to wrong assumption in raw_rcv

[Stephane Grosjean <s.grosjean@peak-system.com>]

Hi everyone,

I follow this thread carefully and, with your permission, I'd like to 
speak in.

As we always help our customers in using the mainline drivers products 
line too, I just wanted to inform you that I currently have one who 
tells that he's facing some loss of frames with the PEAK-System PCAN-USB 
adapter, in case of "relatively"  high bus load... He's running two 
Kernels (3.19 and the last 4.1 patched with this recent "fix"). At the 
moment, he says he always notes some frame leakage, especially when he 
(for example) "resizes windows on his desktop"...

The curious things are:

0 - frames are lost when running his socket-can application over the 
mainline peak-usb driver
1 - frames are lost when running his socket-can application over our 
"pcan" driver (built in so-called "netdev" mode, that is, rx frames are 
forwarded to socket-can using netif_rx())
2 - *but* no frame are lost when running the "ioctl()" test applications 
above our "pcan" driver (built in so-called "chardev" mode, that is, rx 
frames are stored into internal fifos before being given to applications)
3 - in both cases #0 and #1, "can0" network stats don't show any 
rx_dropped nor rx_fifo_errors errors at all (even if sometimes, he says 
he notes some "real" rx_over_errors  raised up by the peak-usb driver).

I don't succeed to reproduce this issue with my "old" 3.16 Kernel at the 
moment. Before going on with installing the 3.19 Kernel, could the above 
"leakage" be explained by the last changes made in linux-can?

Many thanks for your help,

Regards,

Stéphane

Le 24/06/2015 21:56, Oliver Hartkopp a écrit :
> On 24.06.2015 04:13, Tom Evans wrote:
>> On 23/06/15 18:01, Oliver Hartkopp wrote:
>>> An alternative to force the timestamp to be set would be to add another
>>> 'counter' to struct can_skb_priv which is just increased by the 
>>> driver with
>>> every received CAN frame.
>>>
>>> But I wonder if its worth the effort in opposite to just enable 
>>> timestamping
>>> in every skb. What do you think?
>>
>>  From a programming standpoint, using a "timestamp" as a "unique 
>> identifier"
>> is opaque and a poor design. If the SKB requires a "unique 
>> identifier" then it
>> should be provided with something that has that description and purpose.
>
> Hey Tom,
>
>> The timestamp may be optional for a reason. Maybe some platforms 
>> simply don't
>> provide a high resolution timestamp, and/or the timestamp comes with 
>> a high
>> overhead. Can you guarantee that all current, future and past platforms
>> support a low overhead and high resolution timestamp?
>
> I have used the CAN stuff on two PowerPC, one ARM and several x86 
> platforms - always with enabled timestamping - without any problems in 
> performance.
>
>> A new "feature" probably shouldn't change something that was optional 
>> to being
>> compulsory. Would all existing CAN drivers need to be rewritten to 
>> add this
>> timestamping, or is the timestamp added in higher (and more generic) 
>> layers,
>> and in the one place?
>
> The point is that out-of-tree drivers (like the PEAK-System driver) 
> would need to set the timestamp too to take advantage of the joint 
> filter feature.
> The mainline drivers are adapted for now with the stable patch
>
> http://git.kernel.org/cgit/linux/kernel/git/davem/net.git/commit/?id=36c01245eb8046c16eee6431e7dbfbb302635fa8 
>
>
> But I felt not very comfortable with this fix. The point is that I 
> have to take care that the processing of an incoming skb in 
> can_receive() in af_can.c is done in 'one step' with an unique skb. So 
> marking the skb for the joint filter feature in raw_rcv() can probably 
> be done in can_receive().
>
> I'm looking for a SMP save solution for this - to finally revert the 
> fix above. So thanks for the 'wake-up call' by naming the timestamping 
> solution  a 'poor design'  ;-)
>
> Best regards,
> Oliver
>
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
PEAK-System Technik GmbH
Sitz der Gesellschaft Darmstadt
Handelsregister Darmstadt HRB 9183 
Geschaeftsfuehrung: Alexander Gach, Uwe Wilhelm
--