On 13.07.22 09:15, Vincent Mailhol wrote:
On Wed. 13 Jul. 2022 at 16:02, Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> wrote:
On 13.07.2022 08:58:26, Vincent Mailhol wrote:
On Wed. 13 Jul. 2022 at 05:20, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote:
On 12.07.22 03:23, Vincent Mailhol wrote:
On Tue. 12 Jul. 2022 at 03:44, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote:
Enable the PF_CAN infrastructure to handle CAN XL frames. A new ethernet
protocol type ETH_P_CANXL is defined to tag skbuffs containing the CAN XL
frame data structure.
As the length information is now a uint16 value for CAN XL a new helper
function can_get_data_len() is introduced to retrieve the data length
from all types of CAN frames.
Signed-off-by: Oliver Hartkopp <socketcan@xxxxxxxxxxxx>
---
include/linux/can/skb.h | 14 ++++++++++
include/uapi/linux/if_ether.h | 1 +
net/can/af_can.c | 49 +++++++++++++++++++++++++++++------
3 files changed, 56 insertions(+), 8 deletions(-)
diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
index 182749e858b3..d043bc4afd6d 100644
--- a/include/linux/can/skb.h
+++ b/include/linux/can/skb.h
@@ -101,6 +101,20 @@ 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;
}
+/* get data length inside of CAN frame for all frame types */> +static inline unsigned int can_get_data_len(struct sk_buff *skb)
+{
+ if(skb->len == CANXL_MTU) {
+ const struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
+
+ return cfx->len;
+ } else {
+ const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
+
+ return cfd->len;
+ }
+}
What about the RTR frames?
If there are cases in which we intentionally want the declared length
and not the actual length, it might be good to have two distinct
helper functions.
Good idea.
/* get data length inside of CAN frame for all frame types. For
* RTR frames, return zero. */
static inline unsigned int can_get_actual_len(struct sk_buff *skb)
I would name this one can_get_data_len()
ACK.
So, according to Marc's comments (c.f. below):
can_skb_get_data_len()
{
const struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
if (skb->len == CANXL_MTU)
return cfx->len;
/* RTR frames have an actual length of zero */
if (skb->len == CAN_MTU && cfd->flags & CAN_RTR_FLAG)
return 0;
return cfd->len;
}
/* get data length inside of CAN frame for all frame types. For
* RTR frames, return requested length. */
static inline unsigned int can_get_declared_len(struct sk_buff *skb)
I would name this one can_get_len()
I anticipate that most of the time, developers do not want to get the
RTR length but the actual length (e.g. to memcpy data[] or to increase
statistics). People will get confused between can_get_data_len() and
can_get_len() due to the similar names. So I would suggest a more
explicit name to point out that this one is probably not the one you
want to use.
Candidates name I can think of:
* can_get_raw_len()
* can_get_advertised_len()
* can_get_rtr_len()
But these three functions still bconfuse me.
IMO we need two values:
- the data byte length (RTR aware)
- the (raw) length value
My suggestion for a naming would be:
- can_skb_get_data_len()
- can_skb_get_len_value()
This also fits to the existing
can_skb_get_frame_len().
So according to Marc's comments (c.f. below), candidates become:
* can_skb_get_raw_len()
* can_skb_get_advertised_len()
* can_skb_get_rtr_len()
Right.
The only time you want to access the raw len (with real RTR value) is
in the TX path when you fill your device's structures. But here the
can_get_cc_dlc() is a better helper function which is already RTR
aware.
There also is
| unsigned int can_skb_get_frame_len(const struct sk_buff *skb)
I totally forgot about that one, despite the fact that I co-developed
it with you.
It gives the length of the frame on the wire in bytes. We should use a
common naming scheme. Let's always use can_skb_ as a prefix or drop the
skb_ from this function.
You are right. The idea was to use the can_skb_ prefix because the
argument of the function was a struct skb_buff. Same applies here.
Yes, that's fine!
Best regards,
Oliver
