Upstream this driver

[pfink-christ]

We would like to get this driver into mainline linux kernel. Has anybody already done work in that direction? @ericevenchick or somebody else?
I think that it won't be accepted as a new driver as it is right now, because it's largely a copy of what's already there in gs_usb.c.

In my opinion the best way would be to adapt the upstream gs_usb.c in a way that makes it work with the original gs_usb devices and autodetect the newer fd devices. Opinions, thoughts?

I also saw @marckleinebudde and @hartkopp around here having likely the most experience with mainline patch submitting....what do you think? Would you keep it a separate driver?

We also have some improvements ready like @moechr shared the errata bug at linklayer/cantact#19

This would solve #1, too.

[brainstorm]

We also have some improvements ready like @moechr shared the errata bug at linklayer/cantact#19

Those "improvements" are workarounds, the right place to fix that bug is in https://github.com/linklayer/cantact-pro-fw instead, not on gs_usb, IMO. It'd be nice to see FD support upstreamed, that's for sure.

[moechr]

the right place to fix that bug is in https://github.com/linklayer/cantact-pro-fw instead

Well, NXPs solution for this errata is to send NAK. If the driver is still sending messages in the format of the length 4 + N*16 and there is no change in the firmware the problem would still exist.
We made the decision as a reason of performance to add 1 byte to the host frame instead of sending a NAK transfer without any informational data.

There are other things to even worry more about in the firmware. Take a closer look at the can.c RAM assignment. One RX FIFO element uses 64 bytes data + 8 bytes of additional RAM per element = 72 bytes
72 bytes * 16 elements = 0x480 bytes
The difference between CAN0_TX_BUFFER_OFS = 0x20 and CAN1_STD_FILTER_OFS = 0x100 is only 0x80 ...

[brainstorm]

Ah, I see, thanks for explaining @moechr, much appreciated!

I've been quite disappointed by the overall CANtact "Pro" quality lately. I guess what upsets me the most is @ericevenchick not showing up for support after the @crowdsupply campaign (/cc @storborg @lifton?)... despite multiple bugs piling up for userspace tooling, documentation and firmware sides for the affected cantact repositories.

At this point I wouldn't recommend this dongle for any production use until the firmware, tooling and docs don't see thorough testing, bug squashing and benchmarking so that folks know the actual limits of this thing... it's definitely not worth 140eur at this point in time, IMO.

[moechr]

@brainstorm I totally agree with you. Simple "loop-tests" as a verification of the performance of a CAN coupler are way to less.
The big ones like PEAK Systems perform a whole bunch of tests on their CAN products.

There is a lot to to on the Cantact Pro project. As @ericevenchick mentioned - his CAN products are for hacking - maybe the hacking starts on the product itself ;)

[BennyEvans]

@pfink-christ as discussed, I've moved our discussion here (responding to your comment in #3 )

