On Wed. 1 Feb 2023 at 01:04, Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> 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.
