On Sun. 14 Apr. 2024 at 02:29, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> 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@xxxxxxxxx> wrote: > >> Document basic concepts, APIs and behaviour of the ISO 15675-2:2016 > >> (ISO-TP) CAN stack. > >> > >> Signed-off-by: Francesco Valla <valla.francesco@xxxxxxxxx> > >> --- > >> 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
