Message-ID: <CAMZ6Rq+H4u9D41Fdx+J-kj35g3GVRqoYvDiHtR3LGMXfRjcsiA@mail.gmail.com>
Date:   Wed, 18 Aug 2021 23:17:16 +0900
From:   Vincent MAILHOL <mailhol.vincent@...adoo.fr>
To:     Marc Kleine-Budde <mkl@...gutronix.de>
Cc:     linux-can <linux-can@...r.kernel.org>,
        Stefan Mätje <Stefan.Maetje@....eu>,
        netdev <netdev@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 2/7] can: bittiming: allow TDC{V,O} to be zero and add can_tdc_const::tdc{v,o,f}_min

On Wed. 18 Aug 2021 at 21:29, Marc Kleine-Budde <mkl@...gutronix.de> wrote:
> On 18.08.2021 18:22:33, Vincent MAILHOL wrote:
> > > Backwards compatibility using an old ip tool on a new kernel/driver must
> > > work.
> >
> > I am not trying to argue against backward compatibility :)
> > My comment was just to point out that I had other intents as well.
> >
> > > In case of the mcp251xfd the tdc mode must be activated and tdcv
> > > set to the automatic calculated value and tdco automatically measured.
> >
> > Sorry but I am not sure if I will follow you. Here, do you mean
> > that "nothing" should do the "fully automated" calculation?
>
> Sort of.
> The use case is the old ip tool with a driver that supports tdc, for
> CAN-FD to work it must be configured in fully automated mode.

The current patch does that: "nothing" means that both TDC_AUTO
and TDC_MANUAL are not set, same as what an old ip tool would
do. And that triggers the default (fully automated) mode (call
can_calc_tdco()).

> > In your previous message, you said:
> >
> > > Does it make sense to let "mode auto" without a tdco value switch the
> > > controller into full automatic mode and /* nothing */ not tough the tdc
> > > config at all?
> >
> > So, you would like this behavior:
> >
> > | mode auto, no tdco provided -> kernel decides between TDC_AUTO and TDC off.
>
> NACK - mode auto, no tdco -> TDC_AUTO with tdco calculated by the kernel

Currently, the tdco calculation is paired with the decision to
enable or not TDC. If dbrp is one or two, then do the tdco
calculation, else, TDC is off (c.f. can_calc_tdco()). This
behaviour is to follow ISO 11898-1 which states that TDC is only
applicable if data BRP is one or two. In the current patch I
allow to have TDC enabled with a dbrp greater than 2 only if the
tdco is provided by the user (i.e. I allow the user to forcefully
go against ISO but the automatic calculation guarantees
compliance).

So what do you suggest to do when drbp is greater than 2? Still
enable TDC (and violate ISO standard) or return an error
code (e.g. -ENOTSUPP)?

> > | mode auto, tdco provided -> TDC_AUTO
>
> ACK - TDC_AUTO with user supplied tdco
>
> > | mode manual, tdcv and tdco provided -> TDC_MANUAL
>
> ACK - TDC_MANUAL with tdco and tdcv user provided
>
> > | mode off is not needed anymore (redundant with "nothing")
> > (TDCF left out of the picture intentionally)
>
> NACK - TDC is switched off

Same as the current patch then :)

> > | "nothing" -> TDC is off (not touch the tdc config at all)
>
> NACK - do not touch TDC setting, use previous setting

Sorry but I still fail to understand your definition of "do not
touch".

The first time you start a device, all the structures are zeroed
out meaning that TDC is off to begin with.  So the first time the
user do something like:

| ip link set can0 type can bitrate 1000000 dbitrate 8000000 fd on

If you "do not touch" TDC it means that all TDC values stays at
zero, i.e. TDC stays off. This would contradict point 1/.

> > Correct?
>
> See above. Plus a change that addresses your issue 1/ from below.
>
> If driver supports TDC it should be initially brought into TDC auto
> mode, if no TDC mode is given. Maybe we need an explizit TDC off to make
> that work.
>
> > If you do so, I see three issues:
> >
> > 1/ Some of the drivers already implement TDC. Those will
> > automatically do a calculation as long as FD is on. If "nothing"
> > now brings TDC off, some users will find themselves with some
> > error on the bus after the iproute2 update if they continue using
> > the same command.
>
> Nothing would mean "do not touch" and as TDC auto is default a new ip
> would work out of the box. Old ip will work, too. Just failing to decode
> TDC_AUTO...

See above: if you "do not touch", my understanding is that the
old ip tool will indefinitely keep TDC to its initial value:
everything zeroed out.

To turn TDC auto, you will eventually call can_calc_tdco() and
that will touch something.

> > 2/ Users will need to read and understand how to use the TDC
> > parameters of iproute2. And by experience, too many people just
> > don't read the doc. If I can make the interface transparent and
> > do the correct thing by default ("nothing"), I prefer to do so.
>
> ACK, see above
>
> > 3/ Final one is more of a nitpick. The mode auto might result in
> > TDC being off. If we have a TDC_AUTO flag, I would expect the
> > auto mode to always set that flag (unless error occurs). I see
> > this to be slightly counter intuitive (I recognize that my
> > solution also has some aspects which are not intuitive, I just
> > want to point here that none are perfect).
>
> What are the corner cases where TDC_AUTO results in TDC off?

dbrp greater than 2 (see above).

> > To be honest, I really preferred the v1 of this series where
> > there were no tdc-mode {auto,manual,off} and where the "off"
> > behavior was controlled by setting TDCO to zero. However, as we
> > realized, zero is a valid value and thus, I had to add all this
> > complexity just to allow that damn zero value.
>
> Maybe we should not put the TDC mode _not_ into ctrl-mode, but info a
> dedicated tdc-mode (which is not bit-field) inside the struct tdc?

If you do so, then you would need both a tdcmode and a
tdcmode_supported in order for the device to announce which modes
it supports (same as the ctrlmode and ctrlmode_supported in
can_priv). I seriously thought about this option but it seemed
like reinventing the wheel for me.

Also, it needs to be bit field to differentiate between a device
which would only support manual mode, one device which would only
support auto mode and one device which would support both.


Yours sincerely,
Vincent

Powered by blists - more mailing lists