| Message ID | 20241104084705.5005-1-lucas.liu@siengine.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | can: flexcan: simplify the calculation of priv->mb_count | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Series ignored based on subject |
Hi Liu, On Mon. 4 Nov. 2024 at 18:05, baozhu.liu <lucas.liu@siengine.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@siengine.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
On 04.11.2024 19:31:30, Vincent Mailhol wrote: > On Mon. 4 Nov. 2024 at 18:05, baozhu.liu <lucas.liu@siengine.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. Wow, that's an elaborative answer. Thanks Vincent! And yes the current code does the correct logical calculation because of the underlying restrictions of the hardware. A CAN-CC/FD frame cannot cross the boundary between the 2 memory areas. If you want to improve something, feel free to add a comment explaining the reasoning for the existing calculation. regards, Marc
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 =
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. Signed-off-by: baozhu.liu <lucas.liu@siengine.com> --- drivers/net/can/flexcan/flexcan-core.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)