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

[Tom Evans <tom_usenet@optusnet.com.au>]

On 23/06/15 18:01, Oliver Hartkopp wrote:
> Hi Tom,
>
> On 23.06.2015 06:56, Tom Evans wrote:
>> On 22/06/15 20:24, Oliver Hartkopp wrote:
>>> But the point becomes an issue when there's no userspace application that
>>> requires timestamps.
>>>
>>> I did my testing wile having at least one "candump" instances running, which
>>> enables timestamping. So when there's no one requesting timestamps the check
>>> in can_rcv does not perform properly.
>>>
>>> Therefor my patch grabs your idea to set the timestamps for CAN skbs
>>> unconditionally. But there were some more places in the code where we need to
>>> take care about that.
>>
>> The original patch contains:
>>
>> +    /* eliminate multiple filter matches for the same skb */
>> +    if (this_cpu_ptr(ro->uniq)->skb == oskb &&
>> +        ktime_equal(this_cpu_ptr(ro->uniq)->tstamp, oskb->tstamp)) {
>> +        return;
>> +    } else {
>> +        this_cpu_ptr(ro->uniq)->skb = oskb;
>> +        this_cpu_ptr(ro->uniq)->tstamp = oskb->tstamp;
>> +    }
>> +
>>
>> Mightn't it be more robust if the above check didn't filter out a packet if
>> either of the timestamps weren't set (were zero)? Assuming the structures are
>> zeroed before this gets set.
>
> Yes. But the source just moved on with the introduction of joined filters.
> And when you don't have a unique skb detection the joint filter mechanic fails
> totally.
>
> 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.

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?

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?

Tom