Re: [PATCH v5 1/3] can: length: fix bitstuffing count
From: Vincent MAILHOL
Date: Thu Jun 15 2023 - 05:58:23 EST
On Thu. 15 Jun. 2023 at 17:03, Simon Horman <simon.horman@xxxxxxxxxxxx> wrote:
> On Wed, Jun 14, 2023 at 08:40:42PM +0000, HMS Incident Management wrote:
> > [You don't often get email from incidentmanagement@xxxxxx. 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@xxxxxxxxx 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@xxxxxxxxxx/
> 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@xxxxxxxxxxxx>
* Copyright (C) 2020 Marc Kleine-Budde <kernel@xxxxxxxxxxxxxx>
+ * Copyright (C) 2020 Vincent Mailhol <mailhol.vincent@xxxxxxxxxx>
*/
#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