CAN FD support part 1 - uncommented source

Signed-off-by Oliver Hartkopp <[email protected]>

drivers/net/can/dev.c +1 -1

index f03d7a4..1b7e843 100644
@@ -454,7 +454,7 @@ EXPORT_SYMBOL_GPL(can_bus_off);
 static void can_setup(struct net_device *dev)
 {
 	dev->type = ARPHRD_CAN;
-	dev->mtu = sizeof(struct can_frame);
+	dev->mtu = CAN_MTU;
 	dev->hard_header_len = 0;
 	dev->addr_len = 0;
 	dev->tx_queue_len = 10;

drivers/net/can/vcan.c +0 -0

index ea2d942..bddbafb 100644

Quoted 7 lines

drivers/net/can/vcan.c +0 -0

index ea2d942..bddbafb 100644
@@ -74,9 +74,8 @@ static void vcan_rx(struct sk_buff *skb, struct net_device *dev)
 	struct net_device_stats *stats = &dev->stats;
 

drivers/net/can/vcan.c static void vcan_rx(struct sk_buff *skb, struct net_device *dev) · line 77

Quoted 3 lines

drivers/net/can/vcan.c +1 -1

 	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.

Quoted 7 lines · Wolfgang Grandegger

Quoted 7 lines

drivers/net/can/vcan.c +0 -0

index ea2d942..bddbafb 100644
@@ -74,9 +74,8 @@ static void vcan_rx(struct sk_buff *skb, struct net_device *dev)
 	struct net_device_stats *stats = &dev->stats;
 

Quoted 7 lines

drivers/net/can/vcan.c static void vcan_rx(struct sk_buff *skb, struct net_device *dev) · line 77

Quoted 3 lines

drivers/net/can/vcan.c +1 -1

 	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.

Quoted 1 line

From a todays view cf->len would indeed be a better expression :-/

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.

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.

Quoted 18 lines · Oliver Hartkopp

Quoted 7 lines · Wolfgang Grandegger

Quoted 7 lines

drivers/net/can/vcan.c +0 -0

index ea2d942..bddbafb 100644
@@ -74,9 +74,8 @@ static void vcan_rx(struct sk_buff *skb, struct net_device *dev)
 	struct net_device_stats *stats = &dev->stats;
 

Quoted 7 lines

drivers/net/can/vcan.c static void vcan_rx(struct sk_buff *skb, struct net_device *dev) · line 77

Quoted 3 lines

drivers/net/can/vcan.c +1 -1

 	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.

Quoted 9 lines

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.

Quoted 1 line

From a todays view cf->len would indeed be a better expression :-/

Yes, definitely.

Quoted 3 lines

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.

Quoted 2 lines

As you can see here, i also had the idea of providing both values:

Quoted 4 lines

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-can" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 45 lines · Wolfgang Grandegger

Quoted 18 lines · Oliver Hartkopp

Quoted 7 lines · Wolfgang Grandegger

Quoted 7 lines

drivers/net/can/vcan.c +0 -0

index ea2d942..bddbafb 100644
@@ -74,9 +74,8 @@ static void vcan_rx(struct sk_buff *skb, struct net_device *dev)
 	struct net_device_stats *stats = &dev->stats;
 

Quoted 7 lines

drivers/net/can/vcan.c static void vcan_rx(struct sk_buff *skb, struct net_device *dev) · line 77

Quoted 3 lines

drivers/net/can/vcan.c +1 -1

 	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.

Quoted 9 lines

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.

Quoted 1 line

From a todays view cf->len would indeed be a better expression :-/

Yes, definitely.

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 2 lines

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

I share this.
I'd even prefer u16 over u8 even.
No legacy issues exist when changing the meaning of can_dlc, since current CAN frames have equal values for DLC & length!

Quoted 1 line

the struct is also used by the app.

Quoted 54 lines · Kurt Van Dijck

Quoted 40 lines · Wolfgang Grandegger

Quoted 18 lines · Oliver Hartkopp

Quoted 7 lines · Wolfgang Grandegger

Quoted 7 lines

drivers/net/can/vcan.c +0 -0

index ea2d942..bddbafb 100644
@@ -74,9 +74,8 @@ static void vcan_rx(struct sk_buff *skb, struct net_device *dev)
 	struct net_device_stats *stats = &dev->stats;
 

Quoted 7 lines

drivers/net/can/vcan.c static void vcan_rx(struct sk_buff *skb, struct net_device *dev) · line 77

Quoted 3 lines

drivers/net/can/vcan.c +1 -1

 	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.

Quoted 9 lines

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.

Quoted 1 line

From a todays view cf->len would indeed be a better expression :-/

Yes, definitely.

Quoted 3 lines

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.

Quoted 2 lines

As you can see here, i also had the idea of providing both values:

Quoted 7 lines

Quoted 4 lines

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

I share this.
I'd even prefer u16 over u8 even.
No legacy issues exist when changing the meaning of can_dlc, since current CAN frames have equal values for DLC & length!

Quoted 1 line

Quoted 1 line

the struct is also used by the app.

Yes - that's the main problem IMO.

What about this binary compatible introduction of cf->len ...

Furthermore, the user should be allowed to specify *any* lenght below max, als 57 bytes. It's than up to the driver to do the necessary padding.

Quoted 3 lines

What about this binary compatible introduction of cf->len ...

Looks good.

Quoted 3 lines

Furthermore, the user should be allowed to specify *any* lenght below max, als 57 bytes. It's than up to the driver to do the necessary padding.

I think Oliver solved this (on af_can level?) with a table len2dlc (static in the header?)

Quoted 6 lines

Quoted 4 lines

What about this binary compatible introduction of cf->len ...

Looks good.

Yep, given 2 structs, this illustrates the contents very well!

Quoted 6 lines

I share this.
I'd even prefer u16 over u8 even.
No legacy issues exist when changing the meaning of can_dlc, since current CAN frames have equal values for DLC & length!

Quoted 1 line

the struct is also used by the app.

Yes - that's the main problem IMO.

What about this binary compatible introduction of cf->len ...

On 05/03/2012 02:38 PM, Oliver Hartkopp wrote:
[...]

Quoted 8 lines

Quoted 6 lines

I share this.
I'd even prefer u16 over u8 even.
No legacy issues exist when changing the meaning of can_dlc, since current CAN frames have equal values for DLC & length!

Quoted 1 line

the struct is also used by the app.

Quoted 3 lines

Yes - that's the main problem IMO.

What about this binary compatible introduction of cf->len ...

...but it's not 100% source compatible, due to a compiler bug in gcc. You cannot use C99 initializers on anonymous unions in certain gcc versions.

[..]

include/linux/can.h typedef __u32 can_err_mask_t; · line 65

Quoted 13 lines

include/linux/can.h +4 -1

@@ -57,7 +57,10 @@ typedef __u32 can_err_mask_t;
  */
 struct can_frame {
 	canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */
-	__u8    can_dlc; /* data length code: 0 .. 8 */
+	union {
+		__u8    can_dlc; /* data length code: 0 .. 8 */
+		__u8    len; /* data length: 0 .. 8 */
+	};
 	__u8    data[8] __attribute__((aligned(8)));
 };