-DaveM
-Jakub
-netdev
On 15.01.21 02:57, Vincent MAILHOL wrote:
48 */
+ /* 49 - 64 is checked in can_fd_len2dlc() */
Ack
/* map the sanitized data length to an appropriate data length code */
u8 can_fd_len2dlc(u8 len)
{
- if (unlikely(len > 64))
+ if (len > 48)
I personally prefer the use of macros instead of hardcoded values. 48 is the
last index of the table, i.e. it is ARRAY_SIZE(len2dlc) - 1.
In general I'm with you here.
For me, it is like doing this:
for (i = 0; i <= harcoded_value_representing_last_index_of_array; i++)
instead of this:
for (i = 0; i < ARRAY_SIZE(array); i++)
Definitely prefer the later and (len >= ARRAY_SIZE(len2dlc)) is nothing less
than the negation of the i < ARRAY_SIZE(array) that we usually see in a for
loop.
I recognize below patterns to be correct:
i < ARRAY_SIZE(array): check that variable is inbound.
i >= ARRAY_SIZE(array): check that variable is outbound.
Anything which deviates from those patterns is fishy and it is actually how
I spotted the bug.
If we don’t use ARRAY_SIZE() we lose that recognizable pattern and we need
to be aware of the actual content of len2dlc[] to understand the code.
(And I know that the table is just above the function and that this makes my
argument weaker).
So IMO, checks done against the array size should use the ARRAY_SIZE() macro
in order 1/ to make it a recognizable pattern and 2/ to make it work regardless
of the actual size of the table (i.e. no hardcoded value).
The problem is NOT that we make sure to access this array correctly.
This particular array is no set of arbitrary values that may be extended
later on BUT it is a 'translation map' for defined length values which
will never change.
Therefore ARRAY_SIZE(array) hides the fact that every length value
"greater than 48" results to a DLC of 15.
For that reason my former code was very clear:
1. It had a table that mapped 0 .. 64 to a DLC
2. It had a test for '> 64' as sanity test.
Now the sanity test is gone and mixed up with the mapping of length
values - and finally with ARRAY_SIZE(whatever) which doesn't give a hint
why this is checked.
We are writing code to be understandable for the reader and the
suggested 'improvement' which saves 16 bytes does exactly the opposite.
For that reason the entire patch is broken.
An improvement could be to rename
64 -> CANFD_MAX_LEN
0xF -> CANFD_MAX_DLC
but nothing more.
return 0xF;
I would also prefer to use the CANFD_MAX_DLC macro here.
Ack. Me too. Just seen that after pressing the 'send' button :-/
Regards,
Oliver
