[PATCH RFC net-next] can: dev: can_skb_get_dll_len(): introduce function to get data length of frame in data link layer

[PATCH RFC net-next] can: dev: can_skb_get_dll_len(): introduce function to get data length of frame in data link layer

[Marc Kleine-Budde]

From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

This patch adds the function can_skb_get_dll_len() which returns the length of
a CAN frame on the data link layer, including Start-of-frame, Identifier,
various other bits, the CRC, the End-of-frame, the Inter frame spacing and the
actual data.

Co-developed-by: Arunachalam Santhanam <arunachalam.santhanam@in.bosch.com>
Not-Signed-off-by: Arunachalam Santhanam <arunachalam.santhanam@in.bosch.com>
Co-developed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Not-Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Co-developed-by: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
Hello,

while reviewing the etas_es58X driver and hacking on BQL support for the
mcp251xfd driver, it turned out the a function to calculate the length of the
CAN frame that's send over the wire should go into the generic CAN dev code.

Looking at the CAN and OSI layering I think the first layer where we have all
the bits that we see on the wire is the data link layer (DLL).

https://www.can-cia.org/can-knowledge/can/can-data-link-layers

This is why I named the function can_skb_get_dll_len().

I'm planing to add a (better) calculation of the CAN-FD dll_len, depending on
the data and including different CRC lengths.

As this code is copied from the etas_es58X driver, I've added the authors as
Co-developed-by and I'd like to add their S-o-b to this patch.

Please review and commnt on the naming on the functions.

regards,
Marc

 drivers/net/can/dev.c   | 103 ++++++++++++++++++++++++++++++++++++++++
 include/linux/can/dev.h |   3 ++
 2 files changed, 106 insertions(+)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 81e39d7507d8..58c04785b46d 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -58,6 +58,109 @@ u8 can_len2dlc(u8 len)
 }
 EXPORT_SYMBOL_GPL(can_len2dlc);
 
+/* Size of a Classical CAN Standard Frame in Bits:
+ *
+ * Start-of-frame			1
+ * Identifier				11
+ * Remote transmission request (RTR)	1
+ * Identifier extension bit (IDE)	1
+ * Reserved bit (r0)			1
+ * Data length code (DLC)		4
+ * Data field				0...64
+ * CRC					15
+ * CRC delimiter			1
+ * ACK slot				1
+ * ACK delimiter			1
+ * End-of-frame (EOF)			7
+ * Inter frame spacing			3
+ *
+ * Rounded up and ignoring bitsuffing
+ */
+#define CAN_DLL_OVERHEAD_SFF DIV_ROUND_UP(47, 8)
+
+/* Size of a Classical CAN Extended Frame in Bits:
+ *
+ * Start-of-frame			1
+ * Identifier A 			11
+ * Substitute remote request (SRR)	1
+ * Identifier extension bit (IDE)	1
+ * Identifier B				18
+ * Remote transmission request (RTR)	1
+ * Reserved bits (r1, r0)		2
+ * Data length code (DLC)		4
+ * Data field				0...64
+ * CRC					15
+ * CRC delimiter			1
+ * ACK slot				1
+ * ACK delimiter			1
+ * End-of-frame (EOF)			7
+ * Inter frame spacing			3
+ *
+ * Rounded up and ignoring bitsuffing
+ */
+#define CAN_DLL_OVERHEAD_EFF DIV_ROUND_UP(67, 8)
+
+/* Maximum size of a Classical CAN frame
+ * (rounded up and ignoring bitsuffing)
+ */
+#define CAN_DLL_LEN_MAX (CAN_DLL_OVERHEAD_EFF + CAN_MAX_DLEN)
+
+/* Maximum size of a CAN-FD frame (rough estimation because
+ * ES58X_SFF_BYTES() and ES58X_EFF_BYTES() macros are using the
+ * constant values for Classical CAN, not CAN-FD).
+ */
+#define CANFD_DLL_LEN_MAX (CAN_DLL_OVERHEAD_EFF + CANFD_MAX_DLEN)
+
+/**
+ * can_skb_get_dll_len() - Calculate the CAN Data Link Layer length in
+ * bytes of a given skb.
+ * @skb: socket buffer of a CAN message.
+ *
+ * Do a rough calculation: bit stuffing is ignored and length in bits
+ * is rounded up to a length in bytes.
+ *
+ * Rationale: this function is to be used for the BQL functions
+ * (netdev_sent_queue() and netdev_completed_queue()) which expect a
+ * value in bytes. Just using skb->len is insufficient because it will
+ * return the constant value of CAN(FD)_MTU. Doing the bit stuffing
+ * calculation would be too expensive in term of computing resources
+ * for no noticeable gain.
+ *
+ * Remarks:
+ * a.) The payload of CAN FD frames with BRS flag are sent at a
+ * different bitrate. Currently, the can-utils canbusload tool does
+ * not support CAN-FD yet and so we could not run any benchmark to
+ * measure the impact. There might be possible improvement here.
+ * b.) The macro CAN_DLL_OVERHEAD_EFF and CAN_DLL_OVERHEAD_SFF are
+ * for classical CAN. Need to do the addition for CAN-FD (the value
+ * are expected to be close enough so the impact should be minimal or
+ * none).
+ *
+ * Return: length in bytes.
+ */
+unsigned int can_skb_get_dll_len(const struct sk_buff *skb)
+{
+	const struct canfd_frame *cf = (const struct canfd_frame *)skb->data;
+	u8 len;
+
+	if (can_is_canfd_skb(skb)) {
+		len = cf->len + CAN_DLL_OVERHEAD_EFF;
+	} else {
+		if (cf->can_id & CAN_RTR_FLAG)
+			len = 0;
+		else
+			len = cf->len;
+
+		if (cf->can_id & CAN_EFF_FLAG)
+			len += CAN_DLL_OVERHEAD_EFF;
+		else
+			len += CAN_DLL_OVERHEAD_SFF;
+	}
+
+	return len;
+}
+EXPORT_SYMBOL_GPL(can_skb_get_dll_len);
+
 #ifdef CONFIG_CAN_CALC_BITTIMING
 #define CAN_CALC_MAX_ERROR 50 /* in one-tenth of a percent */
 
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 41ff31795320..fc830f919099 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -192,6 +192,9 @@ u8 can_dlc2len(u8 can_dlc);
 /* map the sanitized data length to an appropriate data length code */
 u8 can_len2dlc(u8 len);
 
+/* get data link layer length of CAN/CAN-FD frame */
+unsigned int can_skb_get_dll_len(const struct sk_buff *skb);
+
 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.29.2

Re: [PATCH RFC net-next] can: dev: can_skb_get_dll_len(): introduce function to get data length of frame in data link layer

[Vincent MAILHOL]

