From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wolfgang Grandegger Subject: Re: [RFC] CAN FD support part 1 - uncommented source Date: Thu, 03 May 2012 14:10:41 +0200 Message-ID: <4FA275C1.4080905@grandegger.com> References: <4FA2689D.5030905@hartkopp.net> <4FA26D28.80307@grandegger.com> <4FA26F65.5020804@hartkopp.net> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Return-path: Received: from ngcobalt02.manitu.net ([217.11.48.102]:35242 "EHLO ngcobalt02.manitu.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752482Ab2ECMKo (ORCPT ); Thu, 3 May 2012 08:10:44 -0400 In-Reply-To: <4FA26F65.5020804@hartkopp.net> Sender: linux-can-owner@vger.kernel.org List-ID: To: Oliver Hartkopp Cc: "linux-can@vger.kernel.org" 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.