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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxx>
>   * Copyright (C) 2020 Marc Kleine-Budde <kernel@xxxxxxxxxxxxxx>
> + * Copyright (C) 2020, 2023 Vincent Mailhol <mailhol.vincent@xxxxxxxxxx>
>   */
> 
>  #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