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:
signature.asc
Description: OpenPGP digital signature
