[PATCH] can: length: add definitions for frame lengths in bits

[PATCH] can: length: add definitions for frame lengths in bits

[Vincent Mailhol]

When created in [1], frames length definitions were added to implement
byte queue limits (bql). Because bql expects lengths in bytes, bit
length definitions were not considered back then.

Recently, a need to refer to the exact frame length in bits, with CAN
bit stuffing, appeared in [2].

Add 9 frames length definitions:

 - CAN_FRAME_OVERHEAD_SFF_BITS:
 - CAN_FRAME_OVERHEAD_EFF_BITS
 - CANFD_FRAME_OVERHEAD_SFF_BITS
 - CANFD_FRAME_OVERHEAD_EFF_BITS
 - CAN_BIT_STUFFING_OVERHEAD
 - CAN_FRAME_LEN_MAX_BITS_NO_STUFFING
 - CAN_FRAME_LEN_MAX_BITS_STUFFING
 - CANFD_FRAME_LEN_MAX_BITS_NO_STUFFING
 - CANFD_FRAME_LEN_MAX_BITS_STUFFING

CAN_FRAME_LEN_MAX_BITS_STUFFING and CANFD_FRAME_LEN_MAX_BITS_STUFFING
define respectively the maximum number of bits in a classical CAN and
CAN-FD frame including bit stuffing. The other definitions are
intermediate values.

In addition to the above:

 - Include linux/bits.h and then replace the value 8 by BITS_PER_BYTE
   whenever relevant.
 - Include linux/math.h because of DIV_ROUND_UP(). N.B: the use of
   DIV_ROUND_UP() is not new to this patch, but the include was
   previously omitted.
 - Update the existing length definitions to use the newly defined values.
 - Add myself as copyright owner for 2020 (as coauthor of the initial
   version, c.f. [1]) and for 2023 (this patch).

[1] commit 85d99c3e2a13 ("can: length: can_skb_get_frame_len(): introduce
    function to get data length of frame in data link layer")
Link: https://git.kernel.org/torvalds/c/85d99c3e2a13

[2] RE: [PATCH] can: mcp251xfd: Increase poll timeout
Link: https://lore.kernel.org/linux-can/BL3PR11MB64846C83ACD04E9330B0FE66FB729@BL3PR11MB6484.namprd11.prod.outlook.com/

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
As always, let me know if you have better inspiration than me for the
naming.
---
 include/linux/can/length.h | 84 ++++++++++++++++++++++++++++++++------
 1 file changed, 72 insertions(+), 12 deletions(-)

diff --git a/include/linux/can/length.h b/include/linux/can/length.h
index 6995092b774e..60492fcbe34d 100644
--- a/include/linux/can/length.h
+++ b/include/linux/can/length.h
@@ -1,13 +1,17 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 /* Copyright (C) 2020 Oliver Hartkopp <socketcan@hartkopp.net>
  * Copyright (C) 2020 Marc Kleine-Budde <kernel@pengutronix.de>
+ * Copyright (C) 2020, 2023 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
  */
 
 #ifndef _CAN_LENGTH_H
 #define _CAN_LENGTH_H
 
+#include <linux/bits.h>
+#include <linux/math.h>
+
 /*
- * Size of a Classical CAN Standard Frame
+ * Size of a Classical CAN Standard Frame in bits
  *
  * Name of Field			Bits
  * ---------------------------------------------------------
@@ -25,12 +29,19 @@
  * End-of-frame (EOF)			7
  * Inter frame spacing			3
  *
- * rounded up and ignoring bitstuffing
+ * ignoring bitstuffing
  */
-#define CAN_FRAME_OVERHEAD_SFF DIV_ROUND_UP(47, 8)
+#define CAN_FRAME_OVERHEAD_SFF_BITS 47
 
 /*
- * Size of a Classical CAN Extended Frame
+ * Size of a Classical CAN Standard Frame
+ * (rounded up and ignoring bitstuffing)
+ */
+#define CAN_FRAME_OVERHEAD_SFF \
+	DIV_ROUND_UP(CAN_FRAME_OVERHEAD_SFF_BITS, BITS_PER_BYTE)
+
+/*
+ * Size of a Classical CAN Extended Frame in bits
  *
  * Name of Field			Bits
  * ---------------------------------------------------------
@@ -50,12 +61,19 @@
  * End-of-frame (EOF)			7
  * Inter frame spacing			3
  *
- * rounded up and ignoring bitstuffing
+ * ignoring bitstuffing
  */
-#define CAN_FRAME_OVERHEAD_EFF DIV_ROUND_UP(67, 8)
+#define CAN_FRAME_OVERHEAD_EFF_BITS 67
 
 /*
- * Size of a CAN-FD Standard Frame
+ * Size of a Classical CAN Extended Frame
+ * (rounded up and ignoring bitstuffing)
+ */
+#define CAN_FRAME_OVERHEAD_EFF \
+	DIV_ROUND_UP(CAN_FRAME_OVERHEAD_EFF_BITS, BITS_PER_BYTE)
+
+/*
+ * Size of a CAN-FD Standard Frame in bits
  *
  * Name of Field			Bits
  * ---------------------------------------------------------
@@ -77,12 +95,19 @@
  * End-of-frame (EOF)			7
  * Inter frame spacing			3
  *
- * assuming CRC21, rounded up and ignoring bitstuffing
+ * assuming CRC21 and ignoring bitstuffing
  */
-#define CANFD_FRAME_OVERHEAD_SFF DIV_ROUND_UP(61, 8)
+#define CANFD_FRAME_OVERHEAD_SFF_BITS 61
 
 /*
- * Size of a CAN-FD Extended Frame
+ * Size of a CAN-FD Standard Frame
+ * (assuming CRC21, rounded up and ignoring bitstuffing)
+ */
+#define CANFD_FRAME_OVERHEAD_SFF \
+	DIV_ROUND_UP(CANFD_FRAME_OVERHEAD_SFF_BITS, BITS_PER_BYTE)
+
+/*
+ * Size of a CAN-FD Extended Frame in bits
  *
  * Name of Field			Bits
  * ---------------------------------------------------------
@@ -106,9 +131,32 @@
  * End-of-frame (EOF)			7
  * Inter frame spacing			3
  *
- * assuming CRC21, rounded up and ignoring bitstuffing
+ * assuming CRC21 and ignoring bitstuffing
+ */
+#define CANFD_FRAME_OVERHEAD_EFF_BITS 80
+
+/*
+ * Size of a CAN-FD Extended Frame
+ * (assuming CRC21, rounded up and ignoring bitstuffing)
+ */
+#define CANFD_FRAME_OVERHEAD_EFF \
+	DIV_ROUND_UP(CANFD_FRAME_OVERHEAD_EFF_BITS, BITS_PER_BYTE)
+
+/* CAN bit stuffing overhead multiplication factor */
+#define CAN_BIT_STUFFING_OVERHEAD 1.2
+
+/*
+ * Maximum size of a Classical CAN frame in bits, ignoring bitstuffing
  */
-#define CANFD_FRAME_OVERHEAD_EFF DIV_ROUND_UP(80, 8)
+#define CAN_FRAME_LEN_MAX_BITS_NO_STUFFING \
+	(CAN_FRAME_OVERHEAD_EFF_BITS + CAN_MAX_DLEN * BITS_PER_BYTE)
+
+/*
+ * Maximum size of a Classical CAN frame in bits, including bitstuffing
+ */
+#define CAN_FRAME_LEN_MAX_BITS_STUFFING				\
+	((unsigned int)(CAN_FRAME_LEN_MAX_BITS_NO_STUFFING *	\
+			CAN_BIT_STUFFING_OVERHEAD))
 
 /*
  * Maximum size of a Classical CAN frame
@@ -116,6 +164,18 @@
  */
 #define CAN_FRAME_LEN_MAX (CAN_FRAME_OVERHEAD_EFF + CAN_MAX_DLEN)
 
+/*
+ * Maximum size of a CAN-FD frame in bits, ignoring bitstuffing
+ */
+#define CANFD_FRAME_LEN_MAX_BITS_NO_STUFFING \
+	(CANFD_FRAME_OVERHEAD_EFF_BITS + CANFD_MAX_DLEN * BITS_PER_BYTE)
+
+/*
+ * Maximum size of a CAN-FD frame in bits, ignoring bitstuffing
+ */
+#define CANFD_FRAME_LEN_MAX_BITS_STUFFING			\
+	((unsigned int)(CANFD_FRAME_LEN_MAX_BITS_NO_STUFFING *	\
+			CAN_BIT_STUFFING_OVERHEAD))
 /*
  * Maximum size of a CAN-FD frame
  * (rounded up and ignoring bitstuffing)
-- 
2.39.3

RE: [PATCH] can: length: add definitions for frame lengths in bits

[Thomas.Kopp]

> When created in [1], frames length definitions were added to implement
> byte queue limits (bql). Because bql expects lengths in bytes, bit
> length definitions were not considered back then.
> 
> Recently, a need to refer to the exact frame length in bits, with CAN
> bit stuffing, appeared in [2].
> 
> Add 9 frames length definitions:
> 
>  - CAN_FRAME_OVERHEAD_SFF_BITS:
>  - CAN_FRAME_OVERHEAD_EFF_BITS
>  - CANFD_FRAME_OVERHEAD_SFF_BITS
>  - CANFD_FRAME_OVERHEAD_EFF_BITS
>  - CAN_BIT_STUFFING_OVERHEAD
>  - CAN_FRAME_LEN_MAX_BITS_NO_STUFFING
>  - CAN_FRAME_LEN_MAX_BITS_STUFFING
>  - CANFD_FRAME_LEN_MAX_BITS_NO_STUFFING
>  - CANFD_FRAME_LEN_MAX_BITS_STUFFING
> 
> CAN_FRAME_LEN_MAX_BITS_STUFFING and
> CANFD_FRAME_LEN_MAX_BITS_STUFFING
> define respectively the maximum number of bits in a classical CAN and
> CAN-FD frame including bit stuffing. The other definitions are
> intermediate values.
> 
> In addition to the above:
> 
>  - Include linux/bits.h and then replace the value 8 by BITS_PER_BYTE
>    whenever relevant.
>  - Include linux/math.h because of DIV_ROUND_UP(). N.B: the use of
>    DIV_ROUND_UP() is not new to this patch, but the include was
>    previously omitted.
>  - Update the existing length definitions to use the newly defined values.
>  - Add myself as copyright owner for 2020 (as coauthor of the initial
>    version, c.f. [1]) and for 2023 (this patch).
> 
> [1] commit 85d99c3e2a13 ("can: length: can_skb_get_frame_len(): introduce
>     function to get data length of frame in data link layer")
> Link: https://git.kernel.org/torvalds/c/85d99c3e2a13
> 
> [2] RE: [PATCH] can: mcp251xfd: Increase poll timeout
> Link: https://lore.kernel.org/linux-
> can/BL3PR11MB64846C83ACD04E9330B0FE66FB729@BL3PR11MB6484.n
> amprd11.prod.outlook.com/
> 
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> ---
> As always, let me know if you have better inspiration than me for the
> naming.
> ---
>  include/linux/can/length.h | 84 ++++++++++++++++++++++++++++++++----
> --
>  1 file changed, 72 insertions(+), 12 deletions(-)
> 
> diff --git a/include/linux/can/length.h b/include/linux/can/length.h
> index 6995092b774e..60492fcbe34d 100644
> --- a/include/linux/can/length.h
> +++ b/include/linux/can/length.h
> @@ -1,13 +1,17 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
>  /* Copyright (C) 2020 Oliver Hartkopp <socketcan@hartkopp.net>
>   * Copyright (C) 2020 Marc Kleine-Budde <kernel@pengutronix.de>
> + * Copyright (C) 2020, 2023 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>   */
> 
>  #ifndef _CAN_LENGTH_H
>  #define _CAN_LENGTH_H
> 
> +#include <linux/bits.h>
> +#include <linux/math.h>
> +
>  /*
> - * Size of a Classical CAN Standard Frame
> + * Size of a Classical CAN Standard Frame in bits
>   *
>   * Name of Field                       Bits
>   * ---------------------------------------------------------
> @@ -25,12 +29,19 @@
>   * End-of-frame (EOF)                  7
>   * Inter frame spacing                 3
>   *
> - * rounded up and ignoring bitstuffing
> + * ignoring bitstuffing
>   */
> -#define CAN_FRAME_OVERHEAD_SFF DIV_ROUND_UP(47, 8)
> +#define CAN_FRAME_OVERHEAD_SFF_BITS 47
> 
>  /*
> - * Size of a Classical CAN Extended Frame
> + * Size of a Classical CAN Standard Frame
> + * (rounded up and ignoring bitstuffing)
> + */
> +#define CAN_FRAME_OVERHEAD_SFF \
> +       DIV_ROUND_UP(CAN_FRAME_OVERHEAD_SFF_BITS, BITS_PER_BYTE)
> +
> +/*
> + * Size of a Classical CAN Extended Frame in bits
>   *
>   * Name of Field                       Bits
>   * ---------------------------------------------------------
> @@ -50,12 +61,19 @@
>   * End-of-frame (EOF)                  7
>   * Inter frame spacing                 3
>   *
> - * rounded up and ignoring bitstuffing
> + * ignoring bitstuffing
>   */
> -#define CAN_FRAME_OVERHEAD_EFF DIV_ROUND_UP(67, 8)
> +#define CAN_FRAME_OVERHEAD_EFF_BITS 67
> 
>  /*
> - * Size of a CAN-FD Standard Frame
> + * Size of a Classical CAN Extended Frame
> + * (rounded up and ignoring bitstuffing)
> + */
> +#define CAN_FRAME_OVERHEAD_EFF \
> +       DIV_ROUND_UP(CAN_FRAME_OVERHEAD_EFF_BITS, BITS_PER_BYTE)
> +
> +/*
> + * Size of a CAN-FD Standard Frame in bits
>   *
>   * Name of Field                       Bits
>   * ---------------------------------------------------------
> @@ -77,12 +95,19 @@
>   * End-of-frame (EOF)                  7
>   * Inter frame spacing                 3
>   *
> - * assuming CRC21, rounded up and ignoring bitstuffing
> + * assuming CRC21 and ignoring bitstuffing
>   */
> -#define CANFD_FRAME_OVERHEAD_SFF DIV_ROUND_UP(61, 8)
> +#define CANFD_FRAME_OVERHEAD_SFF_BITS 61
> 
>  /*
> - * Size of a CAN-FD Extended Frame
> + * Size of a CAN-FD Standard Frame
> + * (assuming CRC21, rounded up and ignoring bitstuffing)
> + */
> +#define CANFD_FRAME_OVERHEAD_SFF \
> +       DIV_ROUND_UP(CANFD_FRAME_OVERHEAD_SFF_BITS, BITS_PER_BYTE)
> +
> +/*
> + * Size of a CAN-FD Extended Frame in bits
>   *
>   * Name of Field                       Bits
>   * ---------------------------------------------------------
> @@ -106,9 +131,32 @@
>   * End-of-frame (EOF)                  7
>   * Inter frame spacing                 3
>   *
> - * assuming CRC21, rounded up and ignoring bitstuffing
> + * assuming CRC21 and ignoring bitstuffing
> + */
> +#define CANFD_FRAME_OVERHEAD_EFF_BITS 80
> +
> +/*
> + * Size of a CAN-FD Extended Frame
> + * (assuming CRC21, rounded up and ignoring bitstuffing)
> + */
> +#define CANFD_FRAME_OVERHEAD_EFF \
> +       DIV_ROUND_UP(CANFD_FRAME_OVERHEAD_EFF_BITS, BITS_PER_BYTE)
> +
> +/* CAN bit stuffing overhead multiplication factor */
> +#define CAN_BIT_STUFFING_OVERHEAD 1.2
> +
> +/*
> + * Maximum size of a Classical CAN frame in bits, ignoring bitstuffing
>   */
> -#define CANFD_FRAME_OVERHEAD_EFF DIV_ROUND_UP(80, 8)
> +#define CAN_FRAME_LEN_MAX_BITS_NO_STUFFING \
> +       (CAN_FRAME_OVERHEAD_EFF_BITS + CAN_MAX_DLEN *
> BITS_PER_BYTE)
> +
> +/*
> + * Maximum size of a Classical CAN frame in bits, including bitstuffing
> + */
> +#define CAN_FRAME_LEN_MAX_BITS_STUFFING                                \
> +       ((unsigned int)(CAN_FRAME_LEN_MAX_BITS_NO_STUFFING *    \
> +                       CAN_BIT_STUFFING_OVERHEAD))
> 
>  /*
>   * Maximum size of a Classical CAN frame
> @@ -116,6 +164,18 @@
>   */
>  #define CAN_FRAME_LEN_MAX (CAN_FRAME_OVERHEAD_EFF +
> CAN_MAX_DLEN)
> 
> +/*
> + * Maximum size of a CAN-FD frame in bits, ignoring bitstuffing
> + */
> +#define CANFD_FRAME_LEN_MAX_BITS_NO_STUFFING \
> +       (CANFD_FRAME_OVERHEAD_EFF_BITS + CANFD_MAX_DLEN *
> BITS_PER_BYTE)
> +
> +/*
> + * Maximum size of a CAN-FD frame in bits, ignoring bitstuffing
> + */
> +#define CANFD_FRAME_LEN_MAX_BITS_STUFFING                      \
> +       ((unsigned int)(CANFD_FRAME_LEN_MAX_BITS_NO_STUFFING *  \
> +                       CAN_BIT_STUFFING_OVERHEAD))
>  /*
>   * Maximum size of a CAN-FD frame
>   * (rounded up and ignoring bitstuffing)
> --
I was working on the same thing on Friday but didn't get around to sending it off, so here are a couple thoughts I had when working on the defines in length.h

The definitions for IFS in here are called intermission in the standard and I'd argue they shouldn't be part of the frame at all. To quote the ISO: "DFs and RFs shall be separated from preceding frames, whatever frame type they are (DF, RF, EF, OF),  by a time period called inter-frame space."
So, my suggestion would be to pull out the 3 bit IFS definition that's currently in and introduce 11 bit Bus idle and if necessary 3 bit Intermission separately.

index 6995092b774ec..62e92c1553376 100644
--- a/include/linux/can/length.h
+++ b/include/linux/can/length.h
@@ -6,6 +6,26 @@
 #ifndef _CAN_LENGTH_H
 #define _CAN_LENGTH_H

+/*
+ * First part of the Inter Frame Space
+ */
+#define CAN_INTERMISSION_BITS 3
+
+/*
+ * Number of consecutive recessive bits on the bus for integration etc.
+ */
+#define CAN_IDLE_CONDITION_BITS 11
+

The field currently called Stuff bit count (SBC) is also not correct I'd say. I'm not sure about the history but given that this is dependent on the DLC I think what's meant is the number of Fixed Stuff bits (FSB) . The ISO does not define a term for the Stuff bit Count but the CiA did define/document it this way. What's meant though is not the number of fixed stuff bits (FSB) which the comment implies here but the modulo 8 3 bit gray-code followed by the parity bit. So for the FD frame definitions I'd propose something like this: Renaming the current SBC to FSB and adding the SBC.
/*
  * Size of a CAN-FD Standard Frame
@@ -69,17 +87,17 @@
  * Error Status Indicator (ESI)                1
  * Data length code (DLC)              4
  * Data field                          0...512
- * Stuff Bit Count (SBC)               0...16: 4 20...64:5
+ * Stuff Bit Count (SBC)               4
  * CRC                                 0...16: 17 20...64:21
  * CRC delimiter (CD)                  1
+ * Fixed Stuff bits (FSB)              0...16: 6 20...64:7
  * ACK slot (AS)                       1
  * ACK delimiter (AD)                  1
  * End-of-frame (EOF)                  7
- * Inter frame spacing                 3
  *
- * assuming CRC21, rounded up and ignoring bitstuffing
+ * assuming CRC21, rounded up and ignoring dynamic bitstuffing
  */

Best Regards,
Thomas

Re: RE: [PATCH] can: length: add definitions for frame lengths in bits

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 3190 bytes --]

On 08.05.2023 08:54:18, Thomas.Kopp@microchip.com wrote:
> I was working on the same thing on Friday but didn't get around to
> sending it off, so here are a couple thoughts I had when working on
> the defines in length.h
> 
> The definitions for IFS in here are called intermission in the
> standard

ACK, and IMF seems to be a common abbreviation.

> and I'd argue they shouldn't be part of the frame at all.

The diagram in https://www.can-cia.org/can-knowledge/can/can-fd/
suggests that IMF is part of the frame.

> To
> quote the ISO: "DFs and RFs shall be separated from preceding frames,
> whatever frame type they are (DF, RF, EF, OF), by a time period called
> inter-frame space."
> 
> So, my suggestion would be to pull out the 3 bit IFS definition that's
> currently in and introduce 11 bit Bus idle and if necessary 3 bit
> Intermission separately.
> 
> index 6995092b774ec..62e92c1553376 100644
> --- a/include/linux/can/length.h
> +++ b/include/linux/can/length.h
> @@ -6,6 +6,26 @@
>  #ifndef _CAN_LENGTH_H
>  #define _CAN_LENGTH_H
> 
> +/*
> + * First part of the Inter Frame Space
> + */
> +#define CAN_INTERMISSION_BITS 3
> +
> +/*
> + * Number of consecutive recessive bits on the bus for integration etc.
> + */
> +#define CAN_IDLE_CONDITION_BITS 11
> +
> 
> The field currently called Stuff bit count (SBC) is also not correct
> I'd say. I'm not sure about the history but given that this is
> dependent on the DLC I think what's meant is the number of Fixed Stuff
> bits (FSB) . The ISO does not define a term for the Stuff bit Count
> but the CiA did define/document it this way. What's meant though is
> not the number of fixed stuff bits (FSB) which the comment implies
> here but the modulo 8 3 bit gray-code followed by the parity bit. So
> for the FD frame definitions I'd propose something like this: Renaming
> the current SBC to FSB and adding the SBC.

> /*
>   * Size of a CAN-FD Standard Frame
> @@ -69,17 +87,17 @@
>   * Error Status Indicator (ESI)                1
>   * Data length code (DLC)              4
>   * Data field                          0...512
> - * Stuff Bit Count (SBC)               0...16: 4 20...64:5
> + * Stuff Bit Count (SBC)               4

ACK

>   * CRC                                 0...16: 17 20...64:21
>   * CRC delimiter (CD)                  1
> + * Fixed Stuff bits (FSB)              0...16: 6 20...64:7

As far as I understand
https://ieeexplore.ieee.org/stamp/stamp.jsp?tp=&arnumber=8338047 the FSB
is 5 or 6.

>   * ACK slot (AS)                       1
>   * ACK delimiter (AD)                  1
>   * End-of-frame (EOF)                  7
> - * Inter frame spacing                 3
>   *
> - * assuming CRC21, rounded up and ignoring bitstuffing
> + * assuming CRC21, rounded up and ignoring dynamic bitstuffing
>   */
> 
> Best Regards,
> Thomas
> 

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] can: length: add definitions for frame lengths in bits

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 6551 bytes --]

