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 v3] canxl: add virtual CAN network identifier support
Date: Sun, 21 Jan 2024 11:13:23 +0100 [thread overview]
Message-ID: <2c9f7052-c2bb-4d76-bf58-714103fc6d71@hartkopp.net> (raw)
In-Reply-To: <4a80e73c-0c53-406f-bb81-0ea552480b74@hartkopp.net>
Hi Vincent,
On 2024-01-08 09:26, Oliver Hartkopp wrote:
> On 08.01.24 06:46, Vincent MAILHOL wrote:
>
>> I have not yet reviewed the net/can/raw.c part. It will take me more
>> time. I will do it later (we are the first day of the merge window, so
>> that leaves me time!)
>
> The raw.c changes have become very easy to review since the introduction
> of the vcid element - no hurry ;-)
Some time has already passed now. Did you find some time to take a look
at the v4 patch?
https://lore.kernel.org/linux-can/20240108170644.1887-1-socketcan@hartkopp.net/
Best regards,
Oliver
>>
>> Question: do you think this should also go to stable?
>>
>
> I would have no objections, but I'm not sure whether we can convince the
> stable maintainers to change the UAPI and slightly extend the ABI in
> stable kernels? Let's see.
>
>> More a matter of principle, but you should
>>
>> #include <asm/byteorder.h>
>>
>> c.f.
>> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ip.h#L22
>
> Yes. I already prepared this change for the next patch cycle yesterday:
>
> https://github.com/hartkopp/canxl-utils/commit/9ac1b75b59ccbf668fec4bd5c846542ad0073da0
>
>>> @@ -193,26 +193,39 @@ struct canfd_frame {
>>> #define CANXL_XLF 0x80 /* mandatory CAN XL frame flag (must always
>>> be set!) */
>>> #define CANXL_SEC 0x01 /* Simple Extended Content
>>> (security/segmentation) */
>>>
>>> /**
>>> * struct canxl_frame - CAN with e'X'tended frame 'L'ength frame
>>> structure
>>> - * @prio: 11 bit arbitration priority with zero'ed CAN_*_FLAG flags
>>> - * @flags: additional flags for CAN XL
>>> - * @sdt: SDU (service data unit) type
>>> - * @len: frame payload length in byte (CANXL_MIN_DLEN ..
>>> CANXL_MAX_DLEN)
>>> - * @af: acceptance field
>>> - * @data: CAN XL frame payload (CANXL_MIN_DLEN .. CANXL_MAX_DLEN byte)
>>> + * @prio: 11 bit arbitration priority with zero'ed CAN_*_FLAG flags
>>
>> Now that prio is a __u16, the CAN_*_FLAG flags fall outside prio width
>> (in __res0 as you mentioned below). So I would rather remove the
>> latter part of this sentence. You may instead specify that bits
>> outside of the CANXL_PRIO_MASK shall be zeroed.
>
> Right. Will change that.
>
>>> + * @vcid: virtual CAN network identifier
>>
>> Maybe say that this field is valid only if enabled through
>> CAN_RAW_XL_VCID_OPTS, else should be zero.
>
> No. This is a data structure definition and not the CAN_RAW socket
> documentation. But it has definitely to be mentioned somewhere.
>
>>> - * @prio shares the same position as @can_id from struct can[fd]_frame.
>>> + * @prio shares the lower 16 bits of @can_id from struct can[fd]_frame.
>>> + * @__res0 holds the @can_id flags CAN_xxx_FLAG and has to be set to
>>> zero
>>
>> Just realized now, but I would have personally preferred to merge
>> those two sentences within their respective field documentation. Just
>> feels odd to me to have the information on the prio and __res0 field
>> are scattered in two different locations. Well this is just a nitpick,
>> at the end, just choose which one seems good to you.
>
> Ok. I always tried to get only one line for the element description. But
> this could be changed. No problem.
>
>>> +struct can_raw_vcid_options {
>>> +
>>> + __u8 flags; /* flags for vcid (filter) behaviour */
>>> + __u8 tx_vcid; /* VCID value set into
>>> canxl_frame.prio */
>>> + __u8 rx_vcid; /* VCID value for VCID filter */
>>> + __u8 rx_vcid_mask; /* VCID mask for VCID filter */
>>> +
>>
>> Why the newlines before and after the struct members? This style is
>> not consistent with the other CAN headers.
>
> Ok. Will remove them.
>
>>> /* check for correct padding to be able to use the structs
>>> similarly */
>>> BUILD_BUG_ON(offsetof(struct can_frame, len) !=
>>> offsetof(struct canfd_frame, len) ||
>>> + offsetof(struct can_frame, len) !=
>>> + offsetof(struct canxl_frame, flags) ||
>>> offsetof(struct can_frame, data) !=
>>> offsetof(struct canfd_frame, data));
>>
>> Nitpick: I would expect to see the code structured in this order:
>> check Classical CAN first, then CAN-FD and finally CAN-XL. Not sure
>> why the CAN-XL is in the middle. If the only reason is to minimize the
>> patch diff, then no.
>
> The reason is to check the offset to struct can_frame.len - so all these
> checks are sorted by the first element to be checked.
>
> Thanks for the feedback!
>
> Oliver
>
prev parent reply other threads:[~2024-01-21 10:16 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-07 17:14 [PATCH v3] canxl: add virtual CAN network identifier support Oliver Hartkopp
2024-01-08 5:46 ` Vincent MAILHOL
2024-01-08 8:26 ` Oliver Hartkopp
2024-01-21 10:13 ` 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=2c9f7052-c2bb-4d76-bf58-714103fc6d71@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).