* [PATCH v8 0/7] can: support CAN XL
@ 2022-08-01 19:00 Oliver Hartkopp
2022-08-01 19:00 ` [PATCH v8 1/7] can: skb: unify skb CAN frame identification helpers Oliver Hartkopp
` (7 more replies)
0 siblings, 8 replies; 24+ messages in thread
From: Oliver Hartkopp @ 2022-08-01 19:00 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp
The CAN with eXtended data Length (CAN XL) is a new CAN protocol with a
10Mbit/s data transfer with a new physical layer transceiver (for this
data section). CAN XL allows up to 2048 byte of payload and shares the
arbitration principle (11 bit priority) known from Classical CAN and
CAN FD. RTR and 29 bit identifiers are not implemented in CAN XL.
A short introdution to CAN XL can be found here:
https://www.bosch-semiconductors.com/media/ip_modules/pdf_2/can_xl_1/canxl_intro_20210225.pdf
V2: Major rework after discussion and feedback on Linux-CAN ML
- rework of struct canxl_frame
- CANXL_XLF flag is now the switch between CAN XL and CAN/CANFD
- variable length in r/w operations for CAN XL frames
- write CAN XL frame to raw socket enforces size <-> canxl_frame.len sync
V3: Fix length for CAN XL frames inside the sk_buff
- extend the CAN_RAW sockopt to handle fixed/truncated read/write operations
V4: Fix patch 5 (can: raw: add CAN XL support)
- fix return value (move 'err = -EINVAL' in raw_sendmsg())
- add CAN XL frame handling in can_rcv()
- change comment for CAN_RAW_XL_[RT]X_DYN definition (allow -> enable)
V5: Remove CAN_RAW_XL_[RT]X_DYN definition again
- CAN_RAW_XL_[RT]X_DYN (truncated data) feature is now enabled by default
- use CANXL_MIN_DLEN instead of '1' in canxl_frame definition
- add missing 'err = -EINVAL' initialization in raw_sendmsg())
V6:
- rework an separate skb identification and length helpers
- add CANFD_FDF flag in all CAN FD frame structures
- simplify patches for infrastructure and raw sockets
- add vxcan support in virtual CAN interface patch
V7:
- fixed indention as remarked by Marc
- set CANFD_FDF flag when detecting CAN FD frames generated by PF_PACKET
- Allow to use variable CAN XL MTU sizes to enforce real time requirements
on CAN XL segments (e.g. to support of CAN CiA segmentation concept)
V8:
- fixed typo as remarked by Vincent
- rebased to latest can-next/net-next tree
Oliver Hartkopp (7):
can: skb: unify skb CAN frame identification helpers
can: skb: add skb CAN frame data length helpers
can: set CANFD_FDF flag in all CAN FD frame structures
can: canxl: introduce CAN XL data structure
can: canxl: update CAN infrastructure for CAN XL frames
can: dev: add CAN XL support to virtual CAN
can: raw: add CAN XL support
drivers/net/can/ctucanfd/ctucanfd_base.c | 1 -
drivers/net/can/dev/rx-offload.c | 2 +-
drivers/net/can/dev/skb.c | 113 ++++++++++++++++-------
drivers/net/can/vcan.c | 12 +--
drivers/net/can/vxcan.c | 8 +-
include/linux/can/dev.h | 5 +
include/linux/can/skb.h | 57 +++++++++++-
include/uapi/linux/can.h | 55 ++++++++++-
include/uapi/linux/can/raw.h | 1 +
include/uapi/linux/if_ether.h | 1 +
net/can/af_can.c | 76 ++++++++-------
net/can/bcm.c | 9 +-
net/can/gw.c | 4 +-
net/can/isotp.c | 2 +-
net/can/j1939/main.c | 4 +
net/can/raw.c | 55 ++++++++---
16 files changed, 299 insertions(+), 106 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v8 1/7] can: skb: unify skb CAN frame identification helpers
2022-08-01 19:00 [PATCH v8 0/7] can: support CAN XL Oliver Hartkopp
@ 2022-08-01 19:00 ` Oliver Hartkopp
2022-08-01 19:00 ` [PATCH v8 2/7] can: skb: add skb CAN frame data length helpers Oliver Hartkopp
` (6 subsequent siblings)
7 siblings, 0 replies; 24+ messages in thread
From: Oliver Hartkopp @ 2022-08-01 19:00 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp
Replace open coded checks for sk_buffs containing Classical CAN and
CAN FD frame structures as a preparation for CAN XL support.
With the added length check the unintended processing of CAN XL frames
having the CANXL_XLF bit set can be suppressed even when the skb->len
fits to non CAN XL frames.
The CAN_RAW socket needs a rework to use these helpers. Therefore the
use of these helpers is postponed to the CAN_RAW CAN XL integration.
The J1939 protocol gets a check for Classical CAN frames too.
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
drivers/net/can/dev/skb.c | 18 +++++++-------
include/linux/can/skb.h | 12 +++++++++-
net/can/af_can.c | 50 ++++++++-------------------------------
net/can/bcm.c | 9 +++++--
net/can/gw.c | 4 ++--
net/can/isotp.c | 2 +-
net/can/j1939/main.c | 4 ++++
7 files changed, 45 insertions(+), 54 deletions(-)
diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
index 07e0feac8629..f457c94ba82f 100644
--- a/drivers/net/can/dev/skb.c
+++ b/drivers/net/can/dev/skb.c
@@ -297,22 +297,24 @@ static bool can_skb_headroom_valid(struct net_device *dev, struct sk_buff *skb)
}
/* Drop a given socketbuffer if it does not contain a valid CAN frame. */
bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb)
{
- const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
struct can_priv *priv = netdev_priv(dev);
- if (skb->protocol == htons(ETH_P_CAN)) {
- if (unlikely(skb->len != CAN_MTU ||
- cfd->len > CAN_MAX_DLEN))
+ switch (ntohs(skb->protocol)) {
+ case ETH_P_CAN:
+ if (!can_is_can_skb(skb))
goto inval_skb;
- } else if (skb->protocol == htons(ETH_P_CANFD)) {
- if (unlikely(skb->len != CANFD_MTU ||
- cfd->len > CANFD_MAX_DLEN))
+ break;
+
+ case ETH_P_CANFD:
+ if (!can_is_canfd_skb(skb))
goto inval_skb;
- } else {
+ break;
+
+ default:
goto inval_skb;
}
if (!can_skb_headroom_valid(dev, skb)) {
goto inval_skb;
diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
index 182749e858b3..27ebfc28510f 100644
--- a/include/linux/can/skb.h
+++ b/include/linux/can/skb.h
@@ -95,12 +95,22 @@ static inline struct sk_buff *can_create_echo_skb(struct sk_buff *skb)
can_skb_set_owner(nskb, skb->sk);
consume_skb(skb);
return nskb;
}
+static inline bool can_is_can_skb(const struct sk_buff *skb)
+{
+ struct can_frame *cf = (struct can_frame *)skb->data;
+
+ /* the CAN specific type of skb is identified by its data length */
+ return (skb->len == CAN_MTU && cf->len <= CAN_MAX_DLEN);
+}
+
static inline bool can_is_canfd_skb(const struct sk_buff *skb)
{
+ struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
+
/* the CAN specific type of skb is identified by its data length */
- return skb->len == CANFD_MTU;
+ return (skb->len == CANFD_MTU && cfd->len <= CANFD_MAX_DLEN);
}
#endif /* !_CAN_SKB_H */
diff --git a/net/can/af_can.c b/net/can/af_can.c
index 1fb49d51b25d..afa6c2151bc4 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -197,31 +197,23 @@ static int can_create(struct net *net, struct socket *sock, int protocol,
* -EINVAL when the skb->data does not contain a valid CAN frame
*/
int can_send(struct sk_buff *skb, int loop)
{
struct sk_buff *newskb = NULL;
- struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
struct can_pkg_stats *pkg_stats = dev_net(skb->dev)->can.pkg_stats;
int err = -EINVAL;
- if (skb->len == CAN_MTU) {
+ if (can_is_can_skb(skb)) {
skb->protocol = htons(ETH_P_CAN);
- if (unlikely(cfd->len > CAN_MAX_DLEN))
- goto inval_skb;
- } else if (skb->len == CANFD_MTU) {
+ } else if (can_is_canfd_skb(skb)) {
skb->protocol = htons(ETH_P_CANFD);
- if (unlikely(cfd->len > CANFD_MAX_DLEN))
- goto inval_skb;
} else {
goto inval_skb;
}
- /* Make sure the CAN frame can pass the selected CAN netdevice.
- * As structs can_frame and canfd_frame are similar, we can provide
- * CAN FD frames to legacy CAN drivers as long as the length is <= 8
- */
- if (unlikely(skb->len > skb->dev->mtu && cfd->len > CAN_MAX_DLEN)) {
+ /* Make sure the CAN frame can pass the selected CAN netdevice. */
+ if (unlikely(skb->len > skb->dev->mtu)) {
err = -EMSGSIZE;
goto inval_skb;
}
if (unlikely(skb->dev->type != ARPHRD_CAN)) {
@@ -676,57 +668,35 @@ static void can_receive(struct sk_buff *skb, struct net_device *dev)
}
static int can_rcv(struct sk_buff *skb, struct net_device *dev,
struct packet_type *pt, struct net_device *orig_dev)
{
- struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
-
- if (unlikely(dev->type != ARPHRD_CAN || skb->len != CAN_MTU)) {
+ if (unlikely(dev->type != ARPHRD_CAN || (!can_is_can_skb(skb)))) {
pr_warn_once("PF_CAN: dropped non conform CAN skbuff: dev type %d, len %d\n",
dev->type, skb->len);
- goto free_skb;
- }
- /* This check is made separately since cfd->len would be uninitialized if skb->len = 0. */
- if (unlikely(cfd->len > CAN_MAX_DLEN)) {
- pr_warn_once("PF_CAN: dropped non conform CAN skbuff: dev type %d, len %d, datalen %d\n",
- dev->type, skb->len, cfd->len);
- goto free_skb;
+ kfree_skb(skb);
+ return NET_RX_DROP;
}
can_receive(skb, dev);
return NET_RX_SUCCESS;
-
-free_skb:
- kfree_skb(skb);
- return NET_RX_DROP;
}
static int canfd_rcv(struct sk_buff *skb, struct net_device *dev,
struct packet_type *pt, struct net_device *orig_dev)
{
- struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
-
- if (unlikely(dev->type != ARPHRD_CAN || skb->len != CANFD_MTU)) {
+ if (unlikely(dev->type != ARPHRD_CAN || (!can_is_canfd_skb(skb)))) {
pr_warn_once("PF_CAN: dropped non conform CAN FD skbuff: dev type %d, len %d\n",
dev->type, skb->len);
- goto free_skb;
- }
- /* This check is made separately since cfd->len would be uninitialized if skb->len = 0. */
- if (unlikely(cfd->len > CANFD_MAX_DLEN)) {
- pr_warn_once("PF_CAN: dropped non conform CAN FD skbuff: dev type %d, len %d, datalen %d\n",
- dev->type, skb->len, cfd->len);
- goto free_skb;
+ kfree_skb(skb);
+ return NET_RX_DROP;
}
can_receive(skb, dev);
return NET_RX_SUCCESS;
-
-free_skb:
- kfree_skb(skb);
- return NET_RX_DROP;
}
/* af_can protocol functions */
/**
diff --git a/net/can/bcm.c b/net/can/bcm.c
index e60161bec850..e5d179ba6f7d 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -646,12 +646,17 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
if (op->can_id != rxframe->can_id)
return;
/* make sure to handle the correct frame type (CAN / CAN FD) */
- if (skb->len != op->cfsiz)
- return;
+ if (op->flags & CAN_FD_FRAME) {
+ if (!can_is_canfd_skb(skb))
+ return;
+ } else {
+ if (!can_is_can_skb(skb))
+ return;
+ }
/* disable timeout */
hrtimer_cancel(&op->timer);
/* save rx timestamp */
diff --git a/net/can/gw.c b/net/can/gw.c
index 1ea4cc527db3..23a3d89cad81 100644
--- a/net/can/gw.c
+++ b/net/can/gw.c
@@ -461,14 +461,14 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
struct sk_buff *nskb;
int modidx = 0;
/* process strictly Classic CAN or CAN FD frames */
if (gwj->flags & CGW_FLAGS_CAN_FD) {
- if (skb->len != CANFD_MTU)
+ if (!can_is_canfd_skb(skb))
return;
} else {
- if (skb->len != CAN_MTU)
+ if (!can_is_can_skb(skb))
return;
}
/* Do not handle CAN frames routed more than 'max_hops' times.
* In general we should never catch this delimiter which is intended
diff --git a/net/can/isotp.c b/net/can/isotp.c
index 43a27d19cdac..a9d1357f8489 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -667,11 +667,11 @@ static void isotp_rcv(struct sk_buff *skb, void *data)
sf_dl = cf->data[ae] & 0x0F;
if (cf->len <= CAN_MAX_DLEN) {
isotp_rcv_sf(sk, cf, SF_PCI_SZ4 + ae, skb, sf_dl);
} else {
- if (skb->len == CANFD_MTU) {
+ if (can_is_canfd_skb(skb)) {
/* We have a CAN FD frame and CAN_DL is greater than 8:
* Only frames with the SF_DL == 0 ESC value are valid.
*
* If so take care of the increased SF PCI size
* (SF_PCI_SZ8) to point to the message content behind
diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
index 8452b0fbb78c..144c86b0e3ff 100644
--- a/net/can/j1939/main.c
+++ b/net/can/j1939/main.c
@@ -40,10 +40,14 @@ static void j1939_can_recv(struct sk_buff *iskb, void *data)
struct j1939_priv *priv = data;
struct sk_buff *skb;
struct j1939_sk_buff_cb *skcb, *iskcb;
struct can_frame *cf;
+ /* make sure we only get Classical CAN frames */
+ if (!can_is_can_skb(iskb))
+ return;
+
/* create a copy of the skb
* j1939 only delivers the real data bytes,
* the header goes into sockaddr.
* j1939 may not touch the incoming skb in such way
*/
--
2.30.2
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v8 2/7] can: skb: add skb CAN frame data length helpers
2022-08-01 19:00 [PATCH v8 0/7] can: support CAN XL Oliver Hartkopp
2022-08-01 19:00 ` [PATCH v8 1/7] can: skb: unify skb CAN frame identification helpers Oliver Hartkopp
@ 2022-08-01 19:00 ` Oliver Hartkopp
2022-08-01 19:00 ` [PATCH v8 3/7] can: set CANFD_FDF flag in all CAN FD frame structures Oliver Hartkopp
` (5 subsequent siblings)
7 siblings, 0 replies; 24+ messages in thread
From: Oliver Hartkopp @ 2022-08-01 19:00 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp
Add two helpers to retrieve the data length from CAN sk_buffs and prepare
the length information to be a uint16 value for the CAN XL support.
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
drivers/net/can/dev/rx-offload.c | 2 +-
drivers/net/can/dev/skb.c | 12 ++++--------
include/linux/can/skb.h | 24 +++++++++++++++++++++++-
3 files changed, 28 insertions(+), 10 deletions(-)
diff --git a/drivers/net/can/dev/rx-offload.c b/drivers/net/can/dev/rx-offload.c
index a32a01c172d4..8505e547e922 100644
--- a/drivers/net/can/dev/rx-offload.c
+++ b/drivers/net/can/dev/rx-offload.c
@@ -245,11 +245,11 @@ unsigned int can_rx_offload_get_echo_skb(struct can_rx_offload *offload,
unsigned int *frame_len_ptr)
{
struct net_device *dev = offload->dev;
struct net_device_stats *stats = &dev->stats;
struct sk_buff *skb;
- u8 len;
+ unsigned int len;
int err;
skb = __can_get_echo_skb(dev, idx, &len, frame_len_ptr);
if (!skb)
return 0;
diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
index f457c94ba82f..b896e1ce3b47 100644
--- a/drivers/net/can/dev/skb.c
+++ b/drivers/net/can/dev/skb.c
@@ -89,12 +89,12 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
return 0;
}
EXPORT_SYMBOL_GPL(can_put_echo_skb);
struct sk_buff *
-__can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr,
- unsigned int *frame_len_ptr)
+__can_get_echo_skb(struct net_device *dev, unsigned int idx,
+ unsigned int *len_ptr, unsigned int *frame_len_ptr)
{
struct can_priv *priv = netdev_priv(dev);
if (idx >= priv->echo_skb_max) {
netdev_err(dev, "%s: BUG! Trying to access can_priv::echo_skb out of bounds (%u/max %u)\n",
@@ -106,20 +106,16 @@ __can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr,
/* 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 can_skb_priv *can_skb_priv = can_skb_prv(skb);
- struct canfd_frame *cf = (struct canfd_frame *)skb->data;
if (skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS)
skb_tstamp_tx(skb, skb_hwtstamps(skb));
/* get the real payload length for netdev statistics */
- if (cf->can_id & CAN_RTR_FLAG)
- *len_ptr = 0;
- else
- *len_ptr = cf->len;
+ *len_ptr = can_skb_get_data_len(skb);
if (frame_len_ptr)
*frame_len_ptr = can_skb_priv->frame_len;
priv->echo_skb[idx] = NULL;
@@ -145,11 +141,11 @@ __can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr,
*/
unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx,
unsigned int *frame_len_ptr)
{
struct sk_buff *skb;
- u8 len;
+ unsigned int len;
skb = __can_get_echo_skb(dev, idx, &len, frame_len_ptr);
if (!skb)
return 0;
diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
index 27ebfc28510f..ddffc2fc008c 100644
--- a/include/linux/can/skb.h
+++ b/include/linux/can/skb.h
@@ -18,11 +18,12 @@
void can_flush_echo_skb(struct net_device *dev);
int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
unsigned int idx, unsigned int frame_len);
struct sk_buff *__can_get_echo_skb(struct net_device *dev, unsigned int idx,
- u8 *len_ptr, unsigned int *frame_len_ptr);
+ unsigned int *len_ptr,
+ unsigned int *frame_len_ptr);
unsigned int __must_check can_get_echo_skb(struct net_device *dev,
unsigned int idx,
unsigned int *frame_len_ptr);
void can_free_echo_skb(struct net_device *dev, unsigned int idx,
unsigned int *frame_len_ptr);
@@ -111,6 +112,27 @@ static inline bool can_is_canfd_skb(const struct sk_buff *skb)
/* the CAN specific type of skb is identified by its data length */
return (skb->len == CANFD_MTU && cfd->len <= CANFD_MAX_DLEN);
}
+/* get length element value from can[fd]_frame structure */
+static inline unsigned int can_skb_get_len_val(struct sk_buff *skb)
+{
+ const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
+
+ return cfd->len;
+}
+
+/* get needed data length inside CAN frame for all frame types (RTR aware) */
+static inline unsigned int can_skb_get_data_len(struct sk_buff *skb)
+{
+ unsigned int len = can_skb_get_len_val(skb);
+ const struct can_frame *cf = (struct can_frame *)skb->data;
+
+ /* RTR frames have an actual length of zero */
+ if (can_is_can_skb(skb) && cf->can_id & CAN_RTR_FLAG)
+ return 0;
+
+ return len;
+}
+
#endif /* !_CAN_SKB_H */
--
2.30.2
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v8 3/7] can: set CANFD_FDF flag in all CAN FD frame structures
2022-08-01 19:00 [PATCH v8 0/7] can: support CAN XL Oliver Hartkopp
2022-08-01 19:00 ` [PATCH v8 1/7] can: skb: unify skb CAN frame identification helpers Oliver Hartkopp
2022-08-01 19:00 ` [PATCH v8 2/7] can: skb: add skb CAN frame data length helpers Oliver Hartkopp
@ 2022-08-01 19:00 ` Oliver Hartkopp
2022-09-11 7:51 ` Vincent Mailhol
2022-08-01 19:00 ` [PATCH v8 4/7] can: canxl: introduce CAN XL data structure Oliver Hartkopp
` (4 subsequent siblings)
7 siblings, 1 reply; 24+ messages in thread
From: Oliver Hartkopp @ 2022-08-01 19:00 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp
To simplify the testing in user space all struct canfd_frame's provided by
the CAN subsystem of the Linux kernel now have the CANFD_FDF flag set in
canfd_frame::flags.
NB: Handcrafted ETH_P_CANFD frames introduced via PF_PACKET socket might
not set this bit correctly. During the check for sufficient headroom in
PF_PACKET sk_buffs the uninitialized CAN sk_buff data structures are filled.
In the case of a CAN FD frame the CANFD_FDF flag is set accordingly.
As the CAN frame content is already zero initialized in alloc_canfd_skb()
the obsolete initialization of cf->flags in the CTU CAN FD driver has been
removed as it would overwrite the already set CANFD_FDF flag.
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
drivers/net/can/ctucanfd/ctucanfd_base.c | 1 -
drivers/net/can/dev/skb.c | 11 +++++++++++
include/uapi/linux/can.h | 4 ++--
net/can/af_can.c | 5 +++++
4 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/drivers/net/can/ctucanfd/ctucanfd_base.c b/drivers/net/can/ctucanfd/ctucanfd_base.c
index 3c18d028bd8c..c4026712ab7d 100644
--- a/drivers/net/can/ctucanfd/ctucanfd_base.c
+++ b/drivers/net/can/ctucanfd/ctucanfd_base.c
@@ -655,11 +655,10 @@ static void ctucan_read_rx_frame(struct ctucan_priv *priv, struct canfd_frame *c
cf->can_id = (idw & CAN_EFF_MASK) | CAN_EFF_FLAG;
else
cf->can_id = (idw >> 18) & CAN_SFF_MASK;
/* BRS, ESI, RTR Flags */
- cf->flags = 0;
if (FIELD_GET(REG_FRAME_FORMAT_W_FDF, ffw)) {
if (FIELD_GET(REG_FRAME_FORMAT_W_BRS, ffw))
cf->flags |= CANFD_BRS;
if (FIELD_GET(REG_FRAME_FORMAT_W_ESI_RSV, ffw))
cf->flags |= CANFD_ESI;
diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
index b896e1ce3b47..adb413bdd734 100644
--- a/drivers/net/can/dev/skb.c
+++ b/drivers/net/can/dev/skb.c
@@ -242,10 +242,13 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev,
can_skb_prv(skb)->ifindex = dev->ifindex;
can_skb_prv(skb)->skbcnt = 0;
*cfd = skb_put_zero(skb, sizeof(struct canfd_frame));
+ /* set CAN FD flag by default */
+ (*cfd)->flags = CANFD_FDF;
+
return skb;
}
EXPORT_SYMBOL_GPL(alloc_canfd_skb);
struct sk_buff *alloc_can_err_skb(struct net_device *dev, struct can_frame **cf)
@@ -285,10 +288,18 @@ static bool can_skb_headroom_valid(struct net_device *dev, struct sk_buff *skb)
skb->pkt_type = PACKET_HOST;
skb_reset_mac_header(skb);
skb_reset_network_header(skb);
skb_reset_transport_header(skb);
+
+ /* set CANFD_FDF flag for CAN FD frames */
+ if (can_is_canfd_skb(skb)) {
+ struct canfd_frame *cfd;
+
+ cfd = (struct canfd_frame *)skb->data;
+ cfd->flags |= CANFD_FDF;
+ }
}
return true;
}
diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h
index 90801ada2bbe..7b23eeeb3273 100644
--- a/include/uapi/linux/can.h
+++ b/include/uapi/linux/can.h
@@ -139,12 +139,12 @@ struct can_frame {
* The struct can_frame and struct canfd_frame intentionally share the same
* layout to be able to write CAN frame content into a CAN FD frame structure.
* When this is done the former differentiation via CAN_MTU / CANFD_MTU gets
* lost. CANFD_FDF allows programmers to mark CAN FD frames in the case of
* using struct canfd_frame for mixed CAN / CAN FD content (dual use).
- * N.B. the Kernel APIs do NOT provide mixed CAN / CAN FD content inside of
- * struct canfd_frame therefore the CANFD_FDF flag is disregarded by Linux.
+ * Since the introduction of CAN XL the CANFD_FDF flag is set in all CAN FD
+ * frame structures provided by the CAN subsystem of the Linux kernel.
*/
#define CANFD_BRS 0x01 /* bit rate switch (second bitrate for payload data) */
#define CANFD_ESI 0x02 /* error state indicator of the transmitting node */
#define CANFD_FDF 0x04 /* mark CAN FD for dual use of struct canfd_frame */
diff --git a/net/can/af_can.c b/net/can/af_can.c
index afa6c2151bc4..072a6a5c9dd1 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -203,11 +203,16 @@ int can_send(struct sk_buff *skb, int loop)
int err = -EINVAL;
if (can_is_can_skb(skb)) {
skb->protocol = htons(ETH_P_CAN);
} else if (can_is_canfd_skb(skb)) {
+ struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
+
skb->protocol = htons(ETH_P_CANFD);
+
+ /* set CAN FD flag for CAN FD frames by default */
+ cfd->flags |= CANFD_FDF;
} else {
goto inval_skb;
}
/* Make sure the CAN frame can pass the selected CAN netdevice. */
--
2.30.2
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v8 4/7] can: canxl: introduce CAN XL data structure
2022-08-01 19:00 [PATCH v8 0/7] can: support CAN XL Oliver Hartkopp
` (2 preceding siblings ...)
2022-08-01 19:00 ` [PATCH v8 3/7] can: set CANFD_FDF flag in all CAN FD frame structures Oliver Hartkopp
@ 2022-08-01 19:00 ` Oliver Hartkopp
2022-09-02 6:19 ` Vincent Mailhol
2022-08-01 19:00 ` [PATCH v8 5/7] can: canxl: update CAN infrastructure for CAN XL frames Oliver Hartkopp
` (3 subsequent siblings)
7 siblings, 1 reply; 24+ messages in thread
From: Oliver Hartkopp @ 2022-08-01 19:00 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp
This patch adds defines for data structures and length information for
CAN XL (CAN with eXtended data Length) which can transfer up to 2048
byte inside a single frame.
Notable changes from CAN FD:
- the 11 bit arbitration field is now named 'priority' instead of 'can_id'
(there are no 29 bit identifiers nor RTR frames anymore)
- the data length needs a uint16 value to cover up to 2048 byte
(the length element position is different to struct can[fd]_frame)
- new fields (SDT, AF) and a SEC bit have been introduced
- the virtual CAN interface identifier is not part if the CAN XL frame
struct as this VCID value is stored in struct skbuff (analog to vlan id)
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
include/uapi/linux/can.h | 51 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 51 insertions(+)
diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h
index 7b23eeeb3273..dd645ea72306 100644
--- a/include/uapi/linux/can.h
+++ b/include/uapi/linux/can.h
@@ -46,10 +46,11 @@
#ifndef _UAPI_CAN_H
#define _UAPI_CAN_H
#include <linux/types.h>
#include <linux/socket.h>
+#include <linux/stddef.h> /* for offsetof */
/* controller area network (CAN) kernel definitions */
/* special address description flags for the CAN_ID */
#define CAN_EFF_FLAG 0x80000000U /* EFF/SFF is set in the MSB */
@@ -58,10 +59,11 @@
/* valid bits in CAN ID for frame formats */
#define CAN_SFF_MASK 0x000007FFU /* standard frame format (SFF) */
#define CAN_EFF_MASK 0x1FFFFFFFU /* extended frame format (EFF) */
#define CAN_ERR_MASK 0x1FFFFFFFU /* omit EFF, RTR, ERR flags */
+#define CANXL_PRIO_MASK CAN_SFF_MASK /* 11 bit priority mask */
/*
* Controller Area Network Identifier structure
*
* bit 0-28 : CAN identifier (11/29 bit)
@@ -71,10 +73,11 @@
*/
typedef __u32 canid_t;
#define CAN_SFF_ID_BITS 11
#define CAN_EFF_ID_BITS 29
+#define CANXL_PRIO_BITS CAN_SFF_ID_BITS
/*
* Controller Area Network Error Message Frame Mask structure
*
* bit 0-28 : error class mask (see include/uapi/linux/can/error.h)
@@ -89,10 +92,20 @@ typedef __u32 can_err_mask_t;
/* CAN FD payload length and DLC definitions according to ISO 11898-7 */
#define CANFD_MAX_DLC 15
#define CANFD_MAX_DLEN 64
+/*
+ * CAN XL payload length and DLC definitions according to ISO 11898-1
+ * CAN XL DLC ranges from 0 .. 2047 => data length from 1 .. 2048 byte
+ */
+#define CANXL_MIN_DLC 0
+#define CANXL_MAX_DLC 2047
+#define CANXL_MAX_DLC_MASK 0x07FF
+#define CANXL_MIN_DLEN 1
+#define CANXL_MAX_DLEN 2048
+
/**
* struct can_frame - Classical CAN frame structure (aka CAN 2.0B)
* @can_id: CAN ID of the frame and CAN_*_FLAG flags, see canid_t definition
* @len: CAN frame payload length in byte (0 .. 8)
* @can_dlc: deprecated name for CAN frame payload length in byte (0 .. 8)
@@ -164,12 +177,50 @@ struct canfd_frame {
__u8 __res0; /* reserved / padding */
__u8 __res1; /* reserved / padding */
__u8 data[CANFD_MAX_DLEN] __attribute__((aligned(8)));
};
+/*
+ * defined bits for canxl_frame.flags
+ *
+ * The canxl_frame.flags element contains two bits CANXL_XLF and CANXL_SEC
+ * and shares the relative position of the struct can[fd]_frame.len element.
+ * The CANXL_XLF bit ALWAYS needs to be set to indicate a valid CAN XL frame.
+ * As a side effect setting this bit intentionally breaks the length checks
+ * for Classical CAN and CAN FD frames.
+ *
+ * Undefined bits in canxl_frame.flags are reserved and shall be set to zero.
+ */
+#define CANXL_XLF 0x80 /* mandatory CAN XL frame flag (must always be set!) */
+#define CANXL_SEC 0x01 /* Simple Extended Content (security/segmentation) */
+
+/**
+ * struct canxl_frame - CAN with e'X'tended frame 'L'ength frame structure
+ * @prio: 11 bit arbitration priority with zero'ed CAN_*_FLAG flags
+ * @flags: additional flags for CAN XL
+ * @sdt: SDU (service data unit) type
+ * @len: frame payload length in byte (CANXL_MIN_DLEN .. CANXL_MAX_DLEN)
+ * @af: acceptance field
+ * @data: CAN XL frame payload (CANXL_MIN_DLEN .. CANXL_MAX_DLEN byte)
+ *
+ * @prio shares the same position as @can_id from struct can[fd]_frame.
+ */
+struct canxl_frame {
+ canid_t prio; /* 11 bit priority for arbitration (canid_t) */
+ __u8 flags; /* additional flags for CAN XL */
+ __u8 sdt; /* SDU (service data unit) type */
+ __u16 len; /* frame payload length in byte */
+ __u32 af; /* acceptance field */
+ __u8 data[CANXL_MAX_DLEN];
+};
+
#define CAN_MTU (sizeof(struct can_frame))
#define CANFD_MTU (sizeof(struct canfd_frame))
+#define CANXL_MTU (sizeof(struct canxl_frame))
+#define CANXL_HDR_SIZE (offsetof(struct canxl_frame, data))
+#define CANXL_MIN_MTU (CANXL_HDR_SIZE + 64)
+#define CANXL_MAX_MTU CANXL_MTU
/* particular protocols of the protocol family PF_CAN */
#define CAN_RAW 1 /* RAW sockets */
#define CAN_BCM 2 /* Broadcast Manager */
#define CAN_TP16 3 /* VAG Transport Protocol v1.6 */
--
2.30.2
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v8 5/7] can: canxl: update CAN infrastructure for CAN XL frames
2022-08-01 19:00 [PATCH v8 0/7] can: support CAN XL Oliver Hartkopp
` (3 preceding siblings ...)
2022-08-01 19:00 ` [PATCH v8 4/7] can: canxl: introduce CAN XL data structure Oliver Hartkopp
@ 2022-08-01 19:00 ` Oliver Hartkopp
2022-09-11 7:53 ` Vincent Mailhol
2022-08-01 19:00 ` [PATCH v8 6/7] can: dev: add CAN XL support to virtual CAN Oliver Hartkopp
` (2 subsequent siblings)
7 siblings, 1 reply; 24+ messages in thread
From: Oliver Hartkopp @ 2022-08-01 19:00 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp
- add new ETH_P_CANXL ethernet protocol type
- update skb checks for CAN XL
- add alloc_canxl_skb() which now needs a data length parameter
- introduce init_can_skb_reserve() to reduce code duplication
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
drivers/net/can/dev/skb.c | 72 ++++++++++++++++++++++++++---------
include/linux/can/skb.h | 23 ++++++++++-
include/uapi/linux/if_ether.h | 1 +
net/can/af_can.c | 25 +++++++++++-
4 files changed, 101 insertions(+), 20 deletions(-)
diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
index adb413bdd734..f2ec20d80aba 100644
--- a/drivers/net/can/dev/skb.c
+++ b/drivers/net/can/dev/skb.c
@@ -185,10 +185,24 @@ void can_free_echo_skb(struct net_device *dev, unsigned int idx,
priv->echo_skb[idx] = NULL;
}
}
EXPORT_SYMBOL_GPL(can_free_echo_skb);
+/* fill common values for CAN sk_buffs */
+static void init_can_skb_reserve(struct sk_buff *skb)
+{
+ skb->pkt_type = PACKET_BROADCAST;
+ skb->ip_summed = CHECKSUM_UNNECESSARY;
+
+ skb_reset_mac_header(skb);
+ skb_reset_network_header(skb);
+ skb_reset_transport_header(skb);
+
+ can_skb_reserve(skb);
+ can_skb_prv(skb)->skbcnt = 0;
+}
+
struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf)
{
struct sk_buff *skb;
skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) +
@@ -198,20 +212,12 @@ struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf)
return NULL;
}
skb->protocol = htons(ETH_P_CAN);
- skb->pkt_type = PACKET_BROADCAST;
- skb->ip_summed = CHECKSUM_UNNECESSARY;
-
- skb_reset_mac_header(skb);
- skb_reset_network_header(skb);
- skb_reset_transport_header(skb);
-
- can_skb_reserve(skb);
+ init_can_skb_reserve(skb);
can_skb_prv(skb)->ifindex = dev->ifindex;
- can_skb_prv(skb)->skbcnt = 0;
*cf = skb_put_zero(skb, sizeof(struct can_frame));
return skb;
}
@@ -229,30 +235,55 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev,
return NULL;
}
skb->protocol = htons(ETH_P_CANFD);
- skb->pkt_type = PACKET_BROADCAST;
- skb->ip_summed = CHECKSUM_UNNECESSARY;
-
- skb_reset_mac_header(skb);
- skb_reset_network_header(skb);
- skb_reset_transport_header(skb);
-
- can_skb_reserve(skb);
+ init_can_skb_reserve(skb);
can_skb_prv(skb)->ifindex = dev->ifindex;
- can_skb_prv(skb)->skbcnt = 0;
*cfd = skb_put_zero(skb, sizeof(struct canfd_frame));
/* set CAN FD flag by default */
(*cfd)->flags = CANFD_FDF;
return skb;
}
EXPORT_SYMBOL_GPL(alloc_canfd_skb);
+struct sk_buff *alloc_canxl_skb(struct net_device *dev,
+ struct canxl_frame **cfx,
+ unsigned int data_len)
+{
+ struct sk_buff *skb;
+
+ if (data_len < CANXL_MIN_DLEN || data_len > CANXL_MAX_DLEN)
+ goto out_error;
+
+ skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) +
+ CANXL_HDR_SIZE + data_len);
+ if (unlikely(!skb))
+ goto out_error;
+
+ skb->protocol = htons(ETH_P_CANXL);
+ init_can_skb_reserve(skb);
+ can_skb_prv(skb)->ifindex = dev->ifindex;
+
+ *cfx = skb_put_zero(skb, CANXL_HDR_SIZE + data_len);
+
+ /* set CAN XL flag and length information by default */
+ (*cfx)->flags = CANXL_XLF;
+ (*cfx)->len = data_len;
+
+ return skb;
+
+out_error:
+ *cfx = NULL;
+
+ return NULL;
+}
+EXPORT_SYMBOL_GPL(alloc_canxl_skb);
+
struct sk_buff *alloc_can_err_skb(struct net_device *dev, struct can_frame **cf)
{
struct sk_buff *skb;
skb = alloc_can_skb(dev, cf);
@@ -317,10 +348,15 @@ bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb)
case ETH_P_CANFD:
if (!can_is_canfd_skb(skb))
goto inval_skb;
break;
+ case ETH_P_CANXL:
+ if (!can_is_canxl_skb(skb))
+ goto inval_skb;
+ break;
+
default:
goto inval_skb;
}
if (!can_skb_headroom_valid(dev, skb)) {
diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
index ddffc2fc008c..01c01b1261fa 100644
--- a/include/linux/can/skb.h
+++ b/include/linux/can/skb.h
@@ -28,10 +28,13 @@ unsigned int __must_check can_get_echo_skb(struct net_device *dev,
void can_free_echo_skb(struct net_device *dev, unsigned int idx,
unsigned int *frame_len_ptr);
struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf);
struct sk_buff *alloc_canfd_skb(struct net_device *dev,
struct canfd_frame **cfd);
+struct sk_buff *alloc_canxl_skb(struct net_device *dev,
+ struct canxl_frame **cfx,
+ unsigned int data_len);
struct sk_buff *alloc_can_err_skb(struct net_device *dev,
struct can_frame **cf);
bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb);
/*
@@ -112,15 +115,33 @@ static inline bool can_is_canfd_skb(const struct sk_buff *skb)
/* the CAN specific type of skb is identified by its data length */
return (skb->len == CANFD_MTU && cfd->len <= CANFD_MAX_DLEN);
}
-/* get length element value from can[fd]_frame structure */
+static inline bool can_is_canxl_skb(const struct sk_buff *skb)
+{
+ const struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
+
+ if (skb->len < CANXL_HDR_SIZE + CANXL_MIN_DLEN || skb->len > CANXL_MTU)
+ return false;
+
+ /* this also checks valid CAN XL data length boundaries */
+ if (skb->len != CANXL_HDR_SIZE + cfx->len)
+ return false;
+
+ return cfx->flags & CANXL_XLF;
+}
+
+/* get length element value from can[|fd|xl]_frame structure */
static inline unsigned int can_skb_get_len_val(struct sk_buff *skb)
{
+ const struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
+ if (can_is_canxl_skb(skb))
+ return cfx->len;
+
return cfd->len;
}
/* get needed data length inside CAN frame for all frame types (RTR aware) */
static inline unsigned int can_skb_get_data_len(struct sk_buff *skb)
diff --git a/include/uapi/linux/if_ether.h b/include/uapi/linux/if_ether.h
index d370165bc621..69e0457eb200 100644
--- a/include/uapi/linux/if_ether.h
+++ b/include/uapi/linux/if_ether.h
@@ -136,10 +136,11 @@
#define ETH_P_WAN_PPP 0x0007 /* Dummy type for WAN PPP frames*/
#define ETH_P_PPP_MP 0x0008 /* Dummy type for PPP MP frames */
#define ETH_P_LOCALTALK 0x0009 /* Localtalk pseudo type */
#define ETH_P_CAN 0x000C /* CAN: Controller Area Network */
#define ETH_P_CANFD 0x000D /* CANFD: CAN flexible data rate*/
+#define ETH_P_CANXL 0x000E /* CANXL: eXtended frame Length */
#define ETH_P_PPPTALK 0x0010 /* Dummy type for Atalk over PPP*/
#define ETH_P_TR_802_2 0x0011 /* 802.2 frames */
#define ETH_P_MOBITEX 0x0015 /* Mobitex (kaz@cafe.net) */
#define ETH_P_CONTROL 0x0016 /* Card specific control frames */
#define ETH_P_IRDA 0x0017 /* Linux-IrDA */
diff --git a/net/can/af_can.c b/net/can/af_can.c
index 072a6a5c9dd1..9503ab10f9b8 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -200,11 +200,13 @@ int can_send(struct sk_buff *skb, int loop)
{
struct sk_buff *newskb = NULL;
struct can_pkg_stats *pkg_stats = dev_net(skb->dev)->can.pkg_stats;
int err = -EINVAL;
- if (can_is_can_skb(skb)) {
+ if (can_is_canxl_skb(skb)) {
+ skb->protocol = htons(ETH_P_CANXL);
+ } else if (can_is_can_skb(skb)) {
skb->protocol = htons(ETH_P_CAN);
} else if (can_is_canfd_skb(skb)) {
struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
skb->protocol = htons(ETH_P_CANFD);
@@ -700,10 +702,25 @@ static int canfd_rcv(struct sk_buff *skb, struct net_device *dev,
can_receive(skb, dev);
return NET_RX_SUCCESS;
}
+static int canxl_rcv(struct sk_buff *skb, struct net_device *dev,
+ struct packet_type *pt, struct net_device *orig_dev)
+{
+ if (unlikely(dev->type != ARPHRD_CAN || (!can_is_canxl_skb(skb)))) {
+ pr_warn_once("PF_CAN: dropped non conform CAN XL skbuff: dev type %d, len %d\n",
+ dev->type, skb->len);
+
+ kfree_skb(skb);
+ return NET_RX_DROP;
+ }
+
+ can_receive(skb, dev);
+ return NET_RX_SUCCESS;
+}
+
/* af_can protocol functions */
/**
* can_proto_register - register CAN transport protocol
* @cp: pointer to CAN protocol structure
@@ -824,10 +841,15 @@ static struct packet_type can_packet __read_mostly = {
static struct packet_type canfd_packet __read_mostly = {
.type = cpu_to_be16(ETH_P_CANFD),
.func = canfd_rcv,
};
+static struct packet_type canxl_packet __read_mostly = {
+ .type = cpu_to_be16(ETH_P_CANXL),
+ .func = canxl_rcv,
+};
+
static const struct net_proto_family can_family_ops = {
.family = PF_CAN,
.create = can_create,
.owner = THIS_MODULE,
};
@@ -863,10 +885,11 @@ static __init int can_init(void)
if (err)
goto out_sock;
dev_add_pack(&can_packet);
dev_add_pack(&canfd_packet);
+ dev_add_pack(&canxl_packet);
return 0;
out_sock:
unregister_pernet_subsys(&can_pernet_ops);
--
2.30.2
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v8 6/7] can: dev: add CAN XL support to virtual CAN
2022-08-01 19:00 [PATCH v8 0/7] can: support CAN XL Oliver Hartkopp
` (4 preceding siblings ...)
2022-08-01 19:00 ` [PATCH v8 5/7] can: canxl: update CAN infrastructure for CAN XL frames Oliver Hartkopp
@ 2022-08-01 19:00 ` Oliver Hartkopp
2022-08-01 19:00 ` [PATCH v8 7/7] can: raw: add CAN XL support Oliver Hartkopp
2022-08-31 11:56 ` [PATCH v8 0/7] can: support CAN XL Oliver Hartkopp
7 siblings, 0 replies; 24+ messages in thread
From: Oliver Hartkopp @ 2022-08-01 19:00 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp
Make use of new can_skb_get_data_len() helper.
Add support for variable CANXL MTU using the new can_is_canxl_dev_mtu().
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
drivers/net/can/vcan.c | 12 ++++++------
drivers/net/can/vxcan.c | 8 ++++----
include/linux/can/dev.h | 5 +++++
3 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/drivers/net/can/vcan.c b/drivers/net/can/vcan.c
index 36b6310a2e5b..285635c23443 100644
--- a/drivers/net/can/vcan.c
+++ b/drivers/net/can/vcan.c
@@ -69,33 +69,32 @@ static bool echo; /* echo testing. Default: 0 (Off) */
module_param(echo, bool, 0444);
MODULE_PARM_DESC(echo, "Echo sent frames (for testing). Default: 0 (Off)");
static void vcan_rx(struct sk_buff *skb, struct net_device *dev)
{
- struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
struct net_device_stats *stats = &dev->stats;
stats->rx_packets++;
- stats->rx_bytes += cfd->len;
+ stats->rx_bytes += can_skb_get_data_len(skb);
skb->pkt_type = PACKET_BROADCAST;
skb->dev = dev;
skb->ip_summed = CHECKSUM_UNNECESSARY;
netif_rx(skb);
}
static netdev_tx_t vcan_tx(struct sk_buff *skb, struct net_device *dev)
{
- struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
struct net_device_stats *stats = &dev->stats;
- int loop, len;
+ unsigned int len;
+ int loop;
if (can_dropped_invalid_skb(dev, skb))
return NETDEV_TX_OK;
- len = cfd->can_id & CAN_RTR_FLAG ? 0 : cfd->len;
+ len = can_skb_get_data_len(skb);
stats->tx_packets++;
stats->tx_bytes += len;
/* set flag whether this packet has to be looped back */
loop = skb->pkt_type == PACKET_LOOPBACK;
@@ -135,11 +134,12 @@ static int vcan_change_mtu(struct net_device *dev, int new_mtu)
{
/* Do not allow changing the MTU while running */
if (dev->flags & IFF_UP)
return -EBUSY;
- if (new_mtu != CAN_MTU && new_mtu != CANFD_MTU)
+ if (new_mtu != CAN_MTU && new_mtu != CANFD_MTU &&
+ !can_is_canxl_dev_mtu(new_mtu))
return -EINVAL;
dev->mtu = new_mtu;
return 0;
}
diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c
index cffd107d8b28..26a472d2ea58 100644
--- a/drivers/net/can/vxcan.c
+++ b/drivers/net/can/vxcan.c
@@ -36,14 +36,13 @@ struct vxcan_priv {
static netdev_tx_t vxcan_xmit(struct sk_buff *oskb, struct net_device *dev)
{
struct vxcan_priv *priv = netdev_priv(dev);
struct net_device *peer;
- struct canfd_frame *cfd = (struct canfd_frame *)oskb->data;
struct net_device_stats *peerstats, *srcstats = &dev->stats;
struct sk_buff *skb;
- u8 len;
+ unsigned int len;
if (can_dropped_invalid_skb(dev, oskb))
return NETDEV_TX_OK;
rcu_read_lock();
@@ -68,11 +67,11 @@ static netdev_tx_t vxcan_xmit(struct sk_buff *oskb, struct net_device *dev)
skb->csum_start = 0;
skb->pkt_type = PACKET_BROADCAST;
skb->dev = peer;
skb->ip_summed = CHECKSUM_UNNECESSARY;
- len = cfd->can_id & CAN_RTR_FLAG ? 0 : cfd->len;
+ len = can_skb_get_data_len(skb);
if (netif_rx(skb) == NET_RX_SUCCESS) {
srcstats->tx_packets++;
srcstats->tx_bytes += len;
peerstats = &peer->stats;
peerstats->rx_packets++;
@@ -130,11 +129,12 @@ static int vxcan_change_mtu(struct net_device *dev, int new_mtu)
{
/* Do not allow changing the MTU while running */
if (dev->flags & IFF_UP)
return -EBUSY;
- if (new_mtu != CAN_MTU && new_mtu != CANFD_MTU)
+ if (new_mtu != CAN_MTU && new_mtu != CANFD_MTU &&
+ !can_is_canxl_dev_mtu(new_mtu))
return -EINVAL;
dev->mtu = new_mtu;
return 0;
}
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index c3e50e537e39..58f5431a5559 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -145,10 +145,15 @@ static inline int __must_check can_set_static_ctrlmode(struct net_device *dev,
static inline u32 can_get_static_ctrlmode(struct can_priv *priv)
{
return priv->ctrlmode & ~priv->ctrlmode_supported;
}
+static inline bool can_is_canxl_dev_mtu(unsigned int mtu)
+{
+ return (mtu >= CANXL_MIN_MTU && mtu <= CANXL_MAX_MTU);
+}
+
void can_setup(struct net_device *dev);
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.30.2
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v8 7/7] can: raw: add CAN XL support
2022-08-01 19:00 [PATCH v8 0/7] can: support CAN XL Oliver Hartkopp
` (5 preceding siblings ...)
2022-08-01 19:00 ` [PATCH v8 6/7] can: dev: add CAN XL support to virtual CAN Oliver Hartkopp
@ 2022-08-01 19:00 ` Oliver Hartkopp
2022-08-31 11:56 ` [PATCH v8 0/7] can: support CAN XL Oliver Hartkopp
7 siblings, 0 replies; 24+ messages in thread
From: Oliver Hartkopp @ 2022-08-01 19:00 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp
Enable CAN_RAW sockets to read and write CAN XL frames analogue to the
CAN FD extension (new CAN_RAW_XL_FRAMES sockopt).
A CAN XL network interface is capable to handle Classical CAN, CAN FD and
CAN XL frames. When CAN_RAW_XL_FRAMES is enabled, the CAN_RAW socket checks
whether the addressed CAN network interface is capable to handle the
provided CAN frame.
In opposite to the fixed number of bytes for
- CAN frames (CAN_MTU = sizeof(struct can_frame))
- CAN FD frames (CANFD_MTU = sizeof(struct can_frame))
the number of bytes when reading/writing CAN XL frames depends on the
number of data bytes. For efficiency reasons the length of the struct
canxl_frame is truncated to the needed size for read/write operations.
This leads to a calculated size of CANXL_HDR_SIZE + canxl_frame::len which
is enforced on write() operations and guaranteed on read() operations.
NB: Valid length values are 1 .. 2048 (CANXL_MIN_DLEN .. CANXL_MAX_DLEN).
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
include/uapi/linux/can/raw.h | 1 +
net/can/raw.c | 55 ++++++++++++++++++++++++++++--------
2 files changed, 44 insertions(+), 12 deletions(-)
diff --git a/include/uapi/linux/can/raw.h b/include/uapi/linux/can/raw.h
index 3386aa81fdf2..ff12f525c37c 100644
--- a/include/uapi/linux/can/raw.h
+++ b/include/uapi/linux/can/raw.h
@@ -60,8 +60,9 @@ enum {
CAN_RAW_ERR_FILTER, /* set filter for error frames */
CAN_RAW_LOOPBACK, /* local loopback (default:on) */
CAN_RAW_RECV_OWN_MSGS, /* receive my own msgs (default:off) */
CAN_RAW_FD_FRAMES, /* allow CAN FD frames (default:off) */
CAN_RAW_JOIN_FILTERS, /* all filters must match to trigger */
+ CAN_RAW_XL_FRAMES, /* allow CAN XL frames (default:off) */
};
#endif /* !_UAPI_CAN_RAW_H */
diff --git a/net/can/raw.c b/net/can/raw.c
index d1bd9cc51ebe..c6859a061a7c 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -48,10 +48,11 @@
#include <linux/socket.h>
#include <linux/if_arp.h>
#include <linux/skbuff.h>
#include <linux/can.h>
#include <linux/can/core.h>
+#include <linux/can/dev.h> /* for can_is_canxl_dev_mtu() */
#include <linux/can/skb.h>
#include <linux/can/raw.h>
#include <net/sock.h>
#include <net/net_namespace.h>
@@ -85,10 +86,11 @@ struct raw_sock {
int ifindex;
struct list_head notifier;
int loopback;
int recv_own_msgs;
int fd_frames;
+ int xl_frames;
int join_filters;
int count; /* number of active filters */
struct can_filter dfilter; /* default/single filter */
struct can_filter *filter; /* pointer to filter(s) */
can_err_mask_t err_mask;
@@ -127,12 +129,13 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
/* check the received tx sock reference */
if (!ro->recv_own_msgs && oskb->sk == sk)
return;
- /* do not pass non-CAN2.0 frames to a legacy socket */
- if (!ro->fd_frames && oskb->len != CAN_MTU)
+ /* make sure to not pass oversized frames to the socket */
+ if ((can_is_canfd_skb(oskb) && !ro->fd_frames && !ro->xl_frames) ||
+ (can_is_canxl_skb(oskb) && !ro->xl_frames))
return;
/* eliminate multiple filter matches for the same skb */
if (this_cpu_ptr(ro->uniq)->skb == oskb &&
this_cpu_ptr(ro->uniq)->skbcnt == can_skb_prv(oskb)->skbcnt) {
@@ -344,10 +347,11 @@ static int raw_init(struct sock *sk)
/* set default loopback behaviour */
ro->loopback = 1;
ro->recv_own_msgs = 0;
ro->fd_frames = 0;
+ ro->xl_frames = 0;
ro->join_filters = 0;
/* alloc_percpu provides zero'ed memory */
ro->uniq = alloc_percpu(struct uniqframe);
if (unlikely(!ro->uniq))
@@ -667,10 +671,19 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
if (copy_from_sockptr(&ro->fd_frames, optval, optlen))
return -EFAULT;
break;
+ case CAN_RAW_XL_FRAMES:
+ if (optlen != sizeof(ro->xl_frames))
+ return -EINVAL;
+
+ if (copy_from_sockptr(&ro->xl_frames, optval, optlen))
+ return -EFAULT;
+
+ break;
+
case CAN_RAW_JOIN_FILTERS:
if (optlen != sizeof(ro->join_filters))
return -EINVAL;
if (copy_from_sockptr(&ro->join_filters, optval, optlen))
@@ -749,10 +762,16 @@ static int raw_getsockopt(struct socket *sock, int level, int optname,
if (len > sizeof(int))
len = sizeof(int);
val = &ro->fd_frames;
break;
+ case CAN_RAW_XL_FRAMES:
+ if (len > sizeof(int))
+ len = sizeof(int);
+ val = &ro->xl_frames;
+ break;
+
case CAN_RAW_JOIN_FILTERS:
if (len > sizeof(int))
len = sizeof(int);
val = &ro->join_filters;
break;
@@ -774,11 +793,15 @@ static int raw_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
struct raw_sock *ro = raw_sk(sk);
struct sockcm_cookie sockc;
struct sk_buff *skb;
struct net_device *dev;
int ifindex;
- int err;
+ int err = -EINVAL;
+
+ /* check for valid CAN frame sizes */
+ if (size < CANXL_HDR_SIZE + CANXL_MIN_DLEN || size > CANXL_MTU)
+ return -EINVAL;
if (msg->msg_name) {
DECLARE_SOCKADDR(struct sockaddr_can *, addr, msg->msg_name);
if (msg->msg_namelen < RAW_MIN_NAMELEN)
@@ -794,32 +817,40 @@ static int raw_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
dev = dev_get_by_index(sock_net(sk), ifindex);
if (!dev)
return -ENXIO;
- err = -EINVAL;
- if (ro->fd_frames && dev->mtu == CANFD_MTU) {
- if (unlikely(size != CANFD_MTU && size != CAN_MTU))
- goto put_dev;
- } else {
- if (unlikely(size != CAN_MTU))
- goto put_dev;
- }
-
skb = sock_alloc_send_skb(sk, size + sizeof(struct can_skb_priv),
msg->msg_flags & MSG_DONTWAIT, &err);
if (!skb)
goto put_dev;
can_skb_reserve(skb);
can_skb_prv(skb)->ifindex = dev->ifindex;
can_skb_prv(skb)->skbcnt = 0;
+ /* fill the skb before testing for valid CAN frames */
err = memcpy_from_msg(skb_put(skb, size), msg, size);
if (err < 0)
goto free_skb;
+ err = -EINVAL;
+ if (ro->xl_frames && can_is_canxl_dev_mtu(dev->mtu)) {
+ /* CAN XL, CAN FD and Classical CAN */
+ if (!can_is_canxl_skb(skb) && !can_is_canfd_skb(skb) &&
+ !can_is_can_skb(skb))
+ goto free_skb;
+ } else if (ro->fd_frames && dev->mtu == CANFD_MTU) {
+ /* CAN FD and Classical CAN */
+ if (!can_is_canfd_skb(skb) && !can_is_can_skb(skb))
+ goto free_skb;
+ } else {
+ /* Classical CAN */
+ if (!can_is_can_skb(skb))
+ goto free_skb;
+ }
+
sockcm_init(&sockc, sk);
if (msg->msg_controllen) {
err = sock_cmsg_send(sk, msg, &sockc);
if (unlikely(err))
goto free_skb;
--
2.30.2
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v8 0/7] can: support CAN XL
2022-08-01 19:00 [PATCH v8 0/7] can: support CAN XL Oliver Hartkopp
` (6 preceding siblings ...)
2022-08-01 19:00 ` [PATCH v8 7/7] can: raw: add CAN XL support Oliver Hartkopp
@ 2022-08-31 11:56 ` Oliver Hartkopp
2022-09-11 8:08 ` Vincent MAILHOL
7 siblings, 1 reply; 24+ messages in thread
From: Oliver Hartkopp @ 2022-08-31 11:56 UTC (permalink / raw)
To: Vincent Mailhol; +Cc: linux-can
Hi Vincent,
I have discussed with Marc about this patch set and after our discussion
about len/flags he was missing an ACK from your side:
https://lore.kernel.org/linux-can/247226f6-b9b5-ec47-2234-d1cc70ee6954@hartkopp.net/T/#u
Are you fine with this V8 patch set now?
I already created some CAN CiA p-o-c code for CAN XL segmentation:
https://github.com/hartkopp/can-cia-613-3-poc
When the patch set is committed and the definitions are fixed I would
also start with some documentation.
Best regards,
Oliver
On 01.08.22 21:00, Oliver Hartkopp wrote:
> The CAN with eXtended data Length (CAN XL) is a new CAN protocol with a
> 10Mbit/s data transfer with a new physical layer transceiver (for this
> data section). CAN XL allows up to 2048 byte of payload and shares the
> arbitration principle (11 bit priority) known from Classical CAN and
> CAN FD. RTR and 29 bit identifiers are not implemented in CAN XL.
>
> A short introdution to CAN XL can be found here:
> https://www.bosch-semiconductors.com/media/ip_modules/pdf_2/can_xl_1/canxl_intro_20210225.pdf
>
> V2: Major rework after discussion and feedback on Linux-CAN ML
>
> - rework of struct canxl_frame
> - CANXL_XLF flag is now the switch between CAN XL and CAN/CANFD
> - variable length in r/w operations for CAN XL frames
> - write CAN XL frame to raw socket enforces size <-> canxl_frame.len sync
>
> V3: Fix length for CAN XL frames inside the sk_buff
>
> - extend the CAN_RAW sockopt to handle fixed/truncated read/write operations
>
> V4: Fix patch 5 (can: raw: add CAN XL support)
>
> - fix return value (move 'err = -EINVAL' in raw_sendmsg())
> - add CAN XL frame handling in can_rcv()
> - change comment for CAN_RAW_XL_[RT]X_DYN definition (allow -> enable)
>
> V5: Remove CAN_RAW_XL_[RT]X_DYN definition again
>
> - CAN_RAW_XL_[RT]X_DYN (truncated data) feature is now enabled by default
> - use CANXL_MIN_DLEN instead of '1' in canxl_frame definition
> - add missing 'err = -EINVAL' initialization in raw_sendmsg())
>
> V6:
>
> - rework an separate skb identification and length helpers
> - add CANFD_FDF flag in all CAN FD frame structures
> - simplify patches for infrastructure and raw sockets
> - add vxcan support in virtual CAN interface patch
>
> V7:
>
> - fixed indention as remarked by Marc
> - set CANFD_FDF flag when detecting CAN FD frames generated by PF_PACKET
> - Allow to use variable CAN XL MTU sizes to enforce real time requirements
> on CAN XL segments (e.g. to support of CAN CiA segmentation concept)
>
> V8:
>
> - fixed typo as remarked by Vincent
> - rebased to latest can-next/net-next tree
>
> Oliver Hartkopp (7):
> can: skb: unify skb CAN frame identification helpers
> can: skb: add skb CAN frame data length helpers
> can: set CANFD_FDF flag in all CAN FD frame structures
> can: canxl: introduce CAN XL data structure
> can: canxl: update CAN infrastructure for CAN XL frames
> can: dev: add CAN XL support to virtual CAN
> can: raw: add CAN XL support
>
> drivers/net/can/ctucanfd/ctucanfd_base.c | 1 -
> drivers/net/can/dev/rx-offload.c | 2 +-
> drivers/net/can/dev/skb.c | 113 ++++++++++++++++-------
> drivers/net/can/vcan.c | 12 +--
> drivers/net/can/vxcan.c | 8 +-
> include/linux/can/dev.h | 5 +
> include/linux/can/skb.h | 57 +++++++++++-
> include/uapi/linux/can.h | 55 ++++++++++-
> include/uapi/linux/can/raw.h | 1 +
> include/uapi/linux/if_ether.h | 1 +
> net/can/af_can.c | 76 ++++++++-------
> net/can/bcm.c | 9 +-
> net/can/gw.c | 4 +-
> net/can/isotp.c | 2 +-
> net/can/j1939/main.c | 4 +
> net/can/raw.c | 55 ++++++++---
> 16 files changed, 299 insertions(+), 106 deletions(-)
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v8 4/7] can: canxl: introduce CAN XL data structure
2022-08-01 19:00 ` [PATCH v8 4/7] can: canxl: introduce CAN XL data structure Oliver Hartkopp
@ 2022-09-02 6:19 ` Vincent Mailhol
2022-09-02 13:56 ` Oliver Hartkopp
0 siblings, 1 reply; 24+ messages in thread
From: Vincent Mailhol @ 2022-09-02 6:19 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: linux-can
Hi Oliver,
Sorry for the late reply, I am back from holidays. My previous review
was a best effort review.
Since then, I took time to read the CiA’s CAN knowledge pages of CAN XL:
https://www.can-cia.org/can-knowledge/can/can-xl/
and have a few newer comments.
Unfortunately, I still do not have access to the full specification,
so please forgive if there are some silly remarks.
On Tue. 2 Aug 2022 at 04:02, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> This patch adds defines for data structures and length information for
> CAN XL (CAN with eXtended data Length) which can transfer up to 2048
> byte inside a single frame.
>
> Notable changes from CAN FD:
>
> - the 11 bit arbitration field is now named 'priority' instead of 'can_id'
> (there are no 29 bit identifiers nor RTR frames anymore)
> - the data length needs a uint16 value to cover up to 2048 byte
> (the length element position is different to struct can[fd]_frame)
> - new fields (SDT, AF) and a SEC bit have been introduced
> - the virtual CAN interface identifier is not part if the CAN XL frame
> struct as this VCID value is stored in struct skbuff (analog to vlan id)
>
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> ---
> include/uapi/linux/can.h | 51 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 51 insertions(+)
>
> diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h
> index 7b23eeeb3273..dd645ea72306 100644
> --- a/include/uapi/linux/can.h
> +++ b/include/uapi/linux/can.h
> @@ -46,10 +46,11 @@
> #ifndef _UAPI_CAN_H
> #define _UAPI_CAN_H
>
> #include <linux/types.h>
> #include <linux/socket.h>
> +#include <linux/stddef.h> /* for offsetof */
>
> /* controller area network (CAN) kernel definitions */
>
> /* special address description flags for the CAN_ID */
> #define CAN_EFF_FLAG 0x80000000U /* EFF/SFF is set in the MSB */
> @@ -58,10 +59,11 @@
>
> /* valid bits in CAN ID for frame formats */
> #define CAN_SFF_MASK 0x000007FFU /* standard frame format (SFF) */
> #define CAN_EFF_MASK 0x1FFFFFFFU /* extended frame format (EFF) */
> #define CAN_ERR_MASK 0x1FFFFFFFU /* omit EFF, RTR, ERR flags */
> +#define CANXL_PRIO_MASK CAN_SFF_MASK /* 11 bit priority mask */
>
> /*
> * Controller Area Network Identifier structure
> *
> * bit 0-28 : CAN identifier (11/29 bit)
> @@ -71,10 +73,11 @@
> */
> typedef __u32 canid_t;
>
> #define CAN_SFF_ID_BITS 11
> #define CAN_EFF_ID_BITS 29
> +#define CANXL_PRIO_BITS CAN_SFF_ID_BITS
>
> /*
> * Controller Area Network Error Message Frame Mask structure
> *
> * bit 0-28 : error class mask (see include/uapi/linux/can/error.h)
> @@ -89,10 +92,20 @@ typedef __u32 can_err_mask_t;
>
> /* CAN FD payload length and DLC definitions according to ISO 11898-7 */
> #define CANFD_MAX_DLC 15
> #define CANFD_MAX_DLEN 64
>
> +/*
> + * CAN XL payload length and DLC definitions according to ISO 11898-1
> + * CAN XL DLC ranges from 0 .. 2047 => data length from 1 .. 2048 byte
> + */
> +#define CANXL_MIN_DLC 0
> +#define CANXL_MAX_DLC 2047
> +#define CANXL_MAX_DLC_MASK 0x07FF
> +#define CANXL_MIN_DLEN 1
> +#define CANXL_MAX_DLEN 2048
> +
> /**
> * struct can_frame - Classical CAN frame structure (aka CAN 2.0B)
> * @can_id: CAN ID of the frame and CAN_*_FLAG flags, see canid_t definition
> * @len: CAN frame payload length in byte (0 .. 8)
> * @can_dlc: deprecated name for CAN frame payload length in byte (0 .. 8)
> @@ -164,12 +177,50 @@ struct canfd_frame {
> __u8 __res0; /* reserved / padding */
> __u8 __res1; /* reserved / padding */
> __u8 data[CANFD_MAX_DLEN] __attribute__((aligned(8)));
> };
>
> +/*
> + * defined bits for canxl_frame.flags
> + *
> + * The canxl_frame.flags element contains two bits CANXL_XLF and CANXL_SEC
> + * and shares the relative position of the struct can[fd]_frame.len element.
> + * The CANXL_XLF bit ALWAYS needs to be set to indicate a valid CAN XL frame.
> + * As a side effect setting this bit intentionally breaks the length checks
> + * for Classical CAN and CAN FD frames.
> + *
> + * Undefined bits in canxl_frame.flags are reserved and shall be set to zero.
> + */
> +#define CANXL_XLF 0x80 /* mandatory CAN XL frame flag (must always be set!) */
> +#define CANXL_SEC 0x01 /* Simple Extended Content (security/segmentation) */
Are we sure that no other flags are needed? The CiA can-knowledge
mentions a FTYP (frame type) flag. Do you know what it is?
> +
> +/**
> + * struct canxl_frame - CAN with e'X'tended frame 'L'ength frame structure
> + * @prio: 11 bit arbitration priority with zero'ed CAN_*_FLAG flags
> + * @flags: additional flags for CAN XL
> + * @sdt: SDU (service data unit) type
> + * @len: frame payload length in byte (CANXL_MIN_DLEN .. CANXL_MAX_DLEN)
> + * @af: acceptance field
> + * @data: CAN XL frame payload (CANXL_MIN_DLEN .. CANXL_MAX_DLEN byte)
> + *
> + * @prio shares the same position as @can_id from struct can[fd]_frame.
> + */
> +struct canxl_frame {
> + canid_t prio; /* 11 bit priority for arbitration (canid_t) */
Does it make sense to keep the canid_t? The addressing is done through
both the prio and the af fields. Also, the CAN_EFF_FLAG, CAN_RTR_FLAG
and CAN_ERR_FLAG flags do not apply anymore. Finally, the prio is only
11 bits, no need to reserve 32 bits for it.
Of course, if we declare it as 16 bits, we need to add some padding so
that the other flags remain at the same offset.
> + __u8 flags; /* additional flags for CAN XL */
> + __u8 sdt; /* SDU (service data unit) type */
> + __u16 len; /* frame payload length in byte */
> + __u32 af; /* acceptance field */
> + __u8 data[CANXL_MAX_DLEN];
> +};
The CiA CAN knowledge also mentions a VCID (virtual CAN network ID).
Is this not needed?
> +
> #define CAN_MTU (sizeof(struct can_frame))
> #define CANFD_MTU (sizeof(struct canfd_frame))
> +#define CANXL_MTU (sizeof(struct canxl_frame))
> +#define CANXL_HDR_SIZE (offsetof(struct canxl_frame, data))
> +#define CANXL_MIN_MTU (CANXL_HDR_SIZE + 64)
> +#define CANXL_MAX_MTU CANXL_MTU
>
> /* particular protocols of the protocol family PF_CAN */
> #define CAN_RAW 1 /* RAW sockets */
> #define CAN_BCM 2 /* Broadcast Manager */
> #define CAN_TP16 3 /* VAG Transport Protocol v1.6 */
CAN XL has a resXL flag, suggesting that there may be a fourth
generation of the CAN protocol. One issue in the initial socket CAN
design is that we never reserved a flag for future versions. Here, we
are lucky that the bit 0x80 of the length field is never set.
Back to CAN XL, I would like to discuss how we will distinguish the
CAN GEN4 frames from the CAN XL ones so that we do not find ourselves
stuck in a couple of years because no bits are left to differentiate
things.
One solution would be to set the two most significant bit:
#define CAN_GEN4 0xC0
and the test would be:
if (cf.flags & CAN_GEN4 == CAN_GEN4)
Does anyone have a better idea?
Yours sincerely,
Vincent Mailhol
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v8 4/7] can: canxl: introduce CAN XL data structure
2022-09-02 6:19 ` Vincent Mailhol
@ 2022-09-02 13:56 ` Oliver Hartkopp
2022-09-08 7:24 ` Oliver Hartkopp
0 siblings, 1 reply; 24+ messages in thread
From: Oliver Hartkopp @ 2022-09-02 13:56 UTC (permalink / raw)
To: Vincent Mailhol; +Cc: linux-can
Hi Vincent,
On 02.09.22 08:19, Vincent Mailhol wrote:
> Sorry for the late reply, I am back from holidays. My previous review
> was a best effort review.
Hope you had good holidays ;-)
> Since then, I took time to read the CiA’s CAN knowledge pages of CAN XL:
> https://www.can-cia.org/can-knowledge/can/can-xl/
> and have a few newer comments.
>
> Unfortunately, I still do not have access to the full specification,
> so please forgive if there are some silly remarks.
I think the short introduction to CAN XL from the cover letter fits
quite good.
https://www.bosch-semiconductors.com/media/ip_modules/pdf_2/can_xl_1/canxl_intro_20210225.pdf
> On Tue. 2 Aug 2022 at 04:02, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>> This patch adds defines for data structures and length information for
>> CAN XL (CAN with eXtended data Length) which can transfer up to 2048
>> byte inside a single frame.
>>
>> Notable changes from CAN FD:
>>
>> - the 11 bit arbitration field is now named 'priority' instead of 'can_id'
>> (there are no 29 bit identifiers nor RTR frames anymore)
>> - the data length needs a uint16 value to cover up to 2048 byte
>> (the length element position is different to struct can[fd]_frame)
>> - new fields (SDT, AF) and a SEC bit have been introduced
>> - the virtual CAN interface identifier is not part if the CAN XL frame
>> struct as this VCID value is stored in struct skbuff (analog to vlan id)
Did you note this?
Because you asked about the VCID later.
There is an existing Kernel API to handle VLAN-like ID stuff. And we
should simply use that existing mechanic as it has the same intention
and handling for CAN XL.
>> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
>> ---
>> include/uapi/linux/can.h | 51 ++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 51 insertions(+)
>>
>> diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h
>> index 7b23eeeb3273..dd645ea72306 100644
>> --- a/include/uapi/linux/can.h
>> +++ b/include/uapi/linux/can.h
>> @@ -46,10 +46,11 @@
>> #ifndef _UAPI_CAN_H
>> #define _UAPI_CAN_H
>>
>> #include <linux/types.h>
>> #include <linux/socket.h>
>> +#include <linux/stddef.h> /* for offsetof */
>>
>> /* controller area network (CAN) kernel definitions */
>>
>> /* special address description flags for the CAN_ID */
>> #define CAN_EFF_FLAG 0x80000000U /* EFF/SFF is set in the MSB */
>> @@ -58,10 +59,11 @@
>>
>> /* valid bits in CAN ID for frame formats */
>> #define CAN_SFF_MASK 0x000007FFU /* standard frame format (SFF) */
>> #define CAN_EFF_MASK 0x1FFFFFFFU /* extended frame format (EFF) */
>> #define CAN_ERR_MASK 0x1FFFFFFFU /* omit EFF, RTR, ERR flags */
>> +#define CANXL_PRIO_MASK CAN_SFF_MASK /* 11 bit priority mask */
>>
>> /*
>> * Controller Area Network Identifier structure
>> *
>> * bit 0-28 : CAN identifier (11/29 bit)
>> @@ -71,10 +73,11 @@
>> */
>> typedef __u32 canid_t;
>>
>> #define CAN_SFF_ID_BITS 11
>> #define CAN_EFF_ID_BITS 29
>> +#define CANXL_PRIO_BITS CAN_SFF_ID_BITS
>>
>> /*
>> * Controller Area Network Error Message Frame Mask structure
>> *
>> * bit 0-28 : error class mask (see include/uapi/linux/can/error.h)
>> @@ -89,10 +92,20 @@ typedef __u32 can_err_mask_t;
>>
>> /* CAN FD payload length and DLC definitions according to ISO 11898-7 */
>> #define CANFD_MAX_DLC 15
>> #define CANFD_MAX_DLEN 64
>>
>> +/*
>> + * CAN XL payload length and DLC definitions according to ISO 11898-1
>> + * CAN XL DLC ranges from 0 .. 2047 => data length from 1 .. 2048 byte
>> + */
>> +#define CANXL_MIN_DLC 0
>> +#define CANXL_MAX_DLC 2047
>> +#define CANXL_MAX_DLC_MASK 0x07FF
>> +#define CANXL_MIN_DLEN 1
>> +#define CANXL_MAX_DLEN 2048
>> +
>> /**
>> * struct can_frame - Classical CAN frame structure (aka CAN 2.0B)
>> * @can_id: CAN ID of the frame and CAN_*_FLAG flags, see canid_t definition
>> * @len: CAN frame payload length in byte (0 .. 8)
>> * @can_dlc: deprecated name for CAN frame payload length in byte (0 .. 8)
>> @@ -164,12 +177,50 @@ struct canfd_frame {
>> __u8 __res0; /* reserved / padding */
>> __u8 __res1; /* reserved / padding */
>> __u8 data[CANFD_MAX_DLEN] __attribute__((aligned(8)));
>> };
>>
>> +/*
>> + * defined bits for canxl_frame.flags
>> + *
>> + * The canxl_frame.flags element contains two bits CANXL_XLF and CANXL_SEC
>> + * and shares the relative position of the struct can[fd]_frame.len element.
>> + * The CANXL_XLF bit ALWAYS needs to be set to indicate a valid CAN XL frame.
>> + * As a side effect setting this bit intentionally breaks the length checks
>> + * for Classical CAN and CAN FD frames.
>> + *
>> + * Undefined bits in canxl_frame.flags are reserved and shall be set to zero.
>> + */
>> +#define CANXL_XLF 0x80 /* mandatory CAN XL frame flag (must always be set!) */
>> +#define CANXL_SEC 0x01 /* Simple Extended Content (security/segmentation) */
>
> Are we sure that no other flags are needed? The CiA can-knowledge
> mentions a FTYP (frame type) flag. Do you know what it is?
The LLC frame fields in
https://www.can-cia.org/can-knowledge/can/can-xl/ are a mix(!) of
Classical CAN, CAN FD and CAN XL content (e.g. ESI is only CAN FD).
The frame type flags an abstraction of
(none) -> Classical CAN
(FDF) -> CAN FD
(XLF) -> CAN XL
>> +
>> +/**
>> + * struct canxl_frame - CAN with e'X'tended frame 'L'ength frame structure
>> + * @prio: 11 bit arbitration priority with zero'ed CAN_*_FLAG flags
>> + * @flags: additional flags for CAN XL
>> + * @sdt: SDU (service data unit) type
>> + * @len: frame payload length in byte (CANXL_MIN_DLEN .. CANXL_MAX_DLEN)
>> + * @af: acceptance field
>> + * @data: CAN XL frame payload (CANXL_MIN_DLEN .. CANXL_MAX_DLEN byte)
>> + *
>> + * @prio shares the same position as @can_id from struct can[fd]_frame.
>> + */
>> +struct canxl_frame {
>> + canid_t prio; /* 11 bit priority for arbitration (canid_t) */
>
> Does it make sense to keep the canid_t? The addressing is done through
> both the prio and the af fields. Also, the CAN_EFF_FLAG, CAN_RTR_FLAG
> and CAN_ERR_FLAG flags do not apply anymore. Finally, the prio is only
> 11 bits, no need to reserve 32 bits for it.
The CAN-ID was (due to its arbitration nature) always also a priority field.
So nothing changed here. There is no RTR and no 29-bit priority anymore
now. The RTR flag has already been disabled for CAN FD (see presentation
at the end of this mail).
Therefore it makes sense to handle the SFF 11-bit prio analogue to the
former CAN-ID - and also use canid_t to simply keep the entire CAN
filter handling in af_can.c !
The new idea in CAN XL is to split the priority from the content
identification which has been named 'acceptance field' (AC).
The functionality of AC depends on SDT. Don't know how this will work
out in the future and if we would provide some AF-specific filtering
inside the kernel.
>
> Of course, if we declare it as 16 bits, we need to add some padding so
> that the other flags remain at the same offset.
Answered above.
>> + __u8 flags; /* additional flags for CAN XL */
>> + __u8 sdt; /* SDU (service data unit) type */
>> + __u16 len; /* frame payload length in byte */
>> + __u32 af; /* acceptance field */
>> + __u8 data[CANXL_MAX_DLEN];
>> +};
>
> The CiA CAN knowledge also mentions a VCID (virtual CAN network ID).
> Is this not needed?
Answered above.
>> +
>> #define CAN_MTU (sizeof(struct can_frame))
>> #define CANFD_MTU (sizeof(struct canfd_frame))
>> +#define CANXL_MTU (sizeof(struct canxl_frame))
>> +#define CANXL_HDR_SIZE (offsetof(struct canxl_frame, data))
>> +#define CANXL_MIN_MTU (CANXL_HDR_SIZE + 64)
>> +#define CANXL_MAX_MTU CANXL_MTU
>>
>> /* particular protocols of the protocol family PF_CAN */
>> #define CAN_RAW 1 /* RAW sockets */
>> #define CAN_BCM 2 /* Broadcast Manager */
>> #define CAN_TP16 3 /* VAG Transport Protocol v1.6 */
>
> CAN XL has a resXL flag, suggesting that there may be a fourth
> generation of the CAN protocol. One issue in the initial socket CAN
> design is that we never reserved a flag for future versions. Here, we
> are lucky that the bit 0x80 of the length field is never set.
> Back to CAN XL, I would like to discuss how we will distinguish the
> CAN GEN4 frames from the CAN XL ones so that we do not find ourselves
> stuck in a couple of years because no bits are left to differentiate
> things.
>
> One solution would be to set the two most significant bit:
> #define CAN_GEN4 0xC0
>
> and the test would be:
> if (cf.flags & CAN_GEN4 == CAN_GEN4)
Please do not mix up (incompatible) extensions that have been
implemented in the CAN protocol through the 'wandering' reserved bit
(r0, now resXL) with the CAN(FD/XL) data structure.
The first suggestion for a CAN frame data structure representation in
IEEE 1722 contained the entire bitstream definition including the IDE
bit. That splitted the 11 and 18 bits of the CAN ID inside the data
structure :-D
I really think, that the journey is ending with CAN XL and when there
would be really a new improvement, we will take a look at it. E.g. by
extending the CAN XL flags from the top down (as you suggested with 0xC0).
But who knows how much of the CAN XL frame layout would be re-used by
CAN XY ?? ¯\_(ツ)_/¯
Btw. I uploaded a presentation which shows the way from classical CAN to
CAN XL which might depict some relations of the bits in a clearer way:
https://github.com/linux-can/can-doc/blob/master/presentations/CAN-XL-Intro.pdf
Best regards,
Oliver
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v8 4/7] can: canxl: introduce CAN XL data structure
2022-09-02 13:56 ` Oliver Hartkopp
@ 2022-09-08 7:24 ` Oliver Hartkopp
2022-09-11 7:50 ` Vincent Mailhol
0 siblings, 1 reply; 24+ messages in thread
From: Oliver Hartkopp @ 2022-09-08 7:24 UTC (permalink / raw)
To: Vincent Mailhol, Marc Kleine-Budde; +Cc: linux-can
Hi Vincent,
any update on this or are you fine with the patch set now?
Best regards,
Oliver
On 02.09.22 15:56, Oliver Hartkopp wrote:
> Hi Vincent,
>
> On 02.09.22 08:19, Vincent Mailhol wrote:
>> Sorry for the late reply, I am back from holidays. My previous review
>> was a best effort review.
>
> Hope you had good holidays ;-)
>
>> Since then, I took time to read the CiA’s CAN knowledge pages of CAN XL:
>> https://www.can-cia.org/can-knowledge/can/can-xl/
>> and have a few newer comments.
>>
>> Unfortunately, I still do not have access to the full specification,
>> so please forgive if there are some silly remarks.
>
> I think the short introduction to CAN XL from the cover letter fits
> quite good.
>
> https://www.bosch-semiconductors.com/media/ip_modules/pdf_2/can_xl_1/canxl_intro_20210225.pdf
>
>
>> On Tue. 2 Aug 2022 at 04:02, Oliver Hartkopp <socketcan@hartkopp.net>
>> wrote:
>>> This patch adds defines for data structures and length information for
>>> CAN XL (CAN with eXtended data Length) which can transfer up to 2048
>>> byte inside a single frame.
>>>
>>> Notable changes from CAN FD:
>>>
>>> - the 11 bit arbitration field is now named 'priority' instead of
>>> 'can_id'
>>> (there are no 29 bit identifiers nor RTR frames anymore)
>>> - the data length needs a uint16 value to cover up to 2048 byte
>>> (the length element position is different to struct can[fd]_frame)
>>> - new fields (SDT, AF) and a SEC bit have been introduced
>>> - the virtual CAN interface identifier is not part if the CAN XL frame
>>> struct as this VCID value is stored in struct skbuff (analog to
>>> vlan id)
>
> Did you note this?
>
> Because you asked about the VCID later.
>
> There is an existing Kernel API to handle VLAN-like ID stuff. And we
> should simply use that existing mechanic as it has the same intention
> and handling for CAN XL.
>
>>> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
>>> ---
>>> include/uapi/linux/can.h | 51 ++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 51 insertions(+)
>>>
>>> diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h
>>> index 7b23eeeb3273..dd645ea72306 100644
>>> --- a/include/uapi/linux/can.h
>>> +++ b/include/uapi/linux/can.h
>>> @@ -46,10 +46,11 @@
>>> #ifndef _UAPI_CAN_H
>>> #define _UAPI_CAN_H
>>>
>>> #include <linux/types.h>
>>> #include <linux/socket.h>
>>> +#include <linux/stddef.h> /* for offsetof */
>>>
>>> /* controller area network (CAN) kernel definitions */
>>>
>>> /* special address description flags for the CAN_ID */
>>> #define CAN_EFF_FLAG 0x80000000U /* EFF/SFF is set in the MSB */
>>> @@ -58,10 +59,11 @@
>>>
>>> /* valid bits in CAN ID for frame formats */
>>> #define CAN_SFF_MASK 0x000007FFU /* standard frame format (SFF) */
>>> #define CAN_EFF_MASK 0x1FFFFFFFU /* extended frame format (EFF) */
>>> #define CAN_ERR_MASK 0x1FFFFFFFU /* omit EFF, RTR, ERR flags */
>>> +#define CANXL_PRIO_MASK CAN_SFF_MASK /* 11 bit priority mask */
>>>
>>> /*
>>> * Controller Area Network Identifier structure
>>> *
>>> * bit 0-28 : CAN identifier (11/29 bit)
>>> @@ -71,10 +73,11 @@
>>> */
>>> typedef __u32 canid_t;
>>>
>>> #define CAN_SFF_ID_BITS 11
>>> #define CAN_EFF_ID_BITS 29
>>> +#define CANXL_PRIO_BITS CAN_SFF_ID_BITS
>>>
>>> /*
>>> * Controller Area Network Error Message Frame Mask structure
>>> *
>>> * bit 0-28 : error class mask (see include/uapi/linux/can/error.h)
>>> @@ -89,10 +92,20 @@ typedef __u32 can_err_mask_t;
>>>
>>> /* CAN FD payload length and DLC definitions according to ISO
>>> 11898-7 */
>>> #define CANFD_MAX_DLC 15
>>> #define CANFD_MAX_DLEN 64
>>>
>>> +/*
>>> + * CAN XL payload length and DLC definitions according to ISO 11898-1
>>> + * CAN XL DLC ranges from 0 .. 2047 => data length from 1 .. 2048 byte
>>> + */
>>> +#define CANXL_MIN_DLC 0
>>> +#define CANXL_MAX_DLC 2047
>>> +#define CANXL_MAX_DLC_MASK 0x07FF
>>> +#define CANXL_MIN_DLEN 1
>>> +#define CANXL_MAX_DLEN 2048
>>> +
>>> /**
>>> * struct can_frame - Classical CAN frame structure (aka CAN 2.0B)
>>> * @can_id: CAN ID of the frame and CAN_*_FLAG flags, see canid_t
>>> definition
>>> * @len: CAN frame payload length in byte (0 .. 8)
>>> * @can_dlc: deprecated name for CAN frame payload length in byte
>>> (0 .. 8)
>>> @@ -164,12 +177,50 @@ struct canfd_frame {
>>> __u8 __res0; /* reserved / padding */
>>> __u8 __res1; /* reserved / padding */
>>> __u8 data[CANFD_MAX_DLEN] __attribute__((aligned(8)));
>>> };
>>>
>>> +/*
>>> + * defined bits for canxl_frame.flags
>>> + *
>>> + * The canxl_frame.flags element contains two bits CANXL_XLF and
>>> CANXL_SEC
>>> + * and shares the relative position of the struct can[fd]_frame.len
>>> element.
>>> + * The CANXL_XLF bit ALWAYS needs to be set to indicate a valid CAN
>>> XL frame.
>>> + * As a side effect setting this bit intentionally breaks the length
>>> checks
>>> + * for Classical CAN and CAN FD frames.
>>> + *
>>> + * Undefined bits in canxl_frame.flags are reserved and shall be set
>>> to zero.
>>> + */
>>> +#define CANXL_XLF 0x80 /* mandatory CAN XL frame flag (must always
>>> be set!) */
>>> +#define CANXL_SEC 0x01 /* Simple Extended Content
>>> (security/segmentation) */
>>
>> Are we sure that no other flags are needed? The CiA can-knowledge
>> mentions a FTYP (frame type) flag. Do you know what it is?
>
> The LLC frame fields in
> https://www.can-cia.org/can-knowledge/can/can-xl/ are a mix(!) of
> Classical CAN, CAN FD and CAN XL content (e.g. ESI is only CAN FD).
>
> The frame type flags an abstraction of
> (none) -> Classical CAN
> (FDF) -> CAN FD
> (XLF) -> CAN XL
>
>>> +
>>> +/**
>>> + * struct canxl_frame - CAN with e'X'tended frame 'L'ength frame
>>> structure
>>> + * @prio: 11 bit arbitration priority with zero'ed CAN_*_FLAG flags
>>> + * @flags: additional flags for CAN XL
>>> + * @sdt: SDU (service data unit) type
>>> + * @len: frame payload length in byte (CANXL_MIN_DLEN ..
>>> CANXL_MAX_DLEN)
>>> + * @af: acceptance field
>>> + * @data: CAN XL frame payload (CANXL_MIN_DLEN .. CANXL_MAX_DLEN byte)
>>> + *
>>> + * @prio shares the same position as @can_id from struct can[fd]_frame.
>>> + */
>>> +struct canxl_frame {
>>> + canid_t prio; /* 11 bit priority for arbitration (canid_t) */
>>
>> Does it make sense to keep the canid_t? The addressing is done through
>> both the prio and the af fields. Also, the CAN_EFF_FLAG, CAN_RTR_FLAG
>> and CAN_ERR_FLAG flags do not apply anymore. Finally, the prio is only
>> 11 bits, no need to reserve 32 bits for it.
>
> The CAN-ID was (due to its arbitration nature) always also a priority
> field.
>
> So nothing changed here. There is no RTR and no 29-bit priority anymore
> now. The RTR flag has already been disabled for CAN FD (see presentation
> at the end of this mail).
>
> Therefore it makes sense to handle the SFF 11-bit prio analogue to the
> former CAN-ID - and also use canid_t to simply keep the entire CAN
> filter handling in af_can.c !
>
> The new idea in CAN XL is to split the priority from the content
> identification which has been named 'acceptance field' (AC).
>
> The functionality of AC depends on SDT. Don't know how this will work
> out in the future and if we would provide some AF-specific filtering
> inside the kernel.
>
>>
>> Of course, if we declare it as 16 bits, we need to add some padding so
>> that the other flags remain at the same offset.
>
> Answered above.
>
>>> + __u8 flags; /* additional flags for CAN XL */
>>> + __u8 sdt; /* SDU (service data unit) type */
>>> + __u16 len; /* frame payload length in byte */
>>> + __u32 af; /* acceptance field */
>>> + __u8 data[CANXL_MAX_DLEN];
>>> +};
>>
>> The CiA CAN knowledge also mentions a VCID (virtual CAN network ID).
>> Is this not needed?
>
> Answered above.
>
>>> +
>>> #define CAN_MTU (sizeof(struct can_frame))
>>> #define CANFD_MTU (sizeof(struct canfd_frame))
>>> +#define CANXL_MTU (sizeof(struct canxl_frame))
>>> +#define CANXL_HDR_SIZE (offsetof(struct canxl_frame, data))
>>> +#define CANXL_MIN_MTU (CANXL_HDR_SIZE + 64)
>>> +#define CANXL_MAX_MTU CANXL_MTU
>>>
>>> /* particular protocols of the protocol family PF_CAN */
>>> #define CAN_RAW 1 /* RAW sockets */
>>> #define CAN_BCM 2 /* Broadcast Manager */
>>> #define CAN_TP16 3 /* VAG Transport Protocol v1.6 */
>>
>> CAN XL has a resXL flag, suggesting that there may be a fourth
>> generation of the CAN protocol. One issue in the initial socket CAN
>> design is that we never reserved a flag for future versions. Here, we
>> are lucky that the bit 0x80 of the length field is never set.
>> Back to CAN XL, I would like to discuss how we will distinguish the
>> CAN GEN4 frames from the CAN XL ones so that we do not find ourselves
>> stuck in a couple of years because no bits are left to differentiate
>> things.
>>
>> One solution would be to set the two most significant bit:
>> #define CAN_GEN4 0xC0
>>
>> and the test would be:
>> if (cf.flags & CAN_GEN4 == CAN_GEN4)
>
> Please do not mix up (incompatible) extensions that have been
> implemented in the CAN protocol through the 'wandering' reserved bit
> (r0, now resXL) with the CAN(FD/XL) data structure.
>
> The first suggestion for a CAN frame data structure representation in
> IEEE 1722 contained the entire bitstream definition including the IDE
> bit. That splitted the 11 and 18 bits of the CAN ID inside the data
> structure :-D
>
> I really think, that the journey is ending with CAN XL and when there
> would be really a new improvement, we will take a look at it. E.g. by
> extending the CAN XL flags from the top down (as you suggested with 0xC0).
>
> But who knows how much of the CAN XL frame layout would be re-used by
> CAN XY ?? ¯\_(ツ)_/¯
>
> Btw. I uploaded a presentation which shows the way from classical CAN to
> CAN XL which might depict some relations of the bits in a clearer way:
>
> https://github.com/linux-can/can-doc/blob/master/presentations/CAN-XL-Intro.pdf
>
>
> Best regards,
> Oliver
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v8 4/7] can: canxl: introduce CAN XL data structure
2022-09-08 7:24 ` Oliver Hartkopp
@ 2022-09-11 7:50 ` Vincent Mailhol
2022-09-11 12:11 ` Oliver Hartkopp
0 siblings, 1 reply; 24+ messages in thread
From: Vincent Mailhol @ 2022-09-11 7:50 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: Marc Kleine-Budde, linux-can
On Thu. 8 Sep. 2022 at 16:24, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> Hi Vincent,
>
> any update on this or are you fine with the patch set now?
Sorry for the delay, a lot of personal priorities the last two weeks.
> Best regards,
> Oliver
>
> On 02.09.22 15:56, Oliver Hartkopp wrote:
> > Hi Vincent,
> >
> > On 02.09.22 08:19, Vincent Mailhol wrote:
> >> Sorry for the late reply, I am back from holidays. My previous review
> >> was a best effort review.
> >
> > Hope you had good holidays ;-)
Really great holidays, except maybe for the finale in which I caught
COVID and had to reschedule my flight back.
> >> Since then, I took time to read the CiA’s CAN knowledge pages of CAN XL:
> >> https://www.can-cia.org/can-knowledge/can/can-xl/
> >> and have a few newer comments.
> >>
> >> Unfortunately, I still do not have access to the full specification,
> >> so please forgive if there are some silly remarks.
> >
> > I think the short introduction to CAN XL from the cover letter fits
> > quite good.
> >
> > https://www.bosch-semiconductors.com/media/ip_modules/pdf_2/can_xl_1/canxl_intro_20210225.pdf
This page now returns error 404.
> >> On Tue. 2 Aug 2022 at 04:02, Oliver Hartkopp <socketcan@hartkopp.net>
> >> wrote:
> >>> This patch adds defines for data structures and length information for
> >>> CAN XL (CAN with eXtended data Length) which can transfer up to 2048
> >>> byte inside a single frame.
> >>>
> >>> Notable changes from CAN FD:
> >>>
> >>> - the 11 bit arbitration field is now named 'priority' instead of
> >>> 'can_id'
> >>> (there are no 29 bit identifiers nor RTR frames anymore)
> >>> - the data length needs a uint16 value to cover up to 2048 byte
> >>> (the length element position is different to struct can[fd]_frame)
> >>> - new fields (SDT, AF) and a SEC bit have been introduced
> >>> - the virtual CAN interface identifier is not part if the CAN XL frame
> >>> struct as this VCID value is stored in struct skbuff (analog to
> >>> vlan id)
> >
> > Did you note this?
> >
> > Because you asked about the VCID later.
> >
> > There is an existing Kernel API to handle VLAN-like ID stuff. And we
> > should simply use that existing mechanic as it has the same intention
> > and handling for CAN XL.
ACK.
Thanks.
> >>> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> >>> ---
> >>> include/uapi/linux/can.h | 51 ++++++++++++++++++++++++++++++++++++++++
> >>> 1 file changed, 51 insertions(+)
> >>>
> >>> diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h
> >>> index 7b23eeeb3273..dd645ea72306 100644
> >>> --- a/include/uapi/linux/can.h
> >>> +++ b/include/uapi/linux/can.h
> >>> @@ -46,10 +46,11 @@
> >>> #ifndef _UAPI_CAN_H
> >>> #define _UAPI_CAN_H
> >>>
> >>> #include <linux/types.h>
> >>> #include <linux/socket.h>
> >>> +#include <linux/stddef.h> /* for offsetof */
> >>>
> >>> /* controller area network (CAN) kernel definitions */
> >>>
> >>> /* special address description flags for the CAN_ID */
> >>> #define CAN_EFF_FLAG 0x80000000U /* EFF/SFF is set in the MSB */
> >>> @@ -58,10 +59,11 @@
> >>>
> >>> /* valid bits in CAN ID for frame formats */
> >>> #define CAN_SFF_MASK 0x000007FFU /* standard frame format (SFF) */
> >>> #define CAN_EFF_MASK 0x1FFFFFFFU /* extended frame format (EFF) */
> >>> #define CAN_ERR_MASK 0x1FFFFFFFU /* omit EFF, RTR, ERR flags */
> >>> +#define CANXL_PRIO_MASK CAN_SFF_MASK /* 11 bit priority mask */
> >>>
> >>> /*
> >>> * Controller Area Network Identifier structure
> >>> *
> >>> * bit 0-28 : CAN identifier (11/29 bit)
> >>> @@ -71,10 +73,11 @@
> >>> */
> >>> typedef __u32 canid_t;
> >>>
> >>> #define CAN_SFF_ID_BITS 11
> >>> #define CAN_EFF_ID_BITS 29
> >>> +#define CANXL_PRIO_BITS CAN_SFF_ID_BITS
> >>>
> >>> /*
> >>> * Controller Area Network Error Message Frame Mask structure
> >>> *
> >>> * bit 0-28 : error class mask (see include/uapi/linux/can/error.h)
> >>> @@ -89,10 +92,20 @@ typedef __u32 can_err_mask_t;
> >>>
> >>> /* CAN FD payload length and DLC definitions according to ISO
> >>> 11898-7 */
> >>> #define CANFD_MAX_DLC 15
> >>> #define CANFD_MAX_DLEN 64
> >>>
> >>> +/*
> >>> + * CAN XL payload length and DLC definitions according to ISO 11898-1
> >>> + * CAN XL DLC ranges from 0 .. 2047 => data length from 1 .. 2048 byte
> >>> + */
> >>> +#define CANXL_MIN_DLC 0
> >>> +#define CANXL_MAX_DLC 2047
> >>> +#define CANXL_MAX_DLC_MASK 0x07FF
> >>> +#define CANXL_MIN_DLEN 1
> >>> +#define CANXL_MAX_DLEN 2048
> >>> +
> >>> /**
> >>> * struct can_frame - Classical CAN frame structure (aka CAN 2.0B)
> >>> * @can_id: CAN ID of the frame and CAN_*_FLAG flags, see canid_t
> >>> definition
> >>> * @len: CAN frame payload length in byte (0 .. 8)
> >>> * @can_dlc: deprecated name for CAN frame payload length in byte
> >>> (0 .. 8)
> >>> @@ -164,12 +177,50 @@ struct canfd_frame {
> >>> __u8 __res0; /* reserved / padding */
> >>> __u8 __res1; /* reserved / padding */
> >>> __u8 data[CANFD_MAX_DLEN] __attribute__((aligned(8)));
> >>> };
> >>>
> >>> +/*
> >>> + * defined bits for canxl_frame.flags
> >>> + *
> >>> + * The canxl_frame.flags element contains two bits CANXL_XLF and
> >>> CANXL_SEC
> >>> + * and shares the relative position of the struct can[fd]_frame.len
> >>> element.
> >>> + * The CANXL_XLF bit ALWAYS needs to be set to indicate a valid CAN
> >>> XL frame.
> >>> + * As a side effect setting this bit intentionally breaks the length
> >>> checks
> >>> + * for Classical CAN and CAN FD frames.
> >>> + *
> >>> + * Undefined bits in canxl_frame.flags are reserved and shall be set
> >>> to zero.
> >>> + */
> >>> +#define CANXL_XLF 0x80 /* mandatory CAN XL frame flag (must always
> >>> be set!) */
> >>> +#define CANXL_SEC 0x01 /* Simple Extended Content
> >>> (security/segmentation) */
> >>
> >> Are we sure that no other flags are needed? The CiA can-knowledge
> >> mentions a FTYP (frame type) flag. Do you know what it is?
> >
> > The LLC frame fields in
> > https://www.can-cia.org/can-knowledge/can/can-xl/ are a mix(!) of
> > Classical CAN, CAN FD and CAN XL content (e.g. ESI is only CAN FD).
> >
> > The frame type flags an abstraction of
> > (none) -> Classical CAN
> > (FDF) -> CAN FD
> > (XLF) -> CAN XL
Now, I am curious how the LLC frame format will be able to
differentiate between three types (Classic, FD, XL) using a single
bit. But noted for the explanation.
But this doesn't impact Linux SocketCAN.
> >>> +
> >>> +/**
> >>> + * struct canxl_frame - CAN with e'X'tended frame 'L'ength frame
> >>> structure
> >>> + * @prio: 11 bit arbitration priority with zero'ed CAN_*_FLAG flags
> >>> + * @flags: additional flags for CAN XL
> >>> + * @sdt: SDU (service data unit) type
> >>> + * @len: frame payload length in byte (CANXL_MIN_DLEN ..
> >>> CANXL_MAX_DLEN)
> >>> + * @af: acceptance field
> >>> + * @data: CAN XL frame payload (CANXL_MIN_DLEN .. CANXL_MAX_DLEN byte)
> >>> + *
> >>> + * @prio shares the same position as @can_id from struct can[fd]_frame.
> >>> + */
> >>> +struct canxl_frame {
> >>> + canid_t prio; /* 11 bit priority for arbitration (canid_t) */
> >>
> >> Does it make sense to keep the canid_t? The addressing is done through
> >> both the prio and the af fields. Also, the CAN_EFF_FLAG, CAN_RTR_FLAG
> >> and CAN_ERR_FLAG flags do not apply anymore. Finally, the prio is only
> >> 11 bits, no need to reserve 32 bits for it.
> >
> > The CAN-ID was (due to its arbitration nature) always also a priority
> > field.
The CAN-(FD) ID holds two roles: priority and ID. For CAN XL, it is
only ID. While I agree that both have the priority attributes, my
concerns are on the semantics. The type is canid_t, not canprio_t. I
just want to point that it is weird to had ID in the type when that
field doesn't have an ID property anymore.
> > So nothing changed here. There is no RTR and no 29-bit priority anymore
> > now. The RTR flag has already been disabled for CAN FD (see presentation
> > at the end of this mail).
> >
> > Therefore it makes sense to handle the SFF 11-bit prio analogue to the
> > former CAN-ID - and also use canid_t to simply keep the entire CAN
> > filter handling in af_can.c !
This is a good argument to keep using the canid_t. If we make it a
u16, we lose the alignment (unless we add dirty endianness
preprocessing macros).
> > The new idea in CAN XL is to split the priority from the content
> > identification which has been named 'acceptance field' (AC).
> >
> > The functionality of AC depends on SDT. Don't know how this will work
> > out in the future and if we would provide some AF-specific filtering
> > inside the kernel.
Thanks for the explanation, ACK.
> >>
> >> Of course, if we declare it as 16 bits, we need to add some padding so
> >> that the other flags remain at the same offset.
> >
> > Answered above.
> >
> >>> + __u8 flags; /* additional flags for CAN XL */
> >>> + __u8 sdt; /* SDU (service data unit) type */
> >>> + __u16 len; /* frame payload length in byte */
> >>> + __u32 af; /* acceptance field */
> >>> + __u8 data[CANXL_MAX_DLEN];
> >>> +};
> >>
> >> The CiA CAN knowledge also mentions a VCID (virtual CAN network ID).
> >> Is this not needed?
> >
> > Answered above.
ACK.
> >>> +
> >>> #define CAN_MTU (sizeof(struct can_frame))
> >>> #define CANFD_MTU (sizeof(struct canfd_frame))
> >>> +#define CANXL_MTU (sizeof(struct canxl_frame))
> >>> +#define CANXL_HDR_SIZE (offsetof(struct canxl_frame, data))
> >>> +#define CANXL_MIN_MTU (CANXL_HDR_SIZE + 64)
> >>> +#define CANXL_MAX_MTU CANXL_MTU
> >>>
> >>> /* particular protocols of the protocol family PF_CAN */
> >>> #define CAN_RAW 1 /* RAW sockets */
> >>> #define CAN_BCM 2 /* Broadcast Manager */
> >>> #define CAN_TP16 3 /* VAG Transport Protocol v1.6 */
> >>
> >> CAN XL has a resXL flag, suggesting that there may be a fourth
> >> generation of the CAN protocol. One issue in the initial socket CAN
> >> design is that we never reserved a flag for future versions. Here, we
> >> are lucky that the bit 0x80 of the length field is never set.
> >> Back to CAN XL, I would like to discuss how we will distinguish the
> >> CAN GEN4 frames from the CAN XL ones so that we do not find ourselves
> >> stuck in a couple of years because no bits are left to differentiate
> >> things.
> >>
> >> One solution would be to set the two most significant bit:
> >> #define CAN_GEN4 0xC0
> >>
> >> and the test would be:
> >> if (cf.flags & CAN_GEN4 == CAN_GEN4)
> >
> > Please do not mix up (incompatible) extensions that have been
> > implemented in the CAN protocol through the 'wandering' reserved bit
> > (r0, now resXL) with the CAN(FD/XL) data structure.
> >
> > The first suggestion for a CAN frame data structure representation in
> > IEEE 1722 contained the entire bitstream definition including the IDE
> > bit. That splitted the 11 and 18 bits of the CAN ID inside the data
> > structure :-D
> >
> > I really think, that the journey is ending with CAN XL and when there
> > would be really a new improvement, we will take a look at it. E.g. by
> > extending the CAN XL flags from the top down (as you suggested with 0xC0).
> >
> > But who knows how much of the CAN XL frame layout would be re-used by
> > CAN XY ?? ¯\_(ツ)_/¯
> >
> > Btw. I uploaded a presentation which shows the way from classical CAN to
> > CAN XL which might depict some relations of the bits in a clearer way:
> >
> > https://github.com/linux-can/can-doc/blob/master/presentations/CAN-XL-Intro.pdf
Thanks. With the Bosch PDF now returning error 404, I suggest
replacing the link in the cover letter with your link (or the CIA
knowledge page).
Yours sincerely,
Vincent Mailhol
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v8 3/7] can: set CANFD_FDF flag in all CAN FD frame structures
2022-08-01 19:00 ` [PATCH v8 3/7] can: set CANFD_FDF flag in all CAN FD frame structures Oliver Hartkopp
@ 2022-09-11 7:51 ` Vincent Mailhol
2022-09-11 12:23 ` Oliver Hartkopp
0 siblings, 1 reply; 24+ messages in thread
From: Vincent Mailhol @ 2022-09-11 7:51 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: linux-can
On Tue. 2 Aug. 2022 at 04:02, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> To simplify the testing in user space all struct canfd_frame's provided by
> the CAN subsystem of the Linux kernel now have the CANFD_FDF flag set in
> canfd_frame::flags.
>
> NB: Handcrafted ETH_P_CANFD frames introduced via PF_PACKET socket might
> not set this bit correctly. During the check for sufficient headroom in
> PF_PACKET sk_buffs the uninitialized CAN sk_buff data structures are filled.
> In the case of a CAN FD frame the CANFD_FDF flag is set accordingly.
>
> As the CAN frame content is already zero initialized in alloc_canfd_skb()
> the obsolete initialization of cf->flags in the CTU CAN FD driver has been
> removed as it would overwrite the already set CANFD_FDF flag.
>
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> ---
> drivers/net/can/ctucanfd/ctucanfd_base.c | 1 -
> drivers/net/can/dev/skb.c | 11 +++++++++++
> include/uapi/linux/can.h | 4 ++--
> net/can/af_can.c | 5 +++++
> 4 files changed, 18 insertions(+), 3 deletions(-)
Would it be relevant to also set the flag when the skb is cloned?
diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
index 182749e858b3..24de0bb7092e 100644
--- a/include/linux/can/skb.h
+++ b/include/linux/can/skb.h
@@ -85,6 +85,7 @@ static inline void can_skb_set_owner(struct sk_buff
*skb, struct sock *sk)
static inline struct sk_buff *can_create_echo_skb(struct sk_buff *skb)
{
struct sk_buff *nskb;
+ struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
nskb = skb_clone(skb, GFP_ATOMIC);
if (unlikely(!nskb)) {
@@ -92,6 +93,9 @@ static inline struct sk_buff
*can_create_echo_skb(struct sk_buff *skb)
return NULL;
}
+ if (can_is_canfd_skb(skb))
+ cfd->flags |= CANFD_FDF;
+
can_skb_set_owner(nskb, skb->sk);
consume_skb(skb);
return nskb;
> diff --git a/drivers/net/can/ctucanfd/ctucanfd_base.c b/drivers/net/can/ctucanfd/ctucanfd_base.c
> index 3c18d028bd8c..c4026712ab7d 100644
> --- a/drivers/net/can/ctucanfd/ctucanfd_base.c
> +++ b/drivers/net/can/ctucanfd/ctucanfd_base.c
> @@ -655,11 +655,10 @@ static void ctucan_read_rx_frame(struct ctucan_priv *priv, struct canfd_frame *c
> cf->can_id = (idw & CAN_EFF_MASK) | CAN_EFF_FLAG;
> else
> cf->can_id = (idw >> 18) & CAN_SFF_MASK;
>
> /* BRS, ESI, RTR Flags */
> - cf->flags = 0;
> if (FIELD_GET(REG_FRAME_FORMAT_W_FDF, ffw)) {
> if (FIELD_GET(REG_FRAME_FORMAT_W_BRS, ffw))
> cf->flags |= CANFD_BRS;
> if (FIELD_GET(REG_FRAME_FORMAT_W_ESI_RSV, ffw))
> cf->flags |= CANFD_ESI;
> diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
> index b896e1ce3b47..adb413bdd734 100644
> --- a/drivers/net/can/dev/skb.c
> +++ b/drivers/net/can/dev/skb.c
> @@ -242,10 +242,13 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev,
> can_skb_prv(skb)->ifindex = dev->ifindex;
> can_skb_prv(skb)->skbcnt = 0;
>
> *cfd = skb_put_zero(skb, sizeof(struct canfd_frame));
>
> + /* set CAN FD flag by default */
> + (*cfd)->flags = CANFD_FDF;
> +
> return skb;
> }
> EXPORT_SYMBOL_GPL(alloc_canfd_skb);
>
> struct sk_buff *alloc_can_err_skb(struct net_device *dev, struct can_frame **cf)
> @@ -285,10 +288,18 @@ static bool can_skb_headroom_valid(struct net_device *dev, struct sk_buff *skb)
> skb->pkt_type = PACKET_HOST;
>
> skb_reset_mac_header(skb);
> skb_reset_network_header(skb);
> skb_reset_transport_header(skb);
> +
> + /* set CANFD_FDF flag for CAN FD frames */
> + if (can_is_canfd_skb(skb)) {
> + struct canfd_frame *cfd;
> +
> + cfd = (struct canfd_frame *)skb->data;
> + cfd->flags |= CANFD_FDF;
> + }
> }
>
> return true;
> }
>
> diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h
> index 90801ada2bbe..7b23eeeb3273 100644
> --- a/include/uapi/linux/can.h
> +++ b/include/uapi/linux/can.h
> @@ -139,12 +139,12 @@ struct can_frame {
> * The struct can_frame and struct canfd_frame intentionally share the same
> * layout to be able to write CAN frame content into a CAN FD frame structure.
> * When this is done the former differentiation via CAN_MTU / CANFD_MTU gets
> * lost. CANFD_FDF allows programmers to mark CAN FD frames in the case of
> * using struct canfd_frame for mixed CAN / CAN FD content (dual use).
> - * N.B. the Kernel APIs do NOT provide mixed CAN / CAN FD content inside of
> - * struct canfd_frame therefore the CANFD_FDF flag is disregarded by Linux.
> + * Since the introduction of CAN XL the CANFD_FDF flag is set in all CAN FD
> + * frame structures provided by the CAN subsystem of the Linux kernel.
> */
> #define CANFD_BRS 0x01 /* bit rate switch (second bitrate for payload data) */
> #define CANFD_ESI 0x02 /* error state indicator of the transmitting node */
> #define CANFD_FDF 0x04 /* mark CAN FD for dual use of struct canfd_frame */
>
> diff --git a/net/can/af_can.c b/net/can/af_can.c
> index afa6c2151bc4..072a6a5c9dd1 100644
> --- a/net/can/af_can.c
> +++ b/net/can/af_can.c
> @@ -203,11 +203,16 @@ int can_send(struct sk_buff *skb, int loop)
> int err = -EINVAL;
>
> if (can_is_can_skb(skb)) {
> skb->protocol = htons(ETH_P_CAN);
> } else if (can_is_canfd_skb(skb)) {
> + struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> +
> skb->protocol = htons(ETH_P_CANFD);
> +
> + /* set CAN FD flag for CAN FD frames by default */
> + cfd->flags |= CANFD_FDF;
> } else {
> goto inval_skb;
> }
>
> /* Make sure the CAN frame can pass the selected CAN netdevice. */
Once all the malloc and clone functions set the flag, is there still a
route that would reach the can_send() without setting the CANFD_FDF
flag? If the answer is supposedly no, maybe it is better to remove the
check (or maybe replace it with a debug message to identify missed use
cases).
Yours sincerely,
Vincent Mailhol
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v8 5/7] can: canxl: update CAN infrastructure for CAN XL frames
2022-08-01 19:00 ` [PATCH v8 5/7] can: canxl: update CAN infrastructure for CAN XL frames Oliver Hartkopp
@ 2022-09-11 7:53 ` Vincent Mailhol
2022-09-11 12:35 ` Oliver Hartkopp
0 siblings, 1 reply; 24+ messages in thread
From: Vincent Mailhol @ 2022-09-11 7:53 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: linux-can
On Tue. 2 Aug. 2022 at 04:02, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> - add new ETH_P_CANXL ethernet protocol type
> - update skb checks for CAN XL
> - add alloc_canxl_skb() which now needs a data length parameter
> - introduce init_can_skb_reserve() to reduce code duplication
>
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> ---
> drivers/net/can/dev/skb.c | 72 ++++++++++++++++++++++++++---------
> include/linux/can/skb.h | 23 ++++++++++-
> include/uapi/linux/if_ether.h | 1 +
> net/can/af_can.c | 25 +++++++++++-
> 4 files changed, 101 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
> index adb413bdd734..f2ec20d80aba 100644
> --- a/drivers/net/can/dev/skb.c
> +++ b/drivers/net/can/dev/skb.c
> @@ -185,10 +185,24 @@ void can_free_echo_skb(struct net_device *dev, unsigned int idx,
> priv->echo_skb[idx] = NULL;
> }
> }
> EXPORT_SYMBOL_GPL(can_free_echo_skb);
>
> +/* fill common values for CAN sk_buffs */
> +static void init_can_skb_reserve(struct sk_buff *skb)
> +{
> + skb->pkt_type = PACKET_BROADCAST;
> + skb->ip_summed = CHECKSUM_UNNECESSARY;
> +
> + skb_reset_mac_header(skb);
> + skb_reset_network_header(skb);
> + skb_reset_transport_header(skb);
> +
> + can_skb_reserve(skb);
> + can_skb_prv(skb)->skbcnt = 0;
> +}
> +
> struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf)
> {
> struct sk_buff *skb;
>
> skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) +
> @@ -198,20 +212,12 @@ struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf)
>
> return NULL;
> }
>
> skb->protocol = htons(ETH_P_CAN);
> - skb->pkt_type = PACKET_BROADCAST;
> - skb->ip_summed = CHECKSUM_UNNECESSARY;
> -
> - skb_reset_mac_header(skb);
> - skb_reset_network_header(skb);
> - skb_reset_transport_header(skb);
> -
> - can_skb_reserve(skb);
> + init_can_skb_reserve(skb);
> can_skb_prv(skb)->ifindex = dev->ifindex;
> - can_skb_prv(skb)->skbcnt = 0;
>
> *cf = skb_put_zero(skb, sizeof(struct can_frame));
>
> return skb;
> }
> @@ -229,30 +235,55 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev,
>
> return NULL;
> }
>
> skb->protocol = htons(ETH_P_CANFD);
> - skb->pkt_type = PACKET_BROADCAST;
> - skb->ip_summed = CHECKSUM_UNNECESSARY;
> -
> - skb_reset_mac_header(skb);
> - skb_reset_network_header(skb);
> - skb_reset_transport_header(skb);
> -
> - can_skb_reserve(skb);
> + init_can_skb_reserve(skb);
> can_skb_prv(skb)->ifindex = dev->ifindex;
> - can_skb_prv(skb)->skbcnt = 0;
>
> *cfd = skb_put_zero(skb, sizeof(struct canfd_frame));
>
> /* set CAN FD flag by default */
> (*cfd)->flags = CANFD_FDF;
>
> return skb;
> }
> EXPORT_SYMBOL_GPL(alloc_canfd_skb);
>
> +struct sk_buff *alloc_canxl_skb(struct net_device *dev,
> + struct canxl_frame **cfx,
> + unsigned int data_len)
> +{
> + struct sk_buff *skb;
> +
> + if (data_len < CANXL_MIN_DLEN || data_len > CANXL_MAX_DLEN)
> + goto out_error;
> +
> + skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) +
> + CANXL_HDR_SIZE + data_len);
> + if (unlikely(!skb))
> + goto out_error;
> +
> + skb->protocol = htons(ETH_P_CANXL);
> + init_can_skb_reserve(skb);
> + can_skb_prv(skb)->ifindex = dev->ifindex;
> +
> + *cfx = skb_put_zero(skb, CANXL_HDR_SIZE + data_len);
> +
> + /* set CAN XL flag and length information by default */
> + (*cfx)->flags = CANXL_XLF;
> + (*cfx)->len = data_len;
> +
> + return skb;
> +
> +out_error:
> + *cfx = NULL;
> +
> + return NULL;
> +}
> +EXPORT_SYMBOL_GPL(alloc_canxl_skb);
> +
> struct sk_buff *alloc_can_err_skb(struct net_device *dev, struct can_frame **cf)
> {
> struct sk_buff *skb;
>
> skb = alloc_can_skb(dev, cf);
> @@ -317,10 +348,15 @@ bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb)
> case ETH_P_CANFD:
> if (!can_is_canfd_skb(skb))
> goto inval_skb;
> break;
>
> + case ETH_P_CANXL:
> + if (!can_is_canxl_skb(skb))
> + goto inval_skb;
> + break;
> +
> default:
> goto inval_skb;
> }
>
> if (!can_skb_headroom_valid(dev, skb)) {
> diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
> index ddffc2fc008c..01c01b1261fa 100644
> --- a/include/linux/can/skb.h
> +++ b/include/linux/can/skb.h
> @@ -28,10 +28,13 @@ unsigned int __must_check can_get_echo_skb(struct net_device *dev,
> void can_free_echo_skb(struct net_device *dev, unsigned int idx,
> unsigned int *frame_len_ptr);
> struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf);
> struct sk_buff *alloc_canfd_skb(struct net_device *dev,
> struct canfd_frame **cfd);
> +struct sk_buff *alloc_canxl_skb(struct net_device *dev,
> + struct canxl_frame **cfx,
> + unsigned int data_len);
> struct sk_buff *alloc_can_err_skb(struct net_device *dev,
> struct can_frame **cf);
> bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb);
>
> /*
> @@ -112,15 +115,33 @@ static inline bool can_is_canfd_skb(const struct sk_buff *skb)
>
> /* the CAN specific type of skb is identified by its data length */
> return (skb->len == CANFD_MTU && cfd->len <= CANFD_MAX_DLEN);
> }
>
> -/* get length element value from can[fd]_frame structure */
> +static inline bool can_is_canxl_skb(const struct sk_buff *skb)
> +{
> + const struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
> +
> + if (skb->len < CANXL_HDR_SIZE + CANXL_MIN_DLEN || skb->len > CANXL_MTU)
> + return false;
> +
> + /* this also checks valid CAN XL data length boundaries */
> + if (skb->len != CANXL_HDR_SIZE + cfx->len)
> + return false;
> +
> + return cfx->flags & CANXL_XLF;
> +}
> +
> +/* get length element value from can[|fd|xl]_frame structure */
> static inline unsigned int can_skb_get_len_val(struct sk_buff *skb)
> {
> + const struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
> const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
Nitpick: what would be the acronyms for cfx and cfd? I thought that
cfd was for *C*AN-*FD* frame, and thus I would expect cxl instead of
cfx for *C*AN-*XL* frame.
On the contrary, if cfx stands for *C*AN *F*rame *X*L, then for
CAN-FD, the acronym should be cff (*C*AN *f*rame *F*D).
> + if (can_is_canxl_skb(skb))
> + return cfx->len;
> +
> return cfd->len;
> }
>
> /* get needed data length inside CAN frame for all frame types (RTR aware) */
> static inline unsigned int can_skb_get_data_len(struct sk_buff *skb)
> diff --git a/include/uapi/linux/if_ether.h b/include/uapi/linux/if_ether.h
> index d370165bc621..69e0457eb200 100644
> --- a/include/uapi/linux/if_ether.h
> +++ b/include/uapi/linux/if_ether.h
> @@ -136,10 +136,11 @@
> #define ETH_P_WAN_PPP 0x0007 /* Dummy type for WAN PPP frames*/
> #define ETH_P_PPP_MP 0x0008 /* Dummy type for PPP MP frames */
> #define ETH_P_LOCALTALK 0x0009 /* Localtalk pseudo type */
> #define ETH_P_CAN 0x000C /* CAN: Controller Area Network */
> #define ETH_P_CANFD 0x000D /* CANFD: CAN flexible data rate*/
> +#define ETH_P_CANXL 0x000E /* CANXL: eXtended frame Length */
> #define ETH_P_PPPTALK 0x0010 /* Dummy type for Atalk over PPP*/
> #define ETH_P_TR_802_2 0x0011 /* 802.2 frames */
> #define ETH_P_MOBITEX 0x0015 /* Mobitex (kaz@cafe.net) */
> #define ETH_P_CONTROL 0x0016 /* Card specific control frames */
> #define ETH_P_IRDA 0x0017 /* Linux-IrDA */
> diff --git a/net/can/af_can.c b/net/can/af_can.c
> index 072a6a5c9dd1..9503ab10f9b8 100644
> --- a/net/can/af_can.c
> +++ b/net/can/af_can.c
> @@ -200,11 +200,13 @@ int can_send(struct sk_buff *skb, int loop)
> {
> struct sk_buff *newskb = NULL;
> struct can_pkg_stats *pkg_stats = dev_net(skb->dev)->can.pkg_stats;
> int err = -EINVAL;
>
> - if (can_is_can_skb(skb)) {
> + if (can_is_canxl_skb(skb)) {
> + skb->protocol = htons(ETH_P_CANXL);
> + } else if (can_is_can_skb(skb)) {
> skb->protocol = htons(ETH_P_CAN);
> } else if (can_is_canfd_skb(skb)) {
> struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
>
> skb->protocol = htons(ETH_P_CANFD);
> @@ -700,10 +702,25 @@ static int canfd_rcv(struct sk_buff *skb, struct net_device *dev,
>
> can_receive(skb, dev);
> return NET_RX_SUCCESS;
> }
>
> +static int canxl_rcv(struct sk_buff *skb, struct net_device *dev,
> + struct packet_type *pt, struct net_device *orig_dev)
> +{
> + if (unlikely(dev->type != ARPHRD_CAN || (!can_is_canxl_skb(skb)))) {
> + pr_warn_once("PF_CAN: dropped non conform CAN XL skbuff: dev type %d, len %d\n",
> + dev->type, skb->len);
> +
> + kfree_skb(skb);
> + return NET_RX_DROP;
> + }
> +
> + can_receive(skb, dev);
> + return NET_RX_SUCCESS;
> +}
> +
> /* af_can protocol functions */
>
> /**
> * can_proto_register - register CAN transport protocol
> * @cp: pointer to CAN protocol structure
> @@ -824,10 +841,15 @@ static struct packet_type can_packet __read_mostly = {
> static struct packet_type canfd_packet __read_mostly = {
> .type = cpu_to_be16(ETH_P_CANFD),
> .func = canfd_rcv,
> };
>
> +static struct packet_type canxl_packet __read_mostly = {
> + .type = cpu_to_be16(ETH_P_CANXL),
> + .func = canxl_rcv,
> +};
> +
> static const struct net_proto_family can_family_ops = {
> .family = PF_CAN,
> .create = can_create,
> .owner = THIS_MODULE,
> };
> @@ -863,10 +885,11 @@ static __init int can_init(void)
> if (err)
> goto out_sock;
>
> dev_add_pack(&can_packet);
> dev_add_pack(&canfd_packet);
> + dev_add_pack(&canxl_packet);
>
> return 0;
>
> out_sock:
> unregister_pernet_subsys(&can_pernet_ops);
Yours sincerely,
Vincent Mailhol
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v8 0/7] can: support CAN XL
2022-08-31 11:56 ` [PATCH v8 0/7] can: support CAN XL Oliver Hartkopp
@ 2022-09-11 8:08 ` Vincent MAILHOL
2022-09-11 12:42 ` Oliver Hartkopp
0 siblings, 1 reply; 24+ messages in thread
From: Vincent MAILHOL @ 2022-09-11 8:08 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: linux-can
Hi Oliver,
On Wed. 31 Aug. 2022 at 20:57, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> Hi Vincent,
>
> I have discussed with Marc about this patch set and after our discussion
> about len/flags he was missing an ACK from your side:
>
> https://lore.kernel.org/linux-can/247226f6-b9b5-ec47-2234-d1cc70ee6954@hartkopp.net/T/#u
>
> Are you fine with this V8 patch set now?
>
> I already created some CAN CiA p-o-c code for CAN XL segmentation:
>
> https://github.com/hartkopp/can-cia-613-3-poc
>
> When the patch set is committed and the definitions are fixed I would
> also start with some documentation.
Sorry for the delay, it was tough to find time for the final review.
> On 01.08.22 21:00, Oliver Hartkopp wrote:
> > The CAN with eXtended data Length (CAN XL) is a new CAN protocol with a
> > 10Mbit/s data transfer with a new physical layer transceiver (for this
> > data section). CAN XL allows up to 2048 byte of payload and shares the
> > arbitration principle (11 bit priority) known from Classical CAN and
> > CAN FD. RTR and 29 bit identifiers are not implemented in CAN XL.
> >
> > A short introdution to CAN XL can be found here:
> > https://www.bosch-semiconductors.com/media/ip_modules/pdf_2/can_xl_1/canxl_intro_20210225.pdf
> >
> > V2: Major rework after discussion and feedback on Linux-CAN ML
> >
> > - rework of struct canxl_frame
> > - CANXL_XLF flag is now the switch between CAN XL and CAN/CANFD
> > - variable length in r/w operations for CAN XL frames
> > - write CAN XL frame to raw socket enforces size <-> canxl_frame.len sync
> >
> > V3: Fix length for CAN XL frames inside the sk_buff
> >
> > - extend the CAN_RAW sockopt to handle fixed/truncated read/write operations
> >
> > V4: Fix patch 5 (can: raw: add CAN XL support)
> >
> > - fix return value (move 'err = -EINVAL' in raw_sendmsg())
> > - add CAN XL frame handling in can_rcv()
> > - change comment for CAN_RAW_XL_[RT]X_DYN definition (allow -> enable)
> >
> > V5: Remove CAN_RAW_XL_[RT]X_DYN definition again
> >
> > - CAN_RAW_XL_[RT]X_DYN (truncated data) feature is now enabled by default
> > - use CANXL_MIN_DLEN instead of '1' in canxl_frame definition
> > - add missing 'err = -EINVAL' initialization in raw_sendmsg())
> >
> > V6:
> >
> > - rework an separate skb identification and length helpers
> > - add CANFD_FDF flag in all CAN FD frame structures
> > - simplify patches for infrastructure and raw sockets
> > - add vxcan support in virtual CAN interface patch
> >
> > V7:
> >
> > - fixed indention as remarked by Marc
> > - set CANFD_FDF flag when detecting CAN FD frames generated by PF_PACKET
> > - Allow to use variable CAN XL MTU sizes to enforce real time requirements
> > on CAN XL segments (e.g. to support of CAN CiA segmentation concept)
> >
> > V8:
> >
> > - fixed typo as remarked by Vincent
> > - rebased to latest can-next/net-next tree
> >
> > Oliver Hartkopp (7):
> > can: skb: unify skb CAN frame identification helpers
> > can: skb: add skb CAN frame data length helpers
> > can: set CANFD_FDF flag in all CAN FD frame structures
> > can: canxl: introduce CAN XL data structure
As pointed out in the past, I still prefer to make canxl_fame::data a
flexible array (data[]) instead of specifying the maximum length (i.e.
data[CANXL_MAX_DLEN]). But as no one else manifested any objections on
the mailing, I conclude that the majority is either fine with the
data[CANXL_MAX_DLEN] or indifferent. Thus, I will acknowledge this
patch despite having a different personal opinion on the topic.
> > can: canxl: update CAN infrastructure for CAN XL frames
> > can: dev: add CAN XL support to virtual CAN
> > can: raw: add CAN XL support
I send two additional comments for patches 3 and 5. Those two last
comments being minor ones, I let you directly handle those. With that
said:
Acked-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
(valid for the full series, you can directly add this to the v9).
> > drivers/net/can/ctucanfd/ctucanfd_base.c | 1 -
> > drivers/net/can/dev/rx-offload.c | 2 +-
> > drivers/net/can/dev/skb.c | 113 ++++++++++++++++-------
> > drivers/net/can/vcan.c | 12 +--
> > drivers/net/can/vxcan.c | 8 +-
> > include/linux/can/dev.h | 5 +
> > include/linux/can/skb.h | 57 +++++++++++-
> > include/uapi/linux/can.h | 55 ++++++++++-
> > include/uapi/linux/can/raw.h | 1 +
> > include/uapi/linux/if_ether.h | 1 +
> > net/can/af_can.c | 76 ++++++++-------
> > net/can/bcm.c | 9 +-
> > net/can/gw.c | 4 +-
> > net/can/isotp.c | 2 +-
> > net/can/j1939/main.c | 4 +
> > net/can/raw.c | 55 ++++++++---
> > 16 files changed, 299 insertions(+), 106 deletions(-)
Yours sincerely,
Vincent Mailhol
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v8 4/7] can: canxl: introduce CAN XL data structure
2022-09-11 7:50 ` Vincent Mailhol
@ 2022-09-11 12:11 ` Oliver Hartkopp
2022-09-11 14:00 ` Vincent Mailhol
0 siblings, 1 reply; 24+ messages in thread
From: Oliver Hartkopp @ 2022-09-11 12:11 UTC (permalink / raw)
To: Vincent Mailhol; +Cc: Marc Kleine-Budde, linux-can
Hi Vincent,
On 11.09.22 09:50, Vincent Mailhol wrote:
> On Thu. 8 Sep. 2022 at 16:24, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
(..)
>>> The CAN-ID was (due to its arbitration nature) always also a priority
>>> field.
>
> The CAN-(FD) ID holds two roles: priority and ID. For CAN XL, it is
> only ID.
Typo? It is only the priority ;-)
> While I agree that both have the priority attributes, my
> concerns are on the semantics. The type is canid_t, not canprio_t. I
> just want to point that it is weird to had ID in the type when that
> field doesn't have an ID property anymore.
CAN XL moves away from the two roles combined in the CAN(FD) Identifier.
The main focus is on arbitration now (CAN XL is like CAN arbitration
with Ethernet data).
But in the end the CAN bitstream at the beginning of the frame including
the arbitration is completely identical for all CAN variants.
Even when this arbitration field is now named priority for CAN XL it is
still the exact mechanism of the CAN identifier (what we introduces
canid_t for).
Therefore having canid_t for can_id and prio seems appropriate.
>>> So nothing changed here. There is no RTR and no 29-bit priority anymore
>>> now. The RTR flag has already been disabled for CAN FD (see presentation
>>> at the end of this mail).
>>>
>>> Therefore it makes sense to handle the SFF 11-bit prio analogue to the
>>> former CAN-ID - and also use canid_t to simply keep the entire CAN
>>> filter handling in af_can.c !
>
> This is a good argument to keep using the canid_t. If we make it a
> u16, we lose the alignment (unless we add dirty endianness
> preprocessing macros).
Yep.
(..)
>>> Btw. I uploaded a presentation which shows the way from classical CAN to
>>> CAN XL which might depict some relations of the bits in a clearer way:
>>>
>>> https://github.com/linux-can/can-doc/blob/master/presentations/CAN-XL-Intro.pdf
>
> Thanks. With the Bosch PDF now returning error 404, I suggest
> replacing the link in the cover letter with your link (or the CIA
> knowledge page).
Ok, will do.
Best regards,
Oliver
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v8 3/7] can: set CANFD_FDF flag in all CAN FD frame structures
2022-09-11 7:51 ` Vincent Mailhol
@ 2022-09-11 12:23 ` Oliver Hartkopp
2022-09-11 13:57 ` Vincent Mailhol
0 siblings, 1 reply; 24+ messages in thread
From: Oliver Hartkopp @ 2022-09-11 12:23 UTC (permalink / raw)
To: Vincent Mailhol; +Cc: linux-can
On 11.09.22 09:51, Vincent Mailhol wrote:
> On Tue. 2 Aug. 2022 at 04:02, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>> To simplify the testing in user space all struct canfd_frame's provided by
>> the CAN subsystem of the Linux kernel now have the CANFD_FDF flag set in
>> canfd_frame::flags.
>>
>> NB: Handcrafted ETH_P_CANFD frames introduced via PF_PACKET socket might
>> not set this bit correctly. During the check for sufficient headroom in
>> PF_PACKET sk_buffs the uninitialized CAN sk_buff data structures are filled.
>> In the case of a CAN FD frame the CANFD_FDF flag is set accordingly.
>>
>> As the CAN frame content is already zero initialized in alloc_canfd_skb()
>> the obsolete initialization of cf->flags in the CTU CAN FD driver has been
>> removed as it would overwrite the already set CANFD_FDF flag.
>>
>> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
>> ---
>> drivers/net/can/ctucanfd/ctucanfd_base.c | 1 -
>> drivers/net/can/dev/skb.c | 11 +++++++++++
>> include/uapi/linux/can.h | 4 ++--
>> net/can/af_can.c | 5 +++++
>> 4 files changed, 18 insertions(+), 3 deletions(-)
>
> Would it be relevant to also set the flag when the skb is cloned?
>
> diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
> index 182749e858b3..24de0bb7092e 100644
> --- a/include/linux/can/skb.h
> +++ b/include/linux/can/skb.h
> @@ -85,6 +85,7 @@ static inline void can_skb_set_owner(struct sk_buff
> *skb, struct sock *sk)
> static inline struct sk_buff *can_create_echo_skb(struct sk_buff *skb)
> {
> struct sk_buff *nskb;
> + struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
>
> nskb = skb_clone(skb, GFP_ATOMIC);
> if (unlikely(!nskb)) {
> @@ -92,6 +93,9 @@ static inline struct sk_buff
> *can_create_echo_skb(struct sk_buff *skb)
> return NULL;
> }
>
> + if (can_is_canfd_skb(skb))
> + cfd->flags |= CANFD_FDF;
> +
> can_skb_set_owner(nskb, skb->sk);
> consume_skb(skb);
> return nskb;
>
No, this should be obsolete.
The current patch set should set this bit at all creation/malloc places
of CAN FD skbs.
>> diff --git a/drivers/net/can/ctucanfd/ctucanfd_base.c b/drivers/net/can/ctucanfd/ctucanfd_base.c
>> index 3c18d028bd8c..c4026712ab7d 100644
>> --- a/drivers/net/can/ctucanfd/ctucanfd_base.c
>> +++ b/drivers/net/can/ctucanfd/ctucanfd_base.c
>> @@ -655,11 +655,10 @@ static void ctucan_read_rx_frame(struct ctucan_priv *priv, struct canfd_frame *c
>> cf->can_id = (idw & CAN_EFF_MASK) | CAN_EFF_FLAG;
>> else
>> cf->can_id = (idw >> 18) & CAN_SFF_MASK;
>>
>> /* BRS, ESI, RTR Flags */
>> - cf->flags = 0;
>> if (FIELD_GET(REG_FRAME_FORMAT_W_FDF, ffw)) {
>> if (FIELD_GET(REG_FRAME_FORMAT_W_BRS, ffw))
>> cf->flags |= CANFD_BRS;
>> if (FIELD_GET(REG_FRAME_FORMAT_W_ESI_RSV, ffw))
>> cf->flags |= CANFD_ESI;
>> diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
>> index b896e1ce3b47..adb413bdd734 100644
>> --- a/drivers/net/can/dev/skb.c
>> +++ b/drivers/net/can/dev/skb.c
>> @@ -242,10 +242,13 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev,
>> can_skb_prv(skb)->ifindex = dev->ifindex;
>> can_skb_prv(skb)->skbcnt = 0;
>>
>> *cfd = skb_put_zero(skb, sizeof(struct canfd_frame));
>>
>> + /* set CAN FD flag by default */
>> + (*cfd)->flags = CANFD_FDF;
>> +
>> return skb;
>> }
>> EXPORT_SYMBOL_GPL(alloc_canfd_skb);
>>
>> struct sk_buff *alloc_can_err_skb(struct net_device *dev, struct can_frame **cf)
>> @@ -285,10 +288,18 @@ static bool can_skb_headroom_valid(struct net_device *dev, struct sk_buff *skb)
>> skb->pkt_type = PACKET_HOST;
>>
>> skb_reset_mac_header(skb);
>> skb_reset_network_header(skb);
>> skb_reset_transport_header(skb);
>> +
>> + /* set CANFD_FDF flag for CAN FD frames */
>> + if (can_is_canfd_skb(skb)) {
>> + struct canfd_frame *cfd;
>> +
>> + cfd = (struct canfd_frame *)skb->data;
>> + cfd->flags |= CANFD_FDF;
>> + }
>> }
>>
>> return true;
>> }
>>
>> diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h
>> index 90801ada2bbe..7b23eeeb3273 100644
>> --- a/include/uapi/linux/can.h
>> +++ b/include/uapi/linux/can.h
>> @@ -139,12 +139,12 @@ struct can_frame {
>> * The struct can_frame and struct canfd_frame intentionally share the same
>> * layout to be able to write CAN frame content into a CAN FD frame structure.
>> * When this is done the former differentiation via CAN_MTU / CANFD_MTU gets
>> * lost. CANFD_FDF allows programmers to mark CAN FD frames in the case of
>> * using struct canfd_frame for mixed CAN / CAN FD content (dual use).
>> - * N.B. the Kernel APIs do NOT provide mixed CAN / CAN FD content inside of
>> - * struct canfd_frame therefore the CANFD_FDF flag is disregarded by Linux.
>> + * Since the introduction of CAN XL the CANFD_FDF flag is set in all CAN FD
>> + * frame structures provided by the CAN subsystem of the Linux kernel.
>> */
>> #define CANFD_BRS 0x01 /* bit rate switch (second bitrate for payload data) */
>> #define CANFD_ESI 0x02 /* error state indicator of the transmitting node */
>> #define CANFD_FDF 0x04 /* mark CAN FD for dual use of struct canfd_frame */
>>
>> diff --git a/net/can/af_can.c b/net/can/af_can.c
>> index afa6c2151bc4..072a6a5c9dd1 100644
>> --- a/net/can/af_can.c
>> +++ b/net/can/af_can.c
>> @@ -203,11 +203,16 @@ int can_send(struct sk_buff *skb, int loop)
>> int err = -EINVAL;
>>
>> if (can_is_can_skb(skb)) {
>> skb->protocol = htons(ETH_P_CAN);
>> } else if (can_is_canfd_skb(skb)) {
>> + struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
>> +
>> skb->protocol = htons(ETH_P_CANFD);
>> +
>> + /* set CAN FD flag for CAN FD frames by default */
>> + cfd->flags |= CANFD_FDF;
>> } else {
>> goto inval_skb;
>> }
>>
>> /* Make sure the CAN frame can pass the selected CAN netdevice. */
>
> Once all the malloc and clone functions set the flag, is there still a
> route that would reach the can_send() without setting the CANFD_FDF
> flag? If the answer is supposedly no, maybe it is better to remove the
> check (or maybe replace it with a debug message to identify missed use
> cases).
The flag IS set in all skb malloc places (and therefore does not need to
be set in clone places (see my answer above).
This specific check in can_send() especially covers content that came
from userspace which is not setting this bit today.
The same is done for userspace content provided by a PF_PACKET socket in
can_skb_headroom_valid() above.
Best regards,
Oliver
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v8 5/7] can: canxl: update CAN infrastructure for CAN XL frames
2022-09-11 7:53 ` Vincent Mailhol
@ 2022-09-11 12:35 ` Oliver Hartkopp
2022-09-11 14:10 ` Vincent Mailhol
0 siblings, 1 reply; 24+ messages in thread
From: Oliver Hartkopp @ 2022-09-11 12:35 UTC (permalink / raw)
To: Vincent Mailhol; +Cc: linux-can
On 11.09.22 09:53, Vincent Mailhol wrote:
> On Tue. 2 Aug. 2022 at 04:02, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
(..)
>> +/* get length element value from can[|fd|xl]_frame structure */
>> static inline unsigned int can_skb_get_len_val(struct sk_buff *skb)
>> {
>> + const struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
>> const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
>
> Nitpick: what would be the acronyms for cfx and cfd? I thought that
> cfd was for *C*AN-*FD* frame, and thus I would expect cxl instead of
> cfx for *C*AN-*XL* frame.
> On the contrary, if cfx stands for *C*AN *F*rame *X*L, then for
> CAN-FD, the acronym should be cff (*C*AN *f*rame *F*D).
You need to start from the original
struct can_frame cf; *C*AN *F*RAME
Then CAN FD showed up and the naming moved from 'cf' to 'cfd' for *C*AN
*FD* FRAME where is was not forseable that there ever would be CAN XL.
For me it is more intuitive to generally name CAN frames 'cf<whatever>'.
cf -> cfd -> cfx
So it is about 'cf' with an extra attribute and not an abbreviation of
CAN variants.
Best regards,
Oliver
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v8 0/7] can: support CAN XL
2022-09-11 8:08 ` Vincent MAILHOL
@ 2022-09-11 12:42 ` Oliver Hartkopp
0 siblings, 0 replies; 24+ messages in thread
From: Oliver Hartkopp @ 2022-09-11 12:42 UTC (permalink / raw)
To: Vincent MAILHOL; +Cc: linux-can
On 11.09.22 10:08, Vincent MAILHOL wrote:
> Hi Oliver,
>
> On Wed. 31 Aug. 2022 at 20:57, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>> Hi Vincent,
>>
>> I have discussed with Marc about this patch set and after our discussion
>> about len/flags he was missing an ACK from your side:
>>
>> https://lore.kernel.org/linux-can/247226f6-b9b5-ec47-2234-d1cc70ee6954@hartkopp.net/T/#u
>>
>> Are you fine with this V8 patch set now?
>>
>> I already created some CAN CiA p-o-c code for CAN XL segmentation:
>>
>> https://github.com/hartkopp/can-cia-613-3-poc
>>
>> When the patch set is committed and the definitions are fixed I would
>> also start with some documentation.
>
> Sorry for the delay, it was tough to find time for the final review.
>
>> On 01.08.22 21:00, Oliver Hartkopp wrote:
>>> The CAN with eXtended data Length (CAN XL) is a new CAN protocol with a
>>> 10Mbit/s data transfer with a new physical layer transceiver (for this
>>> data section). CAN XL allows up to 2048 byte of payload and shares the
>>> arbitration principle (11 bit priority) known from Classical CAN and
>>> CAN FD. RTR and 29 bit identifiers are not implemented in CAN XL.
>>>
>>> A short introdution to CAN XL can be found here:
>>> https://www.bosch-semiconductors.com/media/ip_modules/pdf_2/can_xl_1/canxl_intro_20210225.pdf
>>>
>>> V2: Major rework after discussion and feedback on Linux-CAN ML
>>>
>>> - rework of struct canxl_frame
>>> - CANXL_XLF flag is now the switch between CAN XL and CAN/CANFD
>>> - variable length in r/w operations for CAN XL frames
>>> - write CAN XL frame to raw socket enforces size <-> canxl_frame.len sync
>>>
>>> V3: Fix length for CAN XL frames inside the sk_buff
>>>
>>> - extend the CAN_RAW sockopt to handle fixed/truncated read/write operations
>>>
>>> V4: Fix patch 5 (can: raw: add CAN XL support)
>>>
>>> - fix return value (move 'err = -EINVAL' in raw_sendmsg())
>>> - add CAN XL frame handling in can_rcv()
>>> - change comment for CAN_RAW_XL_[RT]X_DYN definition (allow -> enable)
>>>
>>> V5: Remove CAN_RAW_XL_[RT]X_DYN definition again
>>>
>>> - CAN_RAW_XL_[RT]X_DYN (truncated data) feature is now enabled by default
>>> - use CANXL_MIN_DLEN instead of '1' in canxl_frame definition
>>> - add missing 'err = -EINVAL' initialization in raw_sendmsg())
>>>
>>> V6:
>>>
>>> - rework an separate skb identification and length helpers
>>> - add CANFD_FDF flag in all CAN FD frame structures
>>> - simplify patches for infrastructure and raw sockets
>>> - add vxcan support in virtual CAN interface patch
>>>
>>> V7:
>>>
>>> - fixed indention as remarked by Marc
>>> - set CANFD_FDF flag when detecting CAN FD frames generated by PF_PACKET
>>> - Allow to use variable CAN XL MTU sizes to enforce real time requirements
>>> on CAN XL segments (e.g. to support of CAN CiA segmentation concept)
>>>
>>> V8:
>>>
>>> - fixed typo as remarked by Vincent
>>> - rebased to latest can-next/net-next tree
>>>
>>> Oliver Hartkopp (7):
>>> can: skb: unify skb CAN frame identification helpers
>>> can: skb: add skb CAN frame data length helpers
>>> can: set CANFD_FDF flag in all CAN FD frame structures
>>> can: canxl: introduce CAN XL data structure
>
> As pointed out in the past, I still prefer to make canxl_fame::data a
> flexible array (data[]) instead of specifying the maximum length (i.e.
> data[CANXL_MAX_DLEN]). But as no one else manifested any objections on
> the mailing, I conclude that the majority is either fine with the
> data[CANXL_MAX_DLEN] or indifferent. Thus, I will acknowledge this
> patch despite having a different personal opinion on the topic.
>
>>> can: canxl: update CAN infrastructure for CAN XL frames
>>> can: dev: add CAN XL support to virtual CAN
>>> can: raw: add CAN XL support
>
> I send two additional comments for patches 3 and 5. Those two last
> comments being minor ones, I let you directly handle those. With that
> said:
>
> Acked-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>
> (valid for the full series, you can directly add this to the v9).
Thanks Vincent!
I commented your remarks on patch 3, 4 and 5. I assume this to be ok for
you.
Will send a v9 later today with some updates in the cover letter to
point to the online available CAN XL resources.
Thanks and best regards,
Oliver
>
>>> drivers/net/can/ctucanfd/ctucanfd_base.c | 1 -
>>> drivers/net/can/dev/rx-offload.c | 2 +-
>>> drivers/net/can/dev/skb.c | 113 ++++++++++++++++-------
>>> drivers/net/can/vcan.c | 12 +--
>>> drivers/net/can/vxcan.c | 8 +-
>>> include/linux/can/dev.h | 5 +
>>> include/linux/can/skb.h | 57 +++++++++++-
>>> include/uapi/linux/can.h | 55 ++++++++++-
>>> include/uapi/linux/can/raw.h | 1 +
>>> include/uapi/linux/if_ether.h | 1 +
>>> net/can/af_can.c | 76 ++++++++-------
>>> net/can/bcm.c | 9 +-
>>> net/can/gw.c | 4 +-
>>> net/can/isotp.c | 2 +-
>>> net/can/j1939/main.c | 4 +
>>> net/can/raw.c | 55 ++++++++---
>>> 16 files changed, 299 insertions(+), 106 deletions(-)
>
> Yours sincerely,
> Vincent Mailhol
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v8 3/7] can: set CANFD_FDF flag in all CAN FD frame structures
2022-09-11 12:23 ` Oliver Hartkopp
@ 2022-09-11 13:57 ` Vincent Mailhol
0 siblings, 0 replies; 24+ messages in thread
From: Vincent Mailhol @ 2022-09-11 13:57 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: linux-can
On Sun. 11 Sept. 2022 at 21:23, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> On 11.09.22 09:51, Vincent Mailhol wrote:
> > On Tue. 2 Aug. 2022 at 04:02, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> >> To simplify the testing in user space all struct canfd_frame's provided by
> >> the CAN subsystem of the Linux kernel now have the CANFD_FDF flag set in
> >> canfd_frame::flags.
> >>
> >> NB: Handcrafted ETH_P_CANFD frames introduced via PF_PACKET socket might
> >> not set this bit correctly. During the check for sufficient headroom in
> >> PF_PACKET sk_buffs the uninitialized CAN sk_buff data structures are filled.
> >> In the case of a CAN FD frame the CANFD_FDF flag is set accordingly.
> >>
> >> As the CAN frame content is already zero initialized in alloc_canfd_skb()
> >> the obsolete initialization of cf->flags in the CTU CAN FD driver has been
> >> removed as it would overwrite the already set CANFD_FDF flag.
> >>
> >> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> >> ---
> >> drivers/net/can/ctucanfd/ctucanfd_base.c | 1 -
> >> drivers/net/can/dev/skb.c | 11 +++++++++++
> >> include/uapi/linux/can.h | 4 ++--
> >> net/can/af_can.c | 5 +++++
> >> 4 files changed, 18 insertions(+), 3 deletions(-)
> >
> > Would it be relevant to also set the flag when the skb is cloned?
> >
> > diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
> > index 182749e858b3..24de0bb7092e 100644
> > --- a/include/linux/can/skb.h
> > +++ b/include/linux/can/skb.h
> > @@ -85,6 +85,7 @@ static inline void can_skb_set_owner(struct sk_buff
> > *skb, struct sock *sk)
> > static inline struct sk_buff *can_create_echo_skb(struct sk_buff *skb)
> > {
> > struct sk_buff *nskb;
> > + struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> >
> > nskb = skb_clone(skb, GFP_ATOMIC);
> > if (unlikely(!nskb)) {
> > @@ -92,6 +93,9 @@ static inline struct sk_buff
> > *can_create_echo_skb(struct sk_buff *skb)
> > return NULL;
> > }
> >
> > + if (can_is_canfd_skb(skb))
> > + cfd->flags |= CANFD_FDF;
> > +
> > can_skb_set_owner(nskb, skb->sk);
> > consume_skb(skb);
> > return nskb;
> >
>
> No, this should be obsolete.
>
> The current patch set should set this bit at all creation/malloc places
> of CAN FD skbs.
>
> >> diff --git a/drivers/net/can/ctucanfd/ctucanfd_base.c b/drivers/net/can/ctucanfd/ctucanfd_base.c
> >> index 3c18d028bd8c..c4026712ab7d 100644
> >> --- a/drivers/net/can/ctucanfd/ctucanfd_base.c
> >> +++ b/drivers/net/can/ctucanfd/ctucanfd_base.c
> >> @@ -655,11 +655,10 @@ static void ctucan_read_rx_frame(struct ctucan_priv *priv, struct canfd_frame *c
> >> cf->can_id = (idw & CAN_EFF_MASK) | CAN_EFF_FLAG;
> >> else
> >> cf->can_id = (idw >> 18) & CAN_SFF_MASK;
> >>
> >> /* BRS, ESI, RTR Flags */
> >> - cf->flags = 0;
> >> if (FIELD_GET(REG_FRAME_FORMAT_W_FDF, ffw)) {
> >> if (FIELD_GET(REG_FRAME_FORMAT_W_BRS, ffw))
> >> cf->flags |= CANFD_BRS;
> >> if (FIELD_GET(REG_FRAME_FORMAT_W_ESI_RSV, ffw))
> >> cf->flags |= CANFD_ESI;
> >> diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
> >> index b896e1ce3b47..adb413bdd734 100644
> >> --- a/drivers/net/can/dev/skb.c
> >> +++ b/drivers/net/can/dev/skb.c
> >> @@ -242,10 +242,13 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev,
> >> can_skb_prv(skb)->ifindex = dev->ifindex;
> >> can_skb_prv(skb)->skbcnt = 0;
> >>
> >> *cfd = skb_put_zero(skb, sizeof(struct canfd_frame));
> >>
> >> + /* set CAN FD flag by default */
> >> + (*cfd)->flags = CANFD_FDF;
> >> +
> >> return skb;
> >> }
> >> EXPORT_SYMBOL_GPL(alloc_canfd_skb);
> >>
> >> struct sk_buff *alloc_can_err_skb(struct net_device *dev, struct can_frame **cf)
> >> @@ -285,10 +288,18 @@ static bool can_skb_headroom_valid(struct net_device *dev, struct sk_buff *skb)
> >> skb->pkt_type = PACKET_HOST;
> >>
> >> skb_reset_mac_header(skb);
> >> skb_reset_network_header(skb);
> >> skb_reset_transport_header(skb);
> >> +
> >> + /* set CANFD_FDF flag for CAN FD frames */
> >> + if (can_is_canfd_skb(skb)) {
> >> + struct canfd_frame *cfd;
> >> +
> >> + cfd = (struct canfd_frame *)skb->data;
> >> + cfd->flags |= CANFD_FDF;
> >> + }
> >> }
> >>
> >> return true;
> >> }
> >>
> >> diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h
> >> index 90801ada2bbe..7b23eeeb3273 100644
> >> --- a/include/uapi/linux/can.h
> >> +++ b/include/uapi/linux/can.h
> >> @@ -139,12 +139,12 @@ struct can_frame {
> >> * The struct can_frame and struct canfd_frame intentionally share the same
> >> * layout to be able to write CAN frame content into a CAN FD frame structure.
> >> * When this is done the former differentiation via CAN_MTU / CANFD_MTU gets
> >> * lost. CANFD_FDF allows programmers to mark CAN FD frames in the case of
> >> * using struct canfd_frame for mixed CAN / CAN FD content (dual use).
> >> - * N.B. the Kernel APIs do NOT provide mixed CAN / CAN FD content inside of
> >> - * struct canfd_frame therefore the CANFD_FDF flag is disregarded by Linux.
> >> + * Since the introduction of CAN XL the CANFD_FDF flag is set in all CAN FD
> >> + * frame structures provided by the CAN subsystem of the Linux kernel.
> >> */
> >> #define CANFD_BRS 0x01 /* bit rate switch (second bitrate for payload data) */
> >> #define CANFD_ESI 0x02 /* error state indicator of the transmitting node */
> >> #define CANFD_FDF 0x04 /* mark CAN FD for dual use of struct canfd_frame */
> >>
> >> diff --git a/net/can/af_can.c b/net/can/af_can.c
> >> index afa6c2151bc4..072a6a5c9dd1 100644
> >> --- a/net/can/af_can.c
> >> +++ b/net/can/af_can.c
> >> @@ -203,11 +203,16 @@ int can_send(struct sk_buff *skb, int loop)
> >> int err = -EINVAL;
> >>
> >> if (can_is_can_skb(skb)) {
> >> skb->protocol = htons(ETH_P_CAN);
> >> } else if (can_is_canfd_skb(skb)) {
> >> + struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> >> +
> >> skb->protocol = htons(ETH_P_CANFD);
> >> +
> >> + /* set CAN FD flag for CAN FD frames by default */
> >> + cfd->flags |= CANFD_FDF;
> >> } else {
> >> goto inval_skb;
> >> }
> >>
> >> /* Make sure the CAN frame can pass the selected CAN netdevice. */
> >
> > Once all the malloc and clone functions set the flag, is there still a
> > route that would reach the can_send() without setting the CANFD_FDF
> > flag? If the answer is supposedly no, maybe it is better to remove the
> > check (or maybe replace it with a debug message to identify missed use
> > cases).
>
> The flag IS set in all skb malloc places (and therefore does not need to
> be set in clone places (see my answer above).
>
> This specific check in can_send() especially covers content that came
> from userspace which is not setting this bit today.
>
> The same is done for userspace content provided by a PF_PACKET socket in
> can_skb_headroom_valid() above.
I see. So if I take the vcan driver as an example, can_send() would
set the flag and then, no need to set it again when cloning. Same goes
for the can_echo_skb. Thanks for the explanations.
Yours sincerely,
Vincent Mailhol
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v8 4/7] can: canxl: introduce CAN XL data structure
2022-09-11 12:11 ` Oliver Hartkopp
@ 2022-09-11 14:00 ` Vincent Mailhol
0 siblings, 0 replies; 24+ messages in thread
From: Vincent Mailhol @ 2022-09-11 14:00 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: Marc Kleine-Budde, linux-can
On Sun. 11 Sept. 2022 at 21:11, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> Hi Vincent,
>
> On 11.09.22 09:50, Vincent Mailhol wrote:
> > On Thu. 8 Sep. 2022 at 16:24, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> (..)
>
> >>> The CAN-ID was (due to its arbitration nature) always also a priority
> >>> field.
> >
> > The CAN-(FD) ID holds two roles: priority and ID. For CAN XL, it is
> > only ID.
>
> Typo? It is only the priority ;-)
Yes, typo ^^;
> > While I agree that both have the priority attributes, my
> > concerns are on the semantics. The type is canid_t, not canprio_t. I
> > just want to point that it is weird to had ID in the type when that
> > field doesn't have an ID property anymore.
>
> CAN XL moves away from the two roles combined in the CAN(FD) Identifier.
> The main focus is on arbitration now (CAN XL is like CAN arbitration
> with Ethernet data).
>
> But in the end the CAN bitstream at the beginning of the frame including
> the arbitration is completely identical for all CAN variants.
>
> Even when this arbitration field is now named priority for CAN XL it is
> still the exact mechanism of the CAN identifier (what we introduces
> canid_t for).
>
> Therefore having canid_t for can_id and prio seems appropriate.
I guess you have a better insight. Thanks for the details.
> >>> So nothing changed here. There is no RTR and no 29-bit priority anymore
> >>> now. The RTR flag has already been disabled for CAN FD (see presentation
> >>> at the end of this mail).
> >>>
> >>> Therefore it makes sense to handle the SFF 11-bit prio analogue to the
> >>> former CAN-ID - and also use canid_t to simply keep the entire CAN
> >>> filter handling in af_can.c !
> >
> > This is a good argument to keep using the canid_t. If we make it a
> > u16, we lose the alignment (unless we add dirty endianness
> > preprocessing macros).
>
> Yep.
>
> (..)
>
> >>> Btw. I uploaded a presentation which shows the way from classical CAN to
> >>> CAN XL which might depict some relations of the bits in a clearer way:
> >>>
> >>> https://github.com/linux-can/can-doc/blob/master/presentations/CAN-XL-Intro.pdf
> >
> > Thanks. With the Bosch PDF now returning error 404, I suggest
> > replacing the link in the cover letter with your link (or the CIA
> > knowledge page).
>
> Ok, will do.
Yours sincerely,
Vincent Mailhol
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v8 5/7] can: canxl: update CAN infrastructure for CAN XL frames
2022-09-11 12:35 ` Oliver Hartkopp
@ 2022-09-11 14:10 ` Vincent Mailhol
2022-09-12 17:09 ` Oliver Hartkopp
0 siblings, 1 reply; 24+ messages in thread
From: Vincent Mailhol @ 2022-09-11 14:10 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: linux-can
On Sun. 11 Sept. 2022 at 21:35, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> On 11.09.22 09:53, Vincent Mailhol wrote:
> > On Tue. 2 Aug. 2022 at 04:02, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>
> (..)
>
> >> +/* get length element value from can[|fd|xl]_frame structure */
> >> static inline unsigned int can_skb_get_len_val(struct sk_buff *skb)
> >> {
> >> + const struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
> >> const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> >
> > Nitpick: what would be the acronyms for cfx and cfd? I thought that
> > cfd was for *C*AN-*FD* frame, and thus I would expect cxl instead of
> > cfx for *C*AN-*XL* frame.
> > On the contrary, if cfx stands for *C*AN *F*rame *X*L, then for
> > CAN-FD, the acronym should be cff (*C*AN *f*rame *F*D).
>
> You need to start from the original
>
> struct can_frame cf; *C*AN *F*RAME
>
> Then CAN FD showed up and the naming moved from 'cf' to 'cfd' for *C*AN
> *FD* FRAME where is was not forseable that there ever would be CAN XL.
>
> For me it is more intuitive to generally name CAN frames 'cf<whatever>'.
>
> cf -> cfd -> cfx
>
> So it is about 'cf' with an extra attribute and not an abbreviation of
> CAN variants.
I still disagree on that one. For me:
* Classical CAN frames: ccf (or the legacy cf before introduction of CAN-FD)
* CAN FD frames: cfd
* CAN XL frames: cxl
is the most consistent.
cfx is not consistent with the cfd acronym and it is out of order: the
structure name is canxl_frame, not can_frame_xl.
At least, that should be cxf for struct *c*an*x*l_*f*rame. CAN frame
XL just sounds odd to me.
Regardless, this remains a nitpick and I will not NACK the series for
that. So do as you prefer in the v9, my Acked-by remains valid.
Yours sincerely,
Vincent Mailhol
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v8 5/7] can: canxl: update CAN infrastructure for CAN XL frames
2022-09-11 14:10 ` Vincent Mailhol
@ 2022-09-12 17:09 ` Oliver Hartkopp
0 siblings, 0 replies; 24+ messages in thread
From: Oliver Hartkopp @ 2022-09-12 17:09 UTC (permalink / raw)
To: Vincent Mailhol; +Cc: linux-can
On 11.09.22 16:10, Vincent Mailhol wrote:
> On Sun. 11 Sept. 2022 at 21:35, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>> On 11.09.22 09:53, Vincent Mailhol wrote:
>>> On Tue. 2 Aug. 2022 at 04:02, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>>
>> (..)
>>
>>>> +/* get length element value from can[|fd|xl]_frame structure */
>>>> static inline unsigned int can_skb_get_len_val(struct sk_buff *skb)
>>>> {
>>>> + const struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
>>>> const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
>>>
>>> Nitpick: what would be the acronyms for cfx and cfd? I thought that
>>> cfd was for *C*AN-*FD* frame, and thus I would expect cxl instead of
>>> cfx for *C*AN-*XL* frame.
>>> On the contrary, if cfx stands for *C*AN *F*rame *X*L, then for
>>> CAN-FD, the acronym should be cff (*C*AN *f*rame *F*D).
>>
>> You need to start from the original
>>
>> struct can_frame cf; *C*AN *F*RAME
>>
>> Then CAN FD showed up and the naming moved from 'cf' to 'cfd' for *C*AN
>> *FD* FRAME where is was not forseable that there ever would be CAN XL.
>>
>> For me it is more intuitive to generally name CAN frames 'cf<whatever>'.
>>
>> cf -> cfd -> cfx
>>
>> So it is about 'cf' with an extra attribute and not an abbreviation of
>> CAN variants.
>
> I still disagree on that one. For me:
> * Classical CAN frames: ccf (or the legacy cf before introduction of CAN-FD)
> * CAN FD frames: cfd
> * CAN XL frames: cxl
> is the most consistent.
>
> cfx is not consistent with the cfd acronym and it is out of order: the
> structure name is canxl_frame, not can_frame_xl.
> At least, that should be cxf for struct *c*an*x*l_*f*rame. CAN frame
> XL just sounds odd to me.
It's ok for me. Therefore changed in v9.
Thanks!
Oliver
>
> Regardless, this remains a nitpick and I will not NACK the series for
> that. So do as you prefer in the v9, my Acked-by remains valid.
>
>
> Yours sincerely,
> Vincent Mailhol
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2022-09-12 17:09 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-01 19:00 [PATCH v8 0/7] can: support CAN XL Oliver Hartkopp
2022-08-01 19:00 ` [PATCH v8 1/7] can: skb: unify skb CAN frame identification helpers Oliver Hartkopp
2022-08-01 19:00 ` [PATCH v8 2/7] can: skb: add skb CAN frame data length helpers Oliver Hartkopp
2022-08-01 19:00 ` [PATCH v8 3/7] can: set CANFD_FDF flag in all CAN FD frame structures Oliver Hartkopp
2022-09-11 7:51 ` Vincent Mailhol
2022-09-11 12:23 ` Oliver Hartkopp
2022-09-11 13:57 ` Vincent Mailhol
2022-08-01 19:00 ` [PATCH v8 4/7] can: canxl: introduce CAN XL data structure Oliver Hartkopp
2022-09-02 6:19 ` Vincent Mailhol
2022-09-02 13:56 ` Oliver Hartkopp
2022-09-08 7:24 ` Oliver Hartkopp
2022-09-11 7:50 ` Vincent Mailhol
2022-09-11 12:11 ` Oliver Hartkopp
2022-09-11 14:00 ` Vincent Mailhol
2022-08-01 19:00 ` [PATCH v8 5/7] can: canxl: update CAN infrastructure for CAN XL frames Oliver Hartkopp
2022-09-11 7:53 ` Vincent Mailhol
2022-09-11 12:35 ` Oliver Hartkopp
2022-09-11 14:10 ` Vincent Mailhol
2022-09-12 17:09 ` Oliver Hartkopp
2022-08-01 19:00 ` [PATCH v8 6/7] can: dev: add CAN XL support to virtual CAN Oliver Hartkopp
2022-08-01 19:00 ` [PATCH v8 7/7] can: raw: add CAN XL support Oliver Hartkopp
2022-08-31 11:56 ` [PATCH v8 0/7] can: support CAN XL Oliver Hartkopp
2022-09-11 8:08 ` Vincent MAILHOL
2022-09-11 12:42 ` Oliver Hartkopp
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.