On Mon 14 Dec. 2020 at 18:14, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>
> This patch adds the function can_skb_get_dll_len() which returns the length of
> a CAN frame on the data link layer, including Start-of-frame, Identifier,
> various other bits, the CRC, the End-of-frame, the Inter frame spacing and the
> actual data.
>
> Co-developed-by: Arunachalam Santhanam <arunachalam.santhanam@in.bosch.com>
> Not-Signed-off-by: Arunachalam Santhanam <arunachalam.santhanam@in.bosch.com>
> Co-developed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> Not-Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> Co-developed-by: Marc Kleine-Budde <mkl@pengutronix.de>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
> Hello,
>
> while reviewing the etas_es58X driver and hacking on BQL support for the
> mcp251xfd driver, it turned out the a function to calculate the length of the
> CAN frame that's send over the wire should go into the generic CAN dev code.

It took me a bit of time to figure out how to use the BQL. Contrary to
the other features, I couldn't pull inspiration from the other CAN
drivers. I am happy to see that you want to extend it to other
drivers.

Also, on a side topic, please let me know if you have any comments on
the etas_es58x driver ;)

> Looking at the CAN and OSI layering I think the first layer where we have all
> the bits that we see on the wire is the data link layer (DLL).
>
> https://www.can-cia.org/can-knowledge/can/can-data-link-layers
>
> This is why I named the function can_skb_get_dll_len().

I would personally prefer to call it can_skb_get_frame_len(). The ISO
11898 standard defines the "frame" as:

   Protocol Data Unit of the data link layer specifying the
   arrangement and meaning of bits or bit fields in the sequence of
   transfer across the transmission medium.

I imagine that you want to introduce the term "dll" instead of "frame"
to clearly differentiate it from the struct can_frame and struct
can_fd_frame. But "frame" remains the most relevant term according to
the standard. I think that things are clear enough so that other
kernel hackers would understand that the len fields of struct can_frame
and struct can_fd_frame represent the length of the payload and that
can_skb_get_frame_len() is the length of the frame at the data link
layer.

> I'm planing to add a (better) calculation of the CAN-FD dll_len, depending on
> the data and including different CRC lengths.

Great! For my driver, everything seemed OK as it is currently but if
this gets merged to the generic CAN dev code this part should
definitely be improved (the comment I added in my code was there to
warn whoever wanted to reuse it about the limitations).

> As this code is copied from the etas_es58X driver, I've added the authors as
> Co-developed-by and I'd like to add their S-o-b to this patch.
>
> Please review and commnt on the naming on the functions.
>
> regards,
> Marc
>
>  drivers/net/can/dev.c   | 103 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/can/dev.h |   3 ++
>  2 files changed, 106 insertions(+)
>
> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> index 81e39d7507d8..58c04785b46d 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
> @@ -58,6 +58,109 @@ u8 can_len2dlc(u8 len)
>  }
>  EXPORT_SYMBOL_GPL(can_len2dlc);
>
> +/* Size of a Classical CAN Standard Frame in Bits:

Size of a Classical CAN Standard Frame in Bytes:

The macro divides the grand total by 8. The subtotal is indeed
presented in bits but the macro is in bytes.

> + *
> + * Start-of-frame                      1
> + * Identifier                          11
> + * Remote transmission request (RTR)   1
> + * Identifier extension bit (IDE)      1
> + * Reserved bit (r0)                   1
> + * Data length code (DLC)              4
> + * Data field                          0...64
> + * CRC                                 15
> + * CRC delimiter                       1
> + * ACK slot                            1
> + * ACK delimiter                       1
> + * End-of-frame (EOF)                  7
> + * Inter frame spacing                 3
> + *
> + * Rounded up and ignoring bitsuffing
> + */
> +#define CAN_DLL_OVERHEAD_SFF DIV_ROUND_UP(47, 8)
> +
> +/* Size of a Classical CAN Extended Frame in Bits:

Same as above.

> + *
> + * Start-of-frame                      1
> + * Identifier A                        11
> + * Substitute remote request (SRR)     1
> + * Identifier extension bit (IDE)      1
> + * Identifier B                                18
> + * Remote transmission request (RTR)   1
> + * Reserved bits (r1, r0)              2
> + * Data length code (DLC)              4
> + * Data field                          0...64
> + * CRC                                 15
> + * CRC delimiter                       1
> + * ACK slot                            1
> + * ACK delimiter                       1
> + * End-of-frame (EOF)                  7
> + * Inter frame spacing                 3
> + *
> + * Rounded up and ignoring bitsuffing
> + */
> +#define CAN_DLL_OVERHEAD_EFF DIV_ROUND_UP(67, 8)
> +
> +/* Maximum size of a Classical CAN frame
> + * (rounded up and ignoring bitsuffing)
> + */
> +#define CAN_DLL_LEN_MAX (CAN_DLL_OVERHEAD_EFF + CAN_MAX_DLEN)
> +
> +/* Maximum size of a CAN-FD frame (rough estimation because
> + * ES58X_SFF_BYTES() and ES58X_EFF_BYTES() macros are using the
> + * constant values for Classical CAN, not CAN-FD).
> + */

/* Maximum size of a CAN-FD frame (rough estimation because
 * CAN_DLL_OVERHEAD_SFF and CAN_DLL_OVERHEAD_EFF macros are using the
 * constant values for Classical CAN, not CAN-FD).
 */

Some leftovers from the etas_es58x :)

> + */
> +#define CANFD_DLL_LEN_MAX (CAN_DLL_OVERHEAD_EFF + CANFD_MAX_DLEN)
> +
> +/**
> + * can_skb_get_dll_len() - Calculate the CAN Data Link Layer length in
> + * bytes of a given skb.
> + * @skb: socket buffer of a CAN message.
> + *
> + * Do a rough calculation: bit stuffing is ignored and length in bits
> + * is rounded up to a length in bytes.
> + *
> + * Rationale: this function is to be used for the BQL functions
> + * (netdev_sent_queue() and netdev_completed_queue()) which expect a
> + * value in bytes. Just using skb->len is insufficient because it will
> + * return the constant value of CAN(FD)_MTU. Doing the bit stuffing
> + * calculation would be too expensive in term of computing resources
> + * for no noticeable gain.
> + *
> + * Remarks:
> + * a.) The payload of CAN FD frames with BRS flag are sent at a
> + * different bitrate. Currently, the can-utils canbusload tool does
> + * not support CAN-FD yet and so we could not run any benchmark to
> + * measure the impact. There might be possible improvement here.

My initial idea was to convert the data transmitted at data bitrate
speed to their equivalent duration at nominal rate (example: 8 bytes
at 2Mbps are equivalent to 2 bytes at 500 Kbps).

Code would look as below:

unsigned int can_skb_get_frame_len(const struct can_priv *can,
                   const struct sk_buff *skb)
{
/* ... */

    if (cfd->flags & CANFD_BRS)
        len = mult_frac(CANFD_FRAME_OVERHEAD, can->bittiming.bitrate,
                 can->data_bittiming.bitrate);
