Message-ID: <CAMZ6RqJkzLh2Qf1gWo5oZ2XvTKGeeZREWu79Q4zGTdZ3Vv_mkA@mail.gmail.com>
Date:   Fri, 20 Aug 2021 15:12:27 +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 23:17, Vincent MAILHOL
<mailhol.vincent@...adoo.fr> wrote:
> 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.

I just realized something. If the user first sets the TDC
parameters and then does another command without any data
bittiming parameters provided, then the TDC parameters will be
recalculated but the other data bittiming parameters would remain
unchanged.

Example:
$ ip link set can0 type can bitrate 1000000 dbitrate 8000000 fd on
tdcv 33 tdco 16 tdc-mode manual
$ ip link set can0 type can bitrate 500000

Here, can_calc_tdco() will be triggered resulting in a switch to
TDC_AUTO mode.
In this scenario, it is not normal to only have the TDC
recalculated but not the other data bittiming parameters. Is it
what you were trying to explain when saying "do not touch"?

I am preparing a new series with below behavior:

* data bittiming not provided: TDC parameters unchanged
* data bittiming provided: (unchanged from current behavior)
    - tdc-mode not provided: do can_calc_tdco (fully automated)
    - tdc-mode auto and tdco provided: TDC_AUTO
    - tdc-mode manual and both of tdcv and tdco provided: TDC_MANUAL

N.B. TDC parameters must be provided together with data bittiming
parameters, e.g. data bittiming not provided + TDC parameters is
an invalid command.

Does that make more sense?


Yours sincerely,
Vinent

Powered by blists - more mailing lists