On 08.05.2023 00:55:06, Vincent Mailhol wrote:
> When created in [1], frames length definitions were added to implement
> byte queue limits (bql). Because bql expects lengths in bytes, bit
> length definitions were not considered back then.
> 
> Recently, a need to refer to the exact frame length in bits, with CAN
> bit stuffing, appeared in [2].
> 
> Add 9 frames length definitions:
> 
>  - CAN_FRAME_OVERHEAD_SFF_BITS:
>  - CAN_FRAME_OVERHEAD_EFF_BITS
>  - CANFD_FRAME_OVERHEAD_SFF_BITS
>  - CANFD_FRAME_OVERHEAD_EFF_BITS
>  - CAN_BIT_STUFFING_OVERHEAD
>  - CAN_FRAME_LEN_MAX_BITS_NO_STUFFING
>  - CAN_FRAME_LEN_MAX_BITS_STUFFING
>  - CANFD_FRAME_LEN_MAX_BITS_NO_STUFFING
>  - CANFD_FRAME_LEN_MAX_BITS_STUFFING
> 
> CAN_FRAME_LEN_MAX_BITS_STUFFING and CANFD_FRAME_LEN_MAX_BITS_STUFFING
> define respectively the maximum number of bits in a classical CAN and
> CAN-FD frame including bit stuffing. The other definitions are
> intermediate values.
> 
> In addition to the above:
> 
>  - Include linux/bits.h and then replace the value 8 by BITS_PER_BYTE
>    whenever relevant.
>  - Include linux/math.h because of DIV_ROUND_UP(). N.B: the use of
>    DIV_ROUND_UP() is not new to this patch, but the include was
>    previously omitted.
>  - Update the existing length definitions to use the newly defined values.
>  - Add myself as copyright owner for 2020 (as coauthor of the initial
>    version, c.f. [1]) and for 2023 (this patch).
> 
> [1] commit 85d99c3e2a13 ("can: length: can_skb_get_frame_len(): introduce
>     function to get data length of frame in data link layer")
> Link: https://git.kernel.org/torvalds/c/85d99c3e2a13
> 
> [2] RE: [PATCH] can: mcp251xfd: Increase poll timeout
> Link: https://lore.kernel.org/linux-can/BL3PR11MB64846C83ACD04E9330B0FE66FB729@BL3PR11MB6484.namprd11.prod.outlook.com/
> 
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> ---
> As always, let me know if you have better inspiration than me for the
> naming.
> ---
>  include/linux/can/length.h | 84 ++++++++++++++++++++++++++++++++------
>  1 file changed, 72 insertions(+), 12 deletions(-)
> 
> diff --git a/include/linux/can/length.h b/include/linux/can/length.h
> index 6995092b774e..60492fcbe34d 100644
> --- a/include/linux/can/length.h
> +++ b/include/linux/can/length.h
> @@ -1,13 +1,17 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
>  /* Copyright (C) 2020 Oliver Hartkopp <socketcan@hartkopp.net>
>   * Copyright (C) 2020 Marc Kleine-Budde <kernel@pengutronix.de>
> + * Copyright (C) 2020, 2023 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>   */
>  
>  #ifndef _CAN_LENGTH_H
>  #define _CAN_LENGTH_H
>  
> +#include <linux/bits.h>
> +#include <linux/math.h>
> +
>  /*
> - * Size of a Classical CAN Standard Frame
> + * Size of a Classical CAN Standard Frame in bits
>   *
>   * Name of Field			Bits
>   * ---------------------------------------------------------
> @@ -25,12 +29,19 @@
>   * End-of-frame (EOF)			7
>   * Inter frame spacing			3
>   *
> - * rounded up and ignoring bitstuffing
> + * ignoring bitstuffing
>   */
> -#define CAN_FRAME_OVERHEAD_SFF DIV_ROUND_UP(47, 8)
> +#define CAN_FRAME_OVERHEAD_SFF_BITS 47
>  
>  /*
> - * Size of a Classical CAN Extended Frame
> + * Size of a Classical CAN Standard Frame
> + * (rounded up and ignoring bitstuffing)
> + */
> +#define CAN_FRAME_OVERHEAD_SFF \
> +	DIV_ROUND_UP(CAN_FRAME_OVERHEAD_SFF_BITS, BITS_PER_BYTE)
> +
> +/*
> + * Size of a Classical CAN Extended Frame in bits
>   *
>   * Name of Field			Bits
>   * ---------------------------------------------------------
> @@ -50,12 +61,19 @@
>   * End-of-frame (EOF)			7
>   * Inter frame spacing			3
>   *
> - * rounded up and ignoring bitstuffing
> + * ignoring bitstuffing
>   */
> -#define CAN_FRAME_OVERHEAD_EFF DIV_ROUND_UP(67, 8)
> +#define CAN_FRAME_OVERHEAD_EFF_BITS 67
>  
>  /*
> - * Size of a CAN-FD Standard Frame
> + * Size of a Classical CAN Extended Frame
> + * (rounded up and ignoring bitstuffing)
> + */
> +#define CAN_FRAME_OVERHEAD_EFF \
> +	DIV_ROUND_UP(CAN_FRAME_OVERHEAD_EFF_BITS, BITS_PER_BYTE)
> +
> +/*
> + * Size of a CAN-FD Standard Frame in bits
>   *
>   * Name of Field			Bits
>   * ---------------------------------------------------------
> @@ -77,12 +95,19 @@
>   * End-of-frame (EOF)			7
>   * Inter frame spacing			3
>   *
> - * assuming CRC21, rounded up and ignoring bitstuffing
> + * assuming CRC21 and ignoring bitstuffing
>   */
> -#define CANFD_FRAME_OVERHEAD_SFF DIV_ROUND_UP(61, 8)
> +#define CANFD_FRAME_OVERHEAD_SFF_BITS 61
>  
>  /*
> - * Size of a CAN-FD Extended Frame
> + * Size of a CAN-FD Standard Frame
> + * (assuming CRC21, rounded up and ignoring bitstuffing)
> + */
> +#define CANFD_FRAME_OVERHEAD_SFF \
> +	DIV_ROUND_UP(CANFD_FRAME_OVERHEAD_SFF_BITS, BITS_PER_BYTE)
> +
> +/*
> + * Size of a CAN-FD Extended Frame in bits
>   *
>   * Name of Field			Bits
>   * ---------------------------------------------------------
> @@ -106,9 +131,32 @@
>   * End-of-frame (EOF)			7
>   * Inter frame spacing			3
>   *
> - * assuming CRC21, rounded up and ignoring bitstuffing
> + * assuming CRC21 and ignoring bitstuffing
> + */
> +#define CANFD_FRAME_OVERHEAD_EFF_BITS 80
> +
> +/*
> + * Size of a CAN-FD Extended Frame
> + * (assuming CRC21, rounded up and ignoring bitstuffing)
> + */
> +#define CANFD_FRAME_OVERHEAD_EFF \
> +	DIV_ROUND_UP(CANFD_FRAME_OVERHEAD_EFF_BITS, BITS_PER_BYTE)
> +
> +/* CAN bit stuffing overhead multiplication factor */
> +#define CAN_BIT_STUFFING_OVERHEAD 1.2
> +
> +/*
> + * Maximum size of a Classical CAN frame in bits, ignoring bitstuffing
>   */
> -#define CANFD_FRAME_OVERHEAD_EFF DIV_ROUND_UP(80, 8)
> +#define CAN_FRAME_LEN_MAX_BITS_NO_STUFFING \
> +	(CAN_FRAME_OVERHEAD_EFF_BITS + CAN_MAX_DLEN * BITS_PER_BYTE)
> +
> +/*
> + * Maximum size of a Classical CAN frame in bits, including bitstuffing
> + */
> +#define CAN_FRAME_LEN_MAX_BITS_STUFFING				\
> +	((unsigned int)(CAN_FRAME_LEN_MAX_BITS_NO_STUFFING *	\
> +			CAN_BIT_STUFFING_OVERHEAD))

The 1.2 overhead doesn't apply to the whole frame, according to
https://ieeexplore.ieee.org/stamp/stamp.jsp?tp=&arnumber=8338047.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: RE: [PATCH] can: length: add definitions for frame lengths in bits

[Vincent MAILHOL]

Hi Thomas,

Thank you for your review. You had me reopen ISO 11898-1 (haven't done
so for a long time).

I mostly agree with you. Many of your points are not related to this
patch but to the already existing definition. So I will handle these
in a separate patch.

I will prepare a v2 within the next few days.

On Mon. 8 May 2023 à 21:29, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 08.05.2023 08:54:18, Thomas.Kopp@microchip.com wrote:
> > I was working on the same thing on Friday but didn't get around to
> > sending it off, so here are a couple thoughts I had when working on
> > the defines in length.h
> >
> > The definitions for IFS in here are called intermission in the
> > standard
>
> ACK, and IMF seems to be a common abbreviation.

ACK. I think I will put a reference to both naming (intermission and
IMF) and use the IMF forward for conciseness.

> > and I'd argue they shouldn't be part of the frame at all.
>
> The diagram in https://www.can-cia.org/can-knowledge/can/can-fd/
> suggests that IMF is part of the frame.

ISO 11898-1:2015 section 10.4.6 "Specification of inter-frame space"
makes it clear that the intermission is not part of the frame but part
of the "Inter-frame space".

To close the discussion, I would finally argue that the intermission
occurs after the EOF bit. Something after an "End of Frame" is not
part of the frame, right?

I agree with and will follow Thomas's suggestion.

> > To
> > quote the ISO: "DFs and RFs shall be separated from preceding frames,
> > whatever frame type they are (DF, RF, EF, OF), by a time period called
> > inter-frame space."
> >
> > So, my suggestion would be to pull out the 3 bit IFS definition that's
> > currently in and introduce 11 bit Bus idle and if necessary 3 bit
> > Intermission separately.

ACK.

> > index 6995092b774ec..62e92c1553376 100644
> > --- a/include/linux/can/length.h
> > +++ b/include/linux/can/length.h
> > @@ -6,6 +6,26 @@
> >  #ifndef _CAN_LENGTH_H
> >  #define _CAN_LENGTH_H
> >
> > +/*
> > + * First part of the Inter Frame Space
> > + */
> > +#define CAN_INTERMISSION_BITS 3
> > +
> > +/*
> > + * Number of consecutive recessive bits on the bus for integration etc.
> > + */
> > +#define CAN_IDLE_CONDITION_BITS 11
> > +
> >
> > The field currently called Stuff bit count (SBC) is also not correct
> > I'd say. I'm not sure about the history but given that this is
> > dependent on the DLC I think what's meant is the number of Fixed Stuff
> > bits (FSB) . The ISO does not define a term for the Stuff bit Count
> > but the CiA did define/document it this way. What's meant though is
> > not the number of fixed stuff bits (FSB) which the comment implies
> > here but the modulo 8 3 bit gray-code followed by the parity bit. So
> > for the FD frame definitions I'd propose something like this: Renaming
> > the current SBC to FSB and adding the SBC.

ACK. I double checked the standard, you are right.

> > /*
> >   * Size of a CAN-FD Standard Frame
> > @@ -69,17 +87,17 @@
> >   * Error Status Indicator (ESI)                1
> >   * Data length code (DLC)              4
> >   * Data field                          0...512
> > - * Stuff Bit Count (SBC)               0...16: 4 20...64:5
> > + * Stuff Bit Count (SBC)               4
>
> ACK
>
> >   * CRC                                 0...16: 17 20...64:21
> >   * CRC delimiter (CD)                  1
> > + * Fixed Stuff bits (FSB)              0...16: 6 20...64:7
>
> As far as I understand
> https://ieeexplore.ieee.org/stamp/stamp.jsp?tp=&arnumber=8338047 the FSB
> is 5 or 6.

Reading the ISO, the fixed bit stuff applies to the CRC field (which,
in the standard nomenclature, includes both the stuff count and the
CRC sequence).
The CRC field starts with a fixed stuff bit and then has another fixed
stuff bit after each fourth bit.

So the formula would be:

  FSB count = 1 + round_down(len(CRC field)/4)
            = 1 + round_down((len(stuff count) + len(CRC sequence))/4)

In case of CRC_17:

  FSB count = 1 + round_down((4 + 17)/4)
            = 6

This is coherent with the last figure of
https://www.can-cia.org/can-knowledge/can/can-fd/ which shows 6 FSB
for CRC_17.

In case of CRC_21:

  FSB count = 1 + round_down((4 + 21)/4)
            = 7

So, ACK for Thomas, NACK for Marc (sorry :))

> >   * ACK slot (AS)                       1
> >   * ACK delimiter (AD)                  1
> >   * End-of-frame (EOF)                  7
> > - * Inter frame spacing                 3
> >   *
> > - * assuming CRC21, rounded up and ignoring bitstuffing
> > + * assuming CRC21, rounded up and ignoring dynamic bitstuffing
> >   */

Yours sincerely,
Vincent Mailhol

Re: [PATCH] can: length: add definitions for frame lengths in bits

[Vincent MAILHOL]

On Mon. 8 May 2023 at 21:29, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 08.05.2023 00:55:06, Vincent Mailhol wrote:
> > When created in [1], frames length definitions were added to implement
> > byte queue limits (bql). Because bql expects lengths in bytes, bit
> > length definitions were not considered back then.
> >
> > Recently, a need to refer to the exact frame length in bits, with CAN
> > bit stuffing, appeared in [2].
> >
> > Add 9 frames length definitions:
> >
> >  - CAN_FRAME_OVERHEAD_SFF_BITS:
> >  - CAN_FRAME_OVERHEAD_EFF_BITS
> >  - CANFD_FRAME_OVERHEAD_SFF_BITS
> >  - CANFD_FRAME_OVERHEAD_EFF_BITS
> >  - CAN_BIT_STUFFING_OVERHEAD
> >  - CAN_FRAME_LEN_MAX_BITS_NO_STUFFING
> >  - CAN_FRAME_LEN_MAX_BITS_STUFFING
> >  - CANFD_FRAME_LEN_MAX_BITS_NO_STUFFING
> >  - CANFD_FRAME_LEN_MAX_BITS_STUFFING
> >
> > CAN_FRAME_LEN_MAX_BITS_STUFFING and CANFD_FRAME_LEN_MAX_BITS_STUFFING
> > define respectively the maximum number of bits in a classical CAN and
> > CAN-FD frame including bit stuffing. The other definitions are
> > intermediate values.
> >
> > In addition to the above:
> >
> >  - Include linux/bits.h and then replace the value 8 by BITS_PER_BYTE
> >    whenever relevant.
> >  - Include linux/math.h because of DIV_ROUND_UP(). N.B: the use of
> >    DIV_ROUND_UP() is not new to this patch, but the include was
> >    previously omitted.
> >  - Update the existing length definitions to use the newly defined values.
> >  - Add myself as copyright owner for 2020 (as coauthor of the initial
> >    version, c.f. [1]) and for 2023 (this patch).
> >
> > [1] commit 85d99c3e2a13 ("can: length: can_skb_get_frame_len(): introduce
> >     function to get data length of frame in data link layer")
> > Link: https://git.kernel.org/torvalds/c/85d99c3e2a13
> >
> > [2] RE: [PATCH] can: mcp251xfd: Increase poll timeout
> > Link: https://lore.kernel.org/linux-can/BL3PR11MB64846C83ACD04E9330B0FE66FB729@BL3PR11MB6484.namprd11.prod.outlook.com/
> >
> > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > ---
> > As always, let me know if you have better inspiration than me for the
> > naming.
> > ---
> >  include/linux/can/length.h | 84 ++++++++++++++++++++++++++++++++------
> >  1 file changed, 72 insertions(+), 12 deletions(-)
> >
> > diff --git a/include/linux/can/length.h b/include/linux/can/length.h
> > index 6995092b774e..60492fcbe34d 100644
> > --- a/include/linux/can/length.h
> > +++ b/include/linux/can/length.h
> > @@ -1,13 +1,17 @@
> >  /* SPDX-License-Identifier: GPL-2.0 */
> >  /* Copyright (C) 2020 Oliver Hartkopp <socketcan@hartkopp.net>
> >   * Copyright (C) 2020 Marc Kleine-Budde <kernel@pengutronix.de>
> > + * Copyright (C) 2020, 2023 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> >   */
> >
> >  #ifndef _CAN_LENGTH_H
> >  #define _CAN_LENGTH_H
> >
> > +#include <linux/bits.h>
> > +#include <linux/math.h>
> > +
> >  /*
> > - * Size of a Classical CAN Standard Frame
> > + * Size of a Classical CAN Standard Frame in bits
> >   *
> >   * Name of Field                     Bits
> >   * ---------------------------------------------------------
> > @@ -25,12 +29,19 @@
> >   * End-of-frame (EOF)                        7
> >   * Inter frame spacing                       3
> >   *
> > - * rounded up and ignoring bitstuffing
> > + * ignoring bitstuffing
> >   */
> > -#define CAN_FRAME_OVERHEAD_SFF DIV_ROUND_UP(47, 8)
> > +#define CAN_FRAME_OVERHEAD_SFF_BITS 47
> >
> >  /*
> > - * Size of a Classical CAN Extended Frame
> > + * Size of a Classical CAN Standard Frame
> > + * (rounded up and ignoring bitstuffing)
> > + */
> > +#define CAN_FRAME_OVERHEAD_SFF \
> > +     DIV_ROUND_UP(CAN_FRAME_OVERHEAD_SFF_BITS, BITS_PER_BYTE)
> > +
> > +/*
> > + * Size of a Classical CAN Extended Frame in bits
> >   *
> >   * Name of Field                     Bits
> >   * ---------------------------------------------------------
> > @@ -50,12 +61,19 @@
> >   * End-of-frame (EOF)                        7
> >   * Inter frame spacing                       3
> >   *
> > - * rounded up and ignoring bitstuffing
> > + * ignoring bitstuffing
> >   */
> > -#define CAN_FRAME_OVERHEAD_EFF DIV_ROUND_UP(67, 8)
> > +#define CAN_FRAME_OVERHEAD_EFF_BITS 67
> >
> >  /*
> > - * Size of a CAN-FD Standard Frame
> > + * Size of a Classical CAN Extended Frame
> > + * (rounded up and ignoring bitstuffing)
> > + */
> > +#define CAN_FRAME_OVERHEAD_EFF \
> > +     DIV_ROUND_UP(CAN_FRAME_OVERHEAD_EFF_BITS, BITS_PER_BYTE)
> > +
> > +/*
> > + * Size of a CAN-FD Standard Frame in bits
> >   *
> >   * Name of Field                     Bits
> >   * ---------------------------------------------------------
> > @@ -77,12 +95,19 @@
> >   * End-of-frame (EOF)                        7
> >   * Inter frame spacing                       3
> >   *
> > - * assuming CRC21, rounded up and ignoring bitstuffing
> > + * assuming CRC21 and ignoring bitstuffing
> >   */
> > -#define CANFD_FRAME_OVERHEAD_SFF DIV_ROUND_UP(61, 8)
> > +#define CANFD_FRAME_OVERHEAD_SFF_BITS 61
> >
> >  /*
> > - * Size of a CAN-FD Extended Frame
> > + * Size of a CAN-FD Standard Frame
> > + * (assuming CRC21, rounded up and ignoring bitstuffing)
> > + */
> > +#define CANFD_FRAME_OVERHEAD_SFF \
> > +     DIV_ROUND_UP(CANFD_FRAME_OVERHEAD_SFF_BITS, BITS_PER_BYTE)
> > +
> > +/*
> > + * Size of a CAN-FD Extended Frame in bits
> >   *
> >   * Name of Field                     Bits
> >   * ---------------------------------------------------------
> > @@ -106,9 +131,32 @@
> >   * End-of-frame (EOF)                        7
> >   * Inter frame spacing                       3
> >   *
> > - * assuming CRC21, rounded up and ignoring bitstuffing
> > + * assuming CRC21 and ignoring bitstuffing
> > + */
> > +#define CANFD_FRAME_OVERHEAD_EFF_BITS 80
> > +
> > +/*
> > + * Size of a CAN-FD Extended Frame
> > + * (assuming CRC21, rounded up and ignoring bitstuffing)
> > + */
> > +#define CANFD_FRAME_OVERHEAD_EFF \
> > +     DIV_ROUND_UP(CANFD_FRAME_OVERHEAD_EFF_BITS, BITS_PER_BYTE)
> > +
> > +/* CAN bit stuffing overhead multiplication factor */
> > +#define CAN_BIT_STUFFING_OVERHEAD 1.2
> > +
> > +/*
> > + * Maximum size of a Classical CAN frame in bits, ignoring bitstuffing
> >   */
> > -#define CANFD_FRAME_OVERHEAD_EFF DIV_ROUND_UP(80, 8)
> > +#define CAN_FRAME_LEN_MAX_BITS_NO_STUFFING \
> > +     (CAN_FRAME_OVERHEAD_EFF_BITS + CAN_MAX_DLEN * BITS_PER_BYTE)
> > +
> > +/*
> > + * Maximum size of a Classical CAN frame in bits, including bitstuffing
> > + */
> > +#define CAN_FRAME_LEN_MAX_BITS_STUFFING                              \
> > +     ((unsigned int)(CAN_FRAME_LEN_MAX_BITS_NO_STUFFING *    \
> > +                     CAN_BIT_STUFFING_OVERHEAD))
>
> The 1.2 overhead doesn't apply to the whole frame, according to
> https://ieeexplore.ieee.org/stamp/stamp.jsp?tp=&arnumber=8338047.

You are right. In fact, I realized this mistake before reading your
message (while I was studying the standard to answer Thomas).

ISO 11898-1:2015 section 10.5 "Frame coding" says:

  the frame segment as SOF, arbitration field, control field,
  data field, and CRC sequence shall be coded by the method of
  bit stuffing.

and:

  The remaining bit fields of the DF or RF (CRC delimiter, ACK
  field and EOF) shall be of fixed form and not stuffed.

So, indeed, the bit stuffing does not apply to the last 10 bits (1 + 1 + 1 + 7).
Furthermore, for FD frames, the CRC field already contains the fixed
stuff bits. So the overhead shall not be applied again or else, stuff
bits would be counted twice. In conclusion, for FD frames, the
otherhead for dynamic bit stuffing overhead should apply to the SOF,
arbitration field, control field and data field segments.

After reading the standard, I thought again about the overhead ratio
and it is more complicated than we all thought.

Let's use below nomenclature in the following examples (borrowed from ISO):

  - "0": dominant bit
  - "o": dominant stuff bit
  - "1": recessive bit
  - "i": recessive stuff bit

We probably all though below example to be the worst case:

  Destuffed: 11111  11111  11111  11111
  Stuffed:   11111o 11111o 11111o 11111o

Here, indeed, one stuff bit is added every five bits giving us an
overhead of 6/5 = 1.2.

However, ISO 11898-1:2015 section 10.5 "Frame coding" also says:

  Whenever a transmitter detects five consecutive bits (*including
  stuff bits*) of identical value in the bit stream to be
  transmitted, it shall automatically insert a complementary
  bit (called stuff bit) ...

Pay attention to the *including stuff bits* part. The worst case is
actually a sequence in which dominant and recessive alternate every
four bits:

  Destuffed: 1 1111  0000  1111  0000  1111
  Stuffed:   1 1111o 0000i 1111o 0000i 1111o

Ignoring the first bit, one stuff bit is added every four bits giving
us an overhead of 5/4 = 1.25.

The exact formula taking in account the first bit we previously ignored then:

  Number of dynamic stuff bit = 1 + round_down((len(stream) - 5) / 4)
                              = round_down((len(stream) - 1) / 4)


Yours sincerely,
Vincent Mailhol

RE: RE: [PATCH] can: length: add definitions for frame lengths in bits

[Thomas.Kopp]

> On 08.05.2023 08:54:18, Thomas.Kopp@microchip.com wrote:
> > I was working on the same thing on Friday but didn't get around to
> > sending it off, so here are a couple thoughts I had when working on
> > the defines in length.h
> >
> > The definitions for IFS in here are called intermission in the
> > standard
> 
> ACK, and IMF seems to be a common abbreviation.
> 
> > and I'd argue they shouldn't be part of the frame at all.
> 
> The diagram in https://www.can-cia.org/can-knowledge/can/can-fd/
> suggests that IMF is part of the frame.

I'd disagree as the ISO specifically says it's not part of the frame. The diagram on page PDF page 21 of the 2.0 spec: http://esd.cs.ucr.edu/webres/can20.pdf is also in the ISO and shows the Intermission outside the frame. Also the word INTERframe space suggests it shouldn't be part of the frame and lastly the definition is used for a) determining how many bits/bytes are needed to store frames which doesn't need the intermission bits and b) timing, but for those purpose the frame has ended already and if the timing of several frames is needed, complete  interframe spaces need to be added.

> > To
> > quote the ISO: "DFs and RFs shall be separated from preceding frames,
> > whatever frame type they are (DF, RF, EF, OF), by a time period called
> > inter-frame space."
> >
> > So, my suggestion would be to pull out the 3 bit IFS definition that's
> > currently in and introduce 11 bit Bus idle and if necessary 3 bit
> > Intermission separately.
> >
> > index 6995092b774ec..62e92c1553376 100644
> > --- a/include/linux/can/length.h
> > +++ b/include/linux/can/length.h
> > @@ -6,6 +6,26 @@
> >  #ifndef _CAN_LENGTH_H
> >  #define _CAN_LENGTH_H
> >
> > +/*
> > + * First part of the Inter Frame Space
> > + */
> > +#define CAN_INTERMISSION_BITS 3
> > +
> > +/*
> > + * Number of consecutive recessive bits on the bus for integration etc.
> > + */
> > +#define CAN_IDLE_CONDITION_BITS 11
> > +
> >
> > The field currently called Stuff bit count (SBC) is also not correct
> > I'd say. I'm not sure about the history but given that this is
> > dependent on the DLC I think what's meant is the number of Fixed Stuff
> > bits (FSB) . The ISO does not define a term for the Stuff bit Count
> > but the CiA did define/document it this way. What's meant though is
> > not the number of fixed stuff bits (FSB) which the comment implies
> > here but the modulo 8 3 bit gray-code followed by the parity bit. So
> > for the FD frame definitions I'd propose something like this: Renaming
> > the current SBC to FSB and adding the SBC.
> 
> >   * CRC                                 0...16: 17 20...64:21
> >   * CRC delimiter (CD)                  1
> > + * Fixed Stuff bits (FSB)              0...16: 6 20...64:7
> 
> As far as I understand
> https://ieeexplore.ieee.org/stamp/stamp.jsp?tp=&arnumber=8338047 the
> FSB
> is 5 or 6.
I don't know where the paper got its numbers from but it also seems to be missing the SBC field completely? The ISO says: " There shall be a fixed stuff bit before the first bit of the stuff count[...]" " A further fixed stuff bit shall be inserted after each fourth bit of the CRC field" Not that the CRC field in FD frames also contains the SBC so that adds fixed stuff bits.
A good visual representation of the FSBs is on the first page you provided as a source: https://www.can-cia.org/can-knowledge/can/can-fd/ all the way on the bottom. 


Best Regards,
Thomas 

Re: RE: [PATCH] can: length: add definitions for frame lengths in bits

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2488 bytes --]

On 09.05.2023 13:16:08, Vincent MAILHOL wrote:
> > The diagram in https://www.can-cia.org/can-knowledge/can/can-fd/
> > suggests that IMF is part of the frame.
> 
> ISO 11898-1:2015 section 10.4.6 "Specification of inter-frame space"
> makes it clear that the intermission is not part of the frame but part
> of the "Inter-frame space".

For this reason, it is good to have open standards...oh wait!

> To close the discussion, I would finally argue that the intermission
> occurs after the EOF bit. Something after an "End of Frame" is not
> part of the frame, right?
> 
> I agree with and will follow Thomas's suggestion.

[...]

> > > /*
> > >   * Size of a CAN-FD Standard Frame
> > > @@ -69,17 +87,17 @@
> > >   * Error Status Indicator (ESI)                1
> > >   * Data length code (DLC)              4
> > >   * Data field                          0...512
> > > - * Stuff Bit Count (SBC)               0...16: 4 20...64:5
> > > + * Stuff Bit Count (SBC)               4
> >
> > ACK
> >
> > >   * CRC                                 0...16: 17 20...64:21
> > >   * CRC delimiter (CD)                  1
> > > + * Fixed Stuff bits (FSB)              0...16: 6 20...64:7
> >
> > As far as I understand
> > https://ieeexplore.ieee.org/stamp/stamp.jsp?tp=&arnumber=8338047 the FSB
> > is 5 or 6.
> 
> Reading the ISO, the fixed bit stuff applies to the CRC field (which,
> in the standard nomenclature, includes both the stuff count and the
> CRC sequence).
> The CRC field starts with a fixed stuff bit and then has another fixed
> stuff bit after each fourth bit.
> 
> So the formula would be:
> 
>   FSB count = 1 + round_down(len(CRC field)/4)
>             = 1 + round_down((len(stuff count) + len(CRC sequence))/4)
> 
> In case of CRC_17:
> 
>   FSB count = 1 + round_down((4 + 17)/4)
>             = 6
> 
> This is coherent with the last figure of
> https://www.can-cia.org/can-knowledge/can/can-fd/ which shows 6 FSB
> for CRC_17.
> 
> In case of CRC_21:
> 
>   FSB count = 1 + round_down((4 + 21)/4)
>             = 7
> 
> So, ACK for Thomas, NACK for Marc (sorry :))

It seems that the reviewers of this paper missed some parts :)

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH] can: length: add definitions for frame lengths in bits

[Thomas.Kopp]

Hi Vincent,

> On Mon. 8 May 2023 at 21:29, Marc Kleine-Budde <mkl@pengutronix.de>
> wrote:
> > On 08.05.2023 00:55:06, Vincent Mailhol wrote:
> > > When created in [1], frames length definitions were added to implement
> > > byte queue limits (bql). Because bql expects lengths in bytes, bit
> > > length definitions were not considered back then.
> > >
> > > Recently, a need to refer to the exact frame length in bits, with CAN
> > > bit stuffing, appeared in [2].
> > >
> > > Add 9 frames length definitions:
> > >
> > >  - CAN_FRAME_OVERHEAD_SFF_BITS:
> > >  - CAN_FRAME_OVERHEAD_EFF_BITS
> > >  - CANFD_FRAME_OVERHEAD_SFF_BITS
> > >  - CANFD_FRAME_OVERHEAD_EFF_BITS
> > >  - CAN_BIT_STUFFING_OVERHEAD
> > >  - CAN_FRAME_LEN_MAX_BITS_NO_STUFFING
> > >  - CAN_FRAME_LEN_MAX_BITS_STUFFING
> > >  - CANFD_FRAME_LEN_MAX_BITS_NO_STUFFING
> > >  - CANFD_FRAME_LEN_MAX_BITS_STUFFING
> > >
> > > CAN_FRAME_LEN_MAX_BITS_STUFFING and
> CANFD_FRAME_LEN_MAX_BITS_STUFFING
> > > define respectively the maximum number of bits in a classical CAN and
> > > CAN-FD frame including bit stuffing. The other definitions are
> > > intermediate values.
> > >
> > > In addition to the above:
> > >
> > >  - Include linux/bits.h and then replace the value 8 by BITS_PER_BYTE
> > >    whenever relevant.
> > >  - Include linux/math.h because of DIV_ROUND_UP(). N.B: the use of
> > >    DIV_ROUND_UP() is not new to this patch, but the include was
> > >    previously omitted.
> > >  - Update the existing length definitions to use the newly defined values.
> > >  - Add myself as copyright owner for 2020 (as coauthor of the initial
> > >    version, c.f. [1]) and for 2023 (this patch).
> > >
> > > [1] commit 85d99c3e2a13 ("can: length: can_skb_get_frame_len():
> introduce
> > >     function to get data length of frame in data link layer")
> > > Link: https://git.kernel.org/torvalds/c/85d99c3e2a13
> > >
> > > [2] RE: [PATCH] can: mcp251xfd: Increase poll timeout
> > > Link: https://lore.kernel.org/linux-
> can/BL3PR11MB64846C83ACD04E9330B0FE66FB729@BL3PR11MB6484.n
> amprd11.prod.outlook.com/
> > >
> > > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > > ---
> > > As always, let me know if you have better inspiration than me for the
> > > naming.
> > > ---
> > >  include/linux/can/length.h | 84
> ++++++++++++++++++++++++++++++++------
> > >  1 file changed, 72 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/include/linux/can/length.h b/include/linux/can/length.h
> > > index 6995092b774e..60492fcbe34d 100644
> > > --- a/include/linux/can/length.h
> > > +++ b/include/linux/can/length.h
> > > @@ -1,13 +1,17 @@
> > >  /* SPDX-License-Identifier: GPL-2.0 */
> > >  /* Copyright (C) 2020 Oliver Hartkopp <socketcan@hartkopp.net>
> > >   * Copyright (C) 2020 Marc Kleine-Budde <kernel@pengutronix.de>
> > > + * Copyright (C) 2020, 2023 Vincent Mailhol
> <mailhol.vincent@wanadoo.fr>
> > >   */
> > >
> > >  #ifndef _CAN_LENGTH_H
> > >  #define _CAN_LENGTH_H
> > >
> > > +#include <linux/bits.h>
> > > +#include <linux/math.h>
> > > +
> > >  /*
> > > - * Size of a Classical CAN Standard Frame
> > > + * Size of a Classical CAN Standard Frame in bits
> > >   *
> > >   * Name of Field                     Bits
> > >   * ---------------------------------------------------------
> > > @@ -25,12 +29,19 @@
> > >   * End-of-frame (EOF)                        7
> > >   * Inter frame spacing                       3
> > >   *
> > > - * rounded up and ignoring bitstuffing
> > > + * ignoring bitstuffing
> > >   */
> > > -#define CAN_FRAME_OVERHEAD_SFF DIV_ROUND_UP(47, 8)
> > > +#define CAN_FRAME_OVERHEAD_SFF_BITS 47
> > >
> > >  /*
> > > - * Size of a Classical CAN Extended Frame
> > > + * Size of a Classical CAN Standard Frame
> > > + * (rounded up and ignoring bitstuffing)
> > > + */
> > > +#define CAN_FRAME_OVERHEAD_SFF \
> > > +     DIV_ROUND_UP(CAN_FRAME_OVERHEAD_SFF_BITS, BITS_PER_BYTE)
> > > +
> > > +/*
> > > + * Size of a Classical CAN Extended Frame in bits
> > >   *
> > >   * Name of Field                     Bits
> > >   * ---------------------------------------------------------
> > > @@ -50,12 +61,19 @@
> > >   * End-of-frame (EOF)                        7
> > >   * Inter frame spacing                       3
> > >   *
> > > - * rounded up and ignoring bitstuffing
> > > + * ignoring bitstuffing
> > >   */
> > > -#define CAN_FRAME_OVERHEAD_EFF DIV_ROUND_UP(67, 8)
> > > +#define CAN_FRAME_OVERHEAD_EFF_BITS 67
> > >
> > >  /*
> > > - * Size of a CAN-FD Standard Frame
> > > + * Size of a Classical CAN Extended Frame
> > > + * (rounded up and ignoring bitstuffing)
> > > + */
> > > +#define CAN_FRAME_OVERHEAD_EFF \
> > > +     DIV_ROUND_UP(CAN_FRAME_OVERHEAD_EFF_BITS, BITS_PER_BYTE)
> > > +
> > > +/*
> > > + * Size of a CAN-FD Standard Frame in bits
> > >   *
> > >   * Name of Field                     Bits
> > >   * ---------------------------------------------------------
> > > @@ -77,12 +95,19 @@
> > >   * End-of-frame (EOF)                        7
> > >   * Inter frame spacing                       3
> > >   *
> > > - * assuming CRC21, rounded up and ignoring bitstuffing
> > > + * assuming CRC21 and ignoring bitstuffing
> > >   */
> > > -#define CANFD_FRAME_OVERHEAD_SFF DIV_ROUND_UP(61, 8)
> > > +#define CANFD_FRAME_OVERHEAD_SFF_BITS 61
> > >
> > >  /*
> > > - * Size of a CAN-FD Extended Frame
> > > + * Size of a CAN-FD Standard Frame
> > > + * (assuming CRC21, rounded up and ignoring bitstuffing)
> > > + */
> > > +#define CANFD_FRAME_OVERHEAD_SFF \
> > > +     DIV_ROUND_UP(CANFD_FRAME_OVERHEAD_SFF_BITS,
> BITS_PER_BYTE)
> > > +
> > > +/*
> > > + * Size of a CAN-FD Extended Frame in bits
> > >   *
> > >   * Name of Field                     Bits
> > >   * ---------------------------------------------------------
> > > @@ -106,9 +131,32 @@
> > >   * End-of-frame (EOF)                        7
> > >   * Inter frame spacing                       3
> > >   *
> > > - * assuming CRC21, rounded up and ignoring bitstuffing
> > > + * assuming CRC21 and ignoring bitstuffing
> > > + */
> > > +#define CANFD_FRAME_OVERHEAD_EFF_BITS 80
> > > +
> > > +/*
> > > + * Size of a CAN-FD Extended Frame
> > > + * (assuming CRC21, rounded up and ignoring bitstuffing)
> > > + */
> > > +#define CANFD_FRAME_OVERHEAD_EFF \
> > > +     DIV_ROUND_UP(CANFD_FRAME_OVERHEAD_EFF_BITS,
> BITS_PER_BYTE)
> > > +
> > > +/* CAN bit stuffing overhead multiplication factor */
> > > +#define CAN_BIT_STUFFING_OVERHEAD 1.2
> > > +
> > > +/*
> > > + * Maximum size of a Classical CAN frame in bits, ignoring bitstuffing
> > >   */
> > > -#define CANFD_FRAME_OVERHEAD_EFF DIV_ROUND_UP(80, 8)
> > > +#define CAN_FRAME_LEN_MAX_BITS_NO_STUFFING \
> > > +     (CAN_FRAME_OVERHEAD_EFF_BITS + CAN_MAX_DLEN *
> BITS_PER_BYTE)
> > > +
> > > +/*
> > > + * Maximum size of a Classical CAN frame in bits, including bitstuffing
> > > + */
> > > +#define CAN_FRAME_LEN_MAX_BITS_STUFFING                              \
> > > +     ((unsigned int)(CAN_FRAME_LEN_MAX_BITS_NO_STUFFING *    \
> > > +                     CAN_BIT_STUFFING_OVERHEAD))
> >
> > The 1.2 overhead doesn't apply to the whole frame, according to
> > https://ieeexplore.ieee.org/stamp/stamp.jsp?tp=&arnumber=8338047.
> 
> You are right. In fact, I realized this mistake before reading your
> message (while I was studying the standard to answer Thomas).
> 
> ISO 11898-1:2015 section 10.5 "Frame coding" says:
> 
>   the frame segment as SOF, arbitration field, control field,
>   data field, and CRC sequence shall be coded by the method of
>   bit stuffing.
> 
> and:
> 
>   The remaining bit fields of the DF or RF (CRC delimiter, ACK
>   field and EOF) shall be of fixed form and not stuffed.
> 
> So, indeed, the bit stuffing does not apply to the last 10 bits (1 + 1 + 1 + 7).
> Furthermore, for FD frames, the CRC field already contains the fixed
> stuff bits. So the overhead shall not be applied again or else, stuff
> bits would be counted twice. In conclusion, for FD frames, the
> otherhead for dynamic bit stuffing overhead should apply to the SOF,
> arbitration field, control field and data field segments.
> 
> After reading the standard, I thought again about the overhead ratio
> and it is more complicated than we all thought.
> 
> Let's use below nomenclature in the following examples (borrowed from ISO):
> 
>   - "0": dominant bit
>   - "o": dominant stuff bit
>   - "1": recessive bit
>   - "i": recessive stuff bit
> 
> We probably all though below example to be the worst case:
> 
>   Destuffed: 11111  11111  11111  11111
>   Stuffed:   11111o 11111o 11111o 11111o
> 
> Here, indeed, one stuff bit is added every five bits giving us an
> overhead of 6/5 = 1.2.
> 
> However, ISO 11898-1:2015 section 10.5 "Frame coding" also says:
> 
>   Whenever a transmitter detects five consecutive bits (*including
>   stuff bits*) of identical value in the bit stream to be
>   transmitted, it shall automatically insert a complementary
>   bit (called stuff bit) ...
> 
> Pay attention to the *including stuff bits* part. The worst case is
> actually a sequence in which dominant and recessive alternate every
> four bits:
> 
>   Destuffed: 1 1111  0000  1111  0000  1111
>   Stuffed:   1 1111o 0000i 1111o 0000i 1111o
>
> Ignoring the first bit, one stuff bit is added every four bits giving
> us an overhead of 5/4 = 1.25.

Duh, absolutely right.

> The exact formula taking in account the first bit we previously ignored then:
> 
>   Number of dynamic stuff bit = 1 + round_down((len(stream) - 5) / 4)
>                               = round_down((len(stream) - 1) / 4)
> 
Right, do you plan on separating this for Arbitration bitrate and databitrate? It would probably make sense to use a fixed number of worst case stuffbits for the arbitration phase and the formula for the data phase.
 
Best Regards,
Thomas

Re: [PATCH] can: length: add definitions for frame lengths in bits

[Vincent MAILHOL]

Hi Thomas,

On Mon. 9 May 2023 at 16:12, <Thomas.Kopp@microchip.com> wrote:

[...]

> Right, do you plan on separating this for Arbitration bitrate and databitrate? It would probably make sense to use a fixed number of worst case stuffbits for the arbitration phase and the formula for the data phase.

I have a few ideas how to implement it, but seeing how complex things
are going, I am thinking of creating an inline helper function for the
bitstuffing calculation (the compiler should be able to fold it into a
constant expression, so there should be no penalty).

For the exact details, I have not decided yet. I need to experiment.
This not being so trivial and not having so much free time now, please
wait a few days for the v2 ;)

