On Mon. 12 Sept. 2022 at 14:56, <Thomas.Kopp@xxxxxxxxxxxxx> wrote:
> > > In can_calc_bittiming() there are several open coded checks to ensure
> > > that SJW is within certain limits. Replace this by a single call to
> > > min3().
> > >
> > > Signed-off-by: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>
> > > ---
> > > drivers/net/can/dev/calc_bittiming.c | 8 ++------
> > > 1 file changed, 2 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/net/can/dev/calc_bittiming.c
> > b/drivers/net/can/dev/calc_bittiming.c
> > > index d3caa040614d..ce6bef2444a2 100644
> > > --- a/drivers/net/can/dev/calc_bittiming.c
> > > +++ b/drivers/net/can/dev/calc_bittiming.c
> > > @@ -158,12 +158,8 @@ int can_calc_bittiming(const struct net_device
> > *dev, struct can_bittiming *bt,
> > > if (!bt->sjw || !btc->sjw_max) {
> > > bt->sjw = 1;
> > > } else {
> > > - /* bt->sjw is at least 1 -> sanitize upper bound to sjw_max */
> > > - if (bt->sjw > btc->sjw_max)
> > > - bt->sjw = btc->sjw_max;
> > > - /* bt->sjw must not be higher than tseg2 */
> > > - if (tseg2 < bt->sjw)
> > > - bt->sjw = tseg2;
> > > + /* sjw must not be higher than sjw_max and tseg2 */
> > > + bt->sjw = min3(bt->sjw, btc->sjw_max, tseg2);
> >
> > 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.
Yours sincerely,
Vincent Mailhol
