Re: [PATCH v2 1/1] Documentation: networking: document ISO 15765-2:2016

Oliver Hartkopp
2024-04-14T20:22:00Z

On 14.04.24 06:03, Vincent Mailhol wrote:

>
> 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.
>

;-)

>>> 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
>

Yes, I've checked the release notes on the ISO website too.
This really looks like editorial stuff that has nothing to do with the
data protocol and its segmentation.

>>>
>>> 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.

Oh, sorry. Maybe I expressed myself wrong.

IMHO your way to work with an initializer is correct from the C standpoint.

But I think this is pretty unusual for a code example when an
application programmer starts to work with ISO-TP.

You usually get most of these values from the command line an fill the
struct _by hand_ - and not with a static initialization.

That was my suggestion.

>
> ** 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?
>

The if_index derives from struct netdevice.if_index

https://elixir.bootlin.com/linux/v6.8.6/source/include/linux/netdevice.h#L2158

which is an int.

I don't think this would have an effect in real world to change
sockaddr_can.can_ifindex to an unsigned int.

I wonder if it is more critical to existing user space code to change it
to unsigned int or to leave it as-is ...

Best regards,
Oliver