/* ... */
}

This is just an overview, the data bitrate is also applied to some
other fields of the frame (e.g. the CRC). So when creating the macro
for the CANFD_FRAME_OVERHEAD, it should be split between the nominal
and data bitrates.

> + * b.) The macro CAN_DLL_OVERHEAD_EFF and CAN_DLL_OVERHEAD_SFF are
> + * for classical CAN. Need to do the addition for CAN-FD (the value
> + * are expected to be close enough so the impact should be minimal or
> + * none).
> + *
> + * Return: length in bytes.
> + */
> +unsigned int can_skb_get_dll_len(const struct sk_buff *skb)
> +{
> +       const struct canfd_frame *cf = (const struct canfd_frame *)skb->data;
> +       u8 len;
> +
> +       if (can_is_canfd_skb(skb)) {
> +               len = cf->len + CAN_DLL_OVERHEAD_EFF;

The CAN-FD frame can also be sent in SFF. An additional check on the
CAN_EFF_FLAG is needed here.

> +       } else {
> +               if (cf->can_id & CAN_RTR_FLAG)
> +                       len = 0;
> +               else
> +                       len = cf->len;
> +
> +               if (cf->can_id & CAN_EFF_FLAG)
> +                       len += CAN_DLL_OVERHEAD_EFF;
> +               else
> +                       len += CAN_DLL_OVERHEAD_SFF;
> +       }
> +
> +       return len;
> +}
> +EXPORT_SYMBOL_GPL(can_skb_get_dll_len);
> +
>  #ifdef CONFIG_CAN_CALC_BITTIMING
>  #define CAN_CALC_MAX_ERROR 50 /* in one-tenth of a percent */
>
> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
> index 41ff31795320..fc830f919099 100644
> --- a/include/linux/can/dev.h
> +++ b/include/linux/can/dev.h
> @@ -192,6 +192,9 @@ u8 can_dlc2len(u8 can_dlc);
>  /* map the sanitized data length to an appropriate data length code */
>  u8 can_len2dlc(u8 len);
>
> +/* get data link layer length of CAN/CAN-FD frame */
> +unsigned int can_skb_get_dll_len(const struct sk_buff *skb);
> +
>  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) \

Yours sincerely,
Vincent

Re: [PATCH RFC net-next] can: dev: can_skb_get_dll_len(): introduce function to get data length of frame in data link layer

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 2291 bytes --]

On 12/14/20 10:14 AM, Marc Kleine-Budde wrote:
> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> 
> This patch adds the function can_skb_get_dll_len() which returns the length of
> a CAN frame on the data link layer, including Start-of-frame, Identifier,
> various other bits, the CRC, the End-of-frame, the Inter frame spacing and the
> actual data.
> 
> Co-developed-by: Arunachalam Santhanam <arunachalam.santhanam@in.bosch.com>
> Not-Signed-off-by: Arunachalam Santhanam <arunachalam.santhanam@in.bosch.com>
> Co-developed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> Not-Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> Co-developed-by: Marc Kleine-Budde <mkl@pengutronix.de>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
> Hello,
> 
> while reviewing the etas_es58X driver and hacking on BQL support for the
> mcp251xfd driver, it turned out the a function to calculate the length of the
> CAN frame that's send over the wire should go into the generic CAN dev code.
> 
> Looking at the CAN and OSI layering I think the first layer where we have all
> the bits that we see on the wire is the data link layer (DLL).
> 
> https://www.can-cia.org/can-knowledge/can/can-data-link-layers
> 
> This is why I named the function can_skb_get_dll_len().
> 
> I'm planing to add a (better) calculation of the CAN-FD dll_len, depending on
> the data and including different CRC lengths.
> 
> As this code is copied from the etas_es58X driver, I've added the authors as
> Co-developed-by and I'd like to add their S-o-b to this patch.
> 
> Please review and commnt on the naming on the functions.

When implementing BQL we need the CAN frame's DLL len twice:
1. When sending the package to the hardware
2. After the TX-complete IRQ

We can calculate this information twice, but I thought we might put it into the
struct can_skb_priv.

https://elixir.bootlin.com/linux/latest/source/include/linux/can/skb.h#L34

Thoughts?

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH RFC net-next] can: dev: can_skb_get_dll_len(): introduce function to get data length of frame in data link layer

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 10716 bytes --]

On 12/14/20 5:59 PM, Vincent MAILHOL wrote:
>> while reviewing the etas_es58X driver and hacking on BQL support for the
>> mcp251xfd driver, it turned out the a function to calculate the length of the
>> CAN frame that's send over the wire should go into the generic CAN dev code.
> 
> It took me a bit of time to figure out how to use the BQL. Contrary to
> the other features, I couldn't pull inspiration from the other CAN
> drivers. I am happy to see that you want to extend it to other
> drivers.
> 
> Also, on a side topic, please let me know if you have any comments on
> the etas_es58x driver ;)

I'll look more into the driver later this week.

>> Looking at the CAN and OSI layering I think the first layer where we have all
>> the bits that we see on the wire is the data link layer (DLL).
>>
>> https://www.can-cia.org/can-knowledge/can/can-data-link-layers
>>
>> This is why I named the function can_skb_get_dll_len().
> 
> I would personally prefer to call it can_skb_get_frame_len(). The ISO
> 11898 standard defines the "frame" as:
> 
>    Protocol Data Unit of the data link layer specifying the
>    arrangement and meaning of bits or bit fields in the sequence of
>    transfer across the transmission medium.
> 
> I imagine that you want to introduce the term "dll" instead of "frame"
> to clearly differentiate it from the struct can_frame and struct
> can_fd_frame.

ACK

> But "frame" remains the most relevant term according to
> the standard. I think that things are clear enough so that other
> kernel hackers would understand that the len fields of struct can_frame
> and struct can_fd_frame represent the length of the payload and that
> can_skb_get_frame_len() is the length of the frame at the data link
> layer.

Ok.

>> I'm planing to add a (better) calculation of the CAN-FD dll_len, depending on
>> the data and including different CRC lengths.
> 
> Great! For my driver, everything seemed OK as it is currently but if
> this gets merged to the generic CAN dev code this part should
> definitely be improved (the comment I added in my code was there to
> warn whoever wanted to reuse it about the limitations).

:)