The candlelight firmware support is already there so why not add further support for a candlelight version with FD. I don't see a problem with that. The alternative would be huge code duplication or splitting the driver in a gs_usb_common.c and two separate versions of gs_usb.c /gs_usb_fd.c where only the differences are handled. That's another possible solution I have to give some additional thought, because the current status is not perfect, still hacky and not quite mainline quality yet (also comfirmed by hartkopp

Yep, this makes sense, I agree. As for the hack, I assume you're talking about u8 errata_dummy; added to the gs_host_frame struct to fix the NXP errata issue. Bit of a funny story - when first starting to develop my CAN bus hardware I had chosen the LPC54628. After a month or so I got to the point of stress testing the USB and quickly found this issue. I spent a few weeks trying to fix it but after that issue and a few others in the errata I ended up changing to an STM32H7. Sucks to see people creating similar devices running into the same issues. I'll go back over my notes and see where I got to with that issue as it'd be much nicer to solve the issue in the device firmware rather than the gs_usb driver.

What's this candlelight group you both talked about? I don't think there is actually a group in the sense that they sit together and discuss what to do next. I guess there is a maintainer, a small number of contributers and a bunch of users. But let's find out and ask. I guess that might be the main repository? -> https://github.com/candle-usb/candleLight_fw So Hubert Denkmair seems to be the initial author and fenugrec seems to currently maintain the repository.
We could add a comment here that bit 8 is used as FD feature flag or something like that, then it would be documented and nobody could complain -> https://github.com/candle-usb/candleLight_fw/blob/e6b972441b767b8952d9cd8c78cd74a43be091a9/src/usbd_gs_can.c#L267

Yeah spot on, I believe https://github.com/candle-usb/candleLight_fw is the main repository. I'll try to contact them over the next week or so in an attempt to get them involved in this. I think that as long as they're aware of it, we'll be fine. As far as discussion goes, can we open up a discussion board on git (https://docs.github.com/en/discussions/quickstart) even if that's in your repo? Or would you prefer to keep discussion here?

Above all testing is always welcome. But I think first priority should be to find another way of implementing the "switch" between classic can and FD. Because that is the one thing currently keeping me from sending the patches to ML for review and my time is rather limited at the moment.

I've got the same problem with time at the moment but will try to take a look at this over the weekend. I'll fork your repo, add my VID/PID, tidy a couple of things up and will submit a PR. Then we can discuss further on how to proceed.

Cheers

[marckleinebudde]

Yeah spot on, I believe https://github.com/candle-usb/candleLight_fw is the main repository. I'll try to contact them over the next week or so in an attempt to get them involved in this.

The easiest would be to create a PR where you add the various bits for CAN_FD support.

[pfink-christ]

Yep, this makes sense, I agree. As for the hack, I assume you're talking about u8 errata_dummy; added to the gs_host_frame struct to fix the NXP errata issue.

No, I'm talking about the length of the transmitted gs_host_frame. Because the data part has to have

• 8 data bytes in case of classic can device
• 64 data bytes in case of can fd device
• 65 (64 data bytes + 1 byte to circumvent data corruption on usb) in case of fd with the affected nxp mcu(s).

In my previous implementation the struct had the maximum size and only the necessary length was transmitted via usb. The struct was cut off so to speak. That was the hacky part where I wanted to find a better solution.

it'd be much nicer to solve the issue in the device firmware rather than the gs_usb driver.

USB high-speed device in endpoint TX data corruption on NXP LPC546xx controllers (Errata sheet LPC546xx / USB.15)
This corruption occurs when the following conditions are met:
-> A TX (IN) transfer happens after a RX (OUT) transfer.
-> The RX (OUT) transfer length is 4 + N*16 (N>=0) bytes.

IMHO not easily possible. The suggested workaround by NXP would require to add some usb rx/tx control and detection when to send a NAKed TX transfer. I don't think that's useful. Sending one byte more in this case isn't too hard on the performance, whereas NAKing a transfer and causing complete dummy transfers would sum up in a bigger total performance dip. At least that is how I see it in theory. As a benefit it would keep the complexity on the firmware and kernel driver side as it is.

I reworked the code and found a - in my eyes - quite good solution without changing too much.

@marckleinebudde Could you help me out on this macro here? Do I really need to supply data[0] to sizeof_field, which looks a bit weird here or is there another clever kernel macro that I missed to calculate the length of such a flex array? The compiler doesn't complain anymore, but I have no access to the hardware right now. I will test my reworked code on Friday, then I'll be able to share more.

struct gs_host_frame {
	u32 echo_id;
	__le32 can_id;

	u8 can_dlc;
	u8 channel;
	u8 flags;
	u8 reserved;

	DECLARE_FLEX_ARRAY(u8, data);
} __packed;

#define GS_HOSTFRAME_SIZE(data_len) (sizeof(struct gs_host_frame) + sizeof_field(struct gs_host_frame, data[0]) * data_len)

The easiest would be to create a PR where you add the various bits for CAN_FD support.

That was also my line of thinking. Should we also list/define the feature bits in the kernel driver even if not used? 6 and 7 are not used at the moment but are defined in the candlelight firmware. Currently I had them removed, but I guess it wouldn't hurt adding them back.

[pfink-christ]

after that issue and a few others in the errata

@BennyEvans are there any other errata relevant for us (NXP LPC... users) to additionally keep in mind or were they only relevant on your custom hardware? Because I don't think we are aware of any others.
And right now would be a good time to workaround all existing and known bugs and kinks.

[marckleinebudde]

That was also my line of thinking. Should we also list/define the feature bits in the kernel driver even if not used? 6 and 7 are not used at the moment but are defined in the candlelight firmware. Currently I had them removed, but I guess it wouldn't hurt adding them back.

I have already some patches that add these bits to the driver. Are you subscribed to the linux-can mailing list? I'll send these patches around.

[marckleinebudde]

@marckleinebudde Could you help me out on this macro here? Do I really need to supply data[0] to sizeof_field, which looks a bit weird here or is there another clever kernel macro that I missed to calculate the length of such a flex array? The compiler doesn't complain anymore, but I have no access to the hardware right now. I will test my reworked code on Friday, then I'll be able to share more.

Have a look at struct_size https://elixir.bootlin.com/linux/v5.15/source/include/linux/overflow.h#L192

[pfink-christ]

I have already some patches that add these bits to the driver. Are you subscribed to the linux-can mailing list? I'll send these patches around.

Yes, I am subscribed.

Have a look at struct_size https://elixir.bootlin.com/linux/v5.15/source/include/linux/overflow.h#L192

Thanks. That works for the instances where I have a pointer to gs_host_frame available, but not in gs_can_open, where gs_host_frame is not used, but only its size is relevant e.g. for allocation of usb rx buffer with usb_alloc_coherent. I could add a pointer to gs_host_frame, but it would only be used to calculate the size with struct_size.

old: #define GS_HOSTFRAME_SIZE(data_len) (sizeof(struct gs_host_frame) + sizeof_field(struct gs_host_frame, data[0]) * data_len)
new: #define GS_HOSTFRAME_SIZE(p, data_len) struct_size(p, data, data_len)
Apart from that the macro utilizing struct_size needs two parameters. I think I will rather discard GS_HOSTFRAME_SIZE macro altogether and use struct_size directly in every occurrence (7 in total), because it's saving only one parameter and not really improving readability.

[marckleinebudde]

Thanks. That works for the instances where I have a pointer to gs_host_frame available, but not in gs_can_open, where gs_host_frame is not used, but only its size is relevant e.g. for allocation of usb rx buffer with usb_alloc_coherent. I could add a pointer to gs_host_frame, but it would only be used to calculate the size with struct_size.

Make it so.

old: #define GS_HOSTFRAME_SIZE(data_len) (sizeof(struct gs_host_frame) + sizeof_field(struct gs_host_frame, data[0]) * data_len)
new: #define GS_HOSTFRAME_SIZE(p, data_len) struct_size(p, data, data_len)
Apart from that the macro utilizing struct_size needs two parameters. I think I will rather discard GS_HOSTFRAME_SIZE macro altogether and use struct_size directly in every occurrence (7 in total), because it's saving only one parameter and not really improving readability.

Sounds good.

[BennyEvans]

after that issue and a few others in the errata

@BennyEvans are there any other errata relevant for us (NXP LPC... users) to additionally keep in mind or were they only relevant on your custom hardware? Because I don't think we are aware of any others. And right now would be a good time to workaround all existing and known bugs and kinks.

I've just gone over my notes. Errata "3.22 USB.15: USB high-speed device in endpoint TX data corruption" is the only relevant issue that I'm aware of.

[BennyEvans]

@pfink-christ may you please provide a link to the repository you're working out of? I submitted a PR to the branch and repository I thought you were working out of but you then deleted the branch about 10 minutes after which closed the PR. Just want to all be on the same page here.