Message-ID: <CAMZ6RqKGKcYd4hAM8AVV72t78H-Kt92NXowx6Q+YCw=AuSxKuw@mail.gmail.com>
Date: Sun, 14 Apr 2024 13:03:33 +0900
From: Vincent Mailhol <vincent.mailhol@...il.com>
To: Oliver Hartkopp <socketcan@...tkopp.net>
Cc: Francesco Valla <valla.francesco@...il.com>, Marc Kleine-Budde <mkl@...gutronix.de>, 
	linux-can@...r.kernel.org, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Simon Horman <horms@...nel.org>, 
	Bagas Sanjaya <bagasdotme@...il.com>, fabio@...aril.me
Subject: Re: [PATCH v2 1/1] Documentation: networking: document ISO 15765-2:2016

On Sun. 14 Apr. 2024 at 02:29, Oliver Hartkopp <socketcan@...tkopp.net> wrote:
> Hi Vincent,
>
> On 13.04.24 15:11, Vincent Mailhol wrote:
> > Hi Francesco,
> >
> > Thank you for the ISO-TP documentation.
> >
> > I left a few comments, but overall, good work. Also, I did not double
> > check each individual option one by one.
> >
> > On Sat. 30 Mar 2024 at 00:06, Francesco Valla <valla.francesco@...il.com> wrote:
> >> Document basic concepts, APIs and behaviour of the ISO 15675-2:2016
> >> (ISO-TP) CAN stack.
> >>
> >> Signed-off-by: Francesco Valla <valla.francesco@...il.com>
> >> ---
> >>   Documentation/networking/index.rst      |   1 +
> >>   Documentation/networking/iso15765-2.rst | 356 ++++++++++++++++++++++++
> >>   MAINTAINERS                             |   1 +
> >>   3 files changed, 358 insertions(+)
> >>   create mode 100644 Documentation/networking/iso15765-2.rst
> >>
> >> diff --git a/Documentation/networking/index.rst b/Documentation/networking/index.rst
> >> index 473d72c36d61..bbd9bf537793 100644
> >> --- a/Documentation/networking/index.rst
> >> +++ b/Documentation/networking/index.rst
> >> @@ -19,6 +19,7 @@ Contents:
> >>      caif/index
> >>      ethtool-netlink
> >>      ieee802154
> >> +   iso15765-2
> >>      j1939
> >>      kapi
> >>      msg_zerocopy
> >> diff --git a/Documentation/networking/iso15765-2.rst b/Documentation/networking/iso15765-2.rst
> >> new file mode 100644
> >> index 000000000000..bbed4d2ef1a8
> >> --- /dev/null
> >> +++ b/Documentation/networking/iso15765-2.rst
> >> @@ -0,0 +1,356 @@
> >> +.. SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
> >> +
> >> +=========================
> >> +ISO 15765-2:2016 (ISO-TP)
> >> +=========================
> >> +
> >> +Overview
> >> +========
> >> +
> >> +ISO 15765-2:2016, also known as ISO-TP, is a transport protocol specifically
> >> +defined for diagnostic communication on CAN. It is widely used in the automotive
> >> +industry, for example as the transport protocol for UDSonCAN (ISO 14229-3) or
> >> +emission-related diagnostic services (ISO 15031-5).
> >> +
> >> +ISO-TP can be used both on CAN CC (aka Classical CAN, CAN 2.0B) and CAN FD (CAN
> >
> > CC is already the abbreviation of *C*lassical *C*AN. Saying CAN CC, is
> > like saying CAN Classical CAN, c.f. the RAS syndrome:
> >
> >    https://en.wikipedia.org/wiki/RAS_syndrome
> >
> > Then, considering the CAN2.0B, as far as I know, ISO-TP can also be
> > used on CAN2.0A (as long as you only use 11 bits CAN ids).
>
> The suggestion "be used both on CAN CC (aka Classical CAN, CAN 2.0B) and
> CAN FD" was from my side.
>
> And this follows the new CAN in Automation (can-cia.org) naming scheme.
> E.g. https://www.can-cia.org/can-knowledge/can/cybersecurity-for-can/
> "“Safety and security” specifies generic security options for CAN CC and
> CAN FD protocols."
>
> So your hint to the RAS syndrome is right but in this case the this is
> intentional to be able to reference CAN CC/FD/XL content.
>
> For that reason I wanted to introduce the new CAN CC naming scheme which
> is pretty handy IMO.

I double checked. ISO 11898-1:2015 and ISO 15765-2:2016 never use the
"CC" abbreviation a single time, thus my confusion. *BUT* ISO
15765-2:2024 actually uses that naming, in the exact same way that CAN
in Automation does.

This doesn't remove the fact that I think that this naming convention
is stupid because of the RAS syndrome, but I acknowledge that CAN CC
is now the official denomination and thus, that we should adopt it in
our documentation as well.

> > So, I would rather just say:
> >
> >    ISO-TP can be used both on Classical CAN and CAN FD...
> >

(...)

> >> +    NOTE: this is not covered by the ISO15765-2:2016 standard.
> >                                          ^^^^^^^^
> > Add a space between ISO and the number. Also, update the year:
> >
> >    ISO 15765-2:2024
> >
>
> Interesting! Didn't know there's already a new version.
>
> Will check this out whether we really support ISO 15765-2:2024 ...
>
> Do you have the standard at hand right now or should we leave this as
> ISO15765-2:2016 until we know?

I have access to the newer revisions. But I never really invested time
into reading that standard (neither the 2016 nor the 2024 versions).

Regardless, here is a verbatim extract from the Foreworld section of
ISO 15765-2:2024

  This fourth edition cancels and replaces the third edition (ISO
  15765-2:2016), which has been technically revised.

  The main changes are as follows:

    - restructured the document to achieve compatibility with OSI
      7-layers model;

    - introduced T_Data abstract service primitive interface to
      achieve compatibility with ISO 14229-2;

    - moved all transport layer protocol-related information to Clause 9;

    - clarification and editorial corrections

> >> +  - ``CAN_ISOTP_DYN_FC_PARMS``: enable dynamic update of flow control
> >> +    parameters.

(...)

> >
> > Here, I would suggest the C99 designated field initialization:
> >
> >    struct sockaddr_can addr = {
> >            .can_family = AF_CAN;
> >            .can_ifindex = if_nametoindex("can0");
> >            .tp.tx_id = 0x18DA42F1 | CAN_EFF_FLAG;
> >            .tp.rx_id = 0x18DAF142 | CAN_EFF_FLAG;
> >    };

Typo in my previous message: the designated initializers are not
separated by colon ";" but by comma ",". So it should have been:

  struct sockaddr_can addr = {
        .can_family = AF_CAN,
        .can_ifindex = if_nametoindex("can0"),
        .tp.tx_id = 0x18DA42F1 | CAN_EFF_FLAG,
        .tp.rx_id = 0x18DAF142 | CAN_EFF_FLAG,
  };

> > Well, this is just a suggestion, feel free to reject it if you do not like it.
>
> At least I don't like it.
>
> These values are usually interactively given on the command line:
>
>  >            .can_ifindex = if_nametoindex("can0");
>  >            .tp.tx_id = 0x18DA42F1 | CAN_EFF_FLAG;
>  >            .tp.rx_id = 0x18DAF142 | CAN_EFF_FLAG;
>
> So have it in a static field initialization leads to a wrong path IMO.

There is no such limitation that C99 designated initializers should
only work with variables which have static storage duration. In my
suggested example, nothing is static.

I see this as the same thing as below example:

  int foo(void);

  int bar()
  {
          int i = foo();
  }

  int baz()
  {
          int i;

          i = foo();
  }

In bar(), the fact that the variable i is initialized at declaration
does not mean that it is static. In both examples, the variable i uses
automatic storage duration.

Here, my preference goes to bar(), but I recognize that baz() is also
perfectly fine. Replace the int type by the struct sockaddr_can type
and the scalar initialization by designated initializers and you
should see the connection.


** Different topic **

While replying on this, I encountered something which made me worry a bit:

The type of sockaddr_can.can_ifindex is a signed int:

  https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/can.h#L243

But if_nametoindex() returns an unsigned int:

   https://man7.org/linux/man-pages/man3/if_nametoindex.3.html

Shouldn't sockaddr_can.can_ifindex also be declared as an unsigned int?

> >
> >> +  int ret;
> >> +
> >> +  s = socket(PF_CAN, SOCK_DGRAM, CAN_ISOTP);
> >> +  if (s < 0)
> >> +      exit(1);
> >> +
> >> +  addr.can_family = AF_CAN;
> >> +  addr.can_ifindex = if_nametoindex("can0");
> >
> > if_nametoindex() may fail. Because you are doing error handling in
> > this example, do it also here:
> >
> >    if (!addr.can_ifindex)
> >            err("if_nametoindex()");
> >
>
> This is not really needed for an example like this.
>
> When we have a problem here the bind() syscall with fail with -ENODEV

Ack.

> >> +  addr.tp.tx_id = (0x18DA42F1 | CAN_EFF_FLAG);
> >> +  addr.tp.rx_id = (0x18DAF142 | CAN_EFF_FLAG);
> >
> > Nitpick: the bracket are not needed here:
> >
> >    addr.tp.tx_id = 0x18DA42F1 | CAN_EFF_FLAG;
> >    addr.tp.rx_id = 0x18DAF142 | CAN_EFF_FLAG;
> >
> >> +
> >> +  ret = bind(s, (struct sockaddr *)&addr, sizeof(addr));
> >> +  if (ret < 0)
> >> +      exit(1);
> >> +
> >> +  // Data can now be received using read(s, ...) and sent using write(s, ...)
> >
> > Kernel style prefers C style comments over C++. I think that should
> > also apply to the documentation:
> >
> >    /* Data can now be received using read(s, ...) and sent using write(s, ...) */
> >
>
> ACK
>
> >> +
> >> +Additional examples
> >> +-------------------
> >> +
> >> +More complete (and complex) examples can be found inside the ``isotp*`` userland
> >> +tools, distributed as part of the ``can-utils`` utilities at:
> >> +https://github.com/linux-can/can-utils
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index 6a233e1a3cf2..e0190b90d1a8 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -4695,6 +4695,7 @@ W:        https://github.com/linux-can
> >>   T:     git git://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can.git
> >>   T:     git git://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git
> >>   F:     Documentation/networking/can.rst
> >> +F:     Documentation/networking/iso15765-2.rst
> >>   F:     include/linux/can/can-ml.h
> >>   F:     include/linux/can/core.h
> >>   F:     include/linux/can/skb.h
> >> --
> >> 2.44.0
> >>
> >>
> >
>
> Thanks for the review, Vincent!

You are welcome!

Yours sincerely,
Vincent Mailhol

Powered by blists - more mailing lists