Re: [RFC PATCH v5 3/5] can: dev: add CAN XL support

[Vincent Mailhol <vincent.mailhol@gmail.com>]

On Thu. 21 juil. 2022 at 11:37, Vincent Mailhol
<vincent.mailhol@gmail.com> wrote:
> On Wed. 21 Jul. 2022 at 01:43, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> > On 19.07.22 17:16, Vincent Mailhol wrote:
> > > On Tue 19 Jul. 2022 at 23:38, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> > >> No confusion.
> > >>
> > >> The API to the user space is 'truncated' option only.
> > >>
> > >> The data structure inside the kernel sizeof(struct can[|fd|xl]_frame).
> > >>
> > >> See:
> > >> https://lore.kernel.org/linux-can/4c79233f-1301-d5c7-7698-38d39d8702aa@hartkopp.net/
> > >>
> > >> ---8<---
> > >>
> > >> As the sk_buffs are only allocated once and are not copied inside the
> > >> kernel there should be no remarkable drawbacks whether we allocate
> > >> CAN_MTU skbs or CANXL_MTU skbs.
> > >>
> > >> AFAICS both skbs will fit into a single page.
> > >
> > > This is true. A page is at least 4k. So the 2k + alpha will easily fit.
> > > But the page is not the smallest chunk that can return malloc, c.f.
> > > KMALLOC_MIN_SIZE:
> > > https://elixir.bootlin.com/linux/latest/source/include/linux/slab.h#L279
> > >
> > > Also, more than the page size, my concern are the cache misses,
> > > especially when memsetting to zero the full canxl frame. As far as I
> > > understand, cloning an skb does not copy the payload (only increment a
> > > reference to it) so the echo_skb thing should not be impacted.
> > > That said, I am not able to tell whether or not this will have a
> > > noticeable impact (I have some feeling it might but can not assert
> > > it). If this looks good for the people who have the expertise in
> > > kernel performances, then I am fine.
> >
> > The more I think about our discussion and your remark that we were
> > somehow going back to the V2 patch set the more I wonder if this would
> > be a good idea.
>
> I quite liked v2. My comments on the v2 were mostly to argue on the
> data[CANXL_MAX_DLEN] vs the flexible member array, but aside from
> that, it looked pretty good.
>
> > IMO using the struct canxl_frame (including 2048 byte) and allowing
> > truncated sizes can be handled inside the kernel safely.
> >
> > And with V2 we only allocate the needed size for the sk_buff - without
> > any memsetting.
> >
> > When user space gets a truncated frame -> fine
> >
> > When the user forges some CAN XL frame where the length value does not
> > match the spec and the size does not fit the length -> -EINVAL
> >
> > So there is no uninitialized data also.
>
> So basically, forcing the truncation everywhere (TX, RX both userland
> and kernel), correct? i.e. the skb length shall always be equal to
> CANXL_HEADER_SIZE + canxl_frame::len.
>
> I think this is good. As I stated before, getting -EINVAL is benign.
> If developers are doing crazy things because they did not read the
> doc, it is better to fail them early. If we go for truncation then
> always truncating is the safest: less option -> less confusion.
>
> > And even the user space side to handle a mix of CAN frame types is
> > pretty simple IMO:
> >
> > union {
> >          struct can_frame cc;
> >          struct canfd_frame fd;
> >          struct canxl_frame xl;
> > } can;
>
> Do you want to add this union in the kernel uapi or is it just a
> userland example?

More brainstorming. If we want to introduce a generic can structure in
linux/can.h, we could  do:

struct canxl_frame {
        canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
        __u8    xl_flags; /* additional flags for CAN XL */
        __u8    fd_flags; /* CAN(-FD) flags */
        __u16   len;   /* frame payload length in byte */
        __u32   af;    /* acceptance field */
        __u8    sdt;   /* SDU (service data unit) type */
        __u8    __res0;  /* reserved / padding */
        __u8    __res1;  /* reserved / padding */
        __u8    __res2;  /* reserved / padding */
        __u8    data[CANXL_MAX_DLEN] __attribute__((aligned(8)));
};

union can_generic_frame {
         struct {
                union {
                       canid_t can_id;
                       canid_t prio;
                };
                union {
                        __u16 type;
                         struct {
                                __u8 xl_flags;
                                __u8 fd_flags;
                        } __attribute__((packed));
                } __attribute__((packed));
         };
         struct can_frame cc;
         struct canfd_frame fd;
         struct canxl_frame xl;
};

#define CANXL_XLF 0x80 /* apply to canxl_frame::xl_flags */

#define CAN_TYPE_CC 0
#ifdef __LITTLE_ENDIAN
#define CAN_TYPE_FD (CANFD_FDF << 8)
#define CAN_TYPE_XL (CANXL_XLF)
#else /* __BIG_ENDIAN */
#define CAN_TYPE_FD (CANFD_FDF)
#define CAN_TYPE_XL (CANXL_XLF << 8)
#endif

#define CAN_TYPE_MASK (CAN_TYPE_FD | CAN_TYPE_XL)

Because can_generic_frame::type overlaps with the can(fd)_frame::len,
it will contain garbage and thus it is necessary to mask it with
CAN_TYPE_MASK.
The CANFD_FDF is only set in the rx path. In the tx path it is simply
ignored. This done, we can use it as below when *receiving* can
frames:

int foo()
{
  union can_generic_frame can;

  /* receive a frame */

  switch (can.type & CAN_TYPE_MASK) {
  case CAN_TYPE_CC:
    printf("Received classical CAN Frame\n");
    break;

  case CAN_TYPE_FD:
    printf("Received CAN FD Frame\n");
    break;

  case CAN_TYPE_XL:
    printf("Received CAN XL Frame\n");
    break;

  default:
    fprintf(stderr, "Unknown type: %x\n", can.type & CAN_TYPE_MASK);
  }

  return EXIT_SUCCESS;
}


Yours sincerely,
Vincent Mailhol