Module won't compile on 5.13.13
Opened this issue · 9 comments
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:
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:
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;
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);
Are you sure #include <linux/ethtool.h> is needed here??
Yes, torvalds/linux@cc69837 introduced this also in the upstream gs_usb.c driver, because it was removed as implicit include through netdevice.h.
For example enum ethtool_phys_id_state is defined in ethtool.h.
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!
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?
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.