Message-ID: <64586257-3cf6-4c10-a30b-200b1ecc5e80@hartkopp.net>
Date: Sun, 14 Apr 2024 22:21:33 +0200
From: Oliver Hartkopp <socketcan@...tkopp.net>
To: Vincent Mailhol <vincent.mailhol@...il.com>
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 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

Powered by blists - more mailing lists