Re: [net-next 09/17] can: length: can_fd_len2dlc(): simplify length calculcation

[Vincent MAILHOL <mailhol.vincent@wanadoo.fr>]

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.


Yours sincerely,
Vincent

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