netdev - Re: [PATCH] can: flexcan: simplify the calculation of priv->mb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMZ6RqJ-O6PcwUrA9azJHq8vJ4_2GEFcqU-8_epHyAoBmeeEuA@mail.gmail.com>
Date: Mon, 4 Nov 2024 19:31:30 +0900
From: Vincent Mailhol <mailhol.vincent@...adoo.fr>
To: "baozhu.liu" <lucas.liu@...ngine.com>
Cc: mkl@...gutronix.de, wg@...ndegger.com, davem@...emloft.net, 
	edumazet@...gle.com, kuba@...nel.org, linux-can@...r.kernel.org, 
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH] can: flexcan: simplify the calculation of priv->mb_count

Hi Liu,

On Mon. 4 Nov. 2024 at 18:05, baozhu.liu <lucas.liu@...ngine.com> wrote:
> Since mb is a fixed-size two-dimensional array (u8 mb[2][512]),
> "priv->mb_count = sizeof(priv->regs->mb)/priv->mb_size;",
> this expression calculates mb_count correctly and is more concise.

When using integers,

  (a1 / q) + (a2 / q)

is not necessarily equal to

  (a1 + a2) / q.


If the decimal place of

  sizeof(priv->regs->mb[0]) / priv->mb_size

were to be greater than or equal to 0.5, the result would have changed
because of the rounding.

This is illustrated in https://godbolt.org/z/bfnhKcKPo.

Here, luckily enough, the two valid results are, for CAN CC:

  sizeof(priv->regs->mb[0]) / priv->mb_size = 512 / 16
                                            = 64

and for CAN FD:

  sizeof(priv->regs->mb[0]) / priv->mb_size = 512 / 72
                                            = 14.22

and both of these have no rounding issues.

I am not sure if we should take this patch. It is correct at the end
because you "won a coin flip", but the current code is doing the
correct logical calculation which would always yield the correct
result regardless of the rounding.

> Signed-off-by: baozhu.liu <lucas.liu@...ngine.com>
> ---
>  drivers/net/can/flexcan/flexcan-core.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c
> index 6d638c939..e3a8bad21 100644
> --- a/drivers/net/can/flexcan/flexcan-core.c
> +++ b/drivers/net/can/flexcan/flexcan-core.c
> @@ -1371,8 +1371,7 @@ static int flexcan_rx_offload_setup(struct net_device *dev)
>         if (priv->devtype_data.quirks & FLEXCAN_QUIRK_NR_MB_16)
>                 priv->mb_count = 16;
>         else
> -               priv->mb_count = (sizeof(priv->regs->mb[0]) / priv->mb_size) +
> -                                (sizeof(priv->regs->mb[1]) / priv->mb_size);
> +               priv->mb_count = sizeof(priv->regs->mb) / priv->mb_size;
>
>         if (priv->devtype_data.quirks & FLEXCAN_QUIRK_USE_RX_MAILBOX)
>                 priv->tx_mb_reserved =

Yours sincerely,
Vincent Mailhol