Skip to contentSkip to conversation

can: length: can_fd_len2dlc(): make length calculation readable again

PATCH In review Oliver Hartkopp 2 participants 4 messages Jan 18, 2021 – Jan 20, 2021 CAN network drivers

lore
Share & tools
lore
#0 lore raw

PATCH v3 1/1 can: length: can_fd_len2dlc(): make length calculation readable again

In commit 652562e5ff06 ("can: length: can_fd_len2dlc(): simplify length calculcation") the readability of the code degraded and became more error prone. To counteract this, partially convert that patch and replace open coded values (of the original code) with proper defines.
Also double check the correct length mapping table size at build time.

Cc Vincent MAILHOL <[email protected]>
Fixes 652562e5ff06 ("can: length: can_fd_len2dlc(): simplify length calculcation")
---
 drivers/net/can/dev/length.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/net/can/dev/length.c +6 -1
index d35c4e82314d..d085cb26d00d 100644
@@ -25,16 +25,21 @@ static const u8 len2dlc[] = {
 	11, 11, 11, 11,			/* 17 - 20 */
 	12, 12, 12, 12,			/* 21 - 24 */
 	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))
+	/* check for length mapping table size at build time */
+	BUILD_BUG_ON(ARRAY_SIZE(len2dlc) < CANFD_MAX_DLEN + 1);
+
+	if (unlikely(len > CANFD_MAX_DLEN))
 		return CANFD_MAX_DLC;
 
 	return len2dlc[len];
 }
 EXPORT_SYMBOL_GPL(can_fd_len2dlc);
Signature · 1 line
2.29.2
#1 lore raw
Quoted 24 lines · Oliver Hartkopp

In commit 652562e5ff06 ("can: length: can_fd_len2dlc(): simplify length calculcation") the readability of the code degraded and became more error prone. To counteract this, partially convert that patch and replace open coded values (of the original code) with proper defines.
Also double check the correct length mapping table size at build time.

Cc Vincent MAILHOL <[email protected]>
Fixes 652562e5ff06 ("can: length: can_fd_len2dlc(): simplify length calculcation")
---
 drivers/net/can/dev/length.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/net/can/dev/length.c +0 -0
index d35c4e82314d..d085cb26d00d 100644
@@ -25,16 +25,21 @@ static const u8 len2dlc[] = {
 	11, 11, 11, 11,			/* 17 - 20 */
 	12, 12, 12, 12,			/* 21 - 24 */
 	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 */

drivers/net/can/dev/length.c static const u8 len2dlc[] = { · line 38

Quoted 10 lines
drivers/net/can/dev/length.c +4 -1
+	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))
+	/* check for length mapping table size at build time */
+	BUILD_BUG_ON(ARRAY_SIZE(len2dlc) < CANFD_MAX_DLEN + 1);
                                        ^^^

What about "!="?

regards
Marc

Signature · 4 lines
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 |
#2 lore raw
Quoted 29 lines · Marc Kleine-Budde
Quoted 28 lines · Oliver Hartkopp

In commit 652562e5ff06 ("can: length: can_fd_len2dlc(): simplify length calculcation") the readability of the code degraded and became more error prone. To counteract this, partially convert that patch and replace open coded values (of the original code) with proper defines.
Also double check the correct length mapping table size at build time.

Cc Vincent MAILHOL <[email protected]>
Fixes 652562e5ff06 ("can: length: can_fd_len2dlc(): simplify length calculcation")
---
  drivers/net/can/dev/length.c | 7 ++++++-
  1 file changed, 6 insertions(+), 1 deletion(-)
drivers/net/can/dev/length.c +2 -0
index d35c4e82314d..d085cb26d00d 100644
@@ -25,16 +25,21 @@ static const u8 len2dlc[] = {
  	11, 11, 11, 11,			/* 17 - 20 */
  	12, 12, 12, 12,			/* 21 - 24 */
  	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 */
  };
 
Quoted 8 lines

drivers/net/can/dev/length.c static const u8 len2dlc[] = { · line 38

Quoted 6 lines
drivers/net/can/dev/length.c +2 -1
  /* map the sanitized data length to an appropriate data length code */
  u8 can_fd_len2dlc(u8 len)
  {
-	if (len >= ARRAY_SIZE(len2dlc))
+	/* check for length mapping table size at build time */
+	BUILD_BUG_ON(ARRAY_SIZE(len2dlc) < CANFD_MAX_DLEN + 1);
                                         ^^^

What about "!="?

"<" makes sure that the table is long enough
"!=" makes sure it has a specific length

So "<" looks easier to understand to me.

¯\_(ツ)_/¯

Regards,
Oliver

#3 lore raw
Quoted 23 lines · Oliver Hartkopp
Quoted 14 lines
Quoted 14 lines
drivers/net/can/dev/length.c +2 -0
index d35c4e82314d..d085cb26d00d 100644
@@ -25,16 +25,21 @@ static const u8 len2dlc[] = {
  	11, 11, 11, 11,			/* 17 - 20 */
  	12, 12, 12, 12,			/* 21 - 24 */
  	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 */
  };
 
Quoted 8 lines

drivers/net/can/dev/length.c static const u8 len2dlc[] = { · line 38

Quoted 6 lines
drivers/net/can/dev/length.c +2 -1
  /* map the sanitized data length to an appropriate data length code */
  u8 can_fd_len2dlc(u8 len)
  {
-	if (len >= ARRAY_SIZE(len2dlc))
+	/* check for length mapping table size at build time */
+	BUILD_BUG_ON(ARRAY_SIZE(len2dlc) < CANFD_MAX_DLEN + 1);
                                         ^^^

What about "!="?

Quoted 2 lines

"<" makes sure that the table is long enough
"!=" makes sure it has a specific length

From my point of view we want it to have a specific length...

Quoted 3 lines

So "<" looks easier to understand to me.

¯\_(ツ)_/¯

...so changed to != why applying to linux-can-next/testing

regards,
Marc

Signature · 4 lines
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 |