Re: [PATCH 2/5] can: bittiming: can_calc_bittiming(): make use of min3()

[Vincent Mailhol <vincent.mailhol@gmail.com>]

On Wed. 1 Feb 2023 at 01:04, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> Finally I've found some time to look at this again...
>
> On 12.09.2022 17:28:15, Vincent Mailhol wrote:
> > Also, I was assuming that can_fixup_bittiming() was already doing the
> > out of range check:
> > https://elixir.bootlin.com/linux/v6.0-rc1/source/drivers/net/can/dev/bittiming.c#L27
> >
> > But in reality, only one of either can_calc_bittiming(),
> > can_fixup_bittiming() or can_validate_bitrate() is being called. And
> > thus, can_validate_bitrate() might be called with out of range values
> > and in that case the neltink interface should return -ERANGE (for
> > example if sjw > sjw_max).
> >
> > Sees that there is more work to do here than initially anticipated.
>
> I've converted the existing netdev_err() NL_SET_ERR_MSG_FMT(). This
> means the error message is transported via netlink to user space and
> printed by the "ip" tool.

I was not aware of this NL_SET_ERR_MSG_FMT() thing, and let me say
that I really like that solution!

> | # ip link set flexcan0 txqueuelen 10 type can bitrate 2200000
> | Warning: bitrate error 2.5%.

The Linux coding style says:

  Kernel messages do not have to be terminated with a period.

Ref: https://www.kernel.org/doc/html/latest/process/coding-style.html#printing-kernel-messages

However, I am not sure if this also applies to the ip tool.

> |
> | # ip link set flexcan0 txqueuelen 10 type can bitrate 22000000
> | Error: bitrate error 80.5% too high.
>
> This is the error message for the SJW check:
>
> | # ip link set flexcan0 txqueuelen 10 type can bitrate 500000 sjw 3
> | Error: SJW 3 bigger than phase_seg1 6 and/or phase_seg2 2.

At that point in the code, I assume that sjw was already validated
against sjw_max. While not impossible, I think that having sjw bigger
than both phase_seg1 and phase_seg2 at the same time is uncommon. I
suggest to split the error message in two and only print the relevant
one:

  Error: SJW 3 bigger than phase_seg1 x
  Error: SJW 3 bigger than phase_seg2 x

If the user still manages to violate both, only the phase_seg1 error
message is displayed.

> Maybe add a '=' between the phase_seg and the actual number:
>
> | Error: SJW 3 bigger than phase_seg1=6 and/or phase_seg2=2.

If you ask me my preferences, I will go with column:

  Error: SJW: 3 bigger than phase_seg1: 6 and/or phase_seg2: 2.

But I will not complain if you pick anything else.