Re: RE: [PATCH] can: length: add definitions for frame lengths in bits

[Vincent MAILHOL]

On Tue. 9 May 2023 at 15:50, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 09.05.2023 13:16:08, Vincent MAILHOL wrote:
> > > The diagram in https://www.can-cia.org/can-knowledge/can/can-fd/
> > > suggests that IMF is part of the frame.
> >
> > ISO 11898-1:2015 section 10.4.6 "Specification of inter-frame space"
> > makes it clear that the intermission is not part of the frame but part
> > of the "Inter-frame space".
>
> For this reason, it is good to have open standards...oh wait!

It is open is you (or your company, wink, wink) pay CHF 187:

  https://www.iso.org/standard/63648.html

[PATCH v2 0/3] can: length: fix definitions and add bit length calculation

[Vincent Mailhol]

When created in [1], frames length definitions were added to implement
byte queue limits (bql). Because bql expects lengths in bytes, bit
length definitions were not considered back then.
    
Recently, a need to refer to the exact frame length in bits, with CAN
bit stuffing, appeared in [2].

This series introduces can_frame_bits(): an inline function that can
calculate the exact size of a CAN(-FD) frame in bits with or without
bitsuffing.

The review of v1, pointed out a few discrepancies of the current
length definitions. This series got extended to fix those
discrepancies.

[1] commit 85d99c3e2a13 ("can: length: can_skb_get_frame_len(): introduce
    function to get data length of frame in data link layer")
Link: https://git.kernel.org/torvalds/c/85d99c3e2a13

[2] RE: [PATCH] can: mcp251xfd: Increase poll timeout
Link: https://lore.kernel.org/linux-can/BL3PR11MB64846C83ACD04E9330B0FE66FB729@BL3PR11MB6484.namprd11.prod.outlook.com/


* Changelog *

v1 -> v2:

  * as suggested by Thomas Kopp, add a new patch to the series to fix
    the stuff bit count and the fixed stuff bits definitions

  * and another patch to fix documentation of the Remote Request
    Substitution (RRS).

  * refactor the length definition. Instead of using individual macro,
    rely on an inline function. One reason is to minimize the number
    of definitions. An other reason is that because the dynamic bit
    stuff is calculated differently for CAN and CAN-FD, it is just not
    possible to multiply the existing CANFD_FRAME_OVERHEAD_SFF/EFF by
    the overhead ratio to get the bitsuffing: for CAN-FD, the CRC
    field is already stuffed by the fix stuff bits and is out of scope
    of the dynamic bitstuffing.

Link: https://lore.kernel.org/linux-can/20230507155506.3179711-1-mailhol.vincent@wanadoo.fr/T/#u

Vincent Mailhol (3):
  can: length: fix bitstuffing count
  can: length: fix description of the RRS field
  can: length: refactor frame lengths definition to add size in bits

 drivers/net/can/dev/length.c |  15 +-
 include/linux/can/length.h   | 323 +++++++++++++++++++++++++----------
 2 files changed, 238 insertions(+), 100 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] can: length: fix bitstuffing count

[Vincent Mailhol]

The Stuff Bit Count is always coded on 4 bits (ref [1]). Update the
Stuff Bit Count size accordingly.

In addition, the CRC fields of CAN FD Frames contain stuff bits at
fixed positions call fixed stuff bits [2]. The CRC field starts with a
fixed stuff bit and then has another fixed stuff bit after each fourth
bit [2], which allow us to derive this formula:

  FSB count = 1 + round_down(len(CRC field)/4)

The length of the CRC field is [1]:

  len(CRC field) = len(Stuff Bit Count) + len(CRC)
                 = 4 + len(CRC)

with len(CRC) either 17 or 21 bits depending of the payload length.

In conclusion, for CRC17:

  FSB count = 1 + round_down((4 + 17)/4)
            = 6

and for CRC 21:

  FSB count = 1 + round_down((4 + 21)/4)
            = 7

Add a Fixed Stuff bits (FSB) field with above values and update
CANFD_FRAME_OVERHEAD_SFF and CANFD_FRAME_OVERHEAD_EFF accordingly.

[1] ISO 11898-1:2015 section 10.4.2.6 "CRC field":

  The CRC field shall contain the CRC sequence followed by a recessive
  CRC delimiter. For FD Frames, the CRC field shall also contain the
  stuff count.

  Stuff count

  If FD Frames, the stuff count shall be at the beginning of the CRC
  field. It shall consist of the stuff bit count modulo 8 in a 3-bit
  gray code followed by a parity bit [...]

[2] ISO 11898-1:2015 paragraph 10.5 "Frame coding":

  In the CRC field of FD Frames, the stuff bits shall be inserted at
  fixed positions; they are called field stuff bits. There shall be a
  fixed stuff bit before the first bit of the stuff count, even if the
  last bits of the preceding field are a sequence of five consecutive
  bits of identical value, there shall be only the fixed stuff bit,
  there shall not be two consecutive stuff bits. A further fixed stuff
  bit shall be inserted after each fourth bit of the CRC field [...]

Fixes: 85d99c3e2a13 ("can: length: can_skb_get_frame_len(): introduce function to get data length of frame in data link layer")
Suggested-by: Thomas Kopp <Thomas.Kopp@microchip.com>
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 include/linux/can/length.h | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/include/linux/can/length.h b/include/linux/can/length.h
index 69336549d24f..b8c12c83bc51 100644
--- a/include/linux/can/length.h
+++ b/include/linux/can/length.h
@@ -72,17 +72,18 @@
  * Error Status Indicator (ESI)		1
  * Data length code (DLC)		4
  * Data field				0...512
- * Stuff Bit Count (SBC)		0...16: 4 20...64:5
+ * Stuff Bit Count (SBC)		4
  * CRC					0...16: 17 20...64:21
  * CRC delimiter (CD)			1
+ * Fixed Stuff bits (FSB)		0...16: 6 20...64:7
  * ACK slot (AS)			1
  * ACK delimiter (AD)			1
  * End-of-frame (EOF)			7
  * Inter frame spacing			3
  *
- * assuming CRC21, rounded up and ignoring bitstuffing
+ * assuming CRC21, rounded up and ignoring dynamic bitstuffing
  */
-#define CANFD_FRAME_OVERHEAD_SFF DIV_ROUND_UP(61, 8)
+#define CANFD_FRAME_OVERHEAD_SFF DIV_ROUND_UP(67, 8)
 
 /*
  * Size of a CAN-FD Extended Frame
@@ -101,17 +102,18 @@
  * Error Status Indicator (ESI)		1
  * Data length code (DLC)		4
  * Data field				0...512
- * Stuff Bit Count (SBC)		0...16: 4 20...64:5
+ * Stuff Bit Count (SBC)		4
  * CRC					0...16: 17 20...64:21
  * CRC delimiter (CD)			1
+ * Fixed Stuff bits (FSB)		0...16: 6 20...64:7
  * ACK slot (AS)			1
  * ACK delimiter (AD)			1
  * End-of-frame (EOF)			7
  * Inter frame spacing			3
  *
- * assuming CRC21, rounded up and ignoring bitstuffing
+ * assuming CRC21, rounded up and ignoring dynamic bitstuffing
  */
-#define CANFD_FRAME_OVERHEAD_EFF DIV_ROUND_UP(80, 8)
+#define CANFD_FRAME_OVERHEAD_EFF DIV_ROUND_UP(86, 8)
 
 /*
  * Maximum size of a Classical CAN frame
-- 
2.25.1

[PATCH v2 2/3] can: length: fix description of the RRS field

[Vincent Mailhol]

The CAN-FD frames only have one reserved bit. The bit corresponding to
Classical CAN frame's RTR bit is called the "Remote Request
Substitution (RRS)" [1].

N.B. The RRS is not to be confused with the Substitute remote request
(SRR).

Fix the description in the CANFD_FRAME_OVERHEAD_SFF/EFF.

The total remains unchange, so this is just a documentation fix.

In addition to the above add myself as copyright owner for 2020 (as
coauthor of the initial version, c.f. Fixes tag).

[1] ISO 11898-1:2015 paragraph 10.4.2.3 "Arbitration field":

  RSS bit [only in FD Frames]

    The RRS bit shall be transmitted in fD Frames at the position of
    the RTR bit in Classical Frames. The RRS bit shall be transmitted
    dominant, but receivers shall accept recessive and dominant RRS
    bits.

Fixes: 85d99c3e2a13 ("can: length: can_skb_get_frame_len(): introduce function to get data length of frame in data link layer")
Suggested-by: Thomas Kopp <Thomas.Kopp@microchip.com>
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 include/linux/can/length.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/linux/can/length.h b/include/linux/can/length.h
index b8c12c83bc51..521fdbce2d69 100644
--- a/include/linux/can/length.h
+++ b/include/linux/can/length.h
@@ -1,6 +1,7 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 /* Copyright (C) 2020 Oliver Hartkopp <socketcan@hartkopp.net>
  * Copyright (C) 2020 Marc Kleine-Budde <kernel@pengutronix.de>
+ * Copyright (C) 2020 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
  */
 
 #ifndef _CAN_LENGTH_H
@@ -64,7 +65,7 @@
  * ---------------------------------------------------------
  * Start-of-frame			1
  * Identifier				11
- * Reserved bit (r1)			1
+ * Remote Request Substitution (RRS)	1
  * Identifier extension bit (IDE)	1
  * Flexible data rate format (FDF)	1
  * Reserved bit (r0)			1
@@ -95,7 +96,7 @@
  * Substitute remote request (SRR)	1
  * Identifier extension bit (IDE)	1
  * Identifier B				18
- * Reserved bit (r1)			1
+ * Remote Request Substitution (RRS)	1
  * Flexible data rate format (FDF)	1
  * Reserved bit (r0)			1
  * Bit Rate Switch (BRS)		1
-- 
2.25.1

[PATCH v2 3/3] can: length: refactor frame lengths definition to add size in bits

[Vincent Mailhol]

Introduce a method to calculate the exact size in bits of a CAN(-FD)
frame with or without dynamic bitsuffing.

These are all the possible combinations taken into account:

  - Classical CAN or CAN-FD
  - Standard or Extended frame format
  - CAN-FD CRC17 or CRC21
  - Include or not intermission

Instead of doing several macro definitions, declare the
can_frame_bits() static inline function. To this extend, do a full
refactoring of the length definitions.

If given integer constant expressions as argument, can_frame_bits()
folds into a compile time constant expression, giving no penalty over
the use of macros.

Also add the can_frame_bytes(). This function replaces the existing
macro:

  - CAN_FRAME_OVERHEAD_SFF: can_frame_bytes(false, false, 0)
  - CAN_FRAME_OVERHEAD_EFF: can_frame_bytes(false, true, 0)
  - CANFD_FRAME_OVERHEAD_SFF: can_frame_bytes(true, false, 0)
  - CANFD_FRAME_OVERHEAD_EFF: can_frame_bytes(true, true, 0)

The different frame lengths (including intermission) are as follow:

   Frame type				bits	bytes
  ----------------------------------------------------------
   Classic CAN SFF no-bitstuffing	111	14
   Classic CAN EFF no-bitstuffing	131	17
   Classic CAN SFF bitstuffing		135	17
   Classic CAN EFF bitstuffing		160	20
   CAN-FD SFF no-bitstuffing		579	73
   CAN-FD EFF no-bitstuffing		598	75
   CAN-FD SFF bitstuffing		712	89
   CAN-FD EFF bitstuffing		736	92

The macro CAN_FRAME_LEN_MAX and CANFD_FRAME_LEN_MAX are kept to be
used in const struct declarations (folding of can_frame_bytes() occurs
too late in the compilation to be used in struct declarations).

In addition to the above:

 - Use ISO 11898-1:2015 definitions for the name of the CAN frame
   fields.
 - Include linux/bits.h for use of BITS_PER_BYTE.
 - Include linux/math.h for use of mult_frac() and
   DIV_ROUND_UP(). N.B: the use of DIV_ROUND_UP() is not new to this
   patch, but the include was previously omitted.
 - Add copyright 2023 for myself.

[1] commit 85d99c3e2a13 ("can: length: can_skb_get_frame_len(): introduce
    function to get data length of frame in data link layer")
Link: https://git.kernel.org/torvalds/c/85d99c3e2a13

[2] RE: [PATCH] can: mcp251xfd: Increase poll timeout
Link: https://lore.kernel.org/linux-can/BL3PR11MB64846C83ACD04E9330B0FE66FB729@BL3PR11MB6484.namprd11.prod.outlook.com/

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
In case you ask, here is the proof that can_frame_bits() folds into
a compile time constant expression.

This file:

  #include <linux/can/length.h>

  unsigned int canfd_max_len_bitsuffing(void)
  {
	return can_frame_bytes(true, true, true, CANFD_MAX_DLEN);
  }

Produces this assembly code:

  0000000000000010 <canfd_max_len_bitsuffing>:
    10:	f3 0f 1e fa          	endbr64
    14:	b8 5c 00 00 00       	mov    $0x5c,%eax
    19:	e9 00 00 00 00       	jmpq   1e <canfd_max_len_bitsuffing+0xe>

where 0x5c corresponds to the expected value of 92 bytes.
---
 drivers/net/can/dev/length.c |  15 +-
 include/linux/can/length.h   | 326 +++++++++++++++++++++++++----------
 2 files changed, 238 insertions(+), 103 deletions(-)

diff --git a/drivers/net/can/dev/length.c b/drivers/net/can/dev/length.c
index b48140b1102e..b7f4d76dd444 100644
--- a/drivers/net/can/dev/length.c
+++ b/drivers/net/can/dev/length.c
@@ -78,18 +78,7 @@ unsigned int can_skb_get_frame_len(const struct sk_buff *skb)
 	else
 		len = cf->len;
 
-	if (can_is_canfd_skb(skb)) {
-		if (cf->can_id & CAN_EFF_FLAG)
-			len += CANFD_FRAME_OVERHEAD_EFF;
-		else
-			len += CANFD_FRAME_OVERHEAD_SFF;
-	} else {
-		if (cf->can_id & CAN_EFF_FLAG)
-			len += CAN_FRAME_OVERHEAD_EFF;
-		else
-			len += CAN_FRAME_OVERHEAD_SFF;
-	}
-
-	return len;
+	return can_frame_bytes(can_is_canfd_skb(skb), cf->can_id & CAN_EFF_FLAG,
+			       false, len);
 }
 EXPORT_SYMBOL_GPL(can_skb_get_frame_len);
diff --git a/include/linux/can/length.h b/include/linux/can/length.h
index 521fdbce2d69..7518117ee5fc 100644
--- a/include/linux/can/length.h
+++ b/include/linux/can/length.h
@@ -1,132 +1,278 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 /* Copyright (C) 2020 Oliver Hartkopp <socketcan@hartkopp.net>
  * Copyright (C) 2020 Marc Kleine-Budde <kernel@pengutronix.de>
- * Copyright (C) 2020 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
+ * Copyright (C) 2020, 2023 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
  */
 
 #ifndef _CAN_LENGTH_H
 #define _CAN_LENGTH_H
 
+#include <linux/bits.h>
 #include <linux/can.h>
 #include <linux/can/netlink.h>
+#include <linux/math.h>
 
 /*
- * Size of a Classical CAN Standard Frame
+ * Size of a Classical CAN Standard Frame header in bits
+ *
+ * Name of Field				Bits
+ * ---------------------------------------------------------
+ * Start Of Frame (SOF)				1
+ * Arbitration field:
+ *	base ID					11
+ *	Remote Transmission Request (RTR)	1
+ * Control field:
+ *	IDentifier Extension bit (IDE)		1
+ *	FD Format indicatior (FDF)		1
+ *	Data Length Code (DLC)			4
+ *
+ * including all fields preceding the data field, ignoring bitstuffing
+ */
+#define CAN_FRAME_HEADER_SFF_BITS 19
+
+/*
+ * Size of a Classical CAN Extended Frame header in bits
+ *
+ * Name of Field				Bits
+ * ---------------------------------------------------------
+ * Start Of Frame (SOF)				1
+ * Arbitration field:
+ *	base ID					11
+ *	Substitute Remote Request (SRR)		1
+ *	IDentifier Extension bit (IDE)		1
+ *	ID extension				18
+ *	Remote Transmission Request (RTR)	1
+ * Control field:
+ *	FD Format indicatior (FDF)		1
+ *	Reserved bit (r0)			1
+ *	Data length code (DLC)			4
+ *
+ * including all fields preceding the data field, ignoring bitstuffing
+ */
+#define CAN_FRAME_HEADER_EFF_BITS 39
+
+/*
+ * Size of a CAN-FD Standard Frame in bits
+ *
+ * Name of Field				Bits
+ * ---------------------------------------------------------
+ * Start Of Frame (SOF)				1
+ * Arbitration field:
+ *	base ID					11
+ *	Remote Request Substitution (RRS)	1
+ * Control field:
+ *	IDentifier Extension bit (IDE)		1
+ *	FD Format indicator (FDF)		1
+ *	Reserved bit (res)			1
+ *	Bit Rate Switch (BRS)			1
+ *	Error Status Indicator (ESI)		1
+ *	Data length code (DLC)			4
+ *
+ * including all fields preceding the data field, ignoring bitstuffing
+ */
+#define CANFD_FRAME_HEADER_SFF_BITS 22
+
+/*
+ * Size of a CAN-FD Extended Frame in bits
+ *
+ * Name of Field				Bits
+ * ---------------------------------------------------------
+ * Start Of Frame (SOF)				1
+ * Arbitration field:
+ *	base ID					11
+ *	Substitute Remote Request (SRR)		1
+ *	IDentifier Extension bit (IDE)		1
+ *	ID extension				18
+ *	Remote Request Substitution (RRS)	1
+ * Control field:
+ *	FD Format indicator (FDF)		1
+ *	Reserved bit (res)			1
+ *	Bit Rate Switch (BRS)			1
+ *	Error Status Indicator (ESI)		1
+ *	Data length code (DLC)			4
+ *
+ * including all fields preceding the data field, ignoring bitstuffing
+ */
+#define CANFD_FRAME_HEADER_EFF_BITS 41
+
+/*
+ * Size of a CAN CRC Field in bits
+ *
+ * Name of Field			Bits
+ * ---------------------------------------------------------
+ * CRC sequence (CRC15)			15
+ * CRC Delimiter			1
+ *
+ * ignoring bitstuffing
+ */
+#define CAN_FRAME_CRC_FIELD_BITS 16
+
+/*
+ * Size of a CAN-FD CRC17 Field in bits (length: 0..16)
+ *
+ * Name of Field			Bits
+ * ---------------------------------------------------------
+ * Stuff Count				4
+ * CRC Sequence (CRC17)			17
+ * CRC Delimiter			1
+ * Fixed stuff bits			6
+ */
+#define CANFD_FRAME_CRC17_FIELD_BITS 28
+
+/*
+ * Size of a CAN-FD CRC21 Field in bits (length: 20..64)
+ *
+ * Name of Field			Bits
+ * ---------------------------------------------------------
+ * Stuff Count				4
+ * CRC sequence (CRC21)			21
+ * CRC Delimiter			1
+ * Fixed stuff bits			7
+ */
+#define CANFD_FRAME_CRC21_FIELD_BITS 33
+
+/*
+ * Size of a CAN(-FD) Frame footer in bits
  *
  * Name of Field			Bits
  * ---------------------------------------------------------
- * Start-of-frame			1
- * Identifier				11
- * Remote transmission request (RTR)	1
- * Identifier extension bit (IDE)	1
- * Reserved bit (r0)			1
- * Data length code (DLC)		4
- * Data field				0...64
- * CRC					15
- * CRC delimiter			1
  * ACK slot				1
  * ACK delimiter			1
- * End-of-frame (EOF)			7
- * Inter frame spacing			3
+ * End Of Frame (EOF)			7
  *
- * rounded up and ignoring bitstuffing
+ * including all fields following the CRC field
  */
-#define CAN_FRAME_OVERHEAD_SFF DIV_ROUND_UP(47, 8)
+#define CAN_FRAME_FOOTER_BITS 9
 
 /*
- * Size of a Classical CAN Extended Frame
- *
- * Name of Field			Bits
- * ---------------------------------------------------------
- * Start-of-frame			1
- * Identifier A				11
- * Substitute remote request (SRR)	1
- * Identifier extension bit (IDE)	1
- * Identifier B				18
- * Remote transmission request (RTR)	1
- * Reserved bits (r1, r0)		2
- * Data length code (DLC)		4
- * Data field				0...64
- * CRC					15
- * CRC delimiter			1
- * ACK slot				1
- * ACK delimiter			1
- * End-of-frame (EOF)			7
- * Inter frame spacing			3
- *
- * rounded up and ignoring bitstuffing
+ * First part of the Inter Frame Space
+ * (a.k.a. IMF - intermission field)
  */
-#define CAN_FRAME_OVERHEAD_EFF DIV_ROUND_UP(67, 8)
+#define CAN_INTERMISSION_BITS 3
 
 /*
- * Size of a CAN-FD Standard Frame
+ * CAN bit stuffing overhead multiplication factor
  *
- * Name of Field			Bits
- * ---------------------------------------------------------
- * Start-of-frame			1
- * Identifier				11
- * Remote Request Substitution (RRS)	1
- * Identifier extension bit (IDE)	1
- * Flexible data rate format (FDF)	1
- * Reserved bit (r0)			1
- * Bit Rate Switch (BRS)		1
- * Error Status Indicator (ESI)		1
- * Data length code (DLC)		4
- * Data field				0...512
- * Stuff Bit Count (SBC)		4
- * CRC					0...16: 17 20...64:21
- * CRC delimiter (CD)			1
- * Fixed Stuff bits (FSB)		0...16: 6 20...64:7
- * ACK slot (AS)			1
- * ACK delimiter (AD)			1
- * End-of-frame (EOF)			7
- * Inter frame spacing			3
+ * The worst bit stuffing case is a sequence in which dominant and
+ * recessive bits alternate every four bits:
  *
- * assuming CRC21, rounded up and ignoring dynamic bitstuffing
+ *   Destuffed: 1 1111  0000  1111  0000  1111
+ *   Stuffed:   1 1111o 0000i 1111o 0000i 1111o
+ *
+ * Nomenclature:
+ *
+ *  - "0": dominant bit
+ *  - "o": dominant stuff bit
+ *  - "1": recessive bit
+ *  - "i": recessive stuff bit
+ *
+ * Ignoring the first bit, one stuff bit is added every four bits
+ * giving us an overhead of 1/4 = 0.25.
  */
-#define CANFD_FRAME_OVERHEAD_SFF DIV_ROUND_UP(67, 8)
+#define can_bit_stuffing_overhead(stream_len) mult_frac(stream_len, 1, 4)
+
+/**
+ * can_frame_bits() - Calculate the number of bits in on the wire in a
+ *	CAN frame
+ * @is_eff: true: Extended Frame; false: Standard Frame.
+ * @bitstuffing: if true, calculate the bitsuffing worst case, if
+ *	false, calculated the bitsuffing best case (no dynamic
+ *	bitsuffing). Fixed stuff bits always get included.
+ * @intermission: if and only if true, include the inter frame space
+ *	assuming no bus idle (i.e. only the intermission gets added).
+ * @data_len: length of the data field in bytes. Correspond to
+ *	can(fd)_frame->len. Should be zero for remote frames. No
+ *	sanitization is done on @data_len.
+ *
+ * Return: the numbers of bits on the wire of a CAN frame.
+ */
+static inline
+unsigned int can_frame_bits(bool is_fd, bool is_eff,
+			    bool bitstuffing, bool intermission,
+			    unsigned int data_len)
+{
+	unsigned int bits;
+
+	if (is_fd) {
+		if (is_eff)
+			bits = CANFD_FRAME_HEADER_EFF_BITS;
+		else
+			bits = CANFD_FRAME_HEADER_SFF_BITS;
+	} else {
+		if (is_eff)
+			bits = CAN_FRAME_HEADER_EFF_BITS;
+		else
+			bits = CAN_FRAME_HEADER_SFF_BITS;
+	}
+
+	bits += data_len * BITS_PER_BYTE;
+
+	if (!is_fd) {
+		bits += CAN_FRAME_CRC_FIELD_BITS;
+		/*
+		 * In Classical CAN, bit stuffing applies to all field
+		 * from SOF to CRC delimiter
+		 */
+		if (bitstuffing)
+			bits += can_bit_stuffing_overhead(bits);
+	} else {
+		/*
+		 * In CAN-FD, the fields preceding the CRC field have
+		 * dynamic bit stuffing; but the CRC field has fixed
+		 * bitstuffing
+		 */
+		if (bitstuffing)
+			bits += can_bit_stuffing_overhead(bits - 1);
+		if (data_len <= 16)
+			bits += CANFD_FRAME_CRC17_FIELD_BITS;
+		else
+			bits += CANFD_FRAME_CRC21_FIELD_BITS;
+	}
+
+	bits += CAN_FRAME_FOOTER_BITS;
+
+	if (intermission)
+		bits += CAN_INTERMISSION_BITS;
+
+	return bits;
+}
 
 /*
- * Size of a CAN-FD Extended Frame
- *
- * Name of Field			Bits
- * ---------------------------------------------------------
- * Start-of-frame			1
- * Identifier A				11
- * Substitute remote request (SRR)	1
- * Identifier extension bit (IDE)	1
- * Identifier B				18
- * Remote Request Substitution (RRS)	1
- * Flexible data rate format (FDF)	1
- * Reserved bit (r0)			1
- * Bit Rate Switch (BRS)		1
- * Error Status Indicator (ESI)		1
- * Data length code (DLC)		4
- * Data field				0...512
- * Stuff Bit Count (SBC)		4
- * CRC					0...16: 17 20...64:21
- * CRC delimiter (CD)			1
- * Fixed Stuff bits (FSB)		0...16: 6 20...64:7
- * ACK slot (AS)			1
- * ACK delimiter (AD)			1
- * End-of-frame (EOF)			7
- * Inter frame spacing			3
- *
- * assuming CRC21, rounded up and ignoring dynamic bitstuffing
+ * Number of bytes in a CAN frame
+ * (rounded up, including intermission)
  */
-#define CANFD_FRAME_OVERHEAD_EFF DIV_ROUND_UP(86, 8)
+static inline
+unsigned int can_frame_bytes(bool is_fd, bool is_eff, bool bitstuffing,
+			     unsigned int data_len)
+{
+	return DIV_ROUND_UP(can_frame_bits(is_fd, is_eff, bitstuffing, true,
+					   data_len),
+			    BITS_PER_BYTE);
+}
 
 /*
  * Maximum size of a Classical CAN frame
- * (rounded up and ignoring bitstuffing)
+ * (rounded up, ignoring bitstuffing but including intermission)
  */
-#define CAN_FRAME_LEN_MAX (CAN_FRAME_OVERHEAD_EFF + CAN_MAX_DLEN)
+#define CAN_FRAME_LEN_MAX				\
+	DIV_ROUND_UP(CAN_FRAME_HEADER_EFF_BITS +	\
+		     CAN_MAX_DLEN * BITS_PER_BYTE +	\
+		     CAN_FRAME_CRC_FIELD_BITS +		\
+		     CAN_FRAME_FOOTER_BITS +		\
+		     CAN_INTERMISSION_BITS,		\
+		     BITS_PER_BYTE)
 
 /*
  * Maximum size of a CAN-FD frame
  * (rounded up and ignoring bitstuffing)
  */
