netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Oliver Hartkopp <socketcan@hartkopp.net>
To: Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
Cc: Marc Kleine-Budde <mkl@pengutronix.de>,
	netdev <netdev@vger.kernel.org>,
	David Miller <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	linux-can <linux-can@vger.kernel.org>,
	kernel@pengutronix.de
Subject: Re: [net-next 09/17] can: length: can_fd_len2dlc(): simplify length calculcation
Date: Thu, 14 Jan 2021 18:03:14 +0100	[thread overview]
Message-ID: <75d3c8e9-acbd-09e9-e185-94833dbfb391@hartkopp.net> (raw)
In-Reply-To: <CAMZ6RqLKYnGDePueN1ftL9a47Qf-ZR7bc4eLGwzCkncsD6ok2Q@mail.gmail.com>



On 14.01.21 10:16, Vincent MAILHOL wrote:
> On Tue. 14 Jan 2021 at 17:23, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>> On 14.01.21 02:59, Vincent MAILHOL wrote:
>>> On Tue. 14 Jan 2021 at 06:14, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>>>>
>>>> If the length paramter in len2dlc() exceeds the size of the len2dlc array, we
>>>> return 0xF. This is equal to the last 16 members of the array.
>>>>
>>>> This patch removes these members from the array, uses ARRAY_SIZE() for the
>>>> length check, and returns CANFD_MAX_DLC (which is 0xf).
>>>>
>>>> Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>>>> Link: https://lore.kernel.org/r/20210111141930.693847-9-mkl@pengutronix.de
>>>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>>>> ---
>>>>    drivers/net/can/dev/length.c | 6 ++----
>>>>    1 file changed, 2 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/net/can/dev/length.c b/drivers/net/can/dev/length.c
>>>> index 5e7d481717ea..d695a3bee1ed 100644
>>>> --- a/drivers/net/can/dev/length.c
>>>> +++ b/drivers/net/can/dev/length.c
>>>> @@ -27,15 +27,13 @@ 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 (unlikely(len > 64))
>>>> -               return 0xF;
>>>> +       if (len > ARRAY_SIZE(len2dlc))
>>>
>>> Sorry but I missed an of-by-one issue when I did my first
>>> review. Don't know why but it popped to my eyes this morning when
>>> casually reading the emails.
>>
>> Oh, yes.
>>
>> The fist line is 0 .. 8 which has 9 bytes.
>>
>> I also looked on it (from the back), and wondered if it was correct. But
>> didn't see it either at first sight.
>>
>>>
>>> ARRAY_SIZE(len2dlc) is 49. If len is between 0 and 48, use the
>>> array, if len is greater *or equal* return CANFD_MAX_DLC.
>>
>> All these changes and discussions make it very obviously more tricky to
>> understand that code.
>>
>> I don't really like this kind of improvement ...
>>
>> Before that it was pretty clear that we only catch an out of bounds
>> value and usually grab the value from the table.
> 
> I understand your point: all three of us initially missed that
> bug. But now that it is fixed, I would still prefer to keep
> Marc's patch.

No, I'm still against it as it is now.

Even

	if (len >= ARRAY_SIZE(len2dlc))

would need some comment that values > 48 lead to a DLC = 15.

This is not intuitively understandable from that value 
"ARRAY_SIZE(len2dlc)" !

Using ARRAY_SIZE() is a bad choice IMO.

If it's really worth to save 16 bytes I would suggest this:

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 3486704c8a95..0b0a5a16943a 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -42,18 +42,17 @@ static const u8 len2dlc[] = {0, 1, 2, 3, 4, 5, 6, 7, 
8,             /* 0 - 8 */
                              10, 10, 10, 10,                    /* 13 - 
16 */
                              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 */
+                            14, 14, 14, 14, 14, 14, 14, 14};   /* 41 - 
48 */
+                            /* 49 - 64 is checked in  can_fd_len2dlc() */

  /* map the sanitized data length to an appropriate data length code */
  u8 can_fd_len2dlc(u8 len)
  {
-       if (unlikely(len > 64))
+       if (len > 48)
                 return 0xF;

         return len2dlc[len];
  }
  EXPORT_SYMBOL_GPL(can_fd_len2dlc);

