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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMZ6Rq+EgMW4zLPVWP8e0DT2R0WO5swxxuc3qUAewAe6fgubOw@mail.gmail.com>
Date: Tue, 23 May 2023 16:13:00 +0900
From: Vincent MAILHOL <mailhol.vincent@...adoo.fr>
To: Marc Kleine-Budde <mkl@...gutronix.de>, linux-can@...r.kernel.org, 
	Thomas.Kopp@...rochip.com
Cc: Oliver Hartkopp <socketcan@...tkopp.net>, netdev@...r.kernel.org, marex@...x.de
Subject: Re: [PATCH v2 3/3] can: length: refactor frame lengths definition to
 add size in bits

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@...adoo.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