-#define CANFD_FRAME_LEN_MAX (CANFD_FRAME_OVERHEAD_EFF + CANFD_MAX_DLEN)
+#define CANFD_FRAME_LEN_MAX				\
+	DIV_ROUND_UP(CANFD_FRAME_HEADER_EFF_BITS +	\
+		     CANFD_MAX_DLEN * BITS_PER_BYTE +	\
+		     CANFD_FRAME_CRC21_FIELD_BITS	\
+		     CAN_FRAME_FOOTER_BITS +		\
+		     CAN_INTERMISSION_BITS,		\
+		     BITS_PER_BYTE)
 
 /*
  * can_cc_dlc2len(value) - convert a given data length code (dlc) of a
-- 
2.25.1

Re: [PATCH v2 3/3] can: length: refactor frame lengths definition to add size in bits

[Vincent MAILHOL]

Sorry for the late reply, I wanted to have this completed earlier but
other imperatives and the time needed to debug decided differently.

On Tue. 23 May 2023 at 15:52, Vincent Mailhol
<mailhol.vincent@wanadoo.fr> wrote:
> Introduce a method to calculate the exact size in bits of a CAN(-FD)
> frame with or without dynamic bitsuffing.
>
> These are all the possible combinations taken into account:
>
>   - Classical CAN or CAN-FD
>   - Standard or Extended frame format
>   - CAN-FD CRC17 or CRC21
>   - Include or not intermission
>
> Instead of doing several macro definitions, declare the
> can_frame_bits() static inline function. To this extend, do a full
                                                   ^^^^^^
Typo: extent
(I will not send a v3 just for that).

> refactoring of the length definitions.
>
> If given integer constant expressions as argument, can_frame_bits()
> folds into a compile time constant expression, giving no penalty over
> the use of macros.
>
> Also add the can_frame_bytes(). This function replaces the existing
> macro:
>
>   - CAN_FRAME_OVERHEAD_SFF: can_frame_bytes(false, false, 0)
>   - CAN_FRAME_OVERHEAD_EFF: can_frame_bytes(false, true, 0)
>   - CANFD_FRAME_OVERHEAD_SFF: can_frame_bytes(true, false, 0)
>   - CANFD_FRAME_OVERHEAD_EFF: can_frame_bytes(true, true, 0)
>
> The different frame lengths (including intermission) are as follow:
>
>    Frame type                           bits    bytes
>   ----------------------------------------------------------
>    Classic CAN SFF no-bitstuffing       111     14
>    Classic CAN EFF no-bitstuffing       131     17
>    Classic CAN SFF bitstuffing          135     17
>    Classic CAN EFF bitstuffing          160     20
>    CAN-FD SFF no-bitstuffing            579     73
>    CAN-FD EFF no-bitstuffing            598     75
>    CAN-FD SFF bitstuffing               712     89
>    CAN-FD EFF bitstuffing               736     92
>
> The macro CAN_FRAME_LEN_MAX and CANFD_FRAME_LEN_MAX are kept to be
> used in const struct declarations (folding of can_frame_bytes() occurs
> too late in the compilation to be used in struct declarations).

To be fair, I am not fully happy with that part. The fact that
can_frame_bits() and can_frame_bytes() can not be used in structure
declaration even if these are compile time constants is unfortunate.
But after reflection, I still see those inline functions as a better
compromise than a collection of macro definitions. Let me know your
thoughts.

(...)

Yours sincerely,
Vincent Mailhol

Re: [PATCH v2 3/3] can: length: refactor frame lengths definition to add size in bits

[Simon Horman]

On Tue, May 23, 2023 at 03:52:18PM +0900, Vincent Mailhol wrote:
> Introduce a method to calculate the exact size in bits of a CAN(-FD)
> frame with or without dynamic bitsuffing.

...

> +/**
> + * can_frame_bits() - Calculate the number of bits in on the wire in a
> + *	CAN frame

nit: @is_fd should be documented here.

> + * @is_eff: true: Extended Frame; false: Standard Frame.
> + * @bitstuffing: if true, calculate the bitsuffing worst case, if
> + *	false, calculated the bitsuffing best case (no dynamic
> + *	bitsuffing). Fixed stuff bits always get included.
> + * @intermission: if and only if true, include the inter frame space
> + *	assuming no bus idle (i.e. only the intermission gets added).
> + * @data_len: length of the data field in bytes. Correspond to
> + *	can(fd)_frame->len. Should be zero for remote frames. No
> + *	sanitization is done on @data_len.
> + *
> + * Return: the numbers of bits on the wire of a CAN frame.
> + */
> +static inline
> +unsigned int can_frame_bits(bool is_fd, bool is_eff,
> +			    bool bitstuffing, bool intermission,
> +			    unsigned int data_len)
> +{

...

[PATCH v3 0/3] can: length: fix definitions and add bit length calculation

[Vincent Mailhol]

When created in [1], frames length definitions were added to implement
byte queue limits (bql). Because bql expects lengths in bytes, bit
length definitions were not considered back then.
    
Recently, a need to refer to the exact frame length in bits, with CAN
bit stuffing, appeared in [2].

This series introduces can_frame_bits(): a function-like macro that
can calculate the exact size of a CAN(-FD) frame in bits with or
without bitsuffing.

[1] commit 85d99c3e2a13 ("can: length: can_skb_get_frame_len(): introduce
    function to get data length of frame in data link layer")
Link: https://git.kernel.org/torvalds/c/85d99c3e2a13

[2] RE: [PATCH] can: mcp251xfd: Increase poll timeout
Link: https://lore.kernel.org/linux-can/BL3PR11MB64846C83ACD04E9330B0FE66FB729@BL3PR11MB6484.namprd11.prod.outlook.com/


* Changelog *

v2 -> v3:

  * turn can_frame_bits() and can_frame_bytes() into function-like
    macros. The fact that inline functions can not be used to
    initialize constant structure fields was bothering me. I did my
    best to make the macro look as less ugly as possible.

  * as reported by Simon Horman, add missing document for the is_fd
    argument of can_frame_bits().

Link: https://lore.kernel.org/linux-can/20230523065218.51227-1-mailhol.vincent@wanadoo.fr/

v1 -> v2:

  * as suggested by Thomas Kopp, add a new patch to the series to fix
    the stuff bit count and the fixed stuff bits definitions

  * and another patch to fix documentation of the Remote Request
    Substitution (RRS).

  * refactor the length definition. Instead of using individual macro,
    rely on an inline function. One reason is to minimize the number
    of definitions. An other reason is that because the dynamic bit
    stuff is calculated differently for CAN and CAN-FD, it is just not
    possible to multiply the existing CANFD_FRAME_OVERHEAD_SFF/EFF by
    the overhead ratio to get the bitsuffing: for CAN-FD, the CRC
    field is already stuffed by the fix stuff bits and is out of scope
    of the dynamic bitstuffing.

Link: https://lore.kernel.org/linux-can/20230507155506.3179711-1-mailhol.vincent@wanadoo.fr/

Vincent Mailhol (3):
  can: length: fix bitstuffing count
  can: length: fix description of the RRS field
  can: length: refactor frame lengths definition to add size in bits

 drivers/net/can/dev/length.c |  15 +-
 include/linux/can/length.h   | 295 +++++++++++++++++++++++++----------
 2 files changed, 213 insertions(+), 97 deletions(-)

-- 
2.39.3

[PATCH v3 1/3] can: length: fix bitstuffing count

[Vincent Mailhol]

The Stuff Bit Count is always coded on 4 bits (ref [1]). Update the
Stuff Bit Count size accordingly.

In addition, the CRC fields of CAN FD Frames contain stuff bits at
fixed positions called fixed stuff bits [2]. The CRC field starts with
a fixed stuff bit and then has another fixed stuff bit after each
fourth bit [2], which allows us to derive this formula:

  FSB count = 1 + round_down(len(CRC field)/4)

The length of the CRC field is [1]:

  len(CRC field) = len(Stuff Bit Count) + len(CRC)
                 = 4 + len(CRC)

with len(CRC) either 17 or 21 bits depending of the payload length.

In conclusion, for CRC17:

  FSB count = 1 + round_down((4 + 17)/4)
            = 6

and for CRC 21:

  FSB count = 1 + round_down((4 + 21)/4)
            = 7

Add a Fixed Stuff bits (FSB) field with above values and update
CANFD_FRAME_OVERHEAD_SFF and CANFD_FRAME_OVERHEAD_EFF accordingly.

[1] ISO 11898-1:2015 section 10.4.2.6 "CRC field":

  The CRC field shall contain the CRC sequence followed by a recessive
  CRC delimiter. For FD Frames, the CRC field shall also contain the
  stuff count.

  Stuff count

  If FD Frames, the stuff count shall be at the beginning of the CRC
  field. It shall consist of the stuff bit count modulo 8 in a 3-bit
  gray code followed by a parity bit [...]

[2] ISO 11898-1:2015 paragraph 10.5 "Frame coding":

  In the CRC field of FD Frames, the stuff bits shall be inserted at
  fixed positions; they are called field stuff bits. There shall be a
  fixed stuff bit before the first bit of the stuff count, even if the
  last bits of the preceding field are a sequence of five consecutive
  bits of identical value, there shall be only the fixed stuff bit,
  there shall not be two consecutive stuff bits. A further fixed stuff
  bit shall be inserted after each fourth bit of the CRC field [...]

Fixes: 85d99c3e2a13 ("can: length: can_skb_get_frame_len(): introduce function to get data length of frame in data link layer")
Suggested-by: Thomas Kopp <Thomas.Kopp@microchip.com>
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 include/linux/can/length.h | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/include/linux/can/length.h b/include/linux/can/length.h
index 69336549d24f..b8c12c83bc51 100644
--- a/include/linux/can/length.h
+++ b/include/linux/can/length.h
@@ -72,17 +72,18 @@
  * Error Status Indicator (ESI)		1
  * Data length code (DLC)		4
  * Data field				0...512
- * Stuff Bit Count (SBC)		0...16: 4 20...64:5
+ * Stuff Bit Count (SBC)		4
  * CRC					0...16: 17 20...64:21
  * CRC delimiter (CD)			1
+ * Fixed Stuff bits (FSB)		0...16: 6 20...64:7
  * ACK slot (AS)			1
  * ACK delimiter (AD)			1
  * End-of-frame (EOF)			7
  * Inter frame spacing			3
  *
- * assuming CRC21, rounded up and ignoring bitstuffing
+ * assuming CRC21, rounded up and ignoring dynamic bitstuffing
  */
-#define CANFD_FRAME_OVERHEAD_SFF DIV_ROUND_UP(61, 8)
+#define CANFD_FRAME_OVERHEAD_SFF DIV_ROUND_UP(67, 8)
 
 /*
  * Size of a CAN-FD Extended Frame
@@ -101,17 +102,18 @@
  * Error Status Indicator (ESI)		1
  * Data length code (DLC)		4
  * Data field				0...512
- * Stuff Bit Count (SBC)		0...16: 4 20...64:5
+ * Stuff Bit Count (SBC)		4
  * CRC					0...16: 17 20...64:21
  * CRC delimiter (CD)			1
+ * Fixed Stuff bits (FSB)		0...16: 6 20...64:7
  * ACK slot (AS)			1
  * ACK delimiter (AD)			1
  * End-of-frame (EOF)			7
  * Inter frame spacing			3
  *
- * assuming CRC21, rounded up and ignoring bitstuffing
+ * assuming CRC21, rounded up and ignoring dynamic bitstuffing
  */
-#define CANFD_FRAME_OVERHEAD_EFF DIV_ROUND_UP(80, 8)
+#define CANFD_FRAME_OVERHEAD_EFF DIV_ROUND_UP(86, 8)
 
 /*
  * Maximum size of a Classical CAN frame
-- 
2.39.3

[PATCH v3 2/3] can: length: fix description of the RRS field

[Vincent Mailhol]

The CAN-FD frames only have one reserved bit. The bit corresponding to
Classical CAN frame's RTR bit is called the "Remote Request
Substitution (RRS)" [1].

N.B. The RRS is not to be confused with the Substitute remote request
(SRR).

Fix the description in the CANFD_FRAME_OVERHEAD_SFF/EFF.

The total remains unchanged, so this is just a documentation fix.

In addition to the above add myself as copyright owner for 2020 (as
coauthor of the initial version, c.f. Fixes tag).

[1] ISO 11898-1:2015 paragraph 10.4.2.3 "Arbitration field":

  RSS bit [only in FD Frames]

    The RRS bit shall be transmitted in fD Frames at the position of
    the RTR bit in Classical Frames. The RRS bit shall be transmitted
    dominant, but receivers shall accept recessive and dominant RRS
    bits.

Fixes: 85d99c3e2a13 ("can: length: can_skb_get_frame_len(): introduce function to get data length of frame in data link layer")
Suggested-by: Thomas Kopp <Thomas.Kopp@microchip.com>
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 include/linux/can/length.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/linux/can/length.h b/include/linux/can/length.h
index b8c12c83bc51..521fdbce2d69 100644
--- a/include/linux/can/length.h
+++ b/include/linux/can/length.h
@@ -1,6 +1,7 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 /* Copyright (C) 2020 Oliver Hartkopp <socketcan@hartkopp.net>
  * Copyright (C) 2020 Marc Kleine-Budde <kernel@pengutronix.de>
+ * Copyright (C) 2020 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
  */
 
 #ifndef _CAN_LENGTH_H
@@ -64,7 +65,7 @@
  * ---------------------------------------------------------
  * Start-of-frame			1
  * Identifier				11
- * Reserved bit (r1)			1
+ * Remote Request Substitution (RRS)	1
  * Identifier extension bit (IDE)	1
  * Flexible data rate format (FDF)	1
  * Reserved bit (r0)			1
@@ -95,7 +96,7 @@
  * Substitute remote request (SRR)	1
  * Identifier extension bit (IDE)	1
  * Identifier B				18
- * Reserved bit (r1)			1
+ * Remote Request Substitution (RRS)	1
  * Flexible data rate format (FDF)	1
  * Reserved bit (r0)			1
  * Bit Rate Switch (BRS)		1
-- 
2.39.3

[PATCH v3 3/3] can: length: refactor frame lengths definition to add size in bits

[Vincent Mailhol]

Introduce a method to calculate the exact size in bits of a CAN(-FD)
frame with or without dynamic bitsuffing.

These are all the possible combinations taken into account:

  - Classical CAN or CAN-FD
  - Standard or Extended frame format
  - CAN-FD CRC17 or CRC21
  - Include or not intermission

Instead of doing several individual macro definitions, declare the
can_frame_bits() function-like macro. To this extent, do a full
refactoring of the length definitions.

In addition add the can_frame_bytes(). This function-like macro
replaces the existing macro:

  - CAN_FRAME_OVERHEAD_SFF: can_frame_bytes(false, false, 0)
  - CAN_FRAME_OVERHEAD_EFF: can_frame_bytes(false, true, 0)
  - CANFD_FRAME_OVERHEAD_SFF: can_frame_bytes(true, false, 0)
  - CANFD_FRAME_OVERHEAD_EFF: can_frame_bytes(true, true, 0)

The different maximum frame lengths (maximum data length, including
intermission) are as follow:

   Frame type				bits	bytes
  -------------------------------------------------------
   Classic CAN SFF no-bitstuffing	111	14
   Classic CAN EFF no-bitstuffing	131	17
   Classic CAN SFF bitstuffing		135	17
   Classic CAN EFF bitstuffing		160	20
   CAN-FD SFF no-bitstuffing		579	73
   CAN-FD EFF no-bitstuffing		598	75
   CAN-FD SFF bitstuffing		712	89
   CAN-FD EFF bitstuffing		736	92

The macro CAN_FRAME_LEN_MAX and CANFD_FRAME_LEN_MAX are kept as an
alias to, respectively, can_frame_bytes(false, true, CAN_MAX_DLEN) and
can_frame_bytes(true, true, CANFD_MAX_DLEN).

In addition to the above:

 - Use ISO 11898-1:2015 definitions for the name of the CAN frame
   fields.
 - Include linux/bits.h for use of BITS_PER_BYTE.
 - Include linux/math.h for use of mult_frac() and
   DIV_ROUND_UP(). N.B: the use of DIV_ROUND_UP() is not new to this
   patch, but the include was previously omitted.
 - Add copyright 2023 for myself.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/net/can/dev/length.c |  15 +-
 include/linux/can/length.h   | 298 +++++++++++++++++++++++++----------
 2 files changed, 213 insertions(+), 100 deletions(-)

diff --git a/drivers/net/can/dev/length.c b/drivers/net/can/dev/length.c
index b48140b1102e..b7f4d76dd444 100644
--- a/drivers/net/can/dev/length.c
+++ b/drivers/net/can/dev/length.c
@@ -78,18 +78,7 @@ unsigned int can_skb_get_frame_len(const struct sk_buff *skb)
 	else
 		len = cf->len;
 
-	if (can_is_canfd_skb(skb)) {
-		if (cf->can_id & CAN_EFF_FLAG)
-			len += CANFD_FRAME_OVERHEAD_EFF;
-		else
-			len += CANFD_FRAME_OVERHEAD_SFF;
-	} else {
-		if (cf->can_id & CAN_EFF_FLAG)
-			len += CAN_FRAME_OVERHEAD_EFF;
-		else
-			len += CAN_FRAME_OVERHEAD_SFF;
-	}
-
-	return len;
+	return can_frame_bytes(can_is_canfd_skb(skb), cf->can_id & CAN_EFF_FLAG,
+			       false, len);
 }
 EXPORT_SYMBOL_GPL(can_skb_get_frame_len);
diff --git a/include/linux/can/length.h b/include/linux/can/length.h
index 521fdbce2d69..ef6e78fa95b9 100644
--- a/include/linux/can/length.h
+++ b/include/linux/can/length.h
@@ -1,132 +1,256 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 /* Copyright (C) 2020 Oliver Hartkopp <socketcan@hartkopp.net>
  * Copyright (C) 2020 Marc Kleine-Budde <kernel@pengutronix.de>
- * Copyright (C) 2020 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
+ * Copyright (C) 2020, 2023 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
  */
 
 #ifndef _CAN_LENGTH_H
 #define _CAN_LENGTH_H
 
+#include <linux/bits.h>
 #include <linux/can.h>
 #include <linux/can/netlink.h>
+#include <linux/math.h>
 
 /*
- * Size of a Classical CAN Standard Frame
+ * Size of a Classical CAN Standard Frame header in bits
  *
- * Name of Field			Bits
+ * Name of Field				Bits
  * ---------------------------------------------------------
- * Start-of-frame			1
- * Identifier				11
- * Remote transmission request (RTR)	1
- * Identifier extension bit (IDE)	1
- * Reserved bit (r0)			1
- * Data length code (DLC)		4
- * Data field				0...64
- * CRC					15
- * CRC delimiter			1
- * ACK slot				1
- * ACK delimiter			1
- * End-of-frame (EOF)			7
- * Inter frame spacing			3
+ * Start Of Frame (SOF)				1
+ * Arbitration field:
+ *	base ID					11
+ *	Remote Transmission Request (RTR)	1
+ * Control field:
+ *	IDentifier Extension bit (IDE)		1
+ *	FD Format indicatior (FDF)		1
+ *	Data Length Code (DLC)			4
+ *
+ * including all fields preceding the data field, ignoring bitstuffing
+ */
+#define CAN_FRAME_HEADER_SFF_BITS 19
+
+/*
+ * Size of a Classical CAN Extended Frame header in bits
+ *
+ * Name of Field				Bits
+ * ---------------------------------------------------------
+ * Start Of Frame (SOF)				1
+ * Arbitration field:
+ *	base ID					11
+ *	Substitute Remote Request (SRR)		1
+ *	IDentifier Extension bit (IDE)		1
+ *	ID extension				18
+ *	Remote Transmission Request (RTR)	1
+ * Control field:
+ *	FD Format indicatior (FDF)		1
+ *	Reserved bit (r0)			1
+ *	Data length code (DLC)			4
+ *
+ * including all fields preceding the data field, ignoring bitstuffing
+ */
+#define CAN_FRAME_HEADER_EFF_BITS 39
+
+/*
+ * Size of a CAN-FD Standard Frame in bits
+ *
+ * Name of Field				Bits
+ * ---------------------------------------------------------
+ * Start Of Frame (SOF)				1
+ * Arbitration field:
+ *	base ID					11
+ *	Remote Request Substitution (RRS)	1
+ * Control field:
+ *	IDentifier Extension bit (IDE)		1
+ *	FD Format indicator (FDF)		1
+ *	Reserved bit (res)			1
+ *	Bit Rate Switch (BRS)			1
+ *	Error Status Indicator (ESI)		1
+ *	Data length code (DLC)			4
+ *
+ * including all fields preceding the data field, ignoring bitstuffing
+ */
+#define CANFD_FRAME_HEADER_SFF_BITS 22
+
+/*
+ * Size of a CAN-FD Extended Frame in bits
+ *
+ * Name of Field				Bits
+ * ---------------------------------------------------------
+ * Start Of Frame (SOF)				1
+ * Arbitration field:
+ *	base ID					11
+ *	Substitute Remote Request (SRR)		1
+ *	IDentifier Extension bit (IDE)		1
+ *	ID extension				18
+ *	Remote Request Substitution (RRS)	1
+ * Control field:
+ *	FD Format indicator (FDF)		1
+ *	Reserved bit (res)			1
+ *	Bit Rate Switch (BRS)			1
+ *	Error Status Indicator (ESI)		1
+ *	Data length code (DLC)			4
  *
- * rounded up and ignoring bitstuffing
+ * including all fields preceding the data field, ignoring bitstuffing
  */
-#define CAN_FRAME_OVERHEAD_SFF DIV_ROUND_UP(47, 8)
+#define CANFD_FRAME_HEADER_EFF_BITS 41
 
 /*
- * Size of a Classical CAN Extended Frame
+ * Size of a CAN CRC Field in bits
  *
  * Name of Field			Bits
  * ---------------------------------------------------------
- * Start-of-frame			1
- * Identifier A				11
- * Substitute remote request (SRR)	1
- * Identifier extension bit (IDE)	1
- * Identifier B				18
- * Remote transmission request (RTR)	1
- * Reserved bits (r1, r0)		2
- * Data length code (DLC)		4
- * Data field				0...64
- * CRC					15
- * CRC delimiter			1
- * ACK slot				1
- * ACK delimiter			1
- * End-of-frame (EOF)			7
- * Inter frame spacing			3
+ * CRC sequence (CRC15)			15
+ * CRC Delimiter			1
  *
- * rounded up and ignoring bitstuffing
+ * ignoring bitstuffing
  */
-#define CAN_FRAME_OVERHEAD_EFF DIV_ROUND_UP(67, 8)
+#define CAN_FRAME_CRC_FIELD_BITS 16
 
 /*
- * Size of a CAN-FD Standard Frame
+ * Size of a CAN-FD CRC17 Field in bits (length: 0..16)
  *
  * Name of Field			Bits
  * ---------------------------------------------------------
- * Start-of-frame			1
- * Identifier				11
- * Remote Request Substitution (RRS)	1
- * Identifier extension bit (IDE)	1
- * Flexible data rate format (FDF)	1
- * Reserved bit (r0)			1
- * Bit Rate Switch (BRS)		1
- * Error Status Indicator (ESI)		1
- * Data length code (DLC)		4
- * Data field				0...512
- * Stuff Bit Count (SBC)		4
- * CRC					0...16: 17 20...64:21
- * CRC delimiter (CD)			1
- * Fixed Stuff bits (FSB)		0...16: 6 20...64:7
- * ACK slot (AS)			1
- * ACK delimiter (AD)			1
- * End-of-frame (EOF)			7
- * Inter frame spacing			3
- *
- * assuming CRC21, rounded up and ignoring dynamic bitstuffing
- */
-#define CANFD_FRAME_OVERHEAD_SFF DIV_ROUND_UP(67, 8)
+ * Stuff Count				4
+ * CRC Sequence (CRC17)			17
+ * CRC Delimiter			1
+ * Fixed stuff bits			6
+ */
+#define CANFD_FRAME_CRC17_FIELD_BITS 28
 
 /*
- * Size of a CAN-FD Extended Frame
+ * Size of a CAN-FD CRC21 Field in bits (length: 20..64)
  *
  * Name of Field			Bits
  * ---------------------------------------------------------
- * Start-of-frame			1
- * Identifier A				11
- * Substitute remote request (SRR)	1
- * Identifier extension bit (IDE)	1
- * Identifier B				18
- * Remote Request Substitution (RRS)	1
- * Flexible data rate format (FDF)	1
- * Reserved bit (r0)			1
- * Bit Rate Switch (BRS)		1
- * Error Status Indicator (ESI)		1
- * Data length code (DLC)		4
- * Data field				0...512
- * Stuff Bit Count (SBC)		4
- * CRC					0...16: 17 20...64:21
- * CRC delimiter (CD)			1
- * Fixed Stuff bits (FSB)		0...16: 6 20...64:7
- * ACK slot (AS)			1
- * ACK delimiter (AD)			1
- * End-of-frame (EOF)			7
- * Inter frame spacing			3
- *
- * assuming CRC21, rounded up and ignoring dynamic bitstuffing
- */
-#define CANFD_FRAME_OVERHEAD_EFF DIV_ROUND_UP(86, 8)
+ * Stuff Count				4
+ * CRC sequence (CRC21)			21
+ * CRC Delimiter			1
+ * Fixed stuff bits			7
+ */
+#define CANFD_FRAME_CRC21_FIELD_BITS 33
+
+/*
+ * Size of a CAN(-FD) Frame footer in bits
+ *
+ * Name of Field			Bits
+ * ---------------------------------------------------------
+ * ACK slot				1
+ * ACK delimiter			1
+ * End Of Frame (EOF)			7
+ *
+ * including all fields following the CRC field
+ */
+#define CAN_FRAME_FOOTER_BITS 9
+
+/*
+ * First part of the Inter Frame Space
+ * (a.k.a. IMF - intermission field)
+ */
+#define CAN_INTERMISSION_BITS 3
+
+/**
+ * can_bitstuffing_len() - Calculate the maximum length with bitsuffing
+ * @bitstream_len: length of a destuffed bit stream
+ *
+ * The worst bit stuffing case is a sequence in which dominant and
+ * recessive bits alternate every four bits:
+ *
+ *   Destuffed: 1 1111  0000  1111  0000  1111
+ *   Stuffed:   1 1111o 0000i 1111o 0000i 1111o
+ *
+ * Nomenclature
+ *
+ *  - "0": dominant bit
+ *  - "o": dominant stuff bit
+ *  - "1": recessive bit
+ *  - "i": recessive stuff bit
+ *
+ * Aside of the first bit, one stuff bit is added every four bits.
+ *
+ * Return: length of the stuffed bit stream in the worst case scenario.
+ */
+#define can_bitstuffing_len(destuffed_len)			\
+	(destuffed_len + (destuffed_len - 1) / 4)
+
+#define __can_bitstuffing_len(bitstuffing, destuffed_len)	\
+	(bitstuffing ? can_bitstuffing_len(destuffed_len) :	\
+		       destuffed_len)
+
+#define __can_cc_frame_bits(is_eff, bitstuffing,		\
+			    intermission, data_len)		\
+(								\
+	__can_bitstuffing_len(bitstuffing,			\
+		(is_eff ? CAN_FRAME_HEADER_EFF_BITS :		\
+			   CAN_FRAME_HEADER_SFF_BITS) +		\
+		data_len * BITS_PER_BYTE +			\
+		CAN_FRAME_CRC_FIELD_BITS) +			\
+	CAN_FRAME_FOOTER_BITS +					\
+	(intermission ? CAN_INTERMISSION_BITS : 0)		\
+)
+
+#define __can_fd_frame_bits(is_eff, bitstuffing,		\
+			    intermission, data_len)		\
+(								\
+	__can_bitstuffing_len(bitstuffing,			\
+		(is_eff ? CANFD_FRAME_HEADER_EFF_BITS :		\
+			   CANFD_FRAME_HEADER_SFF_BITS) +	\
+		data_len * BITS_PER_BYTE) +			\
+	(data_len <= 16 ?					\
+		CANFD_FRAME_CRC17_FIELD_BITS :			\
+		CANFD_FRAME_CRC21_FIELD_BITS) +			\
+	CAN_FRAME_FOOTER_BITS +					\
+	(intermission ? CAN_INTERMISSION_BITS : 0)		\
+)
+
+/**
+ * can_frame_bits() - Calculate the number of bits in on the wire in a
+ *	CAN frame
+ * @is_fd: true: CAN-FD frame; false: Classical CAN frame.
+ * @is_eff: true: Extended frame; false: Standard frame.
+ * @bitstuffing: true: calculate the bitsuffing worst case; false:
+ *	calculate the bitsuffing best case (no dynamic
+ *	bitsuffing). Fixed stuff bits are always included.
+ * @intermission: if and only if true, include the inter frame space
+ *	assuming no bus idle (i.e. only the intermission gets added).
+ * @data_len: length of the data field in bytes. Correspond to
+ *	can(fd)_frame->len. Should be zero for remote frames. No
+ *	sanitization is done on @data_len.
+ *
+ * Return: the numbers of bits on the wire of a CAN frame.
+ */
+#define can_frame_bits(is_fd, is_eff, bitstuffing,		\
+		       intermission, data_len)			\
+(								\
+	is_fd ? __can_fd_frame_bits(is_eff, bitstuffing,	\
+				    intermission, data_len) :	\
+		__can_cc_frame_bits(is_eff, bitstuffing,	\
+				    intermission, data_len)	\
+)
+
+/*
+ * Number of bytes in a CAN frame
+ * (rounded up, including intermission)
+ */
+#define can_frame_bytes(is_fd, is_eff, bitstuffing, data_len)	\
+	DIV_ROUND_UP(can_frame_bits(is_fd, is_eff, bitstuffing,	\
+				    true, data_len),		\
+		     BITS_PER_BYTE)
 
 /*
  * Maximum size of a Classical CAN frame
- * (rounded up and ignoring bitstuffing)
+ * (rounded up, ignoring bitstuffing but including intermission)
  */
-#define CAN_FRAME_LEN_MAX (CAN_FRAME_OVERHEAD_EFF + CAN_MAX_DLEN)
+#define CAN_FRAME_LEN_MAX \
+	can_frame_bytes(false, true, false, CAN_MAX_DLEN)
 
 /*
  * Maximum size of a CAN-FD frame
  * (rounded up and ignoring bitstuffing)
  */
-#define CANFD_FRAME_LEN_MAX (CANFD_FRAME_OVERHEAD_EFF + CANFD_MAX_DLEN)
+#define CANFD_FRAME_LEN_MAX \
+	can_frame_bytes(true, true, false, CANFD_MAX_DLEN)
 
 /*
  * can_cc_dlc2len(value) - convert a given data length code (dlc) of a
-- 
2.39.3

Re: [PATCH v3 3/3] can: length: refactor frame lengths definition to add size in bits

[Simon Horman]

On Tue, May 30, 2023 at 11:46:37PM +0900, Vincent Mailhol wrote:
> Introduce a method to calculate the exact size in bits of a CAN(-FD)
> frame with or without dynamic bitsuffing.
> 
> These are all the possible combinations taken into account:
> 
>   - Classical CAN or CAN-FD
>   - Standard or Extended frame format
>   - CAN-FD CRC17 or CRC21
>   - Include or not intermission
> 
> Instead of doing several individual macro definitions, declare the
> can_frame_bits() function-like macro. To this extent, do a full
> refactoring of the length definitions.
> 
> In addition add the can_frame_bytes(). This function-like macro
> replaces the existing macro:
> 
>   - CAN_FRAME_OVERHEAD_SFF: can_frame_bytes(false, false, 0)
>   - CAN_FRAME_OVERHEAD_EFF: can_frame_bytes(false, true, 0)
>   - CANFD_FRAME_OVERHEAD_SFF: can_frame_bytes(true, false, 0)
>   - CANFD_FRAME_OVERHEAD_EFF: can_frame_bytes(true, true, 0)
> 
> The different maximum frame lengths (maximum data length, including
> intermission) are as follow:
> 
>    Frame type				bits	bytes
>   -------------------------------------------------------
>    Classic CAN SFF no-bitstuffing	111	14
>    Classic CAN EFF no-bitstuffing	131	17
>    Classic CAN SFF bitstuffing		135	17
>    Classic CAN EFF bitstuffing		160	20
>    CAN-FD SFF no-bitstuffing		579	73
>    CAN-FD EFF no-bitstuffing		598	75
>    CAN-FD SFF bitstuffing		712	89
>    CAN-FD EFF bitstuffing		736	92
> 
> The macro CAN_FRAME_LEN_MAX and CANFD_FRAME_LEN_MAX are kept as an
> alias to, respectively, can_frame_bytes(false, true, CAN_MAX_DLEN) and
> can_frame_bytes(true, true, CANFD_MAX_DLEN).
> 
> In addition to the above:
> 
>  - Use ISO 11898-1:2015 definitions for the name of the CAN frame
>    fields.
>  - Include linux/bits.h for use of BITS_PER_BYTE.
>  - Include linux/math.h for use of mult_frac() and
>    DIV_ROUND_UP(). N.B: the use of DIV_ROUND_UP() is not new to this
>    patch, but the include was previously omitted.
>  - Add copyright 2023 for myself.
> 
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

...

> +/**
> + * can_bitstuffing_len() - Calculate the maximum length with bitsuffing
> + * @bitstream_len: length of a destuffed bit stream

Hi Vincent,

it looks like an editing error has crept in here:

	s/bitstream_len/destuffed_len/

> + *
> + * The worst bit stuffing case is a sequence in which dominant and
> + * recessive bits alternate every four bits:
> + *
> + *   Destuffed: 1 1111  0000  1111  0000  1111
> + *   Stuffed:   1 1111o 0000i 1111o 0000i 1111o
> + *
> + * Nomenclature
> + *
> + *  - "0": dominant bit
> + *  - "o": dominant stuff bit
> + *  - "1": recessive bit
> + *  - "i": recessive stuff bit
> + *
> + * Aside of the first bit, one stuff bit is added every four bits.
> + *
> + * Return: length of the stuffed bit stream in the worst case scenario.
> + */
> +#define can_bitstuffing_len(destuffed_len)			\
> +	(destuffed_len + (destuffed_len - 1) / 4)

