* [PATCH v4] canxl: add virtual CAN network identifier support
@ 2024-01-08 17:06 Oliver Hartkopp
2024-01-28 10:13 ` Vincent MAILHOL
0 siblings, 1 reply; 3+ messages in thread
From: Oliver Hartkopp @ 2024-01-08 17:06 UTC (permalink / raw)
To: linux-can; +Cc: mkl, mailhol.vincent, Oliver Hartkopp
CAN XL data frames contain an 8-bit virtual CAN network identifier (VCID).
A VCID value of zero represents an 'untagged' CAN XL frame.
To receive and send these optional VCIDs via CAN_RAW sockets a new socket
option CAN_RAW_XL_VCID_OPTS is introduced to define/access VCID content:
- tx: set the outgoing VCID value by the kernel (one fixed 8-bit value)
- tx: pass through VCID values from the user space (e.g. for traffic replay)
- rx: apply VCID receive filter (value/mask) to be passed to the user space
With the 'tx pass through' option CAN_RAW_XL_VCID_TX_PASS all valid VCID
values can be sent, e.g. to replay full qualified CAN XL traffic.
The VCID value provided for the CAN_RAW_XL_VCID_TX_SET option will
override the VCID value in the struct canxl_frame.vcid element defined
for CAN_RAW_XL_VCID_TX_PASS when both flags are set.
With a rx_vcid_mask of zero all possible VCID values (0x00 - 0xFF) are
passed to the user space when the CAN_RAW_XL_VCID_RX_FILTER flag is set.
Without this flag only untagged CAN XL frames (VCID = 0x00) are delivered
to the user space (default).
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
include/uapi/linux/can.h | 41 ++++++++++++++--------
include/uapi/linux/can/raw.h | 14 ++++++++
net/can/af_can.c | 2 ++
net/can/raw.c | 66 ++++++++++++++++++++++++++++++++++--
4 files changed, 106 insertions(+), 17 deletions(-)
diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h
index 939db2388208..8dcd8a9b5676 100644
--- a/include/uapi/linux/can.h
+++ b/include/uapi/linux/can.h
@@ -44,10 +44,11 @@
*/
#ifndef _UAPI_CAN_H
#define _UAPI_CAN_H
+#include <asm/byteorder.h>
#include <linux/types.h>
#include <linux/socket.h>
#include <linux/stddef.h> /* for offsetof */
/* controller area network (CAN) kernel definitions */
@@ -193,26 +194,38 @@ struct canfd_frame {
#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.
+ * @prio: 11 bit arbitration priority (unused 5 bits must be set to zero)
+ * Shares the lower 16 bits of @can_id from struct can[fd]_frame
+ * @vcid: CAN XL virtual CAN network identifier
+ * @__res0: reserved / padding must be set to zero to maintain the CAN filter
+ * functionality as is holds the @can_id flags CAN_xxx_FLAG
+ * @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)
*/
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];
+#if defined(__LITTLE_ENDIAN)
+ __u16 prio; /* 11 bit priority for arbitration */
+ __u8 vcid; /* virtual CAN network identifier */
+ __u8 __res0; /* reserved / padding must be set to zero */
+#elif defined(__BIG_ENDIAN)
+ __u8 __res0; /* reserved / padding must be set to zero */
+ __u8 vcid; /* virtual CAN network identifier */
+ __u16 prio; /* 11 bit priority for arbitration */
+#else
+#error "Unknown endianness"
+#endif
+ __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))
diff --git a/include/uapi/linux/can/raw.h b/include/uapi/linux/can/raw.h
index 31622c9b7988..88e4e8176b89 100644
--- a/include/uapi/linux/can/raw.h
+++ b/include/uapi/linux/can/raw.h
@@ -63,8 +63,22 @@ enum {
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) */
+ CAN_RAW_XL_VCID_OPTS, /* CAN XL VCID configuration options */
};
+/* configuration for CAN XL virtual CAN identifier (VCID) handling */
+struct can_raw_vcid_options {
+ __u8 flags; /* flags for vcid (filter) behaviour */
+ __u8 tx_vcid; /* VCID value set into canxl_frame.vcid */
+ __u8 rx_vcid; /* VCID value for VCID filter */
+ __u8 rx_vcid_mask; /* VCID mask for VCID filter */
+};
+
+/* can_raw_vcid_options.flags for CAN XL virtual CAN identifier handling */
+#define CAN_RAW_XL_VCID_TX_SET 0x01
+#define CAN_RAW_XL_VCID_TX_PASS 0x02
+#define CAN_RAW_XL_VCID_RX_FILTER 0x04
+
#endif /* !_UAPI_CAN_RAW_H */
diff --git a/net/can/af_can.c b/net/can/af_can.c
index 7343fd487dbe..707576eeeb58 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -863,10 +863,12 @@ static __init int can_init(void)
int err;
/* check for correct padding to be able to use the structs similarly */
BUILD_BUG_ON(offsetof(struct can_frame, len) !=
offsetof(struct canfd_frame, len) ||
+ offsetof(struct can_frame, len) !=
+ offsetof(struct canxl_frame, flags) ||
offsetof(struct can_frame, data) !=
offsetof(struct canfd_frame, data));
pr_info("can: controller area network core\n");
diff --git a/net/can/raw.c b/net/can/raw.c
index e6b822624ba2..3083df64723c 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -89,10 +89,11 @@ struct raw_sock {
struct list_head notifier;
int loopback;
int recv_own_msgs;
int fd_frames;
int xl_frames;
+ struct can_raw_vcid_options raw_vcid_opts;
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;
@@ -132,14 +133,34 @@ 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;
/* make sure to not pass oversized frames to the socket */
- if ((!ro->fd_frames && can_is_canfd_skb(oskb)) ||
- (!ro->xl_frames && can_is_canxl_skb(oskb)))
+ if (!ro->fd_frames && can_is_canfd_skb(oskb))
return;
+ if (can_is_canxl_skb(oskb)) {
+ struct canxl_frame *cxl = (struct canxl_frame *)oskb->data;
+ struct can_raw_vcid_options *ropts = &ro->raw_vcid_opts;
+
+ /* make sure to not pass oversized frames to the socket */
+ if (!ro->xl_frames)
+ return;
+
+ /* filter CAN XL VCID content */
+ if (ropts->flags & CAN_RAW_XL_VCID_RX_FILTER) {
+ /* apply VCID filter if user enabled the filter */
+ if ((cxl->vcid & ropts->rx_vcid_mask) !=
+ (ropts->rx_vcid & ropts->rx_vcid_mask))
+ return;
+ } else {
+ /* no filter => do not forward VCID tagged frames */
+ if (cxl->vcid)
+ 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) {
if (!ro->join_filters)
return;
@@ -696,10 +717,19 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
/* Enabling CAN XL includes CAN FD */
if (ro->xl_frames)
ro->fd_frames = ro->xl_frames;
break;
+ case CAN_RAW_XL_VCID_OPTS:
+ if (optlen != sizeof(ro->raw_vcid_opts))
+ return -EINVAL;
+
+ if (copy_from_sockptr(&ro->raw_vcid_opts, 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))
@@ -784,10 +814,25 @@ static int raw_getsockopt(struct socket *sock, int level, int optname,
if (len > sizeof(int))
len = sizeof(int);
val = &ro->xl_frames;
break;
+ case CAN_RAW_XL_VCID_OPTS:
+ /* user space buffer to small for VCID opts? */
+ if (len < sizeof(ro->raw_vcid_opts)) {
+ /* return -ERANGE and needed space in optlen */
+ err = -ERANGE;
+ if (put_user(sizeof(ro->raw_vcid_opts), optlen))
+ err = -EFAULT;
+ } else {
+ if (len > sizeof(ro->raw_vcid_opts))
+ len = sizeof(ro->raw_vcid_opts);
+ if (copy_to_user(optval, &ro->raw_vcid_opts, len))
+ err = -EFAULT;
+ }
+ break;
+
case CAN_RAW_JOIN_FILTERS:
if (len > sizeof(int))
len = sizeof(int);
val = &ro->join_filters;
break;
@@ -814,12 +859,27 @@ static bool raw_bad_txframe(struct raw_sock *ro, struct sk_buff *skb, int mtu)
(mtu == CANFD_MTU || can_is_canxl_dev_mtu(mtu)))
return false;
/* CAN XL -> needs to be enabled and a CAN XL device */
if (ro->xl_frames && can_is_canxl_skb(skb) &&
- can_is_canxl_dev_mtu(mtu))
+ can_is_canxl_dev_mtu(mtu)) {
+ struct canxl_frame *cxl = (struct canxl_frame *)skb->data;
+ struct can_raw_vcid_options *ropts = &ro->raw_vcid_opts;
+
+ /* sanitize non CAN XL bits */
+ cxl->__res0 = 0;
+
+ /* clear VCID in CAN XL frame if pass through is disabled */
+ if (!(ropts->flags & CAN_RAW_XL_VCID_TX_PASS))
+ cxl->vcid = 0;
+
+ /* set VCID in CAN XL frame if enabled */
+ if (ropts->flags & CAN_RAW_XL_VCID_TX_SET)
+ cxl->vcid = ropts->tx_vcid;
+
return false;
+ }
return true;
}
static int raw_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
--
2.39.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v4] canxl: add virtual CAN network identifier support
2024-01-08 17:06 [PATCH v4] canxl: add virtual CAN network identifier support Oliver Hartkopp
@ 2024-01-28 10:13 ` Vincent MAILHOL
2024-01-28 19:12 ` Oliver Hartkopp
0 siblings, 1 reply; 3+ messages in thread
From: Vincent MAILHOL @ 2024-01-28 10:13 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: linux-can, mkl
Hi Oliver,
Sorry for the delay, last weeks were busier than average.
On Tue. 9 Jan 2024 at 02:10, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> CAN XL data frames contain an 8-bit virtual CAN network identifier (VCID).
> A VCID value of zero represents an 'untagged' CAN XL frame.
>
> To receive and send these optional VCIDs via CAN_RAW sockets a new socket
> option CAN_RAW_XL_VCID_OPTS is introduced to define/access VCID content:
>
> - tx: set the outgoing VCID value by the kernel (one fixed 8-bit value)
> - tx: pass through VCID values from the user space (e.g. for traffic replay)
> - rx: apply VCID receive filter (value/mask) to be passed to the user space
>
> With the 'tx pass through' option CAN_RAW_XL_VCID_TX_PASS all valid VCID
> values can be sent, e.g. to replay full qualified CAN XL traffic.
>
> The VCID value provided for the CAN_RAW_XL_VCID_TX_SET option will
> override the VCID value in the struct canxl_frame.vcid element defined
> for CAN_RAW_XL_VCID_TX_PASS when both flags are set.
>
> With a rx_vcid_mask of zero all possible VCID values (0x00 - 0xFF) are
> passed to the user space when the CAN_RAW_XL_VCID_RX_FILTER flag is set.
> Without this flag only untagged CAN XL frames (VCID = 0x00) are delivered
> to the user space (default).
>
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
I am not yet aware of all the details of CAN XL and its VCID, but to
the extent of my knowledge, the patch looks good.
I left two nitpicks. Notwithstanding: you can add my review tag to v5.
Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> ---
> include/uapi/linux/can.h | 41 ++++++++++++++--------
> include/uapi/linux/can/raw.h | 14 ++++++++
> net/can/af_can.c | 2 ++
> net/can/raw.c | 66 ++++++++++++++++++++++++++++++++++--
> 4 files changed, 106 insertions(+), 17 deletions(-)
>
> diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h
> index 939db2388208..8dcd8a9b5676 100644
> --- a/include/uapi/linux/can.h
> +++ b/include/uapi/linux/can.h
> @@ -44,10 +44,11 @@
> */
>
> #ifndef _UAPI_CAN_H
> #define _UAPI_CAN_H
>
> +#include <asm/byteorder.h>
> #include <linux/types.h>
> #include <linux/socket.h>
> #include <linux/stddef.h> /* for offsetof */
>
> /* controller area network (CAN) kernel definitions */
> @@ -193,26 +194,38 @@ struct canfd_frame {
> #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.
> + * @prio: 11 bit arbitration priority (unused 5 bits must be set to zero)
> + * Shares the lower 16 bits of @can_id from struct can[fd]_frame
> + * @vcid: CAN XL virtual CAN network identifier
> + * @__res0: reserved / padding must be set to zero to maintain the CAN filter
> + * functionality as is holds the @can_id flags CAN_xxx_FLAG
^^
as it holds
> + * @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)
> */
> 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];
> +#if defined(__LITTLE_ENDIAN)
> + __u16 prio; /* 11 bit priority for arbitration */
> + __u8 vcid; /* virtual CAN network identifier */
> + __u8 __res0; /* reserved / padding must be set to zero */
> +#elif defined(__BIG_ENDIAN)
> + __u8 __res0; /* reserved / padding must be set to zero */
> + __u8 vcid; /* virtual CAN network identifier */
> + __u16 prio; /* 11 bit priority for arbitration */
> +#else
> +#error "Unknown endianness"
> +#endif
> + __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))
> diff --git a/include/uapi/linux/can/raw.h b/include/uapi/linux/can/raw.h
> index 31622c9b7988..88e4e8176b89 100644
> --- a/include/uapi/linux/can/raw.h
> +++ b/include/uapi/linux/can/raw.h
> @@ -63,8 +63,22 @@ enum {
> 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) */
> + CAN_RAW_XL_VCID_OPTS, /* CAN XL VCID configuration options */
> };
>
> +/* configuration for CAN XL virtual CAN identifier (VCID) handling */
> +struct can_raw_vcid_options {
> + __u8 flags; /* flags for vcid (filter) behaviour */
> + __u8 tx_vcid; /* VCID value set into canxl_frame.vcid */
> + __u8 rx_vcid; /* VCID value for VCID filter */
> + __u8 rx_vcid_mask; /* VCID mask for VCID filter */
> +};
> +
> +/* can_raw_vcid_options.flags for CAN XL virtual CAN identifier handling */
> +#define CAN_RAW_XL_VCID_TX_SET 0x01
> +#define CAN_RAW_XL_VCID_TX_PASS 0x02
> +#define CAN_RAW_XL_VCID_RX_FILTER 0x04
> +
> #endif /* !_UAPI_CAN_RAW_H */
> diff --git a/net/can/af_can.c b/net/can/af_can.c
> index 7343fd487dbe..707576eeeb58 100644
> --- a/net/can/af_can.c
> +++ b/net/can/af_can.c
> @@ -863,10 +863,12 @@ static __init int can_init(void)
> int err;
>
> /* check for correct padding to be able to use the structs similarly */
> BUILD_BUG_ON(offsetof(struct can_frame, len) !=
> offsetof(struct canfd_frame, len) ||
> + offsetof(struct can_frame, len) !=
> + offsetof(struct canxl_frame, flags) ||
> offsetof(struct can_frame, data) !=
> offsetof(struct canfd_frame, data));
>
> pr_info("can: controller area network core\n");
>
> diff --git a/net/can/raw.c b/net/can/raw.c
> index e6b822624ba2..3083df64723c 100644
> --- a/net/can/raw.c
> +++ b/net/can/raw.c
> @@ -89,10 +89,11 @@ struct raw_sock {
> struct list_head notifier;
> int loopback;
> int recv_own_msgs;
> int fd_frames;
> int xl_frames;
> + struct can_raw_vcid_options raw_vcid_opts;
> 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;
> @@ -132,14 +133,34 @@ 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;
>
> /* make sure to not pass oversized frames to the socket */
> - if ((!ro->fd_frames && can_is_canfd_skb(oskb)) ||
> - (!ro->xl_frames && can_is_canxl_skb(oskb)))
> + if (!ro->fd_frames && can_is_canfd_skb(oskb))
> return;
>
> + if (can_is_canxl_skb(oskb)) {
> + struct canxl_frame *cxl = (struct canxl_frame *)oskb->data;
> + struct can_raw_vcid_options *ropts = &ro->raw_vcid_opts;
> +
> + /* make sure to not pass oversized frames to the socket */
> + if (!ro->xl_frames)
> + return;
> +
> + /* filter CAN XL VCID content */
> + if (ropts->flags & CAN_RAW_XL_VCID_RX_FILTER) {
> + /* apply VCID filter if user enabled the filter */
> + if ((cxl->vcid & ropts->rx_vcid_mask) !=
> + (ropts->rx_vcid & ropts->rx_vcid_mask))
> + return;
> + } else {
> + /* no filter => do not forward VCID tagged frames */
> + if (cxl->vcid)
> + 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) {
> if (!ro->join_filters)
> return;
> @@ -696,10 +717,19 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
> /* Enabling CAN XL includes CAN FD */
> if (ro->xl_frames)
> ro->fd_frames = ro->xl_frames;
> break;
>
> + case CAN_RAW_XL_VCID_OPTS:
> + if (optlen != sizeof(ro->raw_vcid_opts))
> + return -EINVAL;
> +
> + if (copy_from_sockptr(&ro->raw_vcid_opts, 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))
> @@ -784,10 +814,25 @@ static int raw_getsockopt(struct socket *sock, int level, int optname,
> if (len > sizeof(int))
> len = sizeof(int);
> val = &ro->xl_frames;
> break;
>
> + case CAN_RAW_XL_VCID_OPTS:
> + /* user space buffer to small for VCID opts? */
> + if (len < sizeof(ro->raw_vcid_opts)) {
> + /* return -ERANGE and needed space in optlen */
> + err = -ERANGE;
> + if (put_user(sizeof(ro->raw_vcid_opts), optlen))
> + err = -EFAULT;
> + } else {
> + if (len > sizeof(ro->raw_vcid_opts))
> + len = sizeof(ro->raw_vcid_opts);
> + if (copy_to_user(optval, &ro->raw_vcid_opts, len))
> + err = -EFAULT;
> + }
> + break;
> +
> case CAN_RAW_JOIN_FILTERS:
> if (len > sizeof(int))
> len = sizeof(int);
> val = &ro->join_filters;
> break;
> @@ -814,12 +859,27 @@ static bool raw_bad_txframe(struct raw_sock *ro, struct sk_buff *skb, int mtu)
> (mtu == CANFD_MTU || can_is_canxl_dev_mtu(mtu)))
> return false;
>
> /* CAN XL -> needs to be enabled and a CAN XL device */
> if (ro->xl_frames && can_is_canxl_skb(skb) &&
> - can_is_canxl_dev_mtu(mtu))
> + can_is_canxl_dev_mtu(mtu)) {
> + struct canxl_frame *cxl = (struct canxl_frame *)skb->data;
> + struct can_raw_vcid_options *ropts = &ro->raw_vcid_opts;
> +
> + /* sanitize non CAN XL bits */
> + cxl->__res0 = 0;
> +
> + /* clear VCID in CAN XL frame if pass through is disabled */
> + if (!(ropts->flags & CAN_RAW_XL_VCID_TX_PASS))
> + cxl->vcid = 0;
> +
> + /* set VCID in CAN XL frame if enabled */
> + if (ropts->flags & CAN_RAW_XL_VCID_TX_SET)
> + cxl->vcid = ropts->tx_vcid;
Just in terms of software design, I am not sure why this sanitization
is part of raw_bad_txframe(). The purpose of this function is, so far,
to just return a boolean telling whether or not the frame is valid. It
is surprising to repurpose this to also do the sanitization. I would
rather see the above code go in a new dedicated function (maybe
raw_sanitize_tx_frame()) and call that new function from
raw_sendmsg().
> return false;
> + }
>
> return true;
> }
>
> static int raw_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
> --
> 2.39.2
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v4] canxl: add virtual CAN network identifier support
2024-01-28 10:13 ` Vincent MAILHOL
@ 2024-01-28 19:12 ` Oliver Hartkopp
0 siblings, 0 replies; 3+ messages in thread
From: Oliver Hartkopp @ 2024-01-28 19:12 UTC (permalink / raw)
To: Vincent MAILHOL; +Cc: linux-can, mkl
Hi Vincent,
On 2024-01-28 11:13, Vincent MAILHOL wrote:
> Sorry for the delay, last weeks were busier than average.
No problem. I just wanted to finalize the data structure for further
work on the CAN XL hardware driver and some CAN CiA protocol
implementations.
> I am not yet aware of all the details of CAN XL and its VCID, but to
> the extent of my knowledge, the patch looks good.
Thanks.
> I left two nitpicks. Notwithstanding: you can add my review tag to v5.
>
> Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
I've sent out a v5 that addresses both of your good remarks:
https://lore.kernel.org/linux-can/20240128183758.68401-1-socketcan@hartkopp.net/
>> + * functionality as is holds the @can_id flags CAN_xxx_FLAG
> ^^
> as it holds
Fixed.
>> @@ -814,12 +859,27 @@ static bool raw_bad_txframe(struct raw_sock *ro, struct sk_buff *skb, int mtu)
>> (mtu == CANFD_MTU || can_is_canxl_dev_mtu(mtu)))
>> return false;
(..)
> Just in terms of software design, I am not sure why this sanitization
> is part of raw_bad_txframe(). The purpose of this function is, so far,
> to just return a boolean telling whether or not the frame is valid. It
> is surprising to repurpose this to also do the sanitization. I would
> rather see the above code go in a new dedicated function (maybe
> raw_sanitize_tx_frame()) and call that new function from
> raw_sendmsg().
Right. I felt similar that the naming does not fit that good anymore
when adding the VCID handling :-/
I splitted up the raw_bad_txframe() function and created two new functions:
raw_check_txframe() (is raw_bad_txframe() with a non-bool return value)
raw_put_canxl_vcid() (which handles the VCID content for CAN XL frames)
Best regards,
Oliver
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-01-28 19:24 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-01-08 17:06 [PATCH v4] canxl: add virtual CAN network identifier support Oliver Hartkopp
2024-01-28 10:13 ` Vincent MAILHOL
2024-01-28 19:12 ` 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.