>> As this code is copied from the etas_es58X driver, I've added the authors as
>> Co-developed-by and I'd like to add their S-o-b to this patch.
>>
>> Please review and commnt on the naming on the functions.
>>
>> regards,
>> Marc
>>
>>  drivers/net/can/dev.c   | 103 ++++++++++++++++++++++++++++++++++++++++
>>  include/linux/can/dev.h |   3 ++
>>  2 files changed, 106 insertions(+)
>>
>> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
>> index 81e39d7507d8..58c04785b46d 100644
>> --- a/drivers/net/can/dev.c
>> +++ b/drivers/net/can/dev.c
>> @@ -58,6 +58,109 @@ u8 can_len2dlc(u8 len)
>>  }
>>  EXPORT_SYMBOL_GPL(can_len2dlc);
>>
>> +/* Size of a Classical CAN Standard Frame in Bits:
> 
> Size of a Classical CAN Standard Frame in Bytes:

The table is in bits, the overhead makros are in bytes, I'll try to clarift this.

> The macro divides the grand total by 8. The subtotal is indeed
> presented in bits but the macro is in bytes.
> 
>> + *
>> + * Start-of-frame                      1
>> + * Identifier                          11
>> + * Remote transmission request (RTR)   1
>> + * Identifier extension bit (IDE)      1
>> + * Reserved bit (r0)                   1
>> + * Data length code (DLC)              4
>> + * Data field                          0...64
>> + * CRC                                 15
>> + * CRC delimiter                       1
>> + * ACK slot                            1
>> + * ACK delimiter                       1
>> + * End-of-frame (EOF)                  7
>> + * Inter frame spacing                 3
>> + *
>> + * Rounded up and ignoring bitsuffing
>> + */
>> +#define CAN_DLL_OVERHEAD_SFF DIV_ROUND_UP(47, 8)
>> +
>> +/* Size of a Classical CAN Extended Frame in Bits:
> 
> Same as above.
> 
>> + *
>> + * Start-of-frame                      1
>> + * Identifier A                        11
>> + * Substitute remote request (SRR)     1
>> + * Identifier extension bit (IDE)      1
>> + * Identifier B                                18
>> + * Remote transmission request (RTR)   1
>> + * Reserved bits (r1, r0)              2
>> + * Data length code (DLC)              4
>> + * Data field                          0...64
>> + * CRC                                 15
>> + * CRC delimiter                       1
>> + * ACK slot                            1
>> + * ACK delimiter                       1
>> + * End-of-frame (EOF)                  7
>> + * Inter frame spacing                 3
>> + *
>> + * Rounded up and ignoring bitsuffing
>> + */
>> +#define CAN_DLL_OVERHEAD_EFF DIV_ROUND_UP(67, 8)
>> +
>> +/* Maximum size of a Classical CAN frame
>> + * (rounded up and ignoring bitsuffing)
>> + */
>> +#define CAN_DLL_LEN_MAX (CAN_DLL_OVERHEAD_EFF + CAN_MAX_DLEN)
>> +
>> +/* Maximum size of a CAN-FD frame (rough estimation because
>> + * ES58X_SFF_BYTES() and ES58X_EFF_BYTES() macros are using the
>> + * constant values for Classical CAN, not CAN-FD).
>> + */
> 
> /* Maximum size of a CAN-FD frame (rough estimation because
>  * CAN_DLL_OVERHEAD_SFF and CAN_DLL_OVERHEAD_EFF macros are using the
>  * constant values for Classical CAN, not CAN-FD).
>  */
> 
> Some leftovers from the etas_es58x :)

ACK, in the mean time I've makros for CAN-FD.

> 
>> + */
>> +#define CANFD_DLL_LEN_MAX (CAN_DLL_OVERHEAD_EFF + CANFD_MAX_DLEN)
>> +
>> +/**
>> + * can_skb_get_dll_len() - Calculate the CAN Data Link Layer length in
>> + * bytes of a given skb.
>> + * @skb: socket buffer of a CAN message.
>> + *
>> + * Do a rough calculation: bit stuffing is ignored and length in bits
>> + * is rounded up to a length in bytes.
>> + *
>> + * Rationale: this function is to be used for the BQL functions
>> + * (netdev_sent_queue() and netdev_completed_queue()) which expect a
>> + * value in bytes. Just using skb->len is insufficient because it will
>> + * return the constant value of CAN(FD)_MTU. Doing the bit stuffing
>> + * calculation would be too expensive in term of computing resources
>> + * for no noticeable gain.
>> + *
>> + * Remarks:
>> + * a.) The payload of CAN FD frames with BRS flag are sent at a
>> + * different bitrate. Currently, the can-utils canbusload tool does
>> + * not support CAN-FD yet and so we could not run any benchmark to
>> + * measure the impact. There might be possible improvement here.
> 
> My initial idea was to convert the data transmitted at data bitrate
> speed to their equivalent duration at nominal rate (example: 8 bytes
> at 2Mbps are equivalent to 2 bytes at 500 Kbps).
> 
> Code would look as below:
> 
> unsigned int can_skb_get_frame_len(const struct can_priv *can,
>                    const struct sk_buff *skb)
> {
> /* ... */
> 
>     if (cfd->flags & CANFD_BRS)
>         len = mult_frac(CANFD_FRAME_OVERHEAD, can->bittiming.bitrate,
>                  can->data_bittiming.bitrate);
> /* ... */
> }
> 
> This is just an overview, the data bitrate is also applied to some
> other fields of the frame (e.g. the CRC). So when creating the macro
> for the CANFD_FRAME_OVERHEAD, it should be split between the nominal
> and data bitrates.

I think we shouldn't mess with the number of bytes depending on BRS. When it
turns out that for CAN-FD this is not exact enough, we should look into the wifi
drivers. As wifi doesn't even have a constant channel speed.

>> + * b.) The macro CAN_DLL_OVERHEAD_EFF and CAN_DLL_OVERHEAD_SFF are
>> + * for classical CAN. Need to do the addition for CAN-FD (the value
>> + * are expected to be close enough so the impact should be minimal or
>> + * none).
>> + *
>> + * Return: length in bytes.
>> + */
>> +unsigned int can_skb_get_dll_len(const struct sk_buff *skb)
>> +{
>> +       const struct canfd_frame *cf = (const struct canfd_frame *)skb->data;
>> +       u8 len;
>> +
>> +       if (can_is_canfd_skb(skb)) {
>> +               len = cf->len + CAN_DLL_OVERHEAD_EFF;
> 
> The CAN-FD frame can also be sent in SFF. An additional check on the
> CAN_EFF_FLAG is needed here.

ACK. CAN-FD was work-in-progress in this patch.

>> +       } else {
>> +               if (cf->can_id & CAN_RTR_FLAG)
>> +                       len = 0;
>> +               else
>> +                       len = cf->len;
>> +
>> +               if (cf->can_id & CAN_EFF_FLAG)
>> +                       len += CAN_DLL_OVERHEAD_EFF;
>> +               else
>> +                       len += CAN_DLL_OVERHEAD_SFF;
>> +       }
>> +
>> +       return len;
>> +}
>> +EXPORT_SYMBOL_GPL(can_skb_get_dll_len);
>> +
>>  #ifdef CONFIG_CAN_CALC_BITTIMING
>>  #define CAN_CALC_MAX_ERROR 50 /* in one-tenth of a percent */
>>
>> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
>> index 41ff31795320..fc830f919099 100644
>> --- a/include/linux/can/dev.h
>> +++ b/include/linux/can/dev.h
>> @@ -192,6 +192,9 @@ u8 can_dlc2len(u8 can_dlc);
>>  /* map the sanitized data length to an appropriate data length code */
>>  u8 can_len2dlc(u8 len);
>>
>> +/* get data link layer length of CAN/CAN-FD frame */
>> +unsigned int can_skb_get_dll_len(const struct sk_buff *skb);
>> +
>>  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) \

