The purpose of this patch series is to introduce a new CAN USB
driver to support ETAS USB interfaces (ES58X series).
During development, issues in drivers/net/can/dev.c were discovered,
the fix for those issues are included in this patch series.
We also propose to add one helper functions in include/linux/can/dev.h
which we think can benefit other drivers: get_can_len().
*Side notes*: scripts/checkpatch.pl returns 4 'checks' findings in
[PATCH 4/4]. All those findings are of type: "Macro argument reuse 'x'
possible side-effects?". Those arguments reuse are actually made by
calling either __stringify() or sizeof_field() which are both
pre-processor constant. Furthermore, those macro are never called with
arguments sensible to side-effects. So no actual side effect would
occur.
Changes in v4:
- Remove from the series the patches with have already been merged
into net-next.
Reference: https://lkml.org/lkml/2020/10/4/78
Reference: https://lkml.org/lkml/2020/10/5/355
- Modify [PATCH 4/4] according to comments from Marc.
Reference: https://lkml.org/lkml/2020/10/4/80)
Changes in v3:
- Added one additional patch: [PATCH v3 2/7] can: dev: fix type of
get_can_dlc() and get_canfd_dlc() macros.
- Make get_can_len() return u8 and make the skb const in PATCH 3/7.
- Remove all the calls to likely() and unlikely() in PATCH 6/7.
Changes in v2:
- Fixed -W1 warnings in PATCH 6/7 (v1 was tested with GCC -WExtra
but not with -W1).
- Added lsusb -v information in PATCH 7/7 and rephrased the comment.
- Take care to put everyone in CC of each of the patch of the series
(sorry for the mess in v1...)
Vincent Mailhol (4):
can: dev: can_get_echo_skb(): prevent call to kfree_skb() in hard IRQ
context
can: dev: add a helper function to get the correct length of Classical
frames
can: dev: __can_get_echo_skb(): fix the return length
can: usb: etas_es58X: add support for ETAS ES58X CAN USB interfaces
drivers/net/can/dev.c | 13 +-
drivers/net/can/usb/Kconfig | 9 +
drivers/net/can/usb/Makefile | 1 +
drivers/net/can/usb/etas_es58x/Makefile | 3 +
drivers/net/can/usb/etas_es58x/es581_4.c | 556 ++++
drivers/net/can/usb/etas_es58x/es581_4.h | 209 ++
drivers/net/can/usb/etas_es58x/es58x_core.c | 2639 +++++++++++++++++++
drivers/net/can/usb/etas_es58x/es58x_core.h | 704 +++++
drivers/net/can/usb/etas_es58x/es58x_fd.c | 657 +++++
drivers/net/can/usb/etas_es58x/es58x_fd.h | 241 ++
include/linux/can/dev.h | 23 +
11 files changed, 5048 insertions(+), 7 deletions(-)
create mode 100644 drivers/net/can/usb/etas_es58x/Makefile
create mode 100644 drivers/net/can/usb/etas_es58x/es581_4.c
create mode 100644 drivers/net/can/usb/etas_es58x/es581_4.h
create mode 100644 drivers/net/can/usb/etas_es58x/es58x_core.c
create mode 100644 drivers/net/can/usb/etas_es58x/es58x_core.h
create mode 100644 drivers/net/can/usb/etas_es58x/es58x_fd.c
create mode 100644 drivers/net/can/usb/etas_es58x/es58x_fd.h
--
2.26.2
The length of Remote Transmission Request (RTR) frames is always 0
bytes. The DLC represents the requested length, not the actual length
of the RTR. But __can_get_echo_skb() returns the DLC value regardless.
Apply get_can_len() function to retrieve the correct length.
Signed-off-by: Vincent Mailhol <[email protected]>
---
Changes in v2 to v4: None
---
drivers/net/can/dev.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 73cfcd7e9517..8f91d23c1ca7 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -507,14 +507,9 @@ __can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr)
}
if (priv->echo_skb[idx]) {
- /* Using "struct canfd_frame::len" for the frame
- * length is supported on both CAN and CANFD frames.
- */
struct sk_buff *skb = priv->echo_skb[idx];
- struct canfd_frame *cf = (struct canfd_frame *)skb->data;
- u8 len = cf->len;
- *len_ptr = len;
+ *len_ptr = get_can_len(skb);
priv->echo_skb[idx] = NULL;
return skb;
--
2.26.2
In classical CAN, the length of the data (i.e. CAN payload) is not
always equal to the DLC! If the frame is a Remote Transmission Request
(RTR), data length is always zero regardless of DLC value and else, if
the DLC is greater than 8, the length is 8. Contrary to common belief,
ISO 11898-1 Chapter 8.4.2.3 (DLC field) do allow DLCs greater than 8
for Classical Frames and specifies that those DLCs shall indicate that
the data field is 8 bytes long.
Above facts are widely unknown and so many developpers uses the "len"
field of "struct canfd_frame" to get the length of classical CAN
frames: this is incorrect!
This patch introduces function get_can_len() which can be used in
remediation. The function takes the SKB as an input in order to be
able to determine if the frame is classical or FD.
Signed-off-by: Vincent Mailhol <[email protected]>
---
Changes in v4: None
Changes in v3:
- Make get_can_len() return u8.
- Make the skb const.
Reference: https://lkml.org/lkml/2020/9/30/883
Changes in v2: None
---
include/linux/can/dev.h | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 41ff31795320..d90890172d2a 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -192,6 +192,29 @@ u8 can_dlc2len(u8 can_dlc);
/* map the sanitized data length to an appropriate data length code */
u8 can_len2dlc(u8 len);
+/*
+ * get_can_len(skb) - get the length of the CAN payload.
+ *
+ * In classical CAN, the length of the data (i.e. CAN payload) is not
+ * always equal to the DLC! If the frame is a Remote Transmission
+ * Request (RTR), data length is always zero regardless of DLC value
+ * and else, if the DLC is greater than 8, the length is 8. Contrary
+ * to common belief, ISO 11898-1 Chapter 8.4.2.3 (DLC field) do allow
+ * DLCs greater than 8 for Classical Frames and specifies that those
+ * DLCs shall indicate that the data field is 8 bytes long.
+ */
+static inline u8 get_can_len(const struct sk_buff *skb)
+{
+ const struct canfd_frame *cf = (const struct canfd_frame *)skb->data;
+
+ if (can_is_canfd_skb(skb))
+ return min_t(u8, cf->len, CANFD_MAX_DLEN);
+ else if (cf->can_id & CAN_RTR_FLAG)
+ return 0;
+ else
+ return min_t(u8, cf->len, CAN_MAX_DLEN);
+}
+
struct net_device *alloc_candev_mqs(int sizeof_priv, unsigned int echo_skb_max,
unsigned int txqs, unsigned int rxqs);
#define alloc_candev(sizeof_priv, echo_skb_max) \
--
2.26.2
On 10/16/20 7:13 PM, Vincent Mailhol wrote:
> The purpose of this patch series is to introduce a new CAN USB
> driver to support ETAS USB interfaces (ES58X series).
>
> During development, issues in drivers/net/can/dev.c were discovered,
> the fix for those issues are included in this patch series.
>
> We also propose to add one helper functions in include/linux/can/dev.h
> which we think can benefit other drivers: get_can_len().
I applied patches 1-3 to linux-can, I've changed get_can_len() -> can_get_len()
to use a common can_ prefix for all CAN related functions.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |