On Fri, Jan 15, 2021 at 01:38:12PM +0100, Oliver Hartkopp wrote:
> 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.
I stole the idea of removing the last 16 bytes of the array from the padlen()
function in the ISOTP code.
> 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.
...then I wrongly replaced the 48 with the ARRAY_SIZE().
> 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.
What about this patch:
diff --git a/drivers/net/can/dev/length.c b/drivers/net/can/dev/length.c
index d35c4e82314d..f5509e48fe95 100644
--- a/drivers/net/can/dev/length.c
+++ b/drivers/net/can/dev/length.c
@@ -27,12 +27,14 @@ static const u8 len2dlc[] = {
13, 13, 13, 13, 13, 13, 13, 13, /* 25 - 32 */
14, 14, 14, 14, 14, 14, 14, 14, /* 33 - 40 */
14, 14, 14, 14, 14, 14, 14, 14, /* 41 - 48 */
+ 15, 15, 15, 15, 15, 15, 15, 15, /* 49 - 56 */
+ 15, 15, 15, 15, 15, 15, 15, 15 /* 57 - 64 */
};
/* map the sanitized data length to an appropriate data length code */
u8 can_fd_len2dlc(u8 len)
{
- if (len >= ARRAY_SIZE(len2dlc))
+ if (unlikely(len > CANFD_MAX_LEN))
return CANFD_MAX_DLC;
return len2dlc[len];
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Attachment:
signature.asc
Description: PGP signature
