Module won't compile on 5.13.13

[normaldotcom]

Looks like the CAN interface changed a bit--a couple macros have disappeared and some function prototypes changed.

Snippet of compile output:

/home/ethanzonca/Projects/gs_usb_fd/gs_usb_fd.c: In function ‘gs_usb_receive_bulk_callback’:
/home/ethanzonca/Projects/gs_usb_fd/gs_usb_fd.c:342:36: error: implicit declaration of function ‘can_dlc2len’; did you mean ‘can_fd_dlc2len’? [-Werror=implicit-function-declaration]
342 | cfd->len = can_dlc2len(hf->can_dlc);
| ^~~~~~~~~~~
| can_fd_dlc2len
/home/ethanzonca/Projects/gs_usb_fd/gs_usb_fd.c:355:39: error: implicit declaration of function ‘get_can_dlc’ [-Werror=implicit-function-declaration]
355 | cf->can_dlc = get_can_dlc(hf->can_dlc);
| ^~~~~~~~~~~
/home/ethanzonca/Projects/gs_usb_fd/gs_usb_fd.c:388:17: error: too few arguments to function ‘can_get_echo_skb’
388 | can_get_echo_skb(netdev, hf->echo_id);
| ^~~~~~~~~~~~~~~~
In file included from ./include/linux/can/dev.h:23,
from /home/ethanzonca/Projects/gs_usb_fd/gs_usb_fd.c:19:

[hartkopp]

Looks like the CAN interface changed a bit--a couple macros have disappeared and some function prototypes changed.

Yes. Not only macros have been reworked. There has also been added a functionality to retrieve raw DLC values from Classical CAN controllers (aka len8_dlc).

So the macro improvements have to been adapted and it should be checked if the gs_usb_fd controller supports len8_dlc for Classical CAN.

FYI the mainline gs_usb.c driver supports it:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/net/can/usb/gs_usb.c?id=4c01fc87675e6974d42383eba9a043123d8e13c3

[AndreRenaud]

With fairly minor changes, I've managed to get this driver to compile & work nicely under 5.11.0:

diff --git a/gs_usb_fd.c b/gs_usb_fd.c
index bb69477..9b768ed 100644
--- a/gs_usb_fd.c
+++ b/gs_usb_fd.c
@@ -14,6 +14,7 @@
 #include <linux/module.h>
 #include <linux/netdevice.h>
 #include <linux/usb.h>
+#include <linux/ethtool.h>

 #include <linux/can.h>
 #include <linux/can/dev.h>
@@ -339,7 +340,7 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 				return;

 			cfd->can_id = hf->can_id;
-			cfd->len = can_dlc2len(hf->can_dlc);
+			cfd->len = can_fd_dlc2len(hf->can_dlc);
 			if (hf->flags & GS_CAN_FLAG_BRS) {
 				cfd->flags |= CANFD_BRS;
 			}
@@ -352,7 +353,7 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 			if (!skb)
 				return;
 			cf->can_id = hf->can_id;
-			cf->can_dlc = get_can_dlc(hf->can_dlc);
+			cf->can_dlc = can_cc_dlc2len(hf->can_dlc);
 			memcpy(cf->data, hf->data, 8);

 			/* ERROR frames tell us information about the controller */
@@ -566,7 +567,7 @@ static netdev_tx_t gs_can_start_xmit(struct sk_buff *skb,
 			hf->flags |= GS_CAN_FLAG_ESI;
 		}

