Re: [RFC] CAN FD support part 1 - uncommented source

[Wolfgang Grandegger <wg@grandegger.com>]

On 05/03/2012 01:43 PM, Oliver Hartkopp wrote:
> On 03.05.2012 13:34, Wolfgang Grandegger wrote:
> 
>>> diff --git a/drivers/net/can/vcan.c b/drivers/net/can/vcan.c
>>> index ea2d942..bddbafb 100644
>>> --- a/drivers/net/can/vcan.c
>>> +++ b/drivers/net/can/vcan.c
>>> @@ -74,9 +74,8 @@ static void vcan_rx(struct sk_buff *skb, struct net_device *dev)
>>>  	struct net_device_stats *stats = &dev->stats;
>>>  
>>>  	stats->rx_packets++;
>>> -	stats->rx_bytes += cf->can_dlc;
>>> +	stats->rx_bytes += can_dlc2len(cf->can_dlc);
>>
>> Hm, I think "cf->can_dlc" should contain the *real* length. That would
>> simplify things a lot. The conversion is done when the corresponding DLC
>> registers are read or written.
> 
> 
> I thought about this idea some time too.
> 
> My problem was that 'can_dlc' says
> 
> "CAN data length *code*"
> 
> And the dlc for CAN is already well defined.
> 
>>From a todays view cf->len would indeed be a better expression :-/

Yes, definitely.

> 
> We could generally think about a union of 'can_dlc' and 'len' though, where
> can_dlc would not be part of struct canfd_frame but only the len value.

Well, we do not need dlc in the struct, just len. The conversion
len->dlc needs just to be done once when register is read/written.

> As you can see here, i also had the idea of providing both values:
> 
> https://gitorious.org/~hartkopp/linux-can/hartkopps-linux-can-next/commit/dc730b789cc1a6a3c04aaacde02c6a5a81988869
> 
> But is was a bad design, as you have two APIs modifying one value - you
> never know what to trust ... it was a sanity check hell.

If "cf->can_dlc", or however it is named, does not contain the real
length, we will end up in a dlc2len conversion nightmare. Be aware the
the struct is also used by the app.

Wolfgang.