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

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

[HMS Incident Management]

**We apologize for the delay in delivering this email, which was caused by a mail incident that occurred over the weekend on June 10th. This email was originally sent from vincent.mailhol@gmail.com on 06/11/2023 02:58:08 

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 
Signed-off-by: Vincent Mailhol 
Reviewed-by: Thomas Kopp 
---
 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

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

[Simon Horman]

On Wed, Jun 14, 2023 at 08:40:42PM +0000, HMS Incident Management wrote:
> [You don't often get email from incidentmanagement@hms.se. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> **We apologize for the delay in delivering this email, which was caused by a mail incident that occurred over the weekend on June 10th. This email was originally sent from vincent.mailhol@gmail.com on 06/11/2023 02:58:08
> 
> 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
> Signed-off-by: Vincent Mailhol
> Reviewed-by: Thomas Kopp

Hi,

Some feedback from my side, in the hope that it is useful.

I guess this patch-set has had a bit of a journey, email-wise.
Unfortunately on it's trip the email addresses for the tags above got lost,
which by itself leads me to think it should be resent.

Also, I think it would be best if the From address of the email
was from a human, who features in the Signed-off-by tags of the patches.
But perhaps this is also an artifact of the journey.

It is also unclear to me where this applies - f.e. it doesn't
apply to the main branch of linux-can-next.

Lastly, I'm not a CAN maintainer. But I think it's usual to separate
fixes and enhancements into different series, likely the former
targeting the can tree while the latter targets the can-next tree
(I could be way off here).

If on the other hand, the patches in this series are not bug fixes,
then it is probably best to drop the 'fixes' language.

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

[Vincent MAILHOL]

On Thu. 15 Jun. 2023 at 17:03, Simon Horman <simon.horman@corigine.com> wrote:
> On Wed, Jun 14, 2023 at 08:40:42PM +0000, HMS Incident Management wrote:
> > [You don't often get email from incidentmanagement@hms.se. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> >
> > **We apologize for the delay in delivering this email, which was caused by a mail incident that occurred over the weekend on June 10th. This email was originally sent from vincent.mailhol@gmail.com on 06/11/2023 02:58:08
> >
> > 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
> > Signed-off-by: Vincent Mailhol
> > Reviewed-by: Thomas Kopp
>
> Hi,
>
> Some feedback from my side, in the hope that it is useful.
>
> I guess this patch-set has had a bit of a journey, email-wise.
> Unfortunately on it's trip the email addresses for the tags above got lost,
> which by itself leads me to think it should be resent.

I can see Thomas's email in the To: field.

> Also, I think it would be best if the From address of the email
> was from a human, who features in the Signed-off-by tags of the patches.
> But perhaps this is also an artifact of the journey.

Yes, the original message had a correct From: tag. As far as I can
see, only that From: tag was lost. The other To: and CC: fields seem
correct.

Also, the original message correctly reached the CAN mailing list with
all the good tags.

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

> It is also unclear to me where this applies - f.e. it doesn't
> apply to the main branch of linux-can-next.

I tested it right now, the original series apply well. The one you
received was redacted and indeed does not apply. For example, this:

  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

became that:

  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
    * Copyright (C) 2020 Marc Kleine-Budde
  + * Copyright (C) 2020 Vincent Mailhol
    */

   #ifndef _CAN_LENGTH_H

(note the missing e-mail address).

@HMS incident management team, maybe this is something you should fix.

> Lastly, I'm not a CAN maintainer. But I think it's usual to separate
> fixes and enhancements into different series, likely the former
> targeting the can tree while the latter targets the can-next tree
> (I could be way off here).

Hmm... The fact is that only the first two patches are fixes. The
third one is not.  The fixes being really minor, there is no urgency.
So I was thinking of having the full series go to the next branch and
as long as there is the Fix: tag, the two first patches will
eventually be picked by the stable team. I thought that this approach
was easier than sending two fixes to the stable branch, wait for these
to propagate to next and then send a second series of a single patch
for next.

@Marc, let me know what you prefer. I am fine to split if this works
best for you. Also, I will wait for your answer before doing any
resend.

> If on the other hand, the patches in this series are not bug fixes,
> then it is probably best to drop the 'fixes' language.

I will keep the Fix tags. Even if minor (probably no visible
repercussions) it still fixes an existing inaccuracy (whether you call
it bug or not is another debate, but I often see typo fixes being
backported, and these are a bit more than a typo fix).


Yours sincerely,
Vincent Mailhol

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

[Marc Kleine-Budde]

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

On 15.06.2023 18:58:04, Vincent MAILHOL wrote:
> > Lastly, I'm not a CAN maintainer. But I think it's usual to separate
> > fixes and enhancements into different series, likely the former
> > targeting the can tree while the latter targets the can-next tree
> > (I could be way off here).
> 
> Hmm... The fact is that only the first two patches are fixes. The
> third one is not.  The fixes being really minor, there is no urgency.
> So I was thinking of having the full series go to the next branch and
> as long as there is the Fix: tag, the two first patches will
> eventually be picked by the stable team. I thought that this approach
> was easier than sending two fixes to the stable branch, wait for these
> to propagate to next and then send a second series of a single patch
> for next.
> 
> @Marc, let me know what you prefer. I am fine to split if this works
> best for you. Also, I will wait for your answer before doing any
> resend.
> 
> > If on the other hand, the patches in this series are not bug fixes,
> > then it is probably best to drop the 'fixes' language.
> 
> I will keep the Fix tags. Even if minor (probably no visible
> repercussions) it still fixes an existing inaccuracy (whether you call
> it bug or not is another debate, but I often see typo fixes being
> backported, and these are a bit more than a typo fix).

I've taken the whole series as is to linux-can-next.

Thanks,
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 --]

[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

[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