From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 27E35C43334 for ; Tue, 12 Jul 2022 10:19:43 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232380AbiGLKTl (ORCPT ); Tue, 12 Jul 2022 06:19:41 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:43236 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232569AbiGLKTj (ORCPT ); Tue, 12 Jul 2022 06:19:39 -0400 Received: from mail-yw1-x1131.google.com (mail-yw1-x1131.google.com [IPv6:2607:f8b0:4864:20::1131]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 06996ACF7A for ; Tue, 12 Jul 2022 03:19:37 -0700 (PDT) Received: by mail-yw1-x1131.google.com with SMTP id 00721157ae682-2ef5380669cso75841477b3.9 for ; Tue, 12 Jul 2022 03:19:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=IsJny6EX/4e+Vc3HZZJPeSer/Zt74IHS/k4vu9aiE1s=; b=Ln2NtWsXKTBexsHc4TqeS+MFsdLnCh2/ai/X4DlW9g+IhxuyNRsd+lEUoYrX6yo+Ky kmByNffxOzZMUUwbLS4//oDSVCmpdAnlLIqA488pT3h+RcJUGSJa1wf1XegrwRInYjnN cWSOVg9Bcq/1SPfwkL2V6E/2ZtxSBMd0nIVJZ/2typhMQkguo38/WE675AMYl2qoCft3 6h2miIuj10XxDVhx/vSiiZDDDFro4QGirHdfrLn96veiAFtN5E8xvoLpvsqlKFF8oY5B B7zw8RpIl+8hnUb+OgC6yCjiuHILUsrQ9pfZTSou8ZAx52vcAKf4fp+Wv+VG0HIlUOnu 82UQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=IsJny6EX/4e+Vc3HZZJPeSer/Zt74IHS/k4vu9aiE1s=; b=wy3YQQGJE40UR1pjKKKwRhz7OyLgb8u7U1wtR7fQEITk9oID5j9JjSlhAzfJSRuw3q b5nz7nuU8+LuGMak+LF4z1trnbrcVLEzmJxkL6c901mApLpbCmOyra7r/kulMVs/io52 Caw4wuARPNlEy9F1ZHYyzlF3YeCpoovmpEGFrJByk5gFIlb3pQqQZE3d5k30JCwVUZCe io8gx/PsCz5ZbeomEKul6I7ROhk4kp2dYZrq+1kC4xJMslMy6Y0vCHDrjTI4RUrDrruF lurfjV7cFmCPl+Udlp6xfKmdn4LAKPfbnL+uisBdYXQaCbw2UYWSizuBjT4imHcdoGOn Ccxg== X-Gm-Message-State: AJIora9iwACC6WtEHFe2xuMM46HH1ZJz6FrxV3TSfneD4dqynudmQYDm DfgLJrZUlVsW2fG83cPc/YvJqWG/qXC5vRxGCf4XhArgENM= X-Google-Smtp-Source: AGRyM1sRdh1C+1PoNWyWImdaLecAGIr9OuchliadTXuBpByDtPGd/wr6+p32OwLLnK4/co7Xm691VFqfC82oLblWGoQ= X-Received: by 2002:a0d:fd05:0:b0:31c:ad0f:331d with SMTP id n5-20020a0dfd05000000b0031cad0f331dmr24901013ywf.220.1657621175955; Tue, 12 Jul 2022 03:19:35 -0700 (PDT) MIME-Version: 1.0 References: <20220711183426.96446-1-socketcan@hartkopp.net> <20220711183426.96446-2-socketcan@hartkopp.net> <521fe0a3-a9ad-60ac-3ec6-30f0da228032@hartkopp.net> In-Reply-To: <521fe0a3-a9ad-60ac-3ec6-30f0da228032@hartkopp.net> From: Vincent Mailhol Date: Tue, 12 Jul 2022 19:19:24 +0900 Message-ID: Subject: Re: [RFC PATCH 1/5] can: canxl: introduce CAN XL data structure To: Oliver Hartkopp Cc: linux-can@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: linux-can@vger.kernel.org On Tue. 12 Jul. 2022 at 18:31, Oliver Hartkopp wro= te: > On 12.07.22 10:40, Vincent Mailhol wrote: > > On Tue. 12 juil. 2022 at 16:55, Oliver Hartkopp wrote: > >> On 12.07.22 02:36, Vincent Mailhol wrote: > >>> On Tue. 12 Jul. 2022 at 03:44, Oliver Hartkopp wrote: > >>>> > >>>> This patch adds defines for data structures and length information f= or > >>>> CAN XL (CAN with eXtended data Length) which can transfer up to 2048 > >>>> byte insinde a single frame. > >>>> > >>>> Notable changes from CAN FD: > >>>> > >>>> - the 11 bit arbitration field is now named 'priority' instead of 'c= an_id' > >>>> (there are no 29 bit identifiers nor RTR frames anymore) > >>>> - the data length needs a uint16 value to cover up to 2048 byte > >>>> (the length element position is different to struct can[fd]_fram= e) > >>>> - new fields (SDT, AF) and a SEC bit have been introduced > >>>> - the virtual CAN interface identifier is not part if the CAN XL fra= me > >>>> struct as this VCID value is stored in struct skbuff (analog to = vlan id) > >>>> > >>>> Signed-off-by: Oliver Hartkopp > >>>> --- > >>>> include/uapi/linux/can.h | 38 +++++++++++++++++++++++++++++++++++= +++ > >>>> 1 file changed, 38 insertions(+) > >>>> > >>>> diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h > >>>> index 90801ada2bbe..9f97a5d06f3b 100644 > >>>> --- a/include/uapi/linux/can.h > >>>> +++ b/include/uapi/linux/can.h > >>>> @@ -58,10 +58,11 @@ > >>>> > >>>> /* valid bits in CAN ID for frame formats */ > >>>> #define CAN_SFF_MASK 0x000007FFU /* standard frame format (SFF) *= / > >>>> #define CAN_EFF_MASK 0x1FFFFFFFU /* extended frame format (EFF) *= / > >>>> #define CAN_ERR_MASK 0x1FFFFFFFU /* omit EFF, RTR, ERR flags */ > >>>> +#define CANXL_PRIO_MASK CAN_SFF_MASK /* 11 bit priority mask */ > >>>> > >>>> /* > >>>> * Controller Area Network Identifier structure > >>>> * > >>>> * bit 0-28 : CAN identifier (11/29 bit) > >>>> @@ -71,10 +72,11 @@ > >>>> */ > >>>> typedef __u32 canid_t; > >>>> > >>>> #define CAN_SFF_ID_BITS 11 > >>>> #define CAN_EFF_ID_BITS 29 > >>>> +#define CANXL_PRIO_BITS CAN_SFF_ID_BITS > >>>> > >>>> /* > >>>> * Controller Area Network Error Message Frame Mask structure > >>>> * > >>>> * bit 0-28 : error class mask (see include/uapi/linux/can/err= or.h) > >>>> @@ -89,10 +91,18 @@ typedef __u32 can_err_mask_t; > >>>> > >>>> /* CAN FD payload length and DLC definitions according to ISO 118= 98-7 */ > >>>> #define CANFD_MAX_DLC 15 > >>>> #define CANFD_MAX_DLEN 64 > >>>> > >>>> +/* > >>>> + * CAN XL payload length and DLC definitions according to ISO 11898= -1 > >>>> + * CAN XL DLC ranges from 0 .. 2047 =3D> data length from 1 .. 2048= byte > >>>> + */ > >>>> +#define CANXL_MAX_DLC 2047 > >>>> +#define CANXL_MAX_DLC_MASK 0x07FF > >>>> +#define CANXL_MAX_DLEN 2048 > >>>> + > >>>> /** > >>>> * struct can_frame - Classical CAN frame structure (aka CAN 2.0B= ) > >>>> * @can_id: CAN ID of the frame and CAN_*_FLAG flags, see canid= _t definition > >>>> * @len: CAN frame payload length in byte (0 .. 8) > >>>> * @can_dlc: deprecated name for CAN frame payload length in byt= e (0 .. 8) > >>>> @@ -141,14 +151,20 @@ struct can_frame { > >>>> * When this is done the former differentiation via CAN_MTU / CAN= FD_MTU gets > >>>> * lost. CANFD_FDF allows programmers to mark CAN FD frames in th= e case of > >>>> * using struct canfd_frame for mixed CAN / CAN FD content (dual = use). > >>>> * N.B. the Kernel APIs do NOT provide mixed CAN / CAN FD content= inside of > >>>> * struct canfd_frame therefore the CANFD_FDF flag is disregarded= by Linux. > >>>> + * Same applies to the CANXL_XLF bit. > >>>> + * > >>>> + * For CAN XL the SEC bit has been added to the flags field which s= hares the > >>>> + * same position in struct can[fd|xl]_frame. > >>>> */ > >>>> #define CANFD_BRS 0x01 /* bit rate switch (second bitrate for pay= load data) */ > >>>> #define CANFD_ESI 0x02 /* error state indicator of the transmitti= ng node */ > >>>> #define CANFD_FDF 0x04 /* mark CAN FD for dual use of struct canf= d_frame */ > >>>> +#define CANXL_XLF 0x08 /* mark CAN XL for dual use of struct canfd_= frame */ > >>>> +#define CANXL_SEC 0x10 /* Simple Extended Content (security/segment= ation) */ > >>>> > >>>> /** > >>>> * struct canfd_frame - CAN flexible data rate frame structure > >>>> * @can_id: CAN ID of the frame and CAN_*_FLAG flags, see canid_t= definition > >>>> * @len: frame payload length in byte (0 .. CANFD_MAX_DLEN) > >>>> @@ -164,12 +180,34 @@ struct canfd_frame { > >>>> __u8 __res0; /* reserved / padding */ > >>>> __u8 __res1; /* reserved / padding */ > >>>> __u8 data[CANFD_MAX_DLEN] __attribute__((aligned(8))); > >>>> }; > >>>> > >>>> +/** > >>>> + * struct canxl_frame - CAN with e'X'tended frame 'L'ength frame st= ructure > >>>> + * @prio: 11 bit arbitration priority with zero'ed CAN_*_FLAG flag= s > >>>> + * @sdt: SDU (service data unit) type > >>>> + * @flags: additional flags for CAN XL > >>>> + * @len: frame payload length in byte (1 .. CANXL_MAX_DLEN) > >>>> + * @af: acceptance field > >>>> + * @data: CAN XL frame payload (up to CANXL_MAX_DLEN byte) > >>>> + * > >>>> + * @prio shares the same position as @can_id from struct canfd_fram= e. > >>>> + * Same applies to the relative position and length of @flags. > >>>> + */ > >>>> +struct canxl_frame { > >>>> + canid_t prio; /* 11 bit priority for arbitration (canid_t) = */ > >>>> + __u8 sdt; /* SDU (service data unit) type */ > >>>> + __u8 flags; /* additional flags for CAN XL */ > >>>> + __u16 len; /* frame payload length in byte */ > >>>> + __u32 af; /* acceptance field */ > >>>> + __u8 data[CANXL_MAX_DLEN]; > >>> > >>> __u8 data[]; > >>> > >>> 2 kilobytes start to be a significant size. Would it make sense to us= e > >>> a flexible array member to minimize the copies? A bit like the TCP/IP > >>> stack where you do not have to allocate the MTU but just what is > >>> actually needed for the headers and your payload. > >>> > >>> Of course this is a tradeoff. It will add some complexity. The first > >>> impact is that the skb's data length might be smaller than the MTU an= d > >>> thus any logic using the MTU to differentiate between Classic CAN, > >>> CAN-FD or CAN XL would have to be adjusted. > >> > >> Yes, I've thought about that myself but I wanted a simple start for ou= r > >> discussion to think about improvements in the team. > >> > >> I implemented this first: > >> > >> /* Drop a given socketbuffer if it does not contain a valid CAN fra= me. */ > >> bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff= *skb) > >> { > >> - const struct canfd_frame *cfd =3D (struct canfd_frame *)skb->d= ata; > >> + unsigned int len =3D can_get_data_len(skb); > > > > It is premature to use can_get_data_len() here. You have not yet > > confirmed the skb=E2=80=99s length so this could be an out of band read= . > > > >> struct can_priv *priv =3D netdev_priv(dev); > >> > >> if (skb->protocol =3D=3D htons(ETH_P_CAN)) { > >> if (unlikely(skb->len !=3D CAN_MTU || > >> - cfd->len > CAN_MAX_DLEN)) > >> + len > CAN_MAX_DLEN)) > >> goto inval_skb; > >> } else if (skb->protocol =3D=3D htons(ETH_P_CANFD)) { > >> if (unlikely(skb->len !=3D CANFD_MTU || > >> - cfd->len > CANFD_MAX_DLEN)) > >> + len > CANFD_MAX_DLEN)) > >> + goto inval_skb; > >> + } else if (skb->protocol =3D=3D htons(ETH_P_CANXL)) { > >> + if (unlikely(skb->len < CANXL_MINTU || > >> + skb->len > CANXL_MTU || > >> + len > CANXL_MAX_DLEN || len =3D=3D 0)) > >> goto inval_skb; > >> } else { > >> goto inval_skb; > >> } > > > > I suggest this: > > > > /* Drop a given socket buffer if it does not contain a valid CAN fra= me. */ > > bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff = *skb) > > { > > const struct canfd_frame *cfd =3D (struct canfd_frame *)skb->= data; > > + const struct canxl_frame *cxl =3D (struct canxl_frame *)skb->d= ata; > > struct can_priv *priv =3D netdev_priv(dev); > > > > if (skb->protocol =3D=3D htons(ETH_P_CAN)) { > > if (unlikely(skb->len !=3D CAN_MTU || > > cfd->len > CAN_MAX_DLEN)) > > goto inval_skb; > > } else if (skb->protocol =3D=3D htons(ETH_P_CANFD)) { > > if (unlikely(skb->len !=3D CANFD_MTU || > > cfd->len > CANFD_MAX_DLEN)) > > goto inval_skb; > > + } else if (skb->protocol =3D=3D htons(ETH_P_CANXL)) { > > + if (unlikely(skb->len < sizeof(struct canxl_frame) || > > + skb->len > CANXL_MTU || > > + cxl->len > CANXL_MAX_DLEN || cxl->len =3D= =3D 0)) > > + goto inval_skb; > > } else { > > goto inval_skb; > > } > > > > Once can_dropped_invalid_skb() succeeds, calls to can_get_data_len() > > will be safe. > > Agreed. Will change that. > > > > >> +/* truncated CAN XL structs must contain at least 64 data bytes */ > >> +#define CANXL_MINTU (CANXL_MTU - CANXL_MAX_DLEN + CANFD_MAX_DLEN) > > > > I did not get the concept of the "truncated CAN XL structs". The valid > > data field lengths are 1 to 2048, right? I did not get where this 64 > > comes from. > > Your formula is equivalent to > > #define CANXL_MINTU (sizeof(struct canxl_frame) + CANFD_MAX_DLEN) > > No. CANXL_MINTU becomes sizeof(struct canfd_frame) + sizeof(af) > > So I wanted some size value that is 'more than' CANFD_MTU so that you > know that you have read a CANXL frame. > > Even if the cxf->len would be 14 you would at least copy a struct > canxl_frame with data[64]. OK, I finally got your point. Your concern is that if skb->len could be equal or less than CANFD_MTU, then there would be a collision. My approach here would be to stop using the MTU correlation to differentiate between CAN(-FD) and CANXL. Instead, I suggest using can{fd,xl}_frame::flags. If can{fd,xl}_frame has a CANXL flag set, then it is a CANXL frame regardless of the value of skb->len. If the CANXL flag is not set, then skb->len is used to differentiate between Classic CAN and CAN FD (so that we remain compatible with the existing). That way, no need to impose a minimum length of CANFD_MAX_DLEN. > > > > I would have just expected: > > #define CANXL_MINTU (sizeof(struct canxl_frame)) > > That is CANXL_MTU (max transfer unit). I was writing while thinking that canxl_frame::data was a flexible array member as suggested in this thread. In that case canxl_frame::data counts as zero when doing sizeof(struct canxl_frame). And so sizeof(struct canxl_frame) =3D=3D sizeof(struct canfd_frame) + sizeof(af). Actually, thinking twice, the Minimum transfer unit would be: #define CANXL_MINLEN 1 #define CANXL_MINTU (sizeof(struct canxl_frame) + CANXL_MINLEN) (I forgot that the length can not be zero anymore in CANXL...) > > Or maybe no macro at all, the sizeof is more explicit to me. > > > >> So the idea was to define a CAN XL skb->len which is clearly above > >> CANFD_MTU to distinguish it from the other CAN MTUs. > >> > >> But as the skbuff is zerocopy inside the kernel, > > > > Inside the kernel yes, but there is still one copy between userspace > > and kernel land with the full width. > > Yes. I hope the explanation above made it clearer now. > > > > >> it probably makes sense > >> to stay with the full CANXL_MTU inside the kernel and allow to crop th= e > >> data structure for CAN_RAW socket interactions from/to user space down > >> to CANXL_MINTU ?!? > > > > My guts would tell me to crop it from the initial malloc in userland. > > Not sure what would be the impact in terms of performances. > > > >>> Also, are we fine to drop the __attribute__((aligned(8)))? If I > >>> understand correctly, we moved from a 8 bytes alignment in struct > >>> can(fd)_frame to a 4 bytes alignment in struct canxl_frame. > >> > >> Yes. I hassled with the alignment too. > >> > >> The big cool thing about the 64 bit alignment was the filter and > >> modification efficiency in bcm.c and gw.c > >> > >> I wonder if this is still a relevant use case with CAN XL. > >> > >> Currently the SDU type SDT=3D0x03 defines a Classical CAN and CAN FD > >> 'tunneling' for CAN XL (in CiA 611-1 document). > >> > >> For this SDT=3D0x03 the CAN ID (and EFF/RTR/FD flags) are placed in th= e AF > >> element. > >> > >> And then the first data[0] byte will contain ESI/BRS/DLC and starting > >> with data[1] the CAN frame data content will start. > >> > >> So at least this spec will horribly break and 64 bit access to CAN dat= a > >> content. > >> > >> I've been thinking about creating a 'normal' Classical CAN / CAN FD > >> virtual CAN interface that feels for the user like a standard CAN > >> interface with struct can[fd]_frame - but inside interacts with CAN XL > >> frames with SDT=3D0x03 ... > > > > Here, you lost me. The only reference document I have is the Bosch > > presentation you linked in the cover letter. I would need to get a > > copy of the specification first to follow up on this level of details. > > There is a Special Interest Group for CAN XL at CAN in Automation > (can-cia.org) and these doscuments are currently under development. I wonder how hard it is to join the group. Right now, I was thinking of waiting for the ISO Final Draft International Standard (FDIS) to deep dive in CANXL. > With the current approach SDT=3D3 to 'tunnel' CAN/CANFD frames the aligne= d > access to data[] into the struct canxl_frame is at least not possible. > > >> Don't know if users really will need such stuff with CAN XL as there a= re > >> other PDU tunneling mechanics already specified. > >> > >> For that reason I would not take the 64 bit alignment as a strong > >> requirement. With the current struct canxl_frame layout the data[] wil= l > >> start at a 32 bit boundary. > > > > ACK. The 32 bit alignment is totally acceptable I think. In the worst > > case, on 64 bits architecture, when the payload is a perfect multiple > > of 64 bits, we might lose a couple of assembly instructions but I > > think that would be acceptable. > > +1 > > Best, > Oliver > > > > >> At the end I would see CAN XL as some Ethernet implementation with a > >> cool arbitration concept from CAN that assures CSMA/C[AR] instead of > >> CSMA/CD ;-) > >> > >> Best regards, > >> Oliver > >> > >>> > >>>> > >>>> +}; > >>>> + > >>>> #define CAN_MTU (sizeof(struct can_frame)) > >>>> #define CANFD_MTU (sizeof(struct canfd_frame)) > >>>> +#define CANXL_MTU (sizeof(struct canxl_frame)) > >>> > >>> #define CANXL_MTU (sizeof(struct canxl_frame) + CANXL_MAX_DLEN) > >>> > >>>> /* particular protocols of the protocol family PF_CAN */ > >>>> #define CAN_RAW 1 /* RAW sockets */ > >>>> #define CAN_BCM 2 /* Broadcast Manager */ > >>>> #define CAN_TP16 3 /* VAG Transport Protocol v1.6 */ > >>> > >>> > >>> Yours sincerely, > >>> Vincent Mailhol