Re: [PATCH v3 3/3] can: length: refactor frame lengths definition to add size in bits

[Vincent MAILHOL]

On Wed. 31 May 2023 at 00:56, Simon Horman <simon.horman@corigine.com> wrote:
> On Tue, May 30, 2023 at 11:46:37PM +0900, Vincent Mailhol wrote:
> > Introduce a method to calculate the exact size in bits of a CAN(-FD)
> > frame with or without dynamic bitsuffing.
> >
> > These are all the possible combinations taken into account:
> >
> >   - Classical CAN or CAN-FD
> >   - Standard or Extended frame format
> >   - CAN-FD CRC17 or CRC21
> >   - Include or not intermission
> >
> > Instead of doing several individual macro definitions, declare the
> > can_frame_bits() function-like macro. To this extent, do a full
> > refactoring of the length definitions.
> >
> > In addition add the can_frame_bytes(). This function-like macro
> > replaces the existing macro:
> >
> >   - CAN_FRAME_OVERHEAD_SFF: can_frame_bytes(false, false, 0)
> >   - CAN_FRAME_OVERHEAD_EFF: can_frame_bytes(false, true, 0)
> >   - CANFD_FRAME_OVERHEAD_SFF: can_frame_bytes(true, false, 0)
> >   - CANFD_FRAME_OVERHEAD_EFF: can_frame_bytes(true, true, 0)
> >
> > The different maximum frame lengths (maximum data length, including
> > intermission) are as follow:
> >
> >    Frame type                         bits    bytes
> >   -------------------------------------------------------
> >    Classic CAN SFF no-bitstuffing     111     14
> >    Classic CAN EFF no-bitstuffing     131     17
> >    Classic CAN SFF bitstuffing                135     17
> >    Classic CAN EFF bitstuffing                160     20
> >    CAN-FD SFF no-bitstuffing          579     73
> >    CAN-FD EFF no-bitstuffing          598     75
> >    CAN-FD SFF bitstuffing             712     89
> >    CAN-FD EFF bitstuffing             736     92
> >
> > The macro CAN_FRAME_LEN_MAX and CANFD_FRAME_LEN_MAX are kept as an
> > alias to, respectively, can_frame_bytes(false, true, CAN_MAX_DLEN) and
> > can_frame_bytes(true, true, CANFD_MAX_DLEN).
> >
> > In addition to the above:
> >
> >  - Use ISO 11898-1:2015 definitions for the name of the CAN frame
> >    fields.
> >  - Include linux/bits.h for use of BITS_PER_BYTE.
> >  - Include linux/math.h for use of mult_frac() and
> >    DIV_ROUND_UP(). N.B: the use of DIV_ROUND_UP() is not new to this
> >    patch, but the include was previously omitted.
> >  - Add copyright 2023 for myself.
> >
> > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>
> ...
>
> > +/**
> > + * can_bitstuffing_len() - Calculate the maximum length with bitsuffing
> > + * @bitstream_len: length of a destuffed bit stream
>
> Hi Vincent,
>
> it looks like an editing error has crept in here:
>
>         s/bitstream_len/destuffed_len/

Doh! Thanks for picking this up.

I already prepared a v4 locally. Before sending it, I will wait one
day to see if there are other comments.

> > + *
> > + * The worst bit stuffing case is a sequence in which dominant and
> > + * recessive bits alternate every four bits:
> > + *
> > + *   Destuffed: 1 1111  0000  1111  0000  1111
> > + *   Stuffed:   1 1111o 0000i 1111o 0000i 1111o
> > + *
> > + * Nomenclature
> > + *
> > + *  - "0": dominant bit
> > + *  - "o": dominant stuff bit
> > + *  - "1": recessive bit
> > + *  - "i": recessive stuff bit
> > + *
> > + * Aside of the first bit, one stuff bit is added every four bits.
> > + *
> > + * Return: length of the stuffed bit stream in the worst case scenario.
> > + */
> > +#define can_bitstuffing_len(destuffed_len)                   \
> > +     (destuffed_len + (destuffed_len - 1) / 4)

Re: [PATCH v3 3/3] can: length: refactor frame lengths definition to add size in bits

[Simon Horman]

On Wed, May 31, 2023 at 02:29:43AM +0900, Vincent MAILHOL wrote:
> On Wed. 31 May 2023 at 00:56, Simon Horman <simon.horman@corigine.com> wrote:
> > On Tue, May 30, 2023 at 11:46:37PM +0900, Vincent Mailhol wrote:

...

> > > +/**
> > > + * can_bitstuffing_len() - Calculate the maximum length with bitsuffing
> > > + * @bitstream_len: length of a destuffed bit stream
> >
> > Hi Vincent,
> >
> > it looks like an editing error has crept in here:
> >
> >         s/bitstream_len/destuffed_len/
> 
> Doh! Thanks for picking this up.
> 
> I already prepared a v4 locally. Before sending it, I will wait one
> day to see if there are other comments.

Thanks, sounds good.

-- 
pw-bot: cr

Re: [PATCH v3 3/3] can: length: refactor frame lengths definition to add size in bits

[Vincent MAILHOL]

On Wed. 31 May 2023 at 04:53, Simon Horman <simon.horman@corigine.com> wrote:
> On Wed, May 31, 2023 at 02:29:43AM +0900, Vincent MAILHOL wrote:
> > On Wed. 31 May 2023 at 00:56, Simon Horman <simon.horman@corigine.com> wrote:
> > > On Tue, May 30, 2023 at 11:46:37PM +0900, Vincent Mailhol wrote:
>
> ...
>
> > > > +/**
> > > > + * can_bitstuffing_len() - Calculate the maximum length with bitsuffing
> > > > + * @bitstream_len: length of a destuffed bit stream
> > >
> > > Hi Vincent,
> > >
> > > it looks like an editing error has crept in here:
> > >
> > >         s/bitstream_len/destuffed_len/
> >
> > Doh! Thanks for picking this up.
> >
> > I already prepared a v4 locally. Before sending it, I will wait one
> > day to see if there are other comments.
>
> Thanks, sounds good.

On a side note, I was puzzled because I did not get any documentation
warnings when running a "make W=1". I just realized that only .c files
get checked, not headers. I sent a separate patch to fix the
documentation:

  https://lore.kernel.org/linux-doc/20230531093951.358769-1-mailhol.vincent@wanadoo.fr/

Yours sincerely,
Vincent Mailhol

RE: [PATCH v3 3/3] can: length: refactor frame lengths definition to add size in bits

[Thomas.Kopp]

> Introduce a method to calculate the exact size in bits of a CAN(-FD)
> frame with or without dynamic bitsuffing.
> 
> These are all the possible combinations taken into account:
> 
>   - Classical CAN or CAN-FD
>   - Standard or Extended frame format
>   - CAN-FD CRC17 or CRC21
>   - Include or not intermission
> 
> Instead of doing several individual macro definitions, declare the
> can_frame_bits() function-like macro. To this extent, do a full
> refactoring of the length definitions.
> 
> In addition add the can_frame_bytes(). This function-like macro
> replaces the existing macro:
> 
>   - CAN_FRAME_OVERHEAD_SFF: can_frame_bytes(false, false, 0)
>   - CAN_FRAME_OVERHEAD_EFF: can_frame_bytes(false, true, 0)
>   - CANFD_FRAME_OVERHEAD_SFF: can_frame_bytes(true, false, 0)
>   - CANFD_FRAME_OVERHEAD_EFF: can_frame_bytes(true, true, 0)
> 
> The different maximum frame lengths (maximum data length, including
> intermission) are as follow:
> 
>    Frame type                           bits    bytes
>   -------------------------------------------------------
>    Classic CAN SFF no-bitstuffing       111     14
>    Classic CAN EFF no-bitstuffing       131     17
>    Classic CAN SFF bitstuffing          135     17
>    Classic CAN EFF bitstuffing          160     20
>    CAN-FD SFF no-bitstuffing            579     73
>    CAN-FD EFF no-bitstuffing            598     75
>    CAN-FD SFF bitstuffing               712     89
>    CAN-FD EFF bitstuffing               736     92
> 
> The macro CAN_FRAME_LEN_MAX and CANFD_FRAME_LEN_MAX are kept as
> an
> alias to, respectively, can_frame_bytes(false, true, CAN_MAX_DLEN) and
> can_frame_bytes(true, true, CANFD_MAX_DLEN).
> 
> In addition to the above:
> 
>  - Use ISO 11898-1:2015 definitions for the name of the CAN frame
>    fields.
>  - Include linux/bits.h for use of BITS_PER_BYTE.
>  - Include linux/math.h for use of mult_frac() and
>    DIV_ROUND_UP(). N.B: the use of DIV_ROUND_UP() is not new to this
>    patch, but the include was previously omitted.
>  - Add copyright 2023 for myself.
> 
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> ---
>  drivers/net/can/dev/length.c |  15 +-
>  include/linux/can/length.h   | 298 +++++++++++++++++++++++++----------
>  2 files changed, 213 insertions(+), 100 deletions(-)
> 
> diff --git a/drivers/net/can/dev/length.c b/drivers/net/can/dev/length.c
> index b48140b1102e..b7f4d76dd444 100644
> --- a/drivers/net/can/dev/length.c
> +++ b/drivers/net/can/dev/length.c
> @@ -78,18 +78,7 @@ unsigned int can_skb_get_frame_len(const struct
> sk_buff *skb)
>         else
>                 len = cf->len;
> 
> -       if (can_is_canfd_skb(skb)) {
> -               if (cf->can_id & CAN_EFF_FLAG)
> -                       len += CANFD_FRAME_OVERHEAD_EFF;
> -               else
> -                       len += CANFD_FRAME_OVERHEAD_SFF;
> -       } else {
> -               if (cf->can_id & CAN_EFF_FLAG)
> -                       len += CAN_FRAME_OVERHEAD_EFF;
> -               else
> -                       len += CAN_FRAME_OVERHEAD_SFF;
> -       }
> -
> -       return len;
> +       return can_frame_bytes(can_is_canfd_skb(skb), cf->can_id &
> CAN_EFF_FLAG,
> +                              false, len);
>  }
>  EXPORT_SYMBOL_GPL(can_skb_get_frame_len);
> diff --git a/include/linux/can/length.h b/include/linux/can/length.h
> index 521fdbce2d69..ef6e78fa95b9 100644
> --- a/include/linux/can/length.h
> +++ b/include/linux/can/length.h
> @@ -1,132 +1,256 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
>  /* Copyright (C) 2020 Oliver Hartkopp <socketcan@hartkopp.net>
>   * Copyright (C) 2020 Marc Kleine-Budde <kernel@pengutronix.de>
> - * Copyright (C) 2020 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> + * Copyright (C) 2020, 2023 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>   */
> 
>  #ifndef _CAN_LENGTH_H
>  #define _CAN_LENGTH_H
> 
> +#include <linux/bits.h>
>  #include <linux/can.h>
>  #include <linux/can/netlink.h>
> +#include <linux/math.h>
> 
>  /*
> - * Size of a Classical CAN Standard Frame
> + * Size of a Classical CAN Standard Frame header in bits
>   *
> - * Name of Field                       Bits
> + * Name of Field                               Bits
>   * ---------------------------------------------------------
> - * Start-of-frame                      1
> - * Identifier                          11
> - * Remote transmission request (RTR)   1
> - * Identifier extension bit (IDE)      1
> - * Reserved bit (r0)                   1
> - * Data length code (DLC)              4
> - * Data field                          0...64
> - * CRC                                 15
> - * CRC delimiter                       1
> - * ACK slot                            1
> - * ACK delimiter                       1
> - * End-of-frame (EOF)                  7
> - * Inter frame spacing                 3
> + * Start Of Frame (SOF)                                1
> + * Arbitration field:
> + *     base ID                                 11
> + *     Remote Transmission Request (RTR)       1
> + * Control field:
> + *     IDentifier Extension bit (IDE)          1
> + *     FD Format indicatior (FDF)              1
> + *     Data Length Code (DLC)                  4
> + *
> + * including all fields preceding the data field, ignoring bitstuffing
> + */
> +#define CAN_FRAME_HEADER_SFF_BITS 19
> +
> +/*
> + * Size of a Classical CAN Extended Frame header in bits
> + *
> + * Name of Field                               Bits
> + * ---------------------------------------------------------
> + * Start Of Frame (SOF)                                1
> + * Arbitration field:
> + *     base ID                                 11
> + *     Substitute Remote Request (SRR)         1
> + *     IDentifier Extension bit (IDE)          1
> + *     ID extension                            18
> + *     Remote Transmission Request (RTR)       1
> + * Control field:
> + *     FD Format indicatior (FDF)              1
Nit: indicator, same above
> + *     Reserved bit (r0)                       1
> + *     Data length code (DLC)                  4
> + *
> + * including all fields preceding the data field, ignoring bitstuffing
> + */
> +#define CAN_FRAME_HEADER_EFF_BITS 39
> +
> +/*
> + * Size of a CAN-FD Standard Frame in bits
> + *
> + * Name of Field                               Bits
> + * ---------------------------------------------------------
> + * Start Of Frame (SOF)                                1
> + * Arbitration field:
> + *     base ID                                 11
> + *     Remote Request Substitution (RRS)       1
> + * Control field:
> + *     IDentifier Extension bit (IDE)          1
> + *     FD Format indicator (FDF)               1
> + *     Reserved bit (res)                      1
> + *     Bit Rate Switch (BRS)                   1
> + *     Error Status Indicator (ESI)            1
> + *     Data length code (DLC)                  4
> + *
> + * including all fields preceding the data field, ignoring bitstuffing
> + */
> +#define CANFD_FRAME_HEADER_SFF_BITS 22
> +
> +/*
> + * Size of a CAN-FD Extended Frame in bits
> + *
> + * Name of Field                               Bits
> + * ---------------------------------------------------------
> + * Start Of Frame (SOF)                                1
> + * Arbitration field:
> + *     base ID                                 11
> + *     Substitute Remote Request (SRR)         1
> + *     IDentifier Extension bit (IDE)          1
> + *     ID extension                            18
> + *     Remote Request Substitution (RRS)       1
> + * Control field:
> + *     FD Format indicator (FDF)               1
> + *     Reserved bit (res)                      1
> + *     Bit Rate Switch (BRS)                   1
> + *     Error Status Indicator (ESI)            1
> + *     Data length code (DLC)                  4
>   *
> - * rounded up and ignoring bitstuffing
> + * including all fields preceding the data field, ignoring bitstuffing
>   */
> -#define CAN_FRAME_OVERHEAD_SFF DIV_ROUND_UP(47, 8)
> +#define CANFD_FRAME_HEADER_EFF_BITS 41
> 
>  /*
> - * Size of a Classical CAN Extended Frame
> + * Size of a CAN CRC Field in bits
>   *
>   * Name of Field                       Bits
>   * ---------------------------------------------------------
> - * Start-of-frame                      1
> - * Identifier A                                11
> - * Substitute remote request (SRR)     1
> - * Identifier extension bit (IDE)      1
> - * Identifier B                                18
> - * Remote transmission request (RTR)   1
> - * Reserved bits (r1, r0)              2
> - * Data length code (DLC)              4
> - * Data field                          0...64
> - * CRC                                 15
> - * CRC delimiter                       1
> - * ACK slot                            1
> - * ACK delimiter                       1
> - * End-of-frame (EOF)                  7
> - * Inter frame spacing                 3
> + * CRC sequence (CRC15)                        15
> + * CRC Delimiter                       1
>   *
> - * rounded up and ignoring bitstuffing
> + * ignoring bitstuffing
>   */
> -#define CAN_FRAME_OVERHEAD_EFF DIV_ROUND_UP(67, 8)
> +#define CAN_FRAME_CRC_FIELD_BITS 16
> 
>  /*
> - * Size of a CAN-FD Standard Frame
> + * Size of a CAN-FD CRC17 Field in bits (length: 0..16)
>   *
>   * Name of Field                       Bits
>   * ---------------------------------------------------------
> - * Start-of-frame                      1
> - * Identifier                          11
> - * Remote Request Substitution (RRS)   1
> - * Identifier extension bit (IDE)      1
> - * Flexible data rate format (FDF)     1
> - * Reserved bit (r0)                   1
> - * Bit Rate Switch (BRS)               1
> - * Error Status Indicator (ESI)                1
> - * Data length code (DLC)              4
> - * Data field                          0...512
> - * Stuff Bit Count (SBC)               4
> - * CRC                                 0...16: 17 20...64:21
> - * CRC delimiter (CD)                  1
> - * Fixed Stuff bits (FSB)              0...16: 6 20...64:7
> - * ACK slot (AS)                       1
> - * ACK delimiter (AD)                  1
> - * End-of-frame (EOF)                  7
> - * Inter frame spacing                 3
> - *
> - * assuming CRC21, rounded up and ignoring dynamic bitstuffing
> - */
> -#define CANFD_FRAME_OVERHEAD_SFF DIV_ROUND_UP(67, 8)
> + * Stuff Count                         4
> + * CRC Sequence (CRC17)                        17
> + * CRC Delimiter                       1
> + * Fixed stuff bits                    6
> + */
> +#define CANFD_FRAME_CRC17_FIELD_BITS 28
> 
>  /*
> - * Size of a CAN-FD Extended Frame
> + * Size of a CAN-FD CRC21 Field in bits (length: 20..64)
>   *
>   * Name of Field                       Bits
>   * ---------------------------------------------------------
> - * Start-of-frame                      1
> - * Identifier A                                11
> - * Substitute remote request (SRR)     1
> - * Identifier extension bit (IDE)      1
> - * Identifier B                                18
> - * Remote Request Substitution (RRS)   1
> - * Flexible data rate format (FDF)     1
> - * Reserved bit (r0)                   1
> - * Bit Rate Switch (BRS)               1
> - * Error Status Indicator (ESI)                1
> - * Data length code (DLC)              4
> - * Data field                          0...512
> - * Stuff Bit Count (SBC)               4
> - * CRC                                 0...16: 17 20...64:21
> - * CRC delimiter (CD)                  1
> - * Fixed Stuff bits (FSB)              0...16: 6 20...64:7
> - * ACK slot (AS)                       1
> - * ACK delimiter (AD)                  1
> - * End-of-frame (EOF)                  7
> - * Inter frame spacing                 3
> - *
> - * assuming CRC21, rounded up and ignoring dynamic bitstuffing
> - */
> -#define CANFD_FRAME_OVERHEAD_EFF DIV_ROUND_UP(86, 8)
> + * Stuff Count                         4
> + * CRC sequence (CRC21)                        21
> + * CRC Delimiter                       1
> + * Fixed stuff bits                    7
> + */
> +#define CANFD_FRAME_CRC21_FIELD_BITS 33
> +
> +/*
> + * Size of a CAN(-FD) Frame footer in bits
> + *
> + * Name of Field                       Bits
> + * ---------------------------------------------------------
> + * ACK slot                            1
> + * ACK delimiter                       1
> + * End Of Frame (EOF)                  7
> + *
> + * including all fields following the CRC field
> + */
> +#define CAN_FRAME_FOOTER_BITS 9
> +
> +/*
> + * First part of the Inter Frame Space
> + * (a.k.a. IMF - intermission field)
> + */
> +#define CAN_INTERMISSION_BITS 3
> +
> +/**
> + * can_bitstuffing_len() - Calculate the maximum length with bitsuffing
Nit: bitstuffing, same further down
> + * @bitstream_len: length of a destuffed bit stream
> + *
> + * The worst bit stuffing case is a sequence in which dominant and
> + * recessive bits alternate every four bits:
> + *
> + *   Destuffed: 1 1111  0000  1111  0000  1111
> + *   Stuffed:   1 1111o 0000i 1111o 0000i 1111o
> + *
> + * Nomenclature
> + *
> + *  - "0": dominant bit
> + *  - "o": dominant stuff bit
> + *  - "1": recessive bit
> + *  - "i": recessive stuff bit
> + *
> + * Aside of the first bit, one stuff bit is added every four bits.
> + *
> + * Return: length of the stuffed bit stream in the worst case scenario.
> + */
> +#define can_bitstuffing_len(destuffed_len)                     \
> +       (destuffed_len + (destuffed_len - 1) / 4)
> +
> +#define __can_bitstuffing_len(bitstuffing, destuffed_len)      \
> +       (bitstuffing ? can_bitstuffing_len(destuffed_len) :     \
> +                      destuffed_len)
> +
> +#define __can_cc_frame_bits(is_eff, bitstuffing,               \
> +                           intermission, data_len)             \
> +(                                                              \
> +       __can_bitstuffing_len(bitstuffing,                      \
> +               (is_eff ? CAN_FRAME_HEADER_EFF_BITS :           \
> +                          CAN_FRAME_HEADER_SFF_BITS) +         \
> +               data_len * BITS_PER_BYTE +                      \
> +               CAN_FRAME_CRC_FIELD_BITS) +                     \
> +       CAN_FRAME_FOOTER_BITS +                                 \
> +       (intermission ? CAN_INTERMISSION_BITS : 0)              \
> +)
I think Footer and Intermission need to be pulled out of the parameter for __can_bitstuffing_length as these fields are never stuffed.
> +
> +#define __can_fd_frame_bits(is_eff, bitstuffing,               \
> +                           intermission, data_len)             \
> +(                                                              \
> +       __can_bitstuffing_len(bitstuffing,                      \
> +               (is_eff ? CANFD_FRAME_HEADER_EFF_BITS :         \
> +                          CANFD_FRAME_HEADER_SFF_BITS) +       \
> +               data_len * BITS_PER_BYTE) +                     \
> +       (data_len <= 16 ?                                       \
> +               CANFD_FRAME_CRC17_FIELD_BITS :                  \
> +               CANFD_FRAME_CRC21_FIELD_BITS) +                 \
> +       CAN_FRAME_FOOTER_BITS +                                 \
> +       (intermission ? CAN_INTERMISSION_BITS : 0)              \
> +)
I think Footer and Intermission need to be pulled out of the parameter for __can_bitstuffing_length as these fields are never stuffed.
The CAN_FRAME_CRC_FIELD_BITS bits need to be pulled out of the can_bitstuffing_len. That portion of the Frame is not dynamically stuffed in FD frames.
> +
> +/**
> + * can_frame_bits() - Calculate the number of bits in on the wire in a
Nit: "in on the wire" -in
> + *     CAN frame
> + * @is_fd: true: CAN-FD frame; false: Classical CAN frame.
> + * @is_eff: true: Extended frame; false: Standard frame.
> + * @bitstuffing: true: calculate the bitsuffing worst case; false:
> + *     calculate the bitsuffing best case (no dynamic
> + *     bitsuffing). Fixed stuff bits are always included.
> + * @intermission: if and only if true, include the inter frame space
> + *     assuming no bus idle (i.e. only the intermission gets added).
> + * @data_len: length of the data field in bytes. Correspond to
> + *     can(fd)_frame->len. Should be zero for remote frames. No
> + *     sanitization is done on @data_len.
> + *
> + * Return: the numbers of bits on the wire of a CAN frame.
> + */
> +#define can_frame_bits(is_fd, is_eff, bitstuffing,             \
> +                      intermission, data_len)                  \
> +(                                                              \
> +       is_fd ? __can_fd_frame_bits(is_eff, bitstuffing,        \
> +                                   intermission, data_len) :   \
> +               __can_cc_frame_bits(is_eff, bitstuffing,        \
> +                                   intermission, data_len)     \
> +)
> +
> +/*
> + * Number of bytes in a CAN frame
> + * (rounded up, including intermission)
> + */
> +#define can_frame_bytes(is_fd, is_eff, bitstuffing, data_len)  \
> +       DIV_ROUND_UP(can_frame_bits(is_fd, is_eff, bitstuffing, \
> +                                   true, data_len),            \
> +                    BITS_PER_BYTE)
> 
>  /*
>   * Maximum size of a Classical CAN frame
> - * (rounded up and ignoring bitstuffing)
> + * (rounded up, ignoring bitstuffing but including intermission)
>   */
> -#define CAN_FRAME_LEN_MAX (CAN_FRAME_OVERHEAD_EFF +
> CAN_MAX_DLEN)
> +#define CAN_FRAME_LEN_MAX \
> +       can_frame_bytes(false, true, false, CAN_MAX_DLEN)
> 
>  /*
>   * Maximum size of a CAN-FD frame
>   * (rounded up and ignoring bitstuffing)
Ignoring dynamic bitstuffing
>   */
> -#define CANFD_FRAME_LEN_MAX (CANFD_FRAME_OVERHEAD_EFF +
> CANFD_MAX_DLEN)
> +#define CANFD_FRAME_LEN_MAX \
> +       can_frame_bytes(true, true, false, CANFD_MAX_DLEN)
> 
>  /*
>   * can_cc_dlc2len(value) - convert a given data length code (dlc) of a
> --
> 2.39.3

I think your attribution of suggested-by for myself is mixed up for the patches 2/3 and 3/3 😊

For the entire series you can add my reviewed-by.

Thanks,
Thomas

Re: [PATCH v3 3/3] can: length: refactor frame lengths definition to add size in bits

[Vincent MAILHOL]

On Thu. 1 juin 2023 at19:42, <Thomas.Kopp@microchip.com> wrote:
> > Introduce a method to calculate the exact size in bits of a CAN(-FD)
> > frame with or without dynamic bitsuffing.
> >
> > These are all the possible combinations taken into account:
> >
> >   - Classical CAN or CAN-FD
> >   - Standard or Extended frame format
> >   - CAN-FD CRC17 or CRC21
> >   - Include or not intermission
> >
> > Instead of doing several individual macro definitions, declare the
> > can_frame_bits() function-like macro. To this extent, do a full
> > refactoring of the length definitions.
> >
> > In addition add the can_frame_bytes(). This function-like macro
> > replaces the existing macro:
> >
> >   - CAN_FRAME_OVERHEAD_SFF: can_frame_bytes(false, false, 0)
> >   - CAN_FRAME_OVERHEAD_EFF: can_frame_bytes(false, true, 0)
> >   - CANFD_FRAME_OVERHEAD_SFF: can_frame_bytes(true, false, 0)
> >   - CANFD_FRAME_OVERHEAD_EFF: can_frame_bytes(true, true, 0)
> >
> > The different maximum frame lengths (maximum data length, including
> > intermission) are as follow:
> >
> >    Frame type                           bits    bytes
> >   -------------------------------------------------------
> >    Classic CAN SFF no-bitstuffing       111     14
> >    Classic CAN EFF no-bitstuffing       131     17
> >    Classic CAN SFF bitstuffing          135     17
> >    Classic CAN EFF bitstuffing          160     20
> >    CAN-FD SFF no-bitstuffing            579     73
> >    CAN-FD EFF no-bitstuffing            598     75
> >    CAN-FD SFF bitstuffing               712     89
> >    CAN-FD EFF bitstuffing               736     92
> >
> > The macro CAN_FRAME_LEN_MAX and CANFD_FRAME_LEN_MAX are kept as
> > an
> > alias to, respectively, can_frame_bytes(false, true, CAN_MAX_DLEN) and
> > can_frame_bytes(true, true, CANFD_MAX_DLEN).
> >
> > In addition to the above:
> >
> >  - Use ISO 11898-1:2015 definitions for the name of the CAN frame
> >    fields.
> >  - Include linux/bits.h for use of BITS_PER_BYTE.
> >  - Include linux/math.h for use of mult_frac() and
> >    DIV_ROUND_UP(). N.B: the use of DIV_ROUND_UP() is not new to this
> >    patch, but the include was previously omitted.
> >  - Add copyright 2023 for myself.
> >
> > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > ---
> >  drivers/net/can/dev/length.c |  15 +-
> >  include/linux/can/length.h   | 298 +++++++++++++++++++++++++----------
> >  2 files changed, 213 insertions(+), 100 deletions(-)
> >
> > diff --git a/drivers/net/can/dev/length.c b/drivers/net/can/dev/length.c
> > index b48140b1102e..b7f4d76dd444 100644
> > --- a/drivers/net/can/dev/length.c
> > +++ b/drivers/net/can/dev/length.c
> > @@ -78,18 +78,7 @@ unsigned int can_skb_get_frame_len(const struct
> > sk_buff *skb)
> >         else
> >                 len = cf->len;
> >
> > -       if (can_is_canfd_skb(skb)) {
> > -               if (cf->can_id & CAN_EFF_FLAG)
> > -                       len += CANFD_FRAME_OVERHEAD_EFF;
> > -               else
> > -                       len += CANFD_FRAME_OVERHEAD_SFF;
> > -       } else {
> > -               if (cf->can_id & CAN_EFF_FLAG)
> > -                       len += CAN_FRAME_OVERHEAD_EFF;
> > -               else
> > -                       len += CAN_FRAME_OVERHEAD_SFF;
> > -       }
> > -
> > -       return len;
> > +       return can_frame_bytes(can_is_canfd_skb(skb), cf->can_id &
> > CAN_EFF_FLAG,
> > +                              false, len);
> >  }
> >  EXPORT_SYMBOL_GPL(can_skb_get_frame_len);
> > diff --git a/include/linux/can/length.h b/include/linux/can/length.h
> > index 521fdbce2d69..ef6e78fa95b9 100644
> > --- a/include/linux/can/length.h
> > +++ b/include/linux/can/length.h
> > @@ -1,132 +1,256 @@
> >  /* SPDX-License-Identifier: GPL-2.0 */
> >  /* Copyright (C) 2020 Oliver Hartkopp <socketcan@hartkopp.net>
> >   * Copyright (C) 2020 Marc Kleine-Budde <kernel@pengutronix.de>
> > - * Copyright (C) 2020 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > + * Copyright (C) 2020, 2023 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> >   */
> >
> >  #ifndef _CAN_LENGTH_H
> >  #define _CAN_LENGTH_H
> >
> > +#include <linux/bits.h>
> >  #include <linux/can.h>
> >  #include <linux/can/netlink.h>
> > +#include <linux/math.h>
> >
> >  /*
> > - * Size of a Classical CAN Standard Frame
> > + * Size of a Classical CAN Standard Frame header in bits
> >   *
> > - * Name of Field                       Bits
> > + * Name of Field                               Bits
> >   * ---------------------------------------------------------
> > - * Start-of-frame                      1
> > - * Identifier                          11
> > - * Remote transmission request (RTR)   1
> > - * Identifier extension bit (IDE)      1
> > - * Reserved bit (r0)                   1
> > - * Data length code (DLC)              4
> > - * Data field                          0...64
> > - * CRC                                 15
> > - * CRC delimiter                       1
> > - * ACK slot                            1
> > - * ACK delimiter                       1
> > - * End-of-frame (EOF)                  7
> > - * Inter frame spacing                 3
> > + * Start Of Frame (SOF)                                1
> > + * Arbitration field:
> > + *     base ID                                 11
> > + *     Remote Transmission Request (RTR)       1
> > + * Control field:
> > + *     IDentifier Extension bit (IDE)          1
> > + *     FD Format indicatior (FDF)              1
> > + *     Data Length Code (DLC)                  4
> > + *
> > + * including all fields preceding the data field, ignoring bitstuffing
> > + */
> > +#define CAN_FRAME_HEADER_SFF_BITS 19
> > +
> > +/*
> > + * Size of a Classical CAN Extended Frame header in bits
> > + *
> > + * Name of Field                               Bits
> > + * ---------------------------------------------------------
> > + * Start Of Frame (SOF)                                1
> > + * Arbitration field:
> > + *     base ID                                 11
> > + *     Substitute Remote Request (SRR)         1
> > + *     IDentifier Extension bit (IDE)          1
> > + *     ID extension                            18
> > + *     Remote Transmission Request (RTR)       1
> > + * Control field:
> > + *     FD Format indicatior (FDF)              1
> Nit: indicator, same above

ACK.

> > + *     Reserved bit (r0)                       1
> > + *     Data length code (DLC)                  4
> > + *
> > + * including all fields preceding the data field, ignoring bitstuffing
> > + */
> > +#define CAN_FRAME_HEADER_EFF_BITS 39
> > +
> > +/*
> > + * Size of a CAN-FD Standard Frame in bits
> > + *
> > + * Name of Field                               Bits
> > + * ---------------------------------------------------------
> > + * Start Of Frame (SOF)                                1
> > + * Arbitration field:
> > + *     base ID                                 11
> > + *     Remote Request Substitution (RRS)       1
> > + * Control field:
> > + *     IDentifier Extension bit (IDE)          1
> > + *     FD Format indicator (FDF)               1
> > + *     Reserved bit (res)                      1
> > + *     Bit Rate Switch (BRS)                   1
> > + *     Error Status Indicator (ESI)            1
> > + *     Data length code (DLC)                  4
> > + *
> > + * including all fields preceding the data field, ignoring bitstuffing
> > + */
> > +#define CANFD_FRAME_HEADER_SFF_BITS 22
> > +
> > +/*
> > + * Size of a CAN-FD Extended Frame in bits
> > + *
> > + * Name of Field                               Bits
> > + * ---------------------------------------------------------
> > + * Start Of Frame (SOF)                                1
> > + * Arbitration field:
> > + *     base ID                                 11
> > + *     Substitute Remote Request (SRR)         1
> > + *     IDentifier Extension bit (IDE)          1
> > + *     ID extension                            18
> > + *     Remote Request Substitution (RRS)       1
> > + * Control field:
> > + *     FD Format indicator (FDF)               1
> > + *     Reserved bit (res)                      1
> > + *     Bit Rate Switch (BRS)                   1
> > + *     Error Status Indicator (ESI)            1
> > + *     Data length code (DLC)                  4
> >   *
> > - * rounded up and ignoring bitstuffing
> > + * including all fields preceding the data field, ignoring bitstuffing
> >   */
> > -#define CAN_FRAME_OVERHEAD_SFF DIV_ROUND_UP(47, 8)
> > +#define CANFD_FRAME_HEADER_EFF_BITS 41
> >
> >  /*
> > - * Size of a Classical CAN Extended Frame
> > + * Size of a CAN CRC Field in bits
> >   *
> >   * Name of Field                       Bits
> >   * ---------------------------------------------------------
> > - * Start-of-frame                      1
> > - * Identifier A                                11
> > - * Substitute remote request (SRR)     1
> > - * Identifier extension bit (IDE)      1
> > - * Identifier B                                18
> > - * Remote transmission request (RTR)   1
> > - * Reserved bits (r1, r0)              2
> > - * Data length code (DLC)              4
> > - * Data field                          0...64
> > - * CRC                                 15
> > - * CRC delimiter                       1
> > - * ACK slot                            1
> > - * ACK delimiter                       1
> > - * End-of-frame (EOF)                  7
> > - * Inter frame spacing                 3
> > + * CRC sequence (CRC15)                        15
> > + * CRC Delimiter                       1
> >   *
> > - * rounded up and ignoring bitstuffing
> > + * ignoring bitstuffing
> >   */
> > -#define CAN_FRAME_OVERHEAD_EFF DIV_ROUND_UP(67, 8)
> > +#define CAN_FRAME_CRC_FIELD_BITS 16
> >
> >  /*
> > - * Size of a CAN-FD Standard Frame
> > + * Size of a CAN-FD CRC17 Field in bits (length: 0..16)
> >   *
> >   * Name of Field                       Bits
> >   * ---------------------------------------------------------
> > - * Start-of-frame                      1
> > - * Identifier                          11
> > - * Remote Request Substitution (RRS)   1
> > - * Identifier extension bit (IDE)      1
> > - * Flexible data rate format (FDF)     1
> > - * Reserved bit (r0)                   1
> > - * Bit Rate Switch (BRS)               1
> > - * Error Status Indicator (ESI)                1
> > - * Data length code (DLC)              4
> > - * Data field                          0...512
> > - * Stuff Bit Count (SBC)               4
> > - * CRC                                 0...16: 17 20...64:21
> > - * CRC delimiter (CD)                  1
> > - * Fixed Stuff bits (FSB)              0...16: 6 20...64:7
> > - * ACK slot (AS)                       1
> > - * ACK delimiter (AD)                  1
> > - * End-of-frame (EOF)                  7
> > - * Inter frame spacing                 3
> > - *
> > - * assuming CRC21, rounded up and ignoring dynamic bitstuffing
> > - */
> > -#define CANFD_FRAME_OVERHEAD_SFF DIV_ROUND_UP(67, 8)
> > + * Stuff Count                         4
> > + * CRC Sequence (CRC17)                        17
> > + * CRC Delimiter                       1
> > + * Fixed stuff bits                    6
> > + */
> > +#define CANFD_FRAME_CRC17_FIELD_BITS 28
> >
> >  /*
> > - * Size of a CAN-FD Extended Frame
> > + * Size of a CAN-FD CRC21 Field in bits (length: 20..64)
> >   *
> >   * Name of Field                       Bits
> >   * ---------------------------------------------------------
> > - * Start-of-frame                      1
> > - * Identifier A                                11
> > - * Substitute remote request (SRR)     1
> > - * Identifier extension bit (IDE)      1
> > - * Identifier B                                18
> > - * Remote Request Substitution (RRS)   1
> > - * Flexible data rate format (FDF)     1
> > - * Reserved bit (r0)                   1
> > - * Bit Rate Switch (BRS)               1
> > - * Error Status Indicator (ESI)                1
> > - * Data length code (DLC)              4
> > - * Data field                          0...512
> > - * Stuff Bit Count (SBC)               4
> > - * CRC                                 0...16: 17 20...64:21
> > - * CRC delimiter (CD)                  1
> > - * Fixed Stuff bits (FSB)              0...16: 6 20...64:7
> > - * ACK slot (AS)                       1
> > - * ACK delimiter (AD)                  1
> > - * End-of-frame (EOF)                  7
> > - * Inter frame spacing                 3
> > - *
> > - * assuming CRC21, rounded up and ignoring dynamic bitstuffing
> > - */
> > -#define CANFD_FRAME_OVERHEAD_EFF DIV_ROUND_UP(86, 8)
> > + * Stuff Count                         4
> > + * CRC sequence (CRC21)                        21
> > + * CRC Delimiter                       1
> > + * Fixed stuff bits                    7
> > + */
> > +#define CANFD_FRAME_CRC21_FIELD_BITS 33
> > +
> > +/*
> > + * Size of a CAN(-FD) Frame footer in bits
> > + *
> > + * Name of Field                       Bits
> > + * ---------------------------------------------------------
> > + * ACK slot                            1
> > + * ACK delimiter                       1
> > + * End Of Frame (EOF)                  7
> > + *
> > + * including all fields following the CRC field
> > + */
> > +#define CAN_FRAME_FOOTER_BITS 9
> > +
> > +/*
> > + * First part of the Inter Frame Space
> > + * (a.k.a. IMF - intermission field)
> > + */
> > +#define CAN_INTERMISSION_BITS 3
> > +
> > +/**
> > + * can_bitstuffing_len() - Calculate the maximum length with bitsuffing
> Nit: bitstuffing, same further down

ACK.

> > + * @bitstream_len: length of a destuffed bit stream
> > + *
> > + * The worst bit stuffing case is a sequence in which dominant and
> > + * recessive bits alternate every four bits:
> > + *
> > + *   Destuffed: 1 1111  0000  1111  0000  1111
> > + *   Stuffed:   1 1111o 0000i 1111o 0000i 1111o
> > + *
> > + * Nomenclature
> > + *
> > + *  - "0": dominant bit
> > + *  - "o": dominant stuff bit
> > + *  - "1": recessive bit
> > + *  - "i": recessive stuff bit
> > + *
> > + * Aside of the first bit, one stuff bit is added every four bits.
> > + *
> > + * Return: length of the stuffed bit stream in the worst case scenario.
> > + */
> > +#define can_bitstuffing_len(destuffed_len)                     \
> > +       (destuffed_len + (destuffed_len - 1) / 4)
> > +
> > +#define __can_bitstuffing_len(bitstuffing, destuffed_len)      \
> > +       (bitstuffing ? can_bitstuffing_len(destuffed_len) :     \
> > +                      destuffed_len)
> > +
> > +#define __can_cc_frame_bits(is_eff, bitstuffing,               \
> > +                           intermission, data_len)             \
> > +(                                                              \
> > +       __can_bitstuffing_len(bitstuffing,                      \
> > +               (is_eff ? CAN_FRAME_HEADER_EFF_BITS :           \
> > +                          CAN_FRAME_HEADER_SFF_BITS) +         \
> > +               data_len * BITS_PER_BYTE +                      \
> > +               CAN_FRAME_CRC_FIELD_BITS) +                     \
> > +       CAN_FRAME_FOOTER_BITS +                                 \
> > +       (intermission ? CAN_INTERMISSION_BITS : 0)              \
> > +)
> I think Footer and Intermission need to be pulled out of the parameter for __can_bitstuffing_length as these fields are never stuffed.

