From: Oliver Hartkopp <socketcan@hartkopp.net>
To: Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
Cc: linux-can@vger.kernel.org, mkl@pengutronix.de
Subject: Re: [PATCH] canxl: add virtual CAN network identifier support
Date: Sun, 7 Jan 2024 11:21:05 +0100 [thread overview]
Message-ID: <3c16cab8-b1ff-4091-9eb3-28c5051309e7@hartkopp.net> (raw)
In-Reply-To: <CAMZ6Rq+S7mUWBXQEm2uHTKt31Z5JBj1LK2WNpN7pwujx5DhSzw@mail.gmail.com>
Hi Vincent,
thanks for your review!
On 07.01.24 07:28, Vincent MAILHOL wrote:
> On Sun. 7 Jan. 2024 at 04:47, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>> values can be send, e.g. to replay full qualified CAN XL traffic.
> ^^^^
> sent
ACK
>> provided by the CAN_RAW sockets and kernel infrastruture.
> ^^^^^^^^^^^^^
> infrastructure
ACK
>> struct canxl_frame {
>> - canid_t prio; /* 11 bit priority for arbitration (canid_t) */
>> + canid_t prio; /* 11 bit priority for arbitration / 8 bit VCID */
>
> Isn't this a UAPI breaking change? Prior to this patch, the applications may do:
>
> canxl_frame.prio
>
> to get the prio, but after this patch, applications are required to do:
>
> canxl_frame.prio & CANXL_PRIO_MASK
>
Not really. I also thought about it but you *only* need to take care
about the VCID content when you have enabled it explicitly with the new
sockopt. Otherwise you will never see anything beyond the 32 bit prio.
> in order to mask out the VCID (currently, there are no requirements
> that canxl_frame.prio must be masked before use).
> In the past, I was reluctant to acknowledge the introduction of CANXL
> in the kernel prior to reading the ISO standard because I was afraid
> of such UAPI stability issues. Now we have to deal with it.
Yes, but that kind of extension would be backwards compatible.
> What do you think of:
>
> struct canxl_frame {
> #if defined(__LITTLE_ENDIAN)
> __u16 prio; /* 11 bit priority for arbitration */
> __u8 vcid; /* 8 bit VCID */
> __u8 __reserved; /* must be 0 */
> /* ... */
> #elif defined(__BIG_ENDIAN)
> __u8 __reserved; /* must be 0 */
> __u8 vcid; /* 8 bit VCID */
> __u16 prio; /* 11 bit priority for arbitration */
> #else
> #error "Unknown endianness"
> #endif
> }
>
> Here, canxl_frame.prio always gives a correct value without need for
> CANXL_PRIO_MASK masking. The big/little endianness checks are needed
> to maintain the ABI compatibility. Not yet tested, so forgive if there
> is a mistake. Getting the endianness logic correct on a first try is
> not easy.
Yes, I tested such approach too (with little endian only) and it worked
great - and of course looked better in the code.
> Also, the VCID can now be accessed through canxl_frame.vcid instead of
> relying on some mask and shift logic.
Right. That looked nice.
> The drawback is that you lose the can_id type. For what I understand,
> this is only used for filtering. If we absolutely need to maintain the
> canid_t, then maybe:
>
> struct canxl_frame {
> union {
> canid_t filter;
> struct {
> #if defined(__LITTLE_ENDIAN)
> __u16 prio; /* 11 bit priority for arbitration */
> __u8 vcid; /* 8 bit VCID */
> __u8 __reserved; /* must be 0 */
> #elif defined(__BIG_ENDIAN)
> __u8 __reserved; /* must be 0 */
> __u8 vcid; /* 8 bit VCID */
> __u16 prio; /* 11 bit priority for arbitration */
> #else
> #error "Unknown endianness"
> #endif
> };
> };
> /* ... */
> }
>
> But I think it is better to drop it. If someone wants a canid_t, then
> he or she can just cast the XL frame to either struct can_frame or
> struct canfd_frame.
>
> Though?
My only concern is that it looks really ugly :-/
The change of the prio element from u32 to u16 will also not harm anyone
as I assume to be the only person who's currently working with CAN XL
frames on virtual CAN interfaces:
https://github.com/hartkopp?tab=repositories&q=can-cia
I'll prepare a patch that picks up this suggestion of an __u16 prio etc.
Maybe we can add some compile time checks to ensure the correct struct
layout for this case.
>
>> __u8 flags; /* additional flags for CAN XL */
>
> If CANXL_VCID is set, can vcid be zero? If not, no need for a flag.
> Just need to check if canxl_frame.vcid is not zero.
>
This is probably indeed a leftover which can be removed with my latest
implementation. Will recheck.
Many thanks,
Oliver
prev parent reply other threads:[~2024-01-07 10:24 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-06 19:28 [PATCH] canxl: add virtual CAN network identifier support Oliver Hartkopp
2024-01-06 22:03 ` Oliver Hartkopp
2024-01-07 6:28 ` Vincent MAILHOL
2024-01-07 10:21 ` Oliver Hartkopp [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3c16cab8-b1ff-4091-9eb3-28c5051309e7@hartkopp.net \
--to=socketcan@hartkopp.net \
--cc=linux-can@vger.kernel.org \
--cc=mailhol.vincent@wanadoo.fr \
--cc=mkl@pengutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).