This patch was mainly to get the naming discussion started.

Talking about CAN-FD and naming: For the CAN-FD frame/dll length we need the
actual length of the data field.

The code looks like this:

> static const u8 len2XXX[] = {
> 	0, 1, 2, 3, 4, 5, 6, 7, 8,	/* 0 - 8 */
> 	12, 12, 12, 12,			/* 9 - 12 */
> 	16, 16, 16, 16,			/* 13 - 16 */
> 	20, 20, 20, 20,			/* 17 - 20 */
> 	24, 24, 24, 24,			/* 21 - 24 */
> 	32, 32, 32, 32, 32, 32, 32, 32,	/* 25 - 32 */
> 	40, 40, 40, 40, 40, 40, 40, 40,	/* 33 - 40 */
> 	48, 48, 48, 48, 48, 48, 48, 48	/* 41 - 48 */
> };
> 
> /* map the sanitized data length to an appropriate XXX length */
> u8 can_len2XXX(u8 len)
> {
> 	if (unlikely(len > ARRAY_SIZE(len2XXX)))
> 		return 64;
> 
> 	return len2XXX[len];
> }

Have you got a good name for this, too?

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH RFC net-next] can: dev: can_skb_get_dll_len(): introduce function to get data length of frame in data link layer

[Vincent MAILHOL]

On Tue 15 Dec. 2020 at 17:08, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> On 12/14/20 10:14 AM, Marc Kleine-Budde wrote:
> > From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> >
> > This patch adds the function can_skb_get_dll_len() which returns the length of
> > a CAN frame on the data link layer, including Start-of-frame, Identifier,
> > various other bits, the CRC, the End-of-frame, the Inter frame spacing and the
> > actual data.
> >
> > Co-developed-by: Arunachalam Santhanam <arunachalam.santhanam@in.bosch.com>
> > Not-Signed-off-by: Arunachalam Santhanam <arunachalam.santhanam@in.bosch.com>
> > Co-developed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > Not-Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > Co-developed-by: Marc Kleine-Budde <mkl@pengutronix.de>
> > Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> > ---
> > Hello,
> >
> > while reviewing the etas_es58X driver and hacking on BQL support for the
> > mcp251xfd driver, it turned out the a function to calculate the length of the
> > CAN frame that's send over the wire should go into the generic CAN dev code.
> >
> > Looking at the CAN and OSI layering I think the first layer where we have all
> > the bits that we see on the wire is the data link layer (DLL).
> >
> > https://www.can-cia.org/can-knowledge/can/can-data-link-layers
> >
> > This is why I named the function can_skb_get_dll_len().
> >
> > I'm planing to add a (better) calculation of the CAN-FD dll_len, depending on
> > the data and including different CRC lengths.
> >
> > As this code is copied from the etas_es58X driver, I've added the authors as
> > Co-developed-by and I'd like to add their S-o-b to this patch.
> >
> > Please review and commnt on the naming on the functions.
>
> When implementing BQL we need the CAN frame's DLL len twice:
> 1. When sending the package to the hardware
> 2. After the TX-complete IRQ
>
> We can calculate this information twice, but I thought we might put it into the
> struct can_skb_priv.
>
> https://elixir.bootlin.com/linux/latest/source/include/linux/can/skb.h#L34
>
> Thoughts?

I am not knowledgeable enough on this part to guarantee if there will
be no side effects but regardless, I like the idea.

Also, an u8 is enough to hold the value. I wonder if it would be fine
to change ifindex to, for example, u16, so that we do not lose any
memory.

I would look like that:

struct can_skb_priv {
    u16 ifindex;
    u8 frame_len;
    u8 res;
    int skbcnt;
    struct can_frame cf[];
};

And the final result in the driver would look as below:

/* Tx branch */
can_skb_prv(skb)->frame_len = can_skb_get_frame_len(skb);
netdev_sent_queue(can_skb_prv(skb)->frame_len);

/* Rx branch */
netdev_completed_queue(can_skb_prv(skb)->frame_len);

This is quite neat.

Re: [PATCH RFC net-next] can: dev: can_skb_get_dll_len(): introduce function to get data length of frame in data link layer

[Vincent MAILHOL]

On Tue 15 Dec. 2020 at 18:16, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> On 12/14/20 5:59 PM, Vincent MAILHOL wrote:
>>> while reviewing the etas_es58X driver and hacking on BQL support for the
>>> mcp251xfd driver, it turned out the a function to calculate the length of the
>>> CAN frame that's send over the wire should go into the generic CAN dev code.
>>
>> It took me a bit of time to figure out how to use the BQL. Contrary to
>> the other features, I couldn't pull inspiration from the other CAN
>> drivers. I am happy to see that you want to extend it to other
>> drivers.
>>
>> Also, on a side topic, please let me know if you have any comments on
>> the etas_es58x driver ;)
>
> I'll look more into the driver later this week.

Thanks :)