Look again at the opening and closing bracket of
__can_bitstuffing_len(). These are already out :)
I indented the parameters of __can_bitstuffing_length() to highlight
what is in and out.

Maybe adding some newlines would help readability? Something like that:

  #define __can_cc_frame_bits(is_eff, bitstuffing,                \
                              intermission, data_len)             \
  (                                                               \
          __can_bitstuffing_len(                                  \
                  bitstuffing,                                    \
                  (is_eff ? CAN_FRAME_HEADER_EFF_BITS :           \
                             CAN_FRAME_HEADER_SFF_BITS) +         \
                  data_len * BITS_PER_BYTE +                      \
                  CAN_FRAME_CRC_FIELD_BITS)                       \
          +                                                       \
          CAN_FRAME_FOOTER_BITS +                                 \
          (intermission ? CAN_INTERMISSION_BITS : 0)              \
  )

> > +
> > +#define __can_fd_frame_bits(is_eff, bitstuffing,               \
> > +                           intermission, data_len)             \
> > +(                                                              \
> > +       __can_bitstuffing_len(bitstuffing,                      \
> > +               (is_eff ? CANFD_FRAME_HEADER_EFF_BITS :         \
> > +                          CANFD_FRAME_HEADER_SFF_BITS) +       \
> > +               data_len * BITS_PER_BYTE) +                     \
> > +       (data_len <= 16 ?                                       \
> > +               CANFD_FRAME_CRC17_FIELD_BITS :                  \
> > +               CANFD_FRAME_CRC21_FIELD_BITS) +                 \
> > +       CAN_FRAME_FOOTER_BITS +                                 \
> > +       (intermission ? CAN_INTERMISSION_BITS : 0)              \
> > +)
> I think Footer and Intermission need to be pulled out of the parameter for __can_bitstuffing_length as these fields are never stuffed.
> The CAN_FRAME_CRC_FIELD_BITS bits need to be pulled out of the can_bitstuffing_len. That portion of the Frame is not dynamically stuffed in FD frames.

Same as above, these are already out.

> > +
> > +/**
> > + * can_frame_bits() - Calculate the number of bits in on the wire in a
> Nit: "in on the wire" -in
> > + *     CAN frame
> > + * @is_fd: true: CAN-FD frame; false: Classical CAN frame.
> > + * @is_eff: true: Extended frame; false: Standard frame.
> > + * @bitstuffing: true: calculate the bitsuffing worst case; false:
> > + *     calculate the bitsuffing best case (no dynamic
> > + *     bitsuffing). Fixed stuff bits are always included.
> > + * @intermission: if and only if true, include the inter frame space
> > + *     assuming no bus idle (i.e. only the intermission gets added).
> > + * @data_len: length of the data field in bytes. Correspond to
> > + *     can(fd)_frame->len. Should be zero for remote frames. No
> > + *     sanitization is done on @data_len.
> > + *
> > + * Return: the numbers of bits on the wire of a CAN frame.
> > + */
> > +#define can_frame_bits(is_fd, is_eff, bitstuffing,             \
> > +                      intermission, data_len)                  \
> > +(                                                              \
> > +       is_fd ? __can_fd_frame_bits(is_eff, bitstuffing,        \
> > +                                   intermission, data_len) :   \
> > +               __can_cc_frame_bits(is_eff, bitstuffing,        \
> > +                                   intermission, data_len)     \
> > +)
> > +
> > +/*
> > + * Number of bytes in a CAN frame
> > + * (rounded up, including intermission)
> > + */
> > +#define can_frame_bytes(is_fd, is_eff, bitstuffing, data_len)  \
> > +       DIV_ROUND_UP(can_frame_bits(is_fd, is_eff, bitstuffing, \
> > +                                   true, data_len),            \
> > +                    BITS_PER_BYTE)
> >
> >  /*
> >   * Maximum size of a Classical CAN frame
> > - * (rounded up and ignoring bitstuffing)
> > + * (rounded up, ignoring bitstuffing but including intermission)
> >   */
> > -#define CAN_FRAME_LEN_MAX (CAN_FRAME_OVERHEAD_EFF +
> > CAN_MAX_DLEN)
> > +#define CAN_FRAME_LEN_MAX \
> > +       can_frame_bytes(false, true, false, CAN_MAX_DLEN)
> >
> >  /*
> >   * Maximum size of a CAN-FD frame
> >   * (rounded up and ignoring bitstuffing)
> Ignoring dynamic bitstuffing
> >   */
> > -#define CANFD_FRAME_LEN_MAX (CANFD_FRAME_OVERHEAD_EFF +
> > CANFD_MAX_DLEN)
> > +#define CANFD_FRAME_LEN_MAX \
> > +       can_frame_bytes(true, true, false, CANFD_MAX_DLEN)
> >
> >  /*
> >   * can_cc_dlc2len(value) - convert a given data length code (dlc) of a
> > --
> > 2.39.3
>
> I think your attribution of suggested-by for myself is mixed up for the patches 2/3 and 3/3 😊

ACK. I will remove it from 2/3 and add it to 3/3.

> For the entire series you can add my reviewed-by.

I will do so.
Thanks for picking my typos!

RE: [PATCH v3 3/3] can: length: refactor frame lengths definition to add size in bits

[Thomas.Kopp]

> > > Introduce a method to calculate the exact size in bits of a CAN(-FD)
> > > frame with or without dynamic bitsuffing.
> > >
> > > These are all the possible combinations taken into account:
> > >
> > >   - Classical CAN or CAN-FD
> > >   - Standard or Extended frame format
> > >   - CAN-FD CRC17 or CRC21
> > >   - Include or not intermission
> > >
> > > Instead of doing several individual macro definitions, declare the
> > > can_frame_bits() function-like macro. To this extent, do a full
> > > refactoring of the length definitions.
> > >
> > > In addition add the can_frame_bytes(). This function-like macro
> > > replaces the existing macro:
> > >
> > >   - CAN_FRAME_OVERHEAD_SFF: can_frame_bytes(false, false, 0)
> > >   - CAN_FRAME_OVERHEAD_EFF: can_frame_bytes(false, true, 0)
> > >   - CANFD_FRAME_OVERHEAD_SFF: can_frame_bytes(true, false, 0)
> > >   - CANFD_FRAME_OVERHEAD_EFF: can_frame_bytes(true, true, 0)
> > >
> > > The different maximum frame lengths (maximum data length, including
> > > intermission) are as follow:
> > >
> > >    Frame type                           bits    bytes
> > >   -------------------------------------------------------
> > >    Classic CAN SFF no-bitstuffing       111     14
> > >    Classic CAN EFF no-bitstuffing       131     17
> > >    Classic CAN SFF bitstuffing          135     17
> > >    Classic CAN EFF bitstuffing          160     20
> > >    CAN-FD SFF no-bitstuffing            579     73
> > >    CAN-FD EFF no-bitstuffing            598     75
> > >    CAN-FD SFF bitstuffing               712     89
> > >    CAN-FD EFF bitstuffing               736     92
> > >
> > > The macro CAN_FRAME_LEN_MAX and CANFD_FRAME_LEN_MAX are
> kept as
> > > an
> > > alias to, respectively, can_frame_bytes(false, true, CAN_MAX_DLEN) and
> > > can_frame_bytes(true, true, CANFD_MAX_DLEN).
> > >
> > > In addition to the above:
> > >
> > >  - Use ISO 11898-1:2015 definitions for the name of the CAN frame
> > >    fields.
> > >  - Include linux/bits.h for use of BITS_PER_BYTE.
> > >  - Include linux/math.h for use of mult_frac() and
> > >    DIV_ROUND_UP(). N.B: the use of DIV_ROUND_UP() is not new to this
> > >    patch, but the include was previously omitted.
> > >  - Add copyright 2023 for myself.
> > >
> > > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > > ---
> > >  drivers/net/can/dev/length.c |  15 +-
> > >  include/linux/can/length.h   | 298 +++++++++++++++++++++++++--------
> --
> > >  2 files changed, 213 insertions(+), 100 deletions(-)
> > >
> > > diff --git a/drivers/net/can/dev/length.c b/drivers/net/can/dev/length.c
> > > index b48140b1102e..b7f4d76dd444 100644
> > > --- a/drivers/net/can/dev/length.c
> > > +++ b/drivers/net/can/dev/length.c
> > > @@ -78,18 +78,7 @@ unsigned int can_skb_get_frame_len(const struct
> > > sk_buff *skb)
> > >         else
> > >                 len = cf->len;
> > >
> > > -       if (can_is_canfd_skb(skb)) {
> > > -               if (cf->can_id & CAN_EFF_FLAG)
> > > -                       len += CANFD_FRAME_OVERHEAD_EFF;
> > > -               else
> > > -                       len += CANFD_FRAME_OVERHEAD_SFF;
> > > -       } else {
> > > -               if (cf->can_id & CAN_EFF_FLAG)
> > > -                       len += CAN_FRAME_OVERHEAD_EFF;
> > > -               else
> > > -                       len += CAN_FRAME_OVERHEAD_SFF;
> > > -       }
> > > -
> > > -       return len;
> > > +       return can_frame_bytes(can_is_canfd_skb(skb), cf->can_id &
> > > CAN_EFF_FLAG,
> > > +                              false, len);
> > >  }
> > >  EXPORT_SYMBOL_GPL(can_skb_get_frame_len);
> > > diff --git a/include/linux/can/length.h b/include/linux/can/length.h
> > > index 521fdbce2d69..ef6e78fa95b9 100644
> > > --- a/include/linux/can/length.h
> > > +++ b/include/linux/can/length.h
> > > @@ -1,132 +1,256 @@
> > >  /* SPDX-License-Identifier: GPL-2.0 */
> > >  /* Copyright (C) 2020 Oliver Hartkopp <socketcan@hartkopp.net>
> > >   * Copyright (C) 2020 Marc Kleine-Budde <kernel@pengutronix.de>
> > > - * Copyright (C) 2020 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > > + * Copyright (C) 2020, 2023 Vincent Mailhol
> <mailhol.vincent@wanadoo.fr>
> > >   */
> > >
> > >  #ifndef _CAN_LENGTH_H
> > >  #define _CAN_LENGTH_H
> > >
> > > +#include <linux/bits.h>
> > >  #include <linux/can.h>
> > >  #include <linux/can/netlink.h>
> > > +#include <linux/math.h>
> > >
> > >  /*
> > > - * Size of a Classical CAN Standard Frame
> > > + * Size of a Classical CAN Standard Frame header in bits
> > >   *
> > > - * Name of Field                       Bits
> > > + * Name of Field                               Bits
> > >   * ---------------------------------------------------------
> > > - * Start-of-frame                      1
> > > - * Identifier                          11
> > > - * Remote transmission request (RTR)   1
> > > - * Identifier extension bit (IDE)      1
> > > - * Reserved bit (r0)                   1
> > > - * Data length code (DLC)              4
> > > - * Data field                          0...64
> > > - * CRC                                 15
> > > - * CRC delimiter                       1
> > > - * ACK slot                            1
> > > - * ACK delimiter                       1
> > > - * End-of-frame (EOF)                  7
> > > - * Inter frame spacing                 3
> > > + * Start Of Frame (SOF)                                1
> > > + * Arbitration field:
> > > + *     base ID                                 11
> > > + *     Remote Transmission Request (RTR)       1
> > > + * Control field:
> > > + *     IDentifier Extension bit (IDE)          1
> > > + *     FD Format indicatior (FDF)              1
> > > + *     Data Length Code (DLC)                  4
> > > + *
> > > + * including all fields preceding the data field, ignoring bitstuffing
> > > + */
> > > +#define CAN_FRAME_HEADER_SFF_BITS 19
> > > +
> > > +/*
> > > + * Size of a Classical CAN Extended Frame header in bits
> > > + *
> > > + * Name of Field                               Bits
> > > + * ---------------------------------------------------------
> > > + * Start Of Frame (SOF)                                1
> > > + * Arbitration field:
> > > + *     base ID                                 11
> > > + *     Substitute Remote Request (SRR)         1
> > > + *     IDentifier Extension bit (IDE)          1
> > > + *     ID extension                            18
> > > + *     Remote Transmission Request (RTR)       1
> > > + * Control field:
> > > + *     FD Format indicatior (FDF)              1
> > Nit: indicator, same above
> 
> ACK.
> 
> > > + *     Reserved bit (r0)                       1
> > > + *     Data length code (DLC)                  4
> > > + *
> > > + * including all fields preceding the data field, ignoring bitstuffing
> > > + */
> > > +#define CAN_FRAME_HEADER_EFF_BITS 39
> > > +
> > > +/*
> > > + * Size of a CAN-FD Standard Frame in bits
> > > + *
> > > + * Name of Field                               Bits
> > > + * ---------------------------------------------------------
> > > + * Start Of Frame (SOF)                                1
> > > + * Arbitration field:
> > > + *     base ID                                 11
> > > + *     Remote Request Substitution (RRS)       1
> > > + * Control field:
> > > + *     IDentifier Extension bit (IDE)          1
> > > + *     FD Format indicator (FDF)               1
> > > + *     Reserved bit (res)                      1
> > > + *     Bit Rate Switch (BRS)                   1
> > > + *     Error Status Indicator (ESI)            1
> > > + *     Data length code (DLC)                  4
> > > + *
> > > + * including all fields preceding the data field, ignoring bitstuffing
> > > + */
> > > +#define CANFD_FRAME_HEADER_SFF_BITS 22
> > > +
> > > +/*
> > > + * Size of a CAN-FD Extended Frame in bits
> > > + *
> > > + * Name of Field                               Bits
> > > + * ---------------------------------------------------------
> > > + * Start Of Frame (SOF)                                1
> > > + * Arbitration field:
> > > + *     base ID                                 11
> > > + *     Substitute Remote Request (SRR)         1
> > > + *     IDentifier Extension bit (IDE)          1
> > > + *     ID extension                            18
> > > + *     Remote Request Substitution (RRS)       1
> > > + * Control field:
> > > + *     FD Format indicator (FDF)               1
> > > + *     Reserved bit (res)                      1
> > > + *     Bit Rate Switch (BRS)                   1
> > > + *     Error Status Indicator (ESI)            1
> > > + *     Data length code (DLC)                  4
> > >   *
> > > - * rounded up and ignoring bitstuffing
> > > + * including all fields preceding the data field, ignoring bitstuffing
> > >   */
> > > -#define CAN_FRAME_OVERHEAD_SFF DIV_ROUND_UP(47, 8)
> > > +#define CANFD_FRAME_HEADER_EFF_BITS 41
> > >
> > >  /*
> > > - * Size of a Classical CAN Extended Frame
> > > + * Size of a CAN CRC Field in bits
> > >   *
> > >   * Name of Field                       Bits
> > >   * ---------------------------------------------------------
> > > - * Start-of-frame                      1
> > > - * Identifier A                                11
> > > - * Substitute remote request (SRR)     1
> > > - * Identifier extension bit (IDE)      1
> > > - * Identifier B                                18
> > > - * Remote transmission request (RTR)   1
> > > - * Reserved bits (r1, r0)              2
> > > - * Data length code (DLC)              4
> > > - * Data field                          0...64
> > > - * CRC                                 15
> > > - * CRC delimiter                       1
> > > - * ACK slot                            1
> > > - * ACK delimiter                       1
> > > - * End-of-frame (EOF)                  7
> > > - * Inter frame spacing                 3
> > > + * CRC sequence (CRC15)                        15
> > > + * CRC Delimiter                       1
> > >   *
> > > - * rounded up and ignoring bitstuffing
> > > + * ignoring bitstuffing
> > >   */
> > > -#define CAN_FRAME_OVERHEAD_EFF DIV_ROUND_UP(67, 8)
> > > +#define CAN_FRAME_CRC_FIELD_BITS 16
> > >
> > >  /*
> > > - * Size of a CAN-FD Standard Frame
> > > + * Size of a CAN-FD CRC17 Field in bits (length: 0..16)
> > >   *
> > >   * Name of Field                       Bits
> > >   * ---------------------------------------------------------
> > > - * Start-of-frame                      1
> > > - * Identifier                          11
> > > - * Remote Request Substitution (RRS)   1
> > > - * Identifier extension bit (IDE)      1
> > > - * Flexible data rate format (FDF)     1
> > > - * Reserved bit (r0)                   1
> > > - * Bit Rate Switch (BRS)               1
> > > - * Error Status Indicator (ESI)                1
> > > - * Data length code (DLC)              4
> > > - * Data field                          0...512
> > > - * Stuff Bit Count (SBC)               4
> > > - * CRC                                 0...16: 17 20...64:21
> > > - * CRC delimiter (CD)                  1
> > > - * Fixed Stuff bits (FSB)              0...16: 6 20...64:7
> > > - * ACK slot (AS)                       1
> > > - * ACK delimiter (AD)                  1
> > > - * End-of-frame (EOF)                  7
> > > - * Inter frame spacing                 3
> > > - *
> > > - * assuming CRC21, rounded up and ignoring dynamic bitstuffing
> > > - */
> > > -#define CANFD_FRAME_OVERHEAD_SFF DIV_ROUND_UP(67, 8)
> > > + * Stuff Count                         4
> > > + * CRC Sequence (CRC17)                        17
> > > + * CRC Delimiter                       1
> > > + * Fixed stuff bits                    6
> > > + */
> > > +#define CANFD_FRAME_CRC17_FIELD_BITS 28
> > >
> > >  /*
> > > - * Size of a CAN-FD Extended Frame
> > > + * Size of a CAN-FD CRC21 Field in bits (length: 20..64)
> > >   *
> > >   * Name of Field                       Bits
> > >   * ---------------------------------------------------------
> > > - * Start-of-frame                      1
> > > - * Identifier A                                11
> > > - * Substitute remote request (SRR)     1
> > > - * Identifier extension bit (IDE)      1
> > > - * Identifier B                                18
> > > - * Remote Request Substitution (RRS)   1
> > > - * Flexible data rate format (FDF)     1
> > > - * Reserved bit (r0)                   1
> > > - * Bit Rate Switch (BRS)               1
> > > - * Error Status Indicator (ESI)                1
> > > - * Data length code (DLC)              4
> > > - * Data field                          0...512
> > > - * Stuff Bit Count (SBC)               4
> > > - * CRC                                 0...16: 17 20...64:21
> > > - * CRC delimiter (CD)                  1
> > > - * Fixed Stuff bits (FSB)              0...16: 6 20...64:7
> > > - * ACK slot (AS)                       1
> > > - * ACK delimiter (AD)                  1
> > > - * End-of-frame (EOF)                  7
> > > - * Inter frame spacing                 3
> > > - *
> > > - * assuming CRC21, rounded up and ignoring dynamic bitstuffing
> > > - */
> > > -#define CANFD_FRAME_OVERHEAD_EFF DIV_ROUND_UP(86, 8)
> > > + * Stuff Count                         4
> > > + * CRC sequence (CRC21)                        21
> > > + * CRC Delimiter                       1
> > > + * Fixed stuff bits                    7
> > > + */
> > > +#define CANFD_FRAME_CRC21_FIELD_BITS 33
> > > +
> > > +/*
> > > + * Size of a CAN(-FD) Frame footer in bits
> > > + *
> > > + * Name of Field                       Bits
> > > + * ---------------------------------------------------------
> > > + * ACK slot                            1
> > > + * ACK delimiter                       1
> > > + * End Of Frame (EOF)                  7
> > > + *
> > > + * including all fields following the CRC field
> > > + */
> > > +#define CAN_FRAME_FOOTER_BITS 9
> > > +
> > > +/*
> > > + * First part of the Inter Frame Space
> > > + * (a.k.a. IMF - intermission field)
> > > + */
> > > +#define CAN_INTERMISSION_BITS 3
> > > +
> > > +/**
> > > + * can_bitstuffing_len() - Calculate the maximum length with bitsuffing
> > Nit: bitstuffing, same further down
> 
> ACK.
> 
> > > + * @bitstream_len: length of a destuffed bit stream
> > > + *
> > > + * The worst bit stuffing case is a sequence in which dominant and
> > > + * recessive bits alternate every four bits:
> > > + *
> > > + *   Destuffed: 1 1111  0000  1111  0000  1111
> > > + *   Stuffed:   1 1111o 0000i 1111o 0000i 1111o
> > > + *
> > > + * Nomenclature
> > > + *
> > > + *  - "0": dominant bit
> > > + *  - "o": dominant stuff bit
> > > + *  - "1": recessive bit
> > > + *  - "i": recessive stuff bit
> > > + *
> > > + * Aside of the first bit, one stuff bit is added every four bits.
> > > + *
> > > + * Return: length of the stuffed bit stream in the worst case scenario.
> > > + */
> > > +#define can_bitstuffing_len(destuffed_len)                     \
> > > +       (destuffed_len + (destuffed_len - 1) / 4)
> > > +
> > > +#define __can_bitstuffing_len(bitstuffing, destuffed_len)      \
> > > +       (bitstuffing ? can_bitstuffing_len(destuffed_len) :     \
> > > +                      destuffed_len)
> > > +
> > > +#define __can_cc_frame_bits(is_eff, bitstuffing,               \
> > > +                           intermission, data_len)             \
> > > +(                                                              \
> > > +       __can_bitstuffing_len(bitstuffing,                      \
> > > +               (is_eff ? CAN_FRAME_HEADER_EFF_BITS :           \
> > > +                          CAN_FRAME_HEADER_SFF_BITS) +         \
> > > +               data_len * BITS_PER_BYTE +                      \
> > > +               CAN_FRAME_CRC_FIELD_BITS) +                     \
> > > +       CAN_FRAME_FOOTER_BITS +                                 \
> > > +       (intermission ? CAN_INTERMISSION_BITS : 0)              \
> > > +)
> > I think Footer and Intermission need to be pulled out of the parameter for
> __can_bitstuffing_length as these fields are never stuffed.
> 
> Look again at the opening and closing bracket of
> __can_bitstuffing_len(). These are already out :)
> I indented the parameters of __can_bitstuffing_length() to highlight
> what is in and out.
> 
> Maybe adding some newlines would help readability? Something like that:
> 
>   #define __can_cc_frame_bits(is_eff, bitstuffing,                \
>                               intermission, data_len)             \
>   (                                                               \
>           __can_bitstuffing_len(                                  \
>                   bitstuffing,                                    \
>                   (is_eff ? CAN_FRAME_HEADER_EFF_BITS :           \
>                              CAN_FRAME_HEADER_SFF_BITS) +         \
>                   data_len * BITS_PER_BYTE +                      \
>                   CAN_FRAME_CRC_FIELD_BITS)                       \
>           +                                                       \
>           CAN_FRAME_FOOTER_BITS +                                 \
>           (intermission ? CAN_INTERMISSION_BITS : 0)              \
>   )
> 
> > > +
> > > +#define __can_fd_frame_bits(is_eff, bitstuffing,               \
> > > +                           intermission, data_len)             \
> > > +(                                                              \
> > > +       __can_bitstuffing_len(bitstuffing,                      \
> > > +               (is_eff ? CANFD_FRAME_HEADER_EFF_BITS :         \
> > > +                          CANFD_FRAME_HEADER_SFF_BITS) +       \
> > > +               data_len * BITS_PER_BYTE) +                     \
> > > +       (data_len <= 16 ?                                       \
> > > +               CANFD_FRAME_CRC17_FIELD_BITS :                  \
> > > +               CANFD_FRAME_CRC21_FIELD_BITS) +                 \
> > > +       CAN_FRAME_FOOTER_BITS +                                 \
> > > +       (intermission ? CAN_INTERMISSION_BITS : 0)              \
> > > +)
> > I think Footer and Intermission need to be pulled out of the parameter for
> __can_bitstuffing_length as these fields are never stuffed.
> > The CAN_FRAME_CRC_FIELD_BITS bits need to be pulled out of the
> can_bitstuffing_len. That portion of the Frame is not dynamically stuffed in FD
> frames.
> 
> Same as above, these are already out.
Whoops - my bad, re-reading these portions both old and new look ok to me. No need for extra newlines from my point of view, just better attention at the reader side to the indentation which is already there - that greatly helps 😉

