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

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 2292 bytes --]

On 12.09.2022 17:28:15, Vincent Mailhol wrote:
> > > Not directly a criticism of this patch (as things were already like
> > > that), but if the user provides an incorrect value for SJW (or any
> > > other bittiming argument), wouldn't it be better to inform? Returning
> > > -EINVAL might be too violent. Maybe a dmesg would be good?
> >
> > I'd say it would be consistent to keep returning -EINVAL (at least for the SJW>(min p1,p2)) condition.
> >
> > The same is done for FD Bitrate < Arbitration bitrate and both conditions have the same level of justification in the ISO 11898-1:2015
> > "The data bit time shall have the same length as the nominal bit time or it shall be shorter than the nominal bit time."
> >
> > "In nominal bit time and in data bit time, SJW shall be less than or equal to the minimum of these two items: Phase_Seg1 and Phase_Seg2."
> >
> > Shall is a binding requirement to be ISO conformant in this case.
> 
> What you say makes sense.
> 
> 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.

If you configure the bitrate + sjw there will be a check for sjw within
the limits (< sjw_max, < min(phase_seg1, phase_seg2)).

If you configure the low level timing parameters there will be limit
checks on the user supplied phase_seg1, phase_seg2 and sjw and a limit
check on the calculated brp.

What should be the return value if these checks fail?

I think it's best to always return -EINVAL and set an error message with
NL_SET_ERR_MSG_FMT().

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]