>
>>> Looking at the CAN and OSI layering I think the first layer where we have all
>>> the bits that we see on the wire is the data link layer (DLL).
>>>
>>> https://www.can-cia.org/can-knowledge/can/can-data-link-layers
>>>
>>> This is why I named the function can_skb_get_dll_len().
>>
>> I would personally prefer to call it can_skb_get_frame_len(). The ISO
>> 11898 standard defines the "frame" as:
>>
>>    Protocol Data Unit of the data link layer specifying the
>>    arrangement and meaning of bits or bit fields in the sequence of
>>    transfer across the transmission medium.
>>
>> I imagine that you want to introduce the term "dll" instead of "frame"
>> to clearly differentiate it from the struct can_frame and struct
>> can_fd_frame.
>
> ACK
>
>> But "frame" remains the most relevant term according to
>> the standard. I think that things are clear enough so that other
>> kernel hackers would understand that the len fields of struct can_frame
>> and struct can_fd_frame represent the length of the payload and that
>> can_skb_get_frame_len() is the length of the frame at the data link
>> layer.
>
> Ok.
>
>>> I'm planing to add a (better) calculation of the CAN-FD dll_len, depending on
>>> the data and including different CRC lengths.
>>
>> Great! For my driver, everything seemed OK as it is currently but if
>> this gets merged to the generic CAN dev code this part should
>> definitely be improved (the comment I added in my code was there to
>> warn whoever wanted to reuse it about the limitations).
>
> :)
>
>>> As this code is copied from the etas_es58X driver, I've added the authors as
>>> Co-developed-by and I'd like to add their S-o-b to this patch.
>>>
>>> Please review and commnt on the naming on the functions.
>>>
>>> regards,
>>> Marc
>>>
>>>  drivers/net/can/dev.c   | 103 ++++++++++++++++++++++++++++++++++++++++
>>>  include/linux/can/dev.h |   3 ++
>>>  2 files changed, 106 insertions(+)
>>>
>>> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
>>> index 81e39d7507d8..58c04785b46d 100644
>>> --- a/drivers/net/can/dev.c
>>> +++ b/drivers/net/can/dev.c
>>> @@ -58,6 +58,109 @@ u8 can_len2dlc(u8 len)
>>>  }
>>>  EXPORT_SYMBOL_GPL(can_len2dlc);
>>>
>>> +/* Size of a Classical CAN Standard Frame in Bits:
>>
>> Size of a Classical CAN Standard Frame in Bytes:
>
> The table is in bits, the overhead makros are in bytes, I'll try to clarift this.

ACK

>> The macro divides the grand total by 8. The subtotal is indeed
>> presented in bits but the macro is in bytes.
>>
>>> + *
>>> + * Start-of-frame                      1
>>> + * Identifier                          11
>>> + * Remote transmission request (RTR)   1
>>> + * Identifier extension bit (IDE)      1
>>> + * Reserved bit (r0)                   1
>>> + * Data length code (DLC)              4
>>> + * Data field                          0...64
>>> + * CRC                                 15
>>> + * CRC delimiter                       1
>>> + * ACK slot                            1
>>> + * ACK delimiter                       1
>>> + * End-of-frame (EOF)                  7
>>> + * Inter frame spacing                 3
>>> + *
>>> + * Rounded up and ignoring bitsuffing
>>> + */
>>> +#define CAN_DLL_OVERHEAD_SFF DIV_ROUND_UP(47, 8)
>>> +
>>> +/* Size of a Classical CAN Extended Frame in Bits:
>>
>> Same as above.
>>
>>> + *
>>> + * Start-of-frame                      1
>>> + * Identifier A                        11
>>> + * Substitute remote request (SRR)     1
>>> + * Identifier extension bit (IDE)      1
>>> + * Identifier B                                18
>>> + * Remote transmission request (RTR)   1
>>> + * Reserved bits (r1, r0)              2
>>> + * Data length code (DLC)              4
>>> + * Data field                          0...64
>>> + * CRC                                 15
>>> + * CRC delimiter                       1
>>> + * ACK slot                            1
>>> + * ACK delimiter                       1
>>> + * End-of-frame (EOF)                  7
>>> + * Inter frame spacing                 3
>>> + *
>>> + * Rounded up and ignoring bitsuffing
>>> + */
>>> +#define CAN_DLL_OVERHEAD_EFF DIV_ROUND_UP(67, 8)
>>> +
>>> +/* Maximum size of a Classical CAN frame
>>> + * (rounded up and ignoring bitsuffing)
>>> + */
>>> +#define CAN_DLL_LEN_MAX (CAN_DLL_OVERHEAD_EFF + CAN_MAX_DLEN)
>>> +
>>> +/* Maximum size of a CAN-FD frame (rough estimation because
>>> + * ES58X_SFF_BYTES() and ES58X_EFF_BYTES() macros are using the
>>> + * constant values for Classical CAN, not CAN-FD).
>>> + */
>>
>> /* Maximum size of a CAN-FD frame (rough estimation because
>>  * CAN_DLL_OVERHEAD_SFF and CAN_DLL_OVERHEAD_EFF macros are using the
>>  * constant values for Classical CAN, not CAN-FD).
>>  */
>>
>> Some leftovers from the etas_es58x :)
>
> ACK, in the mean time I've makros for CAN-FD.
>
>>
>>> + */
>>> +#define CANFD_DLL_LEN_MAX (CAN_DLL_OVERHEAD_EFF + CANFD_MAX_DLEN)
>>> +
>>> +/**
>>> + * can_skb_get_dll_len() - Calculate the CAN Data Link Layer length in
>>> + * bytes of a given skb.
>>> + * @skb: socket buffer of a CAN message.
>>> + *
>>> + * Do a rough calculation: bit stuffing is ignored and length in bits
>>> + * is rounded up to a length in bytes.
>>> + *
>>> + * Rationale: this function is to be used for the BQL functions
>>> + * (netdev_sent_queue() and netdev_completed_queue()) which expect a
>>> + * value in bytes. Just using skb->len is insufficient because it will
>>> + * return the constant value of CAN(FD)_MTU. Doing the bit stuffing
>>> + * calculation would be too expensive in term of computing resources
>>> + * for no noticeable gain.
>>> + *
>>> + * Remarks:
>>> + * a.) The payload of CAN FD frames with BRS flag are sent at a
>>> + * different bitrate. Currently, the can-utils canbusload tool does
>>> + * not support CAN-FD yet and so we could not run any benchmark to
>>> + * measure the impact. There might be possible improvement here.
>>
>> My initial idea was to convert the data transmitted at data bitrate
>> speed to their equivalent duration at nominal rate (example: 8 bytes
>> at 2Mbps are equivalent to 2 bytes at 500 Kbps).
>>
>> Code would look as below:
>>
>> unsigned int can_skb_get_frame_len(const struct can_priv *can,
>>                    const struct sk_buff *skb)
>> {
>> /* ... */
>>
>>     if (cfd->flags & CANFD_BRS)
>>         len = mult_frac(CANFD_FRAME_OVERHEAD, can->bittiming.bitrate,
>>                  can->data_bittiming.bitrate);
>> /* ... */
>> }
>>
>> This is just an overview, the data bitrate is also applied to some
>> other fields of the frame (e.g. the CRC). So when creating the macro
>> for the CANFD_FRAME_OVERHEAD, it should be split between the nominal
>> and data bitrates.
>
> I think we shouldn't mess with the number of bytes depending on BRS. When it
> turns out that for CAN-FD this is not exact enough, we should look into the wifi
> drivers. As wifi doesn't even have a constant channel speed.

I have to admit that I was also not a fan of that idea, but it was the
only thing I could think of.

I am curious to see how this issue could be handled in a smarter way.