> 
> > > +
> > > +/**
> > > + * can_frame_bits() - Calculate the number of bits in on the wire in a
> > Nit: "in on the wire" -in
> > > + *     CAN frame
> > > + * @is_fd: true: CAN-FD frame; false: Classical CAN frame.
> > > + * @is_eff: true: Extended frame; false: Standard frame.
> > > + * @bitstuffing: true: calculate the bitsuffing worst case; false:
> > > + *     calculate the bitsuffing best case (no dynamic
> > > + *     bitsuffing). Fixed stuff bits are always included.
> > > + * @intermission: if and only if true, include the inter frame space
> > > + *     assuming no bus idle (i.e. only the intermission gets added).
> > > + * @data_len: length of the data field in bytes. Correspond to
> > > + *     can(fd)_frame->len. Should be zero for remote frames. No
> > > + *     sanitization is done on @data_len.
> > > + *
> > > + * Return: the numbers of bits on the wire of a CAN frame.
> > > + */
> > > +#define can_frame_bits(is_fd, is_eff, bitstuffing,             \
> > > +                      intermission, data_len)                  \
> > > +(                                                              \
> > > +       is_fd ? __can_fd_frame_bits(is_eff, bitstuffing,        \
> > > +                                   intermission, data_len) :   \
> > > +               __can_cc_frame_bits(is_eff, bitstuffing,        \
> > > +                                   intermission, data_len)     \
> > > +)
> > > +
> > > +/*
> > > + * Number of bytes in a CAN frame
> > > + * (rounded up, including intermission)
> > > + */
> > > +#define can_frame_bytes(is_fd, is_eff, bitstuffing, data_len)  \
> > > +       DIV_ROUND_UP(can_frame_bits(is_fd, is_eff, bitstuffing, \
> > > +                                   true, data_len),            \
> > > +                    BITS_PER_BYTE)
> > >
> > >  /*
> > >   * Maximum size of a Classical CAN frame
> > > - * (rounded up and ignoring bitstuffing)
> > > + * (rounded up, ignoring bitstuffing but including intermission)
> > >   */
> > > -#define CAN_FRAME_LEN_MAX (CAN_FRAME_OVERHEAD_EFF +
> > > CAN_MAX_DLEN)
> > > +#define CAN_FRAME_LEN_MAX \
> > > +       can_frame_bytes(false, true, false, CAN_MAX_DLEN)
> > >
> > >  /*
> > >   * Maximum size of a CAN-FD frame
> > >   * (rounded up and ignoring bitstuffing)
> > Ignoring dynamic bitstuffing
> > >   */
> > > -#define CANFD_FRAME_LEN_MAX (CANFD_FRAME_OVERHEAD_EFF +
> > > CANFD_MAX_DLEN)
> > > +#define CANFD_FRAME_LEN_MAX \
> > > +       can_frame_bytes(true, true, false, CANFD_MAX_DLEN)
> > >
> > >  /*
> > >   * can_cc_dlc2len(value) - convert a given data length code (dlc) of a
> > > --
> > > 2.39.3
> >
> > I think your attribution of suggested-by for myself is mixed up for the
> patches 2/3 and 3/3 😊
> 
> ACK. I will remove it from 2/3 and add it to 3/3.
> 
> > For the entire series you can add my reviewed-by.
> 
> I will do so.
> Thanks for picking my typos!

Best Regards,
Thomas

[PATCH v4 0/3] can: length: fix definitions and add bit length calculation

[Vincent Mailhol]

When created in [1], frames length definitions were added to implement
byte queue limits (bql). Because bql expects lengths in bytes, bit
length definitions were not considered back then.
    
Recently, a need to refer to the exact frame length in bits, with CAN
bit stuffing, appeared in [2].

This series introduces can_frame_bits(): a function-like macro that
can calculate the exact size of a CAN(-FD) frame in bits with or
without bitsuffing.

[1] commit 85d99c3e2a13 ("can: length: can_skb_get_frame_len(): introduce
    function to get data length of frame in data link layer")
Link: https://git.kernel.org/torvalds/c/85d99c3e2a13

[2] RE: [PATCH] can: mcp251xfd: Increase poll timeout
Link: https://lore.kernel.org/linux-can/BL3PR11MB64846C83ACD04E9330B0FE66FB729@BL3PR11MB6484.namprd11.prod.outlook.com/


* Changelog *

v3 -> v4:

  * No functional changes.

  * as reported by Simon Horman, fix typo in the documentation of
    can_bitstuffing_len(): "bitstream_len" -> "destuffed_len".

  * as reported by Thomas Kopp, fix several other typos:
      - "indicatior" -> "indicator"
      - "in on the wire" -> "on the wire"
      - "bitsuffing" -> "bitstuffing".

  * in CAN_FRAME_LEN_MAX comment: specify that only the dynamic
    bitstuffing gets ignored but that the intermission is included.

  * move the Suggested-by: Thomas Kopp tag from patch 2 to patch 3.

  * add Reviewed-by: Thomas Kopp tag on the full series.

  * add an additional line of comment for the @intermission argument
    of can_frame_bits().

Link: https://lore.kernel.org/linux-can/20230530144637.4746-1-mailhol.vincent@wanadoo.fr/

v2 -> v3:

  * turn can_frame_bits() and can_frame_bytes() into function-like
    macros. The fact that inline functions can not be used to
    initialize constant struct fields was bothering me. I did my best
    to make the macro look as less ugly as possible.

  * as reported by Simon Horman, add missing document for the is_fd
    argument of can_frame_bits().

Link: https://lore.kernel.org/linux-can/20230523065218.51227-1-mailhol.vincent@wanadoo.fr/

v1 -> v2:

  * as suggested by Thomas Kopp, add a new patch to the series to fix
    the stuff bit count and the fixed stuff bits definitions

  * and another patch to fix documentation of the Remote Request
    Substitution (RRS).

  * refactor the length definition. Instead of using individual macro,
    rely on an inline function. One reason is to minimize the number
    of definitions. Another reason is that because the dynamic bit
    stuff is calculated differently for CAN and CAN-FD, it is just not
    possible to multiply the existing CANFD_FRAME_OVERHEAD_SFF/EFF by
    the overhead ratio to get the bitsuffing: for CAN-FD, the CRC
    field is already stuffed by the fixed stuff bits and is out of
    scope of the dynamic bitstuffing.

Link: https://lore.kernel.org/linux-can/20230507155506.3179711-1-mailhol.vincent@wanadoo.fr/

Vincent Mailhol (3):
  can: length: fix bitstuffing count
  can: length: fix description of the RRS field
  can: length: refactor frame lengths definition to add size in bits

 drivers/net/can/dev/length.c |  15 +-
 include/linux/can/length.h   | 300 +++++++++++++++++++++++++----------
 2 files changed, 217 insertions(+), 98 deletions(-)

-- 
2.39.3

[PATCH v4 1/3] can: length: fix bitstuffing count

[Vincent Mailhol]

The Stuff Bit Count is always coded on 4 bits (ref [1]). Update the
Stuff Bit Count size accordingly.

In addition, the CRC fields of CAN FD Frames contain stuff bits at
fixed positions called fixed stuff bits [2]. The CRC field starts with
a fixed stuff bit and then has another fixed stuff bit after each
fourth bit [2], which allow us to derive this formula:

  FSB count = 1 + round_down(len(CRC field)/4)

The length of the CRC field is [1]:

  len(CRC field) = len(Stuff Bit Count) + len(CRC)
                 = 4 + len(CRC)

with len(CRC) either 17 or 21 bits depending of the payload length.

In conclusion, for CRC17:

  FSB count = 1 + round_down((4 + 17)/4)
            = 6

and for CRC 21:

  FSB count = 1 + round_down((4 + 21)/4)
            = 7

Add a Fixed Stuff bits (FSB) field with above values and update
CANFD_FRAME_OVERHEAD_SFF and CANFD_FRAME_OVERHEAD_EFF accordingly.

[1] ISO 11898-1:2015 section 10.4.2.6 "CRC field":

  The CRC field shall contain the CRC sequence followed by a recessive
  CRC delimiter. For FD Frames, the CRC field shall also contain the
  stuff count.

  Stuff count

  If FD Frames, the stuff count shall be at the beginning of the CRC
  field. It shall consist of the stuff bit count modulo 8 in a 3-bit
  gray code followed by a parity bit [...]

[2] ISO 11898-1:2015 paragraph 10.5 "Frame coding":

  In the CRC field of FD Frames, the stuff bits shall be inserted at
  fixed positions; they are called fixed stuff bits. There shall be a
  fixed stuff bit before the first bit of the stuff count, even if the
  last bits of the preceding field are a sequence of five consecutive
  bits of identical value, there shall be only the fixed stuff bit,
  there shall not be two consecutive stuff bits. A further fixed stuff
  bit shall be inserted after each fourth bit of the CRC field [...]

Fixes: 85d99c3e2a13 ("can: length: can_skb_get_frame_len(): introduce function to get data length of frame in data link layer")
Suggested-by: Thomas Kopp <Thomas.Kopp@microchip.com>
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Reviewed-by: Thomas Kopp <Thomas.Kopp@microchip.com>
---
 include/linux/can/length.h | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/include/linux/can/length.h b/include/linux/can/length.h
index 69336549d24f..b8c12c83bc51 100644
--- a/include/linux/can/length.h
+++ b/include/linux/can/length.h
@@ -72,17 +72,18 @@
  * Error Status Indicator (ESI)		1
  * Data length code (DLC)		4
  * Data field				0...512
- * Stuff Bit Count (SBC)		0...16: 4 20...64:5
+ * Stuff Bit Count (SBC)		4
  * CRC					0...16: 17 20...64:21
  * CRC delimiter (CD)			1
+ * Fixed Stuff bits (FSB)		0...16: 6 20...64:7
  * ACK slot (AS)			1
  * ACK delimiter (AD)			1
  * End-of-frame (EOF)			7
  * Inter frame spacing			3
  *
- * assuming CRC21, rounded up and ignoring bitstuffing
+ * assuming CRC21, rounded up and ignoring dynamic bitstuffing
  */
-#define CANFD_FRAME_OVERHEAD_SFF DIV_ROUND_UP(61, 8)
+#define CANFD_FRAME_OVERHEAD_SFF DIV_ROUND_UP(67, 8)
 
 /*
  * Size of a CAN-FD Extended Frame
@@ -101,17 +102,18 @@
  * Error Status Indicator (ESI)		1
  * Data length code (DLC)		4
  * Data field				0...512
- * Stuff Bit Count (SBC)		0...16: 4 20...64:5
+ * Stuff Bit Count (SBC)		4
  * CRC					0...16: 17 20...64:21
  * CRC delimiter (CD)			1
+ * Fixed Stuff bits (FSB)		0...16: 6 20...64:7
  * ACK slot (AS)			1
  * ACK delimiter (AD)			1
  * End-of-frame (EOF)			7
  * Inter frame spacing			3
  *
- * assuming CRC21, rounded up and ignoring bitstuffing
+ * assuming CRC21, rounded up and ignoring dynamic bitstuffing
  */
-#define CANFD_FRAME_OVERHEAD_EFF DIV_ROUND_UP(80, 8)
+#define CANFD_FRAME_OVERHEAD_EFF DIV_ROUND_UP(86, 8)
 
 /*
  * Maximum size of a Classical CAN frame
-- 
2.39.3

[PATCH v4 2/3] can: length: fix description of the RRS field

[Vincent Mailhol]

The CAN-FD frames only have one reserved bit. The bit corresponding to
Classical CAN frame's RTR bit is called the "Remote Request
Substitution (RRS)" [1].

N.B. The RRS is not to be confused with the Substitute remote request
(SRR).

Fix the description in the CANFD_FRAME_OVERHEAD_SFF/EFF.

The total remains unchanged, so this is just a documentation fix.

In addition to the above add myself as copyright owner for 2020 (as
coauthor of the initial version, c.f. Fixes tag).

[1] ISO 11898-1:2015 paragraph 10.4.2.3 "Arbitration field":

  RSS bit [only in FD Frames]

    The RRS bit shall be transmitted in FD Frames at the position of
    the RTR bit in Classical Frames. The RRS bit shall be transmitted
    dominant, but receivers shall accept recessive and dominant RRS
    bits.

Fixes: 85d99c3e2a13 ("can: length: can_skb_get_frame_len(): introduce function to get data length of frame in data link layer")
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Reviewed-by: Thomas Kopp <Thomas.Kopp@microchip.com>
---
 include/linux/can/length.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/linux/can/length.h b/include/linux/can/length.h
index b8c12c83bc51..521fdbce2d69 100644
--- a/include/linux/can/length.h
+++ b/include/linux/can/length.h
@@ -1,6 +1,7 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 /* Copyright (C) 2020 Oliver Hartkopp <socketcan@hartkopp.net>
  * Copyright (C) 2020 Marc Kleine-Budde <kernel@pengutronix.de>
+ * Copyright (C) 2020 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
  */
 
 #ifndef _CAN_LENGTH_H
@@ -64,7 +65,7 @@
  * ---------------------------------------------------------
  * Start-of-frame			1
  * Identifier				11
- * Reserved bit (r1)			1
+ * Remote Request Substitution (RRS)	1
  * Identifier extension bit (IDE)	1
  * Flexible data rate format (FDF)	1
  * Reserved bit (r0)			1
@@ -95,7 +96,7 @@
  * Substitute remote request (SRR)	1
  * Identifier extension bit (IDE)	1
  * Identifier B				18
- * Reserved bit (r1)			1
+ * Remote Request Substitution (RRS)	1
  * Flexible data rate format (FDF)	1
  * Reserved bit (r0)			1
  * Bit Rate Switch (BRS)		1
-- 
2.39.3

[PATCH v4 3/3] can: length: refactor frame lengths definition to add size in bits

[Vincent Mailhol]

Introduce a method to calculate the exact size in bits of a CAN(-FD)
frame with or without dynamic bitstuffing.

These are all the possible combinations taken into account:

  - Classical CAN or CAN-FD
  - Standard or Extended frame format
  - CAN-FD CRC17 or CRC21
  - Include or not intermission

Instead of doing several individual macro definitions, declare the
can_frame_bits() function-like macro. To this extent, do a full
refactoring of the length definitions.

In addition add the can_frame_bytes(). This function-like macro
replaces the existing macro:

  - CAN_FRAME_OVERHEAD_SFF: can_frame_bytes(false, false, 0)
  - CAN_FRAME_OVERHEAD_EFF: can_frame_bytes(false, true, 0)
  - CANFD_FRAME_OVERHEAD_SFF: can_frame_bytes(true, false, 0)
  - CANFD_FRAME_OVERHEAD_EFF: can_frame_bytes(true, true, 0)

Function-like macros were chosen over inline functions because they
can be used to initialize in const struct fields.

The different maximum frame lengths (maximum data length, including
intermission) are as follow:

   Frame type				bits	bytes
  -------------------------------------------------------
   Classic CAN SFF no bitstuffing	111	14
   Classic CAN EFF no bitstuffing	131	17
   Classic CAN SFF bitstuffing		135	17
   Classic CAN EFF bitstuffing		160	20
   CAN-FD SFF no bitstuffing		579	73
   CAN-FD EFF no bitstuffing		598	75
   CAN-FD SFF bitstuffing		712	89
   CAN-FD EFF bitstuffing		736	92

The macro CAN_FRAME_LEN_MAX and CANFD_FRAME_LEN_MAX are kept as an
alias to, respectively, can_frame_bytes(false, true, CAN_MAX_DLEN) and
can_frame_bytes(true, true, CANFD_MAX_DLEN).

In addition to the above:

 - Use ISO 11898-1:2015 definitions for the names of the CAN frame
   fields.
 - Include linux/bits.h for use of BITS_PER_BYTE.
 - Include linux/math.h for use of mult_frac() and
   DIV_ROUND_UP(). N.B: the use of DIV_ROUND_UP() is not new to this
   patch, but the include was previously omitted.
 - Add copyright 2023 for myself.

Suggested-by: Thomas Kopp <Thomas.Kopp@microchip.com>
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Reviewed-by: Thomas Kopp <Thomas.Kopp@microchip.com>
---
 drivers/net/can/dev/length.c |  15 +-
 include/linux/can/length.h   | 303 +++++++++++++++++++++++++----------
 2 files changed, 217 insertions(+), 101 deletions(-)

diff --git a/drivers/net/can/dev/length.c b/drivers/net/can/dev/length.c
index b48140b1102e..b7f4d76dd444 100644
--- a/drivers/net/can/dev/length.c
+++ b/drivers/net/can/dev/length.c
@@ -78,18 +78,7 @@ unsigned int can_skb_get_frame_len(const struct sk_buff *skb)
 	else
 		len = cf->len;
 
-	if (can_is_canfd_skb(skb)) {
-		if (cf->can_id & CAN_EFF_FLAG)
-			len += CANFD_FRAME_OVERHEAD_EFF;
-		else
-			len += CANFD_FRAME_OVERHEAD_SFF;
-	} else {
-		if (cf->can_id & CAN_EFF_FLAG)
-			len += CAN_FRAME_OVERHEAD_EFF;
-		else
-			len += CAN_FRAME_OVERHEAD_SFF;
-	}
-
-	return len;
+	return can_frame_bytes(can_is_canfd_skb(skb), cf->can_id & CAN_EFF_FLAG,
+			       false, len);
 }
 EXPORT_SYMBOL_GPL(can_skb_get_frame_len);
diff --git a/include/linux/can/length.h b/include/linux/can/length.h
index 521fdbce2d69..637b9322db82 100644
--- a/include/linux/can/length.h
+++ b/include/linux/can/length.h
@@ -1,132 +1,259 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 /* Copyright (C) 2020 Oliver Hartkopp <socketcan@hartkopp.net>
  * Copyright (C) 2020 Marc Kleine-Budde <kernel@pengutronix.de>
- * Copyright (C) 2020 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
+ * Copyright (C) 2020, 2023 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
  */
 
 #ifndef _CAN_LENGTH_H
 #define _CAN_LENGTH_H
 
+#include <linux/bits.h>
 #include <linux/can.h>
 #include <linux/can/netlink.h>
+#include <linux/math.h>
 
 /*
- * Size of a Classical CAN Standard Frame
+ * Size of a Classical CAN Standard Frame header in bits
  *
- * Name of Field			Bits
+ * Name of Field				Bits
  * ---------------------------------------------------------
- * Start-of-frame			1
- * Identifier				11
- * Remote transmission request (RTR)	1
- * Identifier extension bit (IDE)	1
- * Reserved bit (r0)			1
- * Data length code (DLC)		4
- * Data field				0...64
- * CRC					15
- * CRC delimiter			1
- * ACK slot				1
- * ACK delimiter			1
- * End-of-frame (EOF)			7
- * Inter frame spacing			3
+ * Start Of Frame (SOF)				1
+ * Arbitration field:
+ *	base ID					11
+ *	Remote Transmission Request (RTR)	1
+ * Control field:
+ *	IDentifier Extension bit (IDE)		1
+ *	FD Format indicator (FDF)		1
+ *	Data Length Code (DLC)			4
+ *
+ * including all fields preceding the data field, ignoring bitstuffing
+ */
+#define CAN_FRAME_HEADER_SFF_BITS 19
+
+/*
+ * Size of a Classical CAN Extended Frame header in bits
  *
- * rounded up and ignoring bitstuffing
+ * Name of Field				Bits
+ * ---------------------------------------------------------
+ * Start Of Frame (SOF)				1
+ * Arbitration field:
+ *	base ID					11
+ *	Substitute Remote Request (SRR)		1
+ *	IDentifier Extension bit (IDE)		1
+ *	ID extension				18
+ *	Remote Transmission Request (RTR)	1
+ * Control field:
+ *	FD Format indicator (FDF)		1
+ *	Reserved bit (r0)			1
+ *	Data length code (DLC)			4
+ *
+ * including all fields preceding the data field, ignoring bitstuffing
  */
-#define CAN_FRAME_OVERHEAD_SFF DIV_ROUND_UP(47, 8)
+#define CAN_FRAME_HEADER_EFF_BITS 39
 
 /*
- * Size of a Classical CAN Extended Frame
+ * Size of a CAN-FD Standard Frame in bits
+ *
+ * Name of Field				Bits
+ * ---------------------------------------------------------
+ * Start Of Frame (SOF)				1
+ * Arbitration field:
+ *	base ID					11
+ *	Remote Request Substitution (RRS)	1
+ * Control field:
+ *	IDentifier Extension bit (IDE)		1
+ *	FD Format indicator (FDF)		1
+ *	Reserved bit (res)			1
+ *	Bit Rate Switch (BRS)			1
+ *	Error Status Indicator (ESI)		1
+ *	Data length code (DLC)			4
+ *
+ * including all fields preceding the data field, ignoring bitstuffing
+ */
+#define CANFD_FRAME_HEADER_SFF_BITS 22
+
+/*
+ * Size of a CAN-FD Extended Frame in bits
+ *
+ * Name of Field				Bits
+ * ---------------------------------------------------------
+ * Start Of Frame (SOF)				1
+ * Arbitration field:
+ *	base ID					11
+ *	Substitute Remote Request (SRR)		1
+ *	IDentifier Extension bit (IDE)		1
+ *	ID extension				18
+ *	Remote Request Substitution (RRS)	1
+ * Control field:
+ *	FD Format indicator (FDF)		1
+ *	Reserved bit (res)			1
+ *	Bit Rate Switch (BRS)			1
+ *	Error Status Indicator (ESI)		1
+ *	Data length code (DLC)			4
+ *
+ * including all fields preceding the data field, ignoring bitstuffing
+ */
+#define CANFD_FRAME_HEADER_EFF_BITS 41
+
+/*
+ * Size of a CAN CRC Field in bits
  *
  * Name of Field			Bits
  * ---------------------------------------------------------
- * Start-of-frame			1
- * Identifier A				11
- * Substitute remote request (SRR)	1
- * Identifier extension bit (IDE)	1
- * Identifier B				18
- * Remote transmission request (RTR)	1
- * Reserved bits (r1, r0)		2
- * Data length code (DLC)		4
- * Data field				0...64
- * CRC					15
- * CRC delimiter			1
- * ACK slot				1
- * ACK delimiter			1
- * End-of-frame (EOF)			7
- * Inter frame spacing			3
+ * CRC sequence (CRC15)			15
+ * CRC Delimiter			1
+ *
+ * ignoring bitstuffing
+ */
+#define CAN_FRAME_CRC_FIELD_BITS 16
+
+/*
+ * Size of a CAN-FD CRC17 Field in bits (length: 0..16)
  *
- * rounded up and ignoring bitstuffing
+ * Name of Field			Bits
+ * ---------------------------------------------------------
+ * Stuff Count				4
+ * CRC Sequence (CRC17)			17
+ * CRC Delimiter			1
+ * Fixed stuff bits			6
  */
-#define CAN_FRAME_OVERHEAD_EFF DIV_ROUND_UP(67, 8)
+#define CANFD_FRAME_CRC17_FIELD_BITS 28
 
 /*
- * Size of a CAN-FD Standard Frame
+ * Size of a CAN-FD CRC21 Field in bits (length: 20..64)
  *
  * Name of Field			Bits
  * ---------------------------------------------------------
- * Start-of-frame			1
- * Identifier				11
- * Remote Request Substitution (RRS)	1
- * Identifier extension bit (IDE)	1
- * Flexible data rate format (FDF)	1
- * Reserved bit (r0)			1
- * Bit Rate Switch (BRS)		1
- * Error Status Indicator (ESI)		1
- * Data length code (DLC)		4
- * Data field				0...512
- * Stuff Bit Count (SBC)		4
- * CRC					0...16: 17 20...64:21
- * CRC delimiter (CD)			1
- * Fixed Stuff bits (FSB)		0...16: 6 20...64:7
- * ACK slot (AS)			1
- * ACK delimiter (AD)			1
- * End-of-frame (EOF)			7
- * Inter frame spacing			3
- *
- * assuming CRC21, rounded up and ignoring dynamic bitstuffing
- */
-#define CANFD_FRAME_OVERHEAD_SFF DIV_ROUND_UP(67, 8)
+ * Stuff Count				4
+ * CRC sequence (CRC21)			21
+ * CRC Delimiter			1
+ * Fixed stuff bits			7
+ */
+#define CANFD_FRAME_CRC21_FIELD_BITS 33
 
 /*
- * Size of a CAN-FD Extended Frame
+ * Size of a CAN(-FD) Frame footer in bits
  *
  * Name of Field			Bits
  * ---------------------------------------------------------
- * Start-of-frame			1
- * Identifier A				11
- * Substitute remote request (SRR)	1
- * Identifier extension bit (IDE)	1
- * Identifier B				18
- * Remote Request Substitution (RRS)	1
- * Flexible data rate format (FDF)	1
- * Reserved bit (r0)			1
- * Bit Rate Switch (BRS)		1
- * Error Status Indicator (ESI)		1
- * Data length code (DLC)		4
- * Data field				0...512
- * Stuff Bit Count (SBC)		4
- * CRC					0...16: 17 20...64:21
- * CRC delimiter (CD)			1
- * Fixed Stuff bits (FSB)		0...16: 6 20...64:7
- * ACK slot (AS)			1
- * ACK delimiter (AD)			1
- * End-of-frame (EOF)			7
- * Inter frame spacing			3
- *
- * assuming CRC21, rounded up and ignoring dynamic bitstuffing
- */
-#define CANFD_FRAME_OVERHEAD_EFF DIV_ROUND_UP(86, 8)
+ * ACK slot				1
+ * ACK delimiter			1
+ * End Of Frame (EOF)			7
+ *
+ * including all fields following the CRC field
+ */
+#define CAN_FRAME_FOOTER_BITS 9
+
+/*
+ * First part of the Inter Frame Space
+ * (a.k.a. IMF - intermission field)
+ */
+#define CAN_INTERMISSION_BITS 3
+
+/**
+ * can_bitstuffing_len() - Calculate the maximum length with bitstuffing
+ * @destuffed_len: length of a destuffed bit stream
+ *
+ * The worst bit stuffing case is a sequence in which dominant and
+ * recessive bits alternate every four bits:
+ *
+ *   Destuffed: 1 1111  0000  1111  0000  1111
+ *   Stuffed:   1 1111o 0000i 1111o 0000i 1111o
+ *
+ * Nomenclature
+ *
+ *  - "0": dominant bit
+ *  - "o": dominant stuff bit
+ *  - "1": recessive bit
+ *  - "i": recessive stuff bit
+ *
+ * Aside from the first bit, one stuff bit is added every four bits.
+ *
+ * Return: length of the stuffed bit stream in the worst case scenario.
+ */
+#define can_bitstuffing_len(destuffed_len)			\
+	(destuffed_len + (destuffed_len - 1) / 4)
+
+#define __can_bitstuffing_len(bitstuffing, destuffed_len)	\
+	(bitstuffing ? can_bitstuffing_len(destuffed_len) :	\
+		       destuffed_len)
+
+#define __can_cc_frame_bits(is_eff, bitstuffing,		\
+			    intermission, data_len)		\
+(								\
+	__can_bitstuffing_len(bitstuffing,			\
+		(is_eff ? CAN_FRAME_HEADER_EFF_BITS :		\
+			  CAN_FRAME_HEADER_SFF_BITS) +		\
+		data_len * BITS_PER_BYTE +			\
+		CAN_FRAME_CRC_FIELD_BITS) +			\
+	CAN_FRAME_FOOTER_BITS +					\
+	(intermission ? CAN_INTERMISSION_BITS : 0)		\
+)
+
+#define __can_fd_frame_bits(is_eff, bitstuffing,		\
+			    intermission, data_len)		\
+(								\
+	__can_bitstuffing_len(bitstuffing,			\
+		(is_eff ? CANFD_FRAME_HEADER_EFF_BITS :		\
+			  CANFD_FRAME_HEADER_SFF_BITS) +	\
+		data_len * BITS_PER_BYTE) +			\
+	(data_len <= 16 ?					\
+		CANFD_FRAME_CRC17_FIELD_BITS :			\
+		CANFD_FRAME_CRC21_FIELD_BITS) +			\
+	CAN_FRAME_FOOTER_BITS +					\
+	(intermission ? CAN_INTERMISSION_BITS : 0)		\
+)
+
+/**
+ * can_frame_bits() - Calculate the number of bits on the wire in a
+ *	CAN frame
+ * @is_fd: true: CAN-FD frame; false: Classical CAN frame.
+ * @is_eff: true: Extended frame; false: Standard frame.
+ * @bitstuffing: true: calculate the bitstuffing worst case; false:
+ *	calculate the bitstuffing best case (no dynamic
+ *	bitstuffing). CAN-FD's fixed stuff bits are always included.
+ * @intermission: if and only if true, include the inter frame space
+ *	assuming no bus idle (i.e. only the intermission). Strictly
+ *	speaking, the inter frame space is not part of the
+ *	frame. However, it is needed when calculating the delay
+ *	between the Start Of Frame of two consecutive frames.
+ * @data_len: length of the data field in bytes. Correspond to
+ *	can(fd)_frame->len. Should be zero for remote frames. No
+ *	sanitization is done on @data_len.
+ *
+ * Return: the numbers of bits on the wire of a CAN frame.
+ */
+#define can_frame_bits(is_fd, is_eff, bitstuffing,		\
+		       intermission, data_len)			\
+(								\
+	is_fd ? __can_fd_frame_bits(is_eff, bitstuffing,	\
+				    intermission, data_len) :	\
+		__can_cc_frame_bits(is_eff, bitstuffing,	\
+				    intermission, data_len)	\
+)
+
+/*
+ * Number of bytes in a CAN frame
+ * (rounded up, including intermission)
+ */
+#define can_frame_bytes(is_fd, is_eff, bitstuffing, data_len)	\
+	DIV_ROUND_UP(can_frame_bits(is_fd, is_eff, bitstuffing,	\
+				    true, data_len),		\
+		     BITS_PER_BYTE)
 
 /*
  * Maximum size of a Classical CAN frame
- * (rounded up and ignoring bitstuffing)
+ * (rounded up, ignoring bitstuffing but including intermission)
  */
