Merge latest version of gs_usb.c in

[AndreRenaud]

This should make the driver closer to mainline for upstreaming purposes

A better way to view this change is to get the latest checkout of linux mainline, and do a diff from drivers/net/can/usb/gs_usb.c to gs_usb_fd.c.

See here for that diff: https://gist.github.com/AndreRenaud/c2f3848eccbc8375d390f961b43673cf

[normaldotcom]

Awesome work! Do you have any thoughts on how to make this driver work with both FD and non-FD devices? Or do you think it's best to try and get it merged in as a separate driver?

To have a common driver, I think we would need an FD and a non-FD host frame that is used depending on the FD feature flag and whether a message is FD or classic CAN. This would save bit time on the wire as classic messages would not get sent over USB with a full FD-length host frame. Does this seem possible / does this seem like a good approach?

[AndreRenaud]

The driver already does a feature-detect phase where it will automatically enable/disable FD support if it finds the dongle supports it. So I think it should be possible to merge it into the existing gsusb driver with minimal changes.

I don't know about the host frame - I think if you're sending non-FD packets, then the last 56 bytes will just not be sent/received. It's a bit weird to have a structure that you're not fully sending, so maybe having two structures would make more sense (or at least make it more readable/explicit).

One concern is just that there is no GSUSB standard - so Cantact Pro has just said that bit 8 is the feature flag. But we might start getting incompatible with other dongles in the future, like the candlelightfw group?

To be honest, I evaluated the Cantact Pro for its FD support, but it was dropping packets, and I didn't get much feedback on the issue from Linklayer, so I've moved on to using a different (more expensive & proprietary) dongle. I'd love to switch back, but at the moment I don't think its likely.

[pfink-christ]

I don't know about the host frame - I think if you're sending non-FD packets, then the last 56 bytes will just not be sent/received. It's a bit weird to have a structure that you're not fully sending, so maybe having two structures would make more sense (or at least make it more readable/explicit).

In the current state of the driver the whole 64 byte array will be sent and classical candlelight firmware support is broken right now with this PR.
Please have a look at https://github.com/pfink-christ/net-next/tree/canfd-mainline-conservative-rc2 This is my current working state where candlelight, CANtact Pro and our hardware (CANext FD) are working.

This implementation works, but basically I also don't like the idea of sending only parts of the gs_host_frame, but all other options would require to rewrite big parts of the driver, because it is not so easy to use the two different structs in parallel. If you decide at runtime which struct you use, you can't access the struct elements by their names if you have some void pointer to one of two structs...or at least I'm currently not aware of another clever solution to this problem. Ideas welcome.

And submitting as a separate FD driver is no valid option IMHO, because the drivers are almost the same.

One concern is just that there is no GSUSB standard - so Cantact Pro has just said that bit 8 is the feature flag. But we might start getting incompatible with other dongles in the future, like the candlelightfw group?

I don't think that's a big issue, because if nothing is documented right now then it's free to use.

We also added another feature bit to request the nxp errata workaround. This would have to be backported to cantact pro, but I think that's the best solution for this bug.

[pfink-christ]

@AndreRenaud there are small differences in our versions, but would you mind if I add your singed-off-by tag to pfink-christ/net-next@c7daf3e as well?

[AndreRenaud]

@pfink-christ Yes, the changes you've got there look like the best minimal set to get just the FD feature merged in. As you say however, it does end up transferring the full new gs_host_frame, which will be 56 bytes larger for non-FD devices now.

[BennyEvans]

Hi all,

Great to see there is progress on this. I've also created hardware that currently leverages the GS_USB driver. I've been keeping a keen eye on this repository as a basis for interfacing with CAN FD. I've thought about creating my own driver but with so many existing drivers for CAN devices it's my opinion that we all need to decide on one.

One concern is just that there is no GSUSB standard - so Cantact Pro has just said that bit 8 is the feature flag. But we might start getting incompatible with other dongles in the future, like the candlelightfw group?

I don't think that's a big issue, because if nothing is documented right now then it's free to use.

I agree with this, however, I'm not sure everyone will see this the same way. As it was originally a driver for Geschwister Schneider CAN devices, it might be harder for us to push this driver to the mainline without having any connection to the original device the driver was created for. It may be easier for us to push this as a new driver that we can maintain/improve as a group without effecting the existing Geschwister Schneider driver.

I'd love to be able to make this driver not restricted to certain VID/PIDs too so that other people can create their own hardware and easily interface with this driver without performing kernel patches or creating rules to overwrite VID/PID. However, to my knowledge, there is no easy way of doing this (happy to be corrected though).

@pfink-christ I see that you've added your VID/PID to your version of the driver. Would you mind if I submit a PR to add mine too?

Happy to help out where I can on this and assist in getting this into mainline, even if it's just assisting with documentation. Is there a discussion board or group where we can all keep in touch and discuss our ideas/progress further?

Cheers

[pfink-christ]

Hi @BennyEvans,
thanks for joining our effort :)

I agree with this, however, I'm not sure everyone will see this the same way. As it was originally a driver for Geschwister Schneider CAN devices, it might be harder for us to push this driver to the mainline without having any connection to the original device the driver was created for. It may be easier for us to push this as a new driver that we can maintain/improve as a group without effecting the existing Geschwister Schneider driver.

1. Geschwister Schneider company is not existing anymore afaik, so I do not expect any complaints from their side....besides that's not the way the linux kernel community works. Everybody is allowed to contribute if it makes sense and doesn't break anything else (by purpose). Right now I don't see anything breaking by using an unused feature bit.
2. 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 here -> Module won't compile on 5.13.13 #1 (comment)).
3. 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

I'd love to be able to make this driver not restricted to certain VID/PIDs too so that other people can create their own hardware and easily interface with this driver without performing kernel patches or creating rules to overwrite VID/PID. However, to my knowledge, there is no easy way of doing this (happy to be corrected though).

There is no way around it. That's the way usb driver recognition works...look at other drivers, there are tens and more VID/PID combinations for compatible devices - e.g. here is one I know of -> https://github.com/torvalds/linux/blob/136057256686de39cc3a07c2e39ef6bc43003ff6/drivers/net/usb/asix_devices.c#L1261

Happy to help out where I can on this and assist in getting this into mainline, even if it's just assisting with documentation. Is there a discussion board or group where we can all keep in touch and discuss our ideas/progress further?

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.

Could we move further mainlining discussions to -> #2?