>>> + * b.) The macro CAN_DLL_OVERHEAD_EFF and CAN_DLL_OVERHEAD_SFF are
>>> + * for classical CAN. Need to do the addition for CAN-FD (the value
>>> + * are expected to be close enough so the impact should be minimal or
>>> + * none).
>>> + *
>>> + * Return: length in bytes.
>>> + */
>>> +unsigned int can_skb_get_dll_len(const struct sk_buff *skb)
>>> +{
>>> +       const struct canfd_frame *cf = (const struct canfd_frame *)skb->data;
>>> +       u8 len;
>>> +
>>> +       if (can_is_canfd_skb(skb)) {
>>> +               len = cf->len + CAN_DLL_OVERHEAD_EFF;
>>
>> The CAN-FD frame can also be sent in SFF. An additional check on the
>> CAN_EFF_FLAG is needed here.
>
> ACK. CAN-FD was work-in-progress in this patch.
>
>>> +       } else {
>>> +               if (cf->can_id & CAN_RTR_FLAG)
>>> +                       len = 0;
>>> +               else
>>> +                       len = cf->len;
>>> +
>>> +               if (cf->can_id & CAN_EFF_FLAG)
>>> +                       len += CAN_DLL_OVERHEAD_EFF;
>>> +               else
>>> +                       len += CAN_DLL_OVERHEAD_SFF;
>>> +       }
>>> +
>>> +       return len;
>>> +}
>>> +EXPORT_SYMBOL_GPL(can_skb_get_dll_len);
>>> +
>>>  #ifdef CONFIG_CAN_CALC_BITTIMING
>>>  #define CAN_CALC_MAX_ERROR 50 /* in one-tenth of a percent */
>>>
>>> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
>>> index 41ff31795320..fc830f919099 100644
>>> --- a/include/linux/can/dev.h
>>> +++ b/include/linux/can/dev.h
>>> @@ -192,6 +192,9 @@ u8 can_dlc2len(u8 can_dlc);
>>>  /* map the sanitized data length to an appropriate data length code */
>>>  u8 can_len2dlc(u8 len);
>>>
>>> +/* get data link layer length of CAN/CAN-FD frame */
>>> +unsigned int can_skb_get_dll_len(const struct sk_buff *skb);
>>> +
>>>  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) \
>
> This patch was mainly to get the naming discussion started.
>
> Talking about CAN-FD and naming: For the CAN-FD frame/dll length we need the
> actual length of the data field.
>
> The code looks like this:
>
>> static const u8 len2XXX[] = {
>>       0, 1, 2, 3, 4, 5, 6, 7, 8,      /* 0 - 8 */
>>       12, 12, 12, 12,                 /* 9 - 12 */
>>       16, 16, 16, 16,                 /* 13 - 16 */
>>       20, 20, 20, 20,                 /* 17 - 20 */
>>       24, 24, 24, 24,                 /* 21 - 24 */
>>       32, 32, 32, 32, 32, 32, 32, 32, /* 25 - 32 */
>>       40, 40, 40, 40, 40, 40, 40, 40, /* 33 - 40 */
>>       48, 48, 48, 48, 48, 48, 48, 48  /* 41 - 48 */
>> };
>>
>> /* map the sanitized data length to an appropriate XXX length */
>> u8 can_len2XXX(u8 len)
>> {
>>       if (unlikely(len > ARRAY_SIZE(len2XXX)))
>>               return 64;
>>
>>       return len2XXX[len];
>> }
>
> Have you got a good name for this, too?

Before speaking of the naming, let me discuss the how to.

For CAN-FD, the overhead length (i.e. all fields of the frame minus
the data) might vary depending on:
   * Whether the frame is SFF or EFF
   * Size of the CRC: either 15, 17 or 21 depending only on the length
     of the data.

I did not fully check the ISO standard, but I hope I did not forget another
field of variable length in the above list.

The CRC field will be handled in the len2XXX[] table, the SFF/EFF will
be handled in the can_len2XXX() function.

The other fields (sof, dlc, eof, ...) are constant, and can be
squeezed in either the len2XXX[] table or the can_len2XXX() function.

The choice of how to group things is fully arbitrary.

Some candidates would be:
  * can_fd_data_len2crc_len[] which would just do return the length on
    the CRC and leave all other calculation to the can_len2XXX()
    function.
  * can_fd_data_len2ssf_len[] which will return the full frame length
    as if it was a SFF. The can_len2XXX() function would only do the
    very last step and add the length difference between the SFF and
    EFF if relevant.

All in between mix are also possible but would result in more
convoluted naming (e.g. can_fd_data_len2data_crc_len[] that would
return the data length plus the CRC length).

In all scenarios, the can_len2XXX() function would serve the same
purpose: calculate the full frame length. So I propose to name it
can_fd_data_len2frame_len().

All that said, it is hard for me to pick a favorite. Out of all above
idea, I propose to keep the second candidate:
   * can_fd_data_len2ssf_len[]

But this is with no big conviction.

Re: [PATCH RFC net-next] can: dev: can_skb_get_dll_len(): introduce function to get data length of frame in data link layer

[Oliver Hartkopp]

On 15.12.20 12:37, Vincent MAILHOL wrote:
> On Tue 15 Dec. 2020 at 17:08, Marc Kleine-Budde <mkl@pengutronix.de> wrote:

>> We can calculate this information twice, but I thought we might put it into the
>> struct can_skb_priv.
>>
>> https://elixir.bootlin.com/linux/latest/source/include/linux/can/skb.h#L34
>>
>> Thoughts?
> 
> I am not knowledgeable enough on this part to guarantee if there will
> be no side effects but regardless, I like the idea.
> 
> Also, an u8 is enough to hold the value. I wonder if it would be fine
> to change ifindex to, for example, u16, so that we do not lose any
> memory.

We should not make can_skb_priv.ifindex another type than 
net_device.ifindex:

https://elixir.bootlin.com/linux/latest/source/include/linux/netdevice.h#L1899

> 
> I would look like that:
> 
> struct can_skb_priv {
>      u16 ifindex;
>      u8 frame_len;
>      u8 res;
>      int skbcnt;
>      struct can_frame cf[];
> };
> 

Regards,
Oliver

Re: [PATCH RFC net-next] can: dev: can_skb_get_dll_len(): introduce function to get data length of frame in data link layer

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 4785 bytes --]

On 12/15/20 1:40 PM, Vincent MAILHOL wrote:
[...]
>>> This is just an overview, the data bitrate is also applied to some
>>> other fields of the frame (e.g. the CRC). So when creating the macro
>>> for the CANFD_FRAME_OVERHEAD, it should be split between the nominal
>>> and data bitrates.
>>
>> I think we shouldn't mess with the number of bytes depending on BRS. When it
>> turns out that for CAN-FD this is not exact enough, we should look into the wifi
>> drivers. As wifi doesn't even have a constant channel speed.
> 
> I have to admit that I was also not a fan of that idea, but it was the
> only thing I could think of.
> 
> I am curious to see how this issue could be handled in a smarter way.