-		hf->can_dlc = can_len2dlc(cfd->len);
+		hf->can_dlc = can_fd_len2dlc(cfd->len);
 		memcpy(hf->data, cfd->data, cfd->len);
 	} else {
 		cf = (struct can_frame *)skb->data;

[pfink-christ]

For 5.14 there are 3 more changes necessary:
(line numbers may differ for you)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index f2caf36..da79f3c 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -9,6 +9,7 @@
  * Many thanks to all socketcan devs!
  */
 
+#include <linux/ethtool.h>
 #include <linux/init.h>
 #include <linux/signal.h>
 #include <linux/module.h>
@@ -375,7 +376,7 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 				return;
 
 			cfd->can_id = hf->can_id;
-			cfd->len = can_dlc2len(hf->can_dlc);
+			cfd->len = can_fd_dlc2len(hf->can_dlc);
 			if (hf->flags & GS_CAN_FLAG_BRS) {
 				cfd->flags |= CANFD_BRS;
 			}
@@ -388,7 +389,7 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 			if (!skb)
 				return;
 			cf->can_id = hf->can_id;
-			cf->can_dlc = get_can_dlc(hf->can_dlc);
+			cf->can_dlc = can_cc_dlc2len(hf->can_dlc);
 			memcpy(cf->data, hf->data, 8);
 
 			/* ERROR frames tell us information about the controller */
@@ -421,7 +422,7 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 			goto resubmit_urb;
 		}
 
-		can_get_echo_skb(netdev, hf->echo_id);
+		can_get_echo_skb(netdev, hf->echo_id, NULL);
 
 		gs_free_tx_context(txc);
 
@@ -603,7 +604,7 @@ static netdev_tx_t gs_can_start_xmit(struct sk_buff *skb,
 			hf->flags |= GS_CAN_FLAG_ESI;
 		}
 
-		hf->can_dlc = can_len2dlc(cfd->len);
+		hf->can_dlc = can_fd_len2dlc(cfd->len);
 		memcpy(hf->data, cfd->data, cfd->len);
 	} else {
 		cf = (struct can_frame *)skb->data;
@@ -624,7 +625,7 @@ static netdev_tx_t gs_can_start_xmit(struct sk_buff *skb,
 	urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
 	usb_anchor_urb(urb, &dev->tx_submitted);
 
-	can_put_echo_skb(skb, netdev, idx);
+	can_put_echo_skb(skb, netdev, idx, 0);
 
 	atomic_inc(&dev->active_tx_urbs);
 
@@ -632,7 +633,7 @@ static netdev_tx_t gs_can_start_xmit(struct sk_buff *skb,
 	if (unlikely(rc)) {	/* usb send failed */
 		atomic_dec(&dev->active_tx_urbs);
 
-		can_free_echo_skb(netdev, idx);
+		can_free_echo_skb(netdev, idx, NULL);
 		gs_free_tx_context(txc);
 
 		usb_unanchor_urb(urb);

[hartkopp]

Are you sure #include <linux/ethtool.h> is needed here??

[pfink-christ]

Yes, torvalds/linux@cc69837 introduced this also in the upstream gs_usb.c driver, because it was removed as implicit include through netdevice.h.

[pfink-christ]

For example enum ethtool_phys_id_state is defined in ethtool.h.

[hartkopp]

For example enum ethtool_phys_id_state is defined in ethtool.h.

Hm, that's interesting!

Seems to be the only CAN driver using this include ...

Thanks!

[pfink-christ]

Hi @hartkopp, would you mind taking a peek at https://github.com/pfink-christ/net-next/tree/canfd-mainline-conservative-rc2?
That's our current working copy to get this driver mainline ready.

I don't completely like the idea of only sending parts of the gs_host_frame in case of classical candlelight or if the workaround is not used, but the alternative would be to use at least two different gs_host_frame structs and then quite some parts of the driver would have to be rewritten to cope with that. What do you think?

[hartkopp]

Hi @pfink-christ ,
I took a quick look on the patches. Beside the gs_host_frame hack done in the last patch, it looks pretty quite forward and ok.
IMO we should probably create two gs_host_frame structs and send/fill them depending on CAN/CANFD or build a union on these structs or anything else. But that calculated length hack doesn't look nice.

The best way to discuss these patches is to post them with git send-email on the linux-can mailing list. This is also the best way to let more CAN driver specialists (especially the Linux CAN driver maintainer @marckleinebudde ) look on the changes and give feedback for mainlining.