-#define CAN_FRAME_LEN_MAX (CAN_FRAME_OVERHEAD_EFF + CAN_MAX_DLEN)
+#define CAN_FRAME_LEN_MAX \
+	can_frame_bytes(false, true, false, CAN_MAX_DLEN)
 
 /*
  * Maximum size of a CAN-FD frame
- * (rounded up and ignoring bitstuffing)
+ * (rounded up, ignoring dynamic bitstuffing but including intermission)
  */
-#define CANFD_FRAME_LEN_MAX (CANFD_FRAME_OVERHEAD_EFF + CANFD_MAX_DLEN)
+#define CANFD_FRAME_LEN_MAX \
+	can_frame_bytes(true, true, false, CANFD_MAX_DLEN)
 
 /*
  * can_cc_dlc2len(value) - convert a given data length code (dlc) of a
-- 
2.39.3

[PATCH v5 0/3] can: length: fix definitions and add bit length calculation

[Vincent Mailhol]

When created in [1], frames length definitions were added to implement
byte queue limits (bql). Because bql expects lengths in bytes, bit
length definitions were not considered back then.
    
Recently, a need to refer to the exact frame length in bits, with CAN
bit stuffing, appeared in [2].

This series introduces can_frame_bits(): a function-like macro that
can calculate the exact size of a CAN(-FD) frame in bits with or
without bitsuffing.

[1] commit 85d99c3e2a13 ("can: length: can_skb_get_frame_len(): introduce
    function to get data length of frame in data link layer")
Link: https://git.kernel.org/torvalds/c/85d99c3e2a13

[2] RE: [PATCH] can: mcp251xfd: Increase poll timeout
Link: https://lore.kernel.org/linux-can/BL3PR11MB64846C83ACD04E9330B0FE66FB729@BL3PR11MB6484.namprd11.prod.outlook.com/


* Changelog *

v4 -> v5:

  * In __can_cc_frame_bits() and __can_fd_frame_bits(), enclose
    data_len in brackets to prevent operator precedence issues.

  * Add a note in can_frame_bits() documentation to explain that
    data_len shall have no side effects.

  * While at it, make CAN(FD)_FRAME_LEN_MAX definition fit on a single
    line.

  * A few typo/grammar small fixes in the commit descriptions.

Link: https://lore.kernel.org/linux-can/20230601165625.100040-1-mailhol.vincent@wanadoo.fr/

v3 -> v4:

  * No functional changes.

  * as reported by Simon Horman, fix typo in the documentation of
    can_bitstuffing_len(): "bitstream_len" -> "destuffed_len".

  * as reported by Thomas Kopp, fix several other typos:
      - "indicatior" -> "indicator"
      - "in on the wire" -> "on the wire"
      - "bitsuffing" -> "bitstuffing".

  * in CAN_FRAME_LEN_MAX comment: specify that only the dynamic
    bitstuffing gets ignored but that the intermission is included.

  * move the Suggested-by: Thomas Kopp tag from patch 2 to patch 3.

  * add Reviewed-by: Thomas Kopp tag on the full series.

  * add an additional line of comment for the @intermission argument
    of can_frame_bits().

Link: https://lore.kernel.org/linux-can/20230530144637.4746-1-mailhol.vincent@wanadoo.fr/

v2 -> v3:

  * turn can_frame_bits() and can_frame_bytes() into function-like
    macros. The fact that inline functions can not be used to
    initialize constant struct fields was bothering me. I did my best
    to make the macro look as less ugly as possible.

  * as reported by Simon Horman, add missing document for the is_fd
    argument of can_frame_bits().

Link: https://lore.kernel.org/linux-can/20230523065218.51227-1-mailhol.vincent@wanadoo.fr/

v1 -> v2:

  * as suggested by Thomas Kopp, add a new patch to the series to fix
    the stuff bit count and the fixed stuff bits definitions

  * and another patch to fix documentation of the Remote Request
    Substitution (RRS).

  * refactor the length definition. Instead of using individual macro,
    rely on an inline function. One reason is to minimize the number
    of definitions. Another reason is that because the dynamic bit
    stuff is calculated differently for CAN and CAN-FD, it is just not
    possible to multiply the existing CANFD_FRAME_OVERHEAD_SFF/EFF by
    the overhead ratio to get the bitsuffing: for CAN-FD, the CRC
    field is already stuffed by the fixed stuff bits and is out of
    scope of the dynamic bitstuffing.

Link: https://lore.kernel.org/linux-can/20230507155506.3179711-1-mailhol.vincent@wanadoo.fr/

Vincent Mailhol (3):
  can: length: fix bitstuffing count
  can: length: fix description of the RRS field
  can: length: refactor frame lengths definition to add size in bits

 drivers/net/can/dev/length.c |  15 +-
 include/linux/can/length.h   | 299 +++++++++++++++++++++++++----------
 2 files changed, 216 insertions(+), 98 deletions(-)

-- 
2.39.3

[PATCH v5 1/3] can: length: fix bitstuffing count

[Vincent Mailhol]

The Stuff Bit Count is always coded on 4 bits [1]. Update the Stuff
Bit Count size accordingly.

In addition, the CRC fields of CAN FD Frames contain stuff bits at
fixed positions called fixed stuff bits [2]. The CRC field starts with
a fixed stuff bit and then has another fixed stuff bit after each
fourth bit [2], which allows us to derive this formula:

  FSB count = 1 + round_down(len(CRC field)/4)

The length of the CRC field is [1]:

  len(CRC field) = len(Stuff Bit Count) + len(CRC)
                 = 4 + len(CRC)

with len(CRC) either 17 or 21 bits depending of the payload length.

In conclusion, for CRC17:

  FSB count = 1 + round_down((4 + 17)/4)
            = 6

and for CRC 21:

  FSB count = 1 + round_down((4 + 21)/4)
            = 7

Add a Fixed Stuff bits (FSB) field with above values and update
CANFD_FRAME_OVERHEAD_SFF and CANFD_FRAME_OVERHEAD_EFF accordingly.

[1] ISO 11898-1:2015 section 10.4.2.6 "CRC field":

  The CRC field shall contain the CRC sequence followed by a recessive
  CRC delimiter. For FD Frames, the CRC field shall also contain the
  stuff count.

  Stuff count

  If FD Frames, the stuff count shall be at the beginning of the CRC
  field. It shall consist of the stuff bit count modulo 8 in a 3-bit
  gray code followed by a parity bit [...]

[2] ISO 11898-1:2015 paragraph 10.5 "Frame coding":

  In the CRC field of FD Frames, the stuff bits shall be inserted at
  fixed positions; they are called fixed stuff bits. There shall be a
  fixed stuff bit before the first bit of the stuff count, even if the
  last bits of the preceding field are a sequence of five consecutive
  bits of identical value, there shall be only the fixed stuff bit,
  there shall not be two consecutive stuff bits. A further fixed stuff
  bit shall be inserted after each fourth bit of the CRC field [...]

Fixes: 85d99c3e2a13 ("can: length: can_skb_get_frame_len(): introduce function to get data length of frame in data link layer")
Suggested-by: Thomas Kopp <Thomas.Kopp@microchip.com>
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Reviewed-by: Thomas Kopp <Thomas.Kopp@microchip.com>
---
 include/linux/can/length.h | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/include/linux/can/length.h b/include/linux/can/length.h
index 69336549d24f..b8c12c83bc51 100644
--- a/include/linux/can/length.h
+++ b/include/linux/can/length.h
@@ -72,17 +72,18 @@
  * Error Status Indicator (ESI)		1
  * Data length code (DLC)		4
  * Data field				0...512
- * Stuff Bit Count (SBC)		0...16: 4 20...64:5
+ * Stuff Bit Count (SBC)		4
  * CRC					0...16: 17 20...64:21
  * CRC delimiter (CD)			1
+ * Fixed Stuff bits (FSB)		0...16: 6 20...64:7
  * ACK slot (AS)			1
  * ACK delimiter (AD)			1
  * End-of-frame (EOF)			7
  * Inter frame spacing			3
  *
- * assuming CRC21, rounded up and ignoring bitstuffing
+ * assuming CRC21, rounded up and ignoring dynamic bitstuffing
  */
-#define CANFD_FRAME_OVERHEAD_SFF DIV_ROUND_UP(61, 8)
+#define CANFD_FRAME_OVERHEAD_SFF DIV_ROUND_UP(67, 8)
 
 /*
  * Size of a CAN-FD Extended Frame
@@ -101,17 +102,18 @@
  * Error Status Indicator (ESI)		1
  * Data length code (DLC)		4
  * Data field				0...512
- * Stuff Bit Count (SBC)		0...16: 4 20...64:5
+ * Stuff Bit Count (SBC)		4
  * CRC					0...16: 17 20...64:21
  * CRC delimiter (CD)			1
+ * Fixed Stuff bits (FSB)		0...16: 6 20...64:7
  * ACK slot (AS)			1
  * ACK delimiter (AD)			1
  * End-of-frame (EOF)			7
  * Inter frame spacing			3
  *
- * assuming CRC21, rounded up and ignoring bitstuffing
+ * assuming CRC21, rounded up and ignoring dynamic bitstuffing
  */
-#define CANFD_FRAME_OVERHEAD_EFF DIV_ROUND_UP(80, 8)
+#define CANFD_FRAME_OVERHEAD_EFF DIV_ROUND_UP(86, 8)
 
 /*
  * Maximum size of a Classical CAN frame
-- 
2.39.3

[PATCH v5 2/3] can: length: fix description of the RRS field

[Vincent Mailhol]

The CAN-FD frames only have one reserved bit. The bit corresponding to
Classical CAN frame's RTR bit is called the "Remote Request
Substitution (RRS)" [1].

N.B. The RRS is not to be confused with the Substitute Remote Request
(SRR).

Fix the description in the CANFD_FRAME_OVERHEAD_SFF/EFF macros.

The total remains unchanged, so this is just a documentation fix.

In addition to the above add myself as copyright owner for 2020 (as
coauthor of the initial version, c.f. Fixes tag).

[1] ISO 11898-1:2015 paragraph 10.4.2.3 "Arbitration field":

  RSS bit [only in FD Frames]

    The RRS bit shall be transmitted in FD Frames at the position of
    the RTR bit in Classical Frames. The RRS bit shall be transmitted
    dominant, but receivers shall accept recessive and dominant RRS
    bits.

Fixes: 85d99c3e2a13 ("can: length: can_skb_get_frame_len(): introduce function to get data length of frame in data link layer")
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Reviewed-by: Thomas Kopp <Thomas.Kopp@microchip.com>
---
 include/linux/can/length.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/linux/can/length.h b/include/linux/can/length.h
index b8c12c83bc51..521fdbce2d69 100644
--- a/include/linux/can/length.h
+++ b/include/linux/can/length.h
@@ -1,6 +1,7 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 /* Copyright (C) 2020 Oliver Hartkopp <socketcan@hartkopp.net>
  * Copyright (C) 2020 Marc Kleine-Budde <kernel@pengutronix.de>
+ * Copyright (C) 2020 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
  */
 
 #ifndef _CAN_LENGTH_H
@@ -64,7 +65,7 @@
  * ---------------------------------------------------------
  * Start-of-frame			1
  * Identifier				11
- * Reserved bit (r1)			1
+ * Remote Request Substitution (RRS)	1
  * Identifier extension bit (IDE)	1
  * Flexible data rate format (FDF)	1
  * Reserved bit (r0)			1
@@ -95,7 +96,7 @@
  * Substitute remote request (SRR)	1
  * Identifier extension bit (IDE)	1
  * Identifier B				18
- * Reserved bit (r1)			1
+ * Remote Request Substitution (RRS)	1
  * Flexible data rate format (FDF)	1
  * Reserved bit (r0)			1
  * Bit Rate Switch (BRS)		1
-- 
2.39.3

[PATCH v5 3/3] can: length: refactor frame lengths definition to add size in bits

[Vincent Mailhol]

Introduce a method to calculate the exact size in bits of a CAN(-FD)
frame with or without dynamic bitstuffing.

These are all the possible combinations taken into account:

  - Classical CAN or CAN-FD
  - Standard or Extended frame format
  - CAN-FD CRC17 or CRC21
  - Include or not intermission

Instead of doing several individual macro definitions, declare the
can_frame_bits() function-like macro. To this extent, do a full
refactoring of the length definitions.

In addition add the can_frame_bytes(). This function-like macro
replaces the existing macro:

  - CAN_FRAME_OVERHEAD_SFF: can_frame_bytes(false, false, 0)
  - CAN_FRAME_OVERHEAD_EFF: can_frame_bytes(false, true, 0)
  - CANFD_FRAME_OVERHEAD_SFF: can_frame_bytes(true, false, 0)
  - CANFD_FRAME_OVERHEAD_EFF: can_frame_bytes(true, true, 0)

Function-like macros were chosen over inline functions because they
can be used to initialize const struct fields.

The different maximum frame lengths (maximum data length, including
intermission) are as follow:

   Frame type				bits	bytes
  -------------------------------------------------------
   Classic CAN SFF no bitstuffing	111	14
   Classic CAN EFF no bitstuffing	131	17
   Classic CAN SFF bitstuffing		135	17
   Classic CAN EFF bitstuffing		160	20
   CAN-FD SFF no bitstuffing		579	73
   CAN-FD EFF no bitstuffing		598	75
   CAN-FD SFF bitstuffing		712	89
   CAN-FD EFF bitstuffing		736	92

The macro CAN_FRAME_LEN_MAX and CANFD_FRAME_LEN_MAX are kept as an
alias to, respectively, can_frame_bytes(false, true, CAN_MAX_DLEN) and
can_frame_bytes(true, true, CANFD_MAX_DLEN).

In addition to the above:

 - Use ISO 11898-1:2015 definitions for the names of the CAN frame
   fields.
 - Include linux/bits.h for use of BITS_PER_BYTE.
 - Include linux/math.h for use of mult_frac() and
   DIV_ROUND_UP(). N.B: the use of DIV_ROUND_UP() is not new to this
   patch, but the include was previously omitted.
 - Add copyright 2023 for myself.

Suggested-by: Thomas Kopp <Thomas.Kopp@microchip.com>
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Reviewed-by: Thomas Kopp <Thomas.Kopp@microchip.com>
---
 drivers/net/can/dev/length.c |  15 +-
 include/linux/can/length.h   | 302 +++++++++++++++++++++++++----------
 2 files changed, 216 insertions(+), 101 deletions(-)

diff --git a/drivers/net/can/dev/length.c b/drivers/net/can/dev/length.c
index b48140b1102e..b7f4d76dd444 100644
--- a/drivers/net/can/dev/length.c
+++ b/drivers/net/can/dev/length.c
@@ -78,18 +78,7 @@ unsigned int can_skb_get_frame_len(const struct sk_buff *skb)
 	else
 		len = cf->len;
 
-	if (can_is_canfd_skb(skb)) {
-		if (cf->can_id & CAN_EFF_FLAG)
-			len += CANFD_FRAME_OVERHEAD_EFF;
-		else
-			len += CANFD_FRAME_OVERHEAD_SFF;
-	} else {
-		if (cf->can_id & CAN_EFF_FLAG)
-			len += CAN_FRAME_OVERHEAD_EFF;
-		else
-			len += CAN_FRAME_OVERHEAD_SFF;
-	}
-
-	return len;
+	return can_frame_bytes(can_is_canfd_skb(skb), cf->can_id & CAN_EFF_FLAG,
+			       false, len);
 }
 EXPORT_SYMBOL_GPL(can_skb_get_frame_len);
diff --git a/include/linux/can/length.h b/include/linux/can/length.h
index 521fdbce2d69..abc978b38f79 100644
--- a/include/linux/can/length.h
+++ b/include/linux/can/length.h
@@ -1,132 +1,258 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 /* Copyright (C) 2020 Oliver Hartkopp <socketcan@hartkopp.net>
  * Copyright (C) 2020 Marc Kleine-Budde <kernel@pengutronix.de>
- * Copyright (C) 2020 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
+ * Copyright (C) 2020, 2023 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
  */
 
 #ifndef _CAN_LENGTH_H
 #define _CAN_LENGTH_H
 
+#include <linux/bits.h>
 #include <linux/can.h>
 #include <linux/can/netlink.h>
+#include <linux/math.h>
 
 /*
- * Size of a Classical CAN Standard Frame
+ * Size of a Classical CAN Standard Frame header in bits
  *
- * Name of Field			Bits
+ * Name of Field				Bits
  * ---------------------------------------------------------
- * Start-of-frame			1
- * Identifier				11
- * Remote transmission request (RTR)	1
- * Identifier extension bit (IDE)	1
- * Reserved bit (r0)			1
- * Data length code (DLC)		4
- * Data field				0...64
- * CRC					15
- * CRC delimiter			1
- * ACK slot				1
- * ACK delimiter			1
- * End-of-frame (EOF)			7
- * Inter frame spacing			3
+ * Start Of Frame (SOF)				1
+ * Arbitration field:
+ *	base ID					11
+ *	Remote Transmission Request (RTR)	1
+ * Control field:
+ *	IDentifier Extension bit (IDE)		1
+ *	FD Format indicator (FDF)		1
+ *	Data Length Code (DLC)			4
+ *
+ * including all fields preceding the data field, ignoring bitstuffing
+ */
+#define CAN_FRAME_HEADER_SFF_BITS 19
+
+/*
+ * Size of a Classical CAN Extended Frame header in bits
  *
- * rounded up and ignoring bitstuffing
+ * Name of Field				Bits
+ * ---------------------------------------------------------
+ * Start Of Frame (SOF)				1
+ * Arbitration field:
+ *	base ID					11
+ *	Substitute Remote Request (SRR)		1
+ *	IDentifier Extension bit (IDE)		1
+ *	ID extension				18
+ *	Remote Transmission Request (RTR)	1
+ * Control field:
+ *	FD Format indicator (FDF)		1
+ *	Reserved bit (r0)			1
+ *	Data length code (DLC)			4
+ *
+ * including all fields preceding the data field, ignoring bitstuffing
  */
-#define CAN_FRAME_OVERHEAD_SFF DIV_ROUND_UP(47, 8)
+#define CAN_FRAME_HEADER_EFF_BITS 39
 
 /*
- * Size of a Classical CAN Extended Frame
+ * Size of a CAN-FD Standard Frame in bits
+ *
+ * Name of Field				Bits
+ * ---------------------------------------------------------
+ * Start Of Frame (SOF)				1
+ * Arbitration field:
+ *	base ID					11
+ *	Remote Request Substitution (RRS)	1
+ * Control field:
+ *	IDentifier Extension bit (IDE)		1
+ *	FD Format indicator (FDF)		1
+ *	Reserved bit (res)			1
+ *	Bit Rate Switch (BRS)			1
+ *	Error Status Indicator (ESI)		1
+ *	Data length code (DLC)			4
+ *
+ * including all fields preceding the data field, ignoring bitstuffing
+ */
+#define CANFD_FRAME_HEADER_SFF_BITS 22
+
+/*
+ * Size of a CAN-FD Extended Frame in bits
+ *
+ * Name of Field				Bits
+ * ---------------------------------------------------------
+ * Start Of Frame (SOF)				1
+ * Arbitration field:
+ *	base ID					11
+ *	Substitute Remote Request (SRR)		1
+ *	IDentifier Extension bit (IDE)		1
+ *	ID extension				18
+ *	Remote Request Substitution (RRS)	1
+ * Control field:
+ *	FD Format indicator (FDF)		1
+ *	Reserved bit (res)			1
+ *	Bit Rate Switch (BRS)			1
+ *	Error Status Indicator (ESI)		1
+ *	Data length code (DLC)			4
+ *
+ * including all fields preceding the data field, ignoring bitstuffing
+ */
+#define CANFD_FRAME_HEADER_EFF_BITS 41
+
+/*
+ * Size of a CAN CRC Field in bits
  *
  * Name of Field			Bits
  * ---------------------------------------------------------
- * Start-of-frame			1
- * Identifier A				11
- * Substitute remote request (SRR)	1
- * Identifier extension bit (IDE)	1
- * Identifier B				18
- * Remote transmission request (RTR)	1
- * Reserved bits (r1, r0)		2
- * Data length code (DLC)		4
- * Data field				0...64
- * CRC					15
- * CRC delimiter			1
- * ACK slot				1
- * ACK delimiter			1
- * End-of-frame (EOF)			7
- * Inter frame spacing			3
+ * CRC sequence (CRC15)			15
+ * CRC Delimiter			1
+ *
+ * ignoring bitstuffing
+ */
+#define CAN_FRAME_CRC_FIELD_BITS 16
+
+/*
+ * Size of a CAN-FD CRC17 Field in bits (length: 0..16)
  *
- * rounded up and ignoring bitstuffing
+ * Name of Field			Bits
+ * ---------------------------------------------------------
+ * Stuff Count				4
+ * CRC Sequence (CRC17)			17
+ * CRC Delimiter			1
+ * Fixed stuff bits			6
  */
-#define CAN_FRAME_OVERHEAD_EFF DIV_ROUND_UP(67, 8)
+#define CANFD_FRAME_CRC17_FIELD_BITS 28
 
 /*
- * Size of a CAN-FD Standard Frame
+ * Size of a CAN-FD CRC21 Field in bits (length: 20..64)
  *
  * Name of Field			Bits
  * ---------------------------------------------------------
- * Start-of-frame			1
- * Identifier				11
- * Remote Request Substitution (RRS)	1
- * Identifier extension bit (IDE)	1
- * Flexible data rate format (FDF)	1
- * Reserved bit (r0)			1
- * Bit Rate Switch (BRS)		1
- * Error Status Indicator (ESI)		1
- * Data length code (DLC)		4
- * Data field				0...512
- * Stuff Bit Count (SBC)		4
- * CRC					0...16: 17 20...64:21
- * CRC delimiter (CD)			1
- * Fixed Stuff bits (FSB)		0...16: 6 20...64:7
- * ACK slot (AS)			1
- * ACK delimiter (AD)			1
- * End-of-frame (EOF)			7
- * Inter frame spacing			3
- *
- * assuming CRC21, rounded up and ignoring dynamic bitstuffing
- */
-#define CANFD_FRAME_OVERHEAD_SFF DIV_ROUND_UP(67, 8)
+ * Stuff Count				4
+ * CRC sequence (CRC21)			21
+ * CRC Delimiter			1
+ * Fixed stuff bits			7
+ */
+#define CANFD_FRAME_CRC21_FIELD_BITS 33
 
 /*
- * Size of a CAN-FD Extended Frame
+ * Size of a CAN(-FD) Frame footer in bits
  *
  * Name of Field			Bits
  * ---------------------------------------------------------
- * Start-of-frame			1
- * Identifier A				11
- * Substitute remote request (SRR)	1
- * Identifier extension bit (IDE)	1
- * Identifier B				18
- * Remote Request Substitution (RRS)	1
- * Flexible data rate format (FDF)	1
- * Reserved bit (r0)			1
- * Bit Rate Switch (BRS)		1
- * Error Status Indicator (ESI)		1
- * Data length code (DLC)		4
- * Data field				0...512
- * Stuff Bit Count (SBC)		4
- * CRC					0...16: 17 20...64:21
- * CRC delimiter (CD)			1
- * Fixed Stuff bits (FSB)		0...16: 6 20...64:7
- * ACK slot (AS)			1
- * ACK delimiter (AD)			1
- * End-of-frame (EOF)			7
- * Inter frame spacing			3
- *
- * assuming CRC21, rounded up and ignoring dynamic bitstuffing
- */
-#define CANFD_FRAME_OVERHEAD_EFF DIV_ROUND_UP(86, 8)
+ * ACK slot				1
+ * ACK delimiter			1
+ * End Of Frame (EOF)			7
+ *
+ * including all fields following the CRC field
+ */
+#define CAN_FRAME_FOOTER_BITS 9
+
+/*
+ * First part of the Inter Frame Space
+ * (a.k.a. IMF - intermission field)
+ */
+#define CAN_INTERMISSION_BITS 3
+
+/**
+ * can_bitstuffing_len() - Calculate the maximum length with bitstuffing
+ * @destuffed_len: length of a destuffed bit stream
+ *
+ * The worst bit stuffing case is a sequence in which dominant and
+ * recessive bits alternate every four bits:
+ *
+ *   Destuffed: 1 1111  0000  1111  0000  1111
+ *   Stuffed:   1 1111o 0000i 1111o 0000i 1111o
+ *
+ * Nomenclature
+ *
+ *  - "0": dominant bit
+ *  - "o": dominant stuff bit
+ *  - "1": recessive bit
+ *  - "i": recessive stuff bit
+ *
+ * Aside from the first bit, one stuff bit is added every four bits.
+ *
+ * Return: length of the stuffed bit stream in the worst case scenario.
+ */
+#define can_bitstuffing_len(destuffed_len)			\
+	(destuffed_len + (destuffed_len - 1) / 4)
+
+#define __can_bitstuffing_len(bitstuffing, destuffed_len)	\
+	(bitstuffing ? can_bitstuffing_len(destuffed_len) :	\
+		       destuffed_len)
+
+#define __can_cc_frame_bits(is_eff, bitstuffing,		\
+			    intermission, data_len)		\
+(								\
+	__can_bitstuffing_len(bitstuffing,			\
+		(is_eff ? CAN_FRAME_HEADER_EFF_BITS :		\
+			  CAN_FRAME_HEADER_SFF_BITS) +		\
+		(data_len) * BITS_PER_BYTE +			\
+		CAN_FRAME_CRC_FIELD_BITS) +			\
+	CAN_FRAME_FOOTER_BITS +					\
+	(intermission ? CAN_INTERMISSION_BITS : 0)		\
+)
+
+#define __can_fd_frame_bits(is_eff, bitstuffing,		\
+			    intermission, data_len)		\
+(								\
+	__can_bitstuffing_len(bitstuffing,			\
+		(is_eff ? CANFD_FRAME_HEADER_EFF_BITS :		\
+			  CANFD_FRAME_HEADER_SFF_BITS) +	\
+		(data_len) * BITS_PER_BYTE) +			\
+	((data_len) <= 16 ?					\
+		CANFD_FRAME_CRC17_FIELD_BITS :			\
+		CANFD_FRAME_CRC21_FIELD_BITS) +			\
+	CAN_FRAME_FOOTER_BITS +					\
+	(intermission ? CAN_INTERMISSION_BITS : 0)		\
+)
+
+/**
+ * can_frame_bits() - Calculate the number of bits on the wire in a
+ *	CAN frame
+ * @is_fd: true: CAN-FD frame; false: Classical CAN frame.
+ * @is_eff: true: Extended frame; false: Standard frame.
+ * @bitstuffing: true: calculate the bitstuffing worst case; false:
+ *	calculate the bitstuffing best case (no dynamic
+ *	bitstuffing). CAN-FD's fixed stuff bits are always included.
+ * @intermission: if and only if true, include the inter frame space
+ *	assuming no bus idle (i.e. only the intermission). Strictly
+ *	speaking, the inter frame space is not part of the
+ *	frame. However, it is needed when calculating the delay
+ *	between the Start Of Frame of two consecutive frames.
+ * @data_len: length of the data field in bytes. Correspond to
+ *	can(fd)_frame->len. Should be zero for remote frames. No
+ *	sanitization is done on @data_len and it shall have no side
+ *	effects.
+ *
+ * Return: the numbers of bits on the wire of a CAN frame.
+ */
+#define can_frame_bits(is_fd, is_eff, bitstuffing,		\
+		       intermission, data_len)			\
+(								\
+	is_fd ? __can_fd_frame_bits(is_eff, bitstuffing,	\
+				    intermission, data_len) :	\
+		__can_cc_frame_bits(is_eff, bitstuffing,	\
+				    intermission, data_len)	\
+)
+
+/*
+ * Number of bytes in a CAN frame
+ * (rounded up, including intermission)
+ */
+#define can_frame_bytes(is_fd, is_eff, bitstuffing, data_len)	\
+	DIV_ROUND_UP(can_frame_bits(is_fd, is_eff, bitstuffing,	\
+				    true, data_len),		\
+		     BITS_PER_BYTE)
 
 /*
  * Maximum size of a Classical CAN frame
- * (rounded up and ignoring bitstuffing)
+ * (rounded up, ignoring bitstuffing but including intermission)
  */
-#define CAN_FRAME_LEN_MAX (CAN_FRAME_OVERHEAD_EFF + CAN_MAX_DLEN)
+#define CAN_FRAME_LEN_MAX can_frame_bytes(false, true, false, CAN_MAX_DLEN)
 
 /*
  * Maximum size of a CAN-FD frame
- * (rounded up and ignoring bitstuffing)
+ * (rounded up, ignoring dynamic bitstuffing but including intermission)
  */
-#define CANFD_FRAME_LEN_MAX (CANFD_FRAME_OVERHEAD_EFF + CANFD_MAX_DLEN)
+#define CANFD_FRAME_LEN_MAX can_frame_bytes(true, true, false, CANFD_MAX_DLEN)
 
 /*
  * can_cc_dlc2len(value) - convert a given data length code (dlc) of a
-- 
2.39.3