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

[Vincent Mailhol]

Hi Liu,

On Mon. 4 Nov. 2024 at 18:05, baozhu.liu <lucas.liu@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> ---
>  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