Let's wait until we have a "problem" with that :)

[...]

>> Talking about CAN-FD and naming: For the CAN-FD frame/dll length we need the
>> actual length of the data field.
>>
>> The code looks like this:
>>
>>> static const u8 len2XXX[] = {
>>>       0, 1, 2, 3, 4, 5, 6, 7, 8,      /* 0 - 8 */
>>>       12, 12, 12, 12,                 /* 9 - 12 */
>>>       16, 16, 16, 16,                 /* 13 - 16 */
>>>       20, 20, 20, 20,                 /* 17 - 20 */
>>>       24, 24, 24, 24,                 /* 21 - 24 */
>>>       32, 32, 32, 32, 32, 32, 32, 32, /* 25 - 32 */
>>>       40, 40, 40, 40, 40, 40, 40, 40, /* 33 - 40 */
>>>       48, 48, 48, 48, 48, 48, 48, 48  /* 41 - 48 */
>>> };
>>>
>>> /* map the sanitized data length to an appropriate XXX length */
>>> u8 can_len2XXX(u8 len)
>>> {
>>>       if (unlikely(len > ARRAY_SIZE(len2XXX)))
>>>               return 64;
>>>
>>>       return len2XXX[len];
>>> }
>>
>> Have you got a good name for this, too?

I've send a v2...(hmmm, but missed the v2)

> Before speaking of the naming, let me discuss the how to.
> 
> For CAN-FD, the overhead length (i.e. all fields of the frame minus
> the data) might vary depending on:
>    * Whether the frame is SFF or EFF

ACK, included in the v2

>    * Size of the CRC: either 15, 17 or 21 depending only on the length
>      of the data.

I haven't looked into the actual standard, but from what I have found, the CRC
for CAN-FD frames is 17 (for data length 0...16 bytes) or 21 (for data length >
16 byes). Also there are 4 or 5 fixed stuff bits (corresponding to 17 or 21 bits
of CRC) in the CRC.

I got the impression that even 0...8 byte CAN-FD frames use a CRC of 17.

> I did not fully check the ISO standard, but I hope I did not forget another
> field of variable length in the above list.
> 
> The CRC field will be handled in the len2XXX[] table, the SFF/EFF will
> be handled in the can_len2XXX() function.

For now I've selected the 21 bit CRC, for CAN-FD:

> #define CANFD_DLL_OVERHEAD_SFF DIV_ROUND_UP(61, 8)
== 8 bytes

for CRC17 it would be 5 bits less => 56 bit => 7 bytes


> #define CANFD_DLL_OVERHEAD_EFF DIV_ROUND_UP(80, 8)
== 10 bytes

for CRC17 it would be 5 bits less => 75 bits => 8 bytes

I think that 1 bit is not worth the trouble.

> The other fields (sof, dlc, eof, ...) are constant, and can be
> squeezed in either the len2XXX[] table or the can_len2XXX() function.

I want to use the can_fd_data_len2frame_data_len() in the mcp251xfd driver, so
I'd like to keep that.

> The choice of how to group things is fully arbitrary.
> 
> Some candidates would be:
>   * can_fd_data_len2crc_len[] which would just do return the length on
>     the CRC and leave all other calculation to the can_len2XXX()
>     function.
>   * can_fd_data_len2ssf_len[] which will return the full frame length
>     as if it was a SFF. The can_len2XXX() function would only do the
>     very last step and add the length difference between the SFF and
>     EFF if relevant.
> 
> All in between mix are also possible but would result in more
> convoluted naming (e.g. can_fd_data_len2data_crc_len[] that would
> return the data length plus the CRC length).
> 
> In all scenarios, the can_len2XXX() function would serve the same
> purpose: calculate the full frame length. So I propose to name it
> can_fd_data_len2frame_len().

What about: can_fd_data_len2frame_data_len()

As it only the frame's data length, not the complete frame len. (I don't mind
long function names :))

> All that said, it is hard for me to pick a favorite. Out of all above
> idea, I propose to keep the second candidate:
>    * can_fd_data_len2ssf_len[]
> But this is with no big conviction.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH RFC net-next] can: dev: can_skb_get_dll_len(): introduce function to get data length of frame in data link layer

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1889 bytes --]

On 12/15/20 12:37 PM, Vincent MAILHOL wrote:
>> When implementing BQL we need the CAN frame's DLL len twice:
>> 1. When sending the package to the hardware
>> 2. After the TX-complete IRQ
>>
>> We can calculate this information twice, but I thought we might put it into the
>> struct can_skb_priv.
>>
>> https://elixir.bootlin.com/linux/latest/source/include/linux/can/skb.h#L34
>>
>> Thoughts?
> 
> I am not knowledgeable enough on this part to guarantee if there will
> be no side effects but regardless, I like the idea.
> 
> Also, an u8 is enough to hold the value.

ACK

> I wonder if it would be fine
> to change ifindex to, for example, u16, so that we do not lose any
> memory.

ifindex is an int:

https://elixir.bootlin.com/linux/v5.10.1/source/include/linux/netdevice.h#L1899

> I would look like that:
> 
> struct can_skb_priv {
>     u16 ifindex;
>     u8 frame_len;
>     u8 res;

No need for any reserved bytes, as this is not packed and not a stable API. The
compiler will take care of the packing. If you want to save space in structures
take a look at pahole:

https://lwn.net/Articles/206805/

>     int skbcnt;
>     struct can_frame cf[];
> };
> 
> And the final result in the driver would look as below:
> 
> /* Tx branch */
> can_skb_prv(skb)->frame_len = can_skb_get_frame_len(skb);
> netdev_sent_queue(can_skb_prv(skb)->frame_len);
> 
> /* Rx branch */
> netdev_completed_queue(can_skb_prv(skb)->frame_len);

I think, I'll add another variety of the get_echo_skb() function, that returns
the frame_len.

> This is quite neat.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH RFC net-next] can: dev: can_skb_get_dll_len(): introduce function to get data length of frame in data link layer

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 724 bytes --]

On 12/15/20 2:30 PM, Marc Kleine-Budde wrote:
> For now I've selected the 21 bit CRC, for CAN-FD:
> 
>> #define CANFD_DLL_OVERHEAD_SFF DIV_ROUND_UP(61, 8)
> == 8 bytes
> 
> for CRC17 it would be 5 bits less => 56 bit => 7 bytes
> 
> 
>> #define CANFD_DLL_OVERHEAD_EFF DIV_ROUND_UP(80, 8)
> == 10 bytes
> 
> for CRC17 it would be 5 bits less => 75 bits => 8 bytes
> 
> I think that 1 bit is not worth the trouble.

s/bit/byte/

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]