Regards,
Oliver

> 
> 
> Yours sincerely,
> Vincent
> 
>>>
>>> In short, replace > by >=:
>>> +       if (len >= ARRAY_SIZE(len2dlc))
>>>
>>>> +               return CANFD_MAX_DLC;
>>>>
>>>>           return len2dlc[len];
>>>>    }

  reply	other threads:[~2021-01-14 17:06 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-13 21:13 pull-request: can-next 2021-01-13 Marc Kleine-Budde
2021-01-13 21:13 ` [net-next 01/17] MAINTAINERS: adjust entry to tcan4x5x file split Marc Kleine-Budde
2021-01-13 21:13 ` [net-next 02/17] MAINTAINERS: CAN network layer: add missing header file can-ml.h Marc Kleine-Budde
2021-01-13 21:13 ` [net-next 03/17] can: dev: move driver related infrastructure into separate subdir Marc Kleine-Budde
2021-01-13 21:13 ` [net-next 04/17] can: dev: move bittiming related code into seperate file Marc Kleine-Budde
2021-01-13 21:13 ` [net-next 05/17] can: dev: move length " Marc Kleine-Budde
2021-01-13 21:13 ` [net-next 06/17] can: dev: move skb related " Marc Kleine-Budde
2021-01-13 21:14 ` [net-next 07/17] can: dev: move netlink related code " Marc Kleine-Budde
2021-01-13 21:14 ` [net-next 08/17] can: length: convert to kernel coding style Marc Kleine-Budde
2021-01-13 21:14 ` [net-next 09/17] can: length: can_fd_len2dlc(): simplify length calculcation Marc Kleine-Budde
2021-01-14  1:59   ` Vincent MAILHOL
2021-01-14  7:34     ` Marc Kleine-Budde
2021-01-14  8:23     ` Oliver Hartkopp
2021-01-14  9:16       ` Vincent MAILHOL
2021-01-14 17:03         ` Oliver Hartkopp [this message]
2021-01-15  1:57           ` Vincent MAILHOL
2021-01-13 21:14 ` [net-next 10/17] can: length: canfd_sanitize_len(): add function to sanitize CAN-FD data length Marc Kleine-Budde
2021-01-13 21:14 ` [net-next 11/17] can: length: can_skb_get_frame_len(): introduce function to get data length of frame in data link layer Marc Kleine-Budde
2021-01-13 21:14 ` [net-next 12/17] can: dev: extend struct can_skb_priv to hold CAN frame length Marc Kleine-Budde
2021-01-13 21:14 ` [net-next 13/17] can: dev: can_put_echo_skb(): extend to handle frame_len Marc Kleine-Budde
2021-01-13 21:14 ` [net-next 14/17] can: dev: can_get_echo_skb(): extend to return can frame length Marc Kleine-Budde
2021-01-13 21:14 ` [net-next 15/17] can: dev: can_rx_offload_get_echo_skb(): " Marc Kleine-Budde
2021-01-13 21:14 ` [net-next 16/17] can: dev: can_put_echo_skb(): add software tx timestamps Marc Kleine-Budde
2021-01-13 21:14 ` [net-next 17/17] can: tcan4x5x: remove __packed attribute from struct tcan4x5x_map_buf Marc Kleine-Budde
2021-01-14  7:32 ` pull-request: can-next 2021-01-13 Marc Kleine-Budde
2021-01-14  7:56 pull-request: can-next 2021-01-14 Marc Kleine-Budde
2021-01-14  7:56 ` [net-next 09/17] can: length: can_fd_len2dlc(): simplify length calculcation Marc Kleine-Budde

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=75d3c8e9-acbd-09e9-e185-94833dbfb391@hartkopp.net \
    --to=socketcan@hartkopp.net \
    --cc=davem@davemloft.net \
    --cc=kernel@pengutronix.de \
    --cc=kuba@kernel.org \
    --cc=linux-can@vger.kernel.org \
    --cc=mailhol.vincent@wanadoo.fr \
    --cc=mkl@pengutronix.de \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).