On 10/19/20 10:35 PM, Oliver Hartkopp wrote: > > > On 19.10.20 21:05, Marc Kleine-Budde wrote: >> From: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> >> >> In classical CAN, the length of the data (i.e. CAN payload) is not >> always equal to the DLC! If the frame is a Remote Transmission Request >> (RTR), data length is always zero regardless of DLC value and else, if >> the DLC is greater than 8, the length is 8. Contrary to common belief, >> ISO 11898-1 Chapter 8.4.2.3 (DLC field) do allow DLCs greater than 8 >> for Classical Frames and specifies that those DLCs shall indicate that >> the data field is 8 bytes long. >> >> Above facts are widely unknown and so many developpers uses the "len" >> field of "struct canfd_frame" to get the length of classical CAN >> frames: this is incorrect! >> >> This patch introduces function get_can_len() which can be used in >> remediation. The function takes the SKB as an input in order to be >> able to determine if the frame is classical or FD. >> >> Signed-off-by: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> >> Link: https://lore.kernel.org/r/20201002154219.4887-4-mailhol.vincent@xxxxxxxxxx >> [mkl: renamed get_can_len() -> can_get_len()] >> Signed-off-by: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> >> --- >> include/linux/can/dev.h | 23 +++++++++++++++++++++++ >> 1 file changed, 23 insertions(+) >> >> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h >> index 41ff31795320..2bb132fc6d88 100644 >> --- a/include/linux/can/dev.h >> +++ b/include/linux/can/dev.h >> @@ -192,6 +192,29 @@ u8 can_dlc2len(u8 can_dlc); >> /* map the sanitized data length to an appropriate data length code */ >> u8 can_len2dlc(u8 len); >> >> +/* >> + * can_get_len(skb) - get the length of the CAN payload. >> + * >> + * In classical CAN, the length of the data (i.e. CAN payload) is not >> + * always equal to the DLC! If the frame is a Remote Transmission >> + * Request (RTR), data length is always zero regardless of DLC value >> + * and else, if the DLC is greater than 8, the length is 8. Contrary >> + * to common belief, ISO 11898-1 Chapter 8.4.2.3 (DLC field) do allow >> + * DLCs greater than 8 for Classical Frames and specifies that those >> + * DLCs shall indicate that the data field is 8 bytes long. >> + */ >> +static inline u8 can_get_len(const struct sk_buff *skb) >> +{ >> + const struct canfd_frame *cf = (const struct canfd_frame *)skb->data; >> + >> + if (can_is_canfd_skb(skb)) >> + return min_t(u8, cf->len, CANFD_MAX_DLEN); >> + else if (cf->can_id & CAN_RTR_FLAG) >> + return 0; >> + else >> + return min_t(u8, cf->len, CAN_MAX_DLEN); >> +} > > The main idea behind this patch and patch 05/16 is to provide a correct > statistic in the tx bytes, right? > > A simple test for the CAN_RTR_FLAG will do the job as all the length > sanitizing is already done in the tx path by can_dropped_invalid_skb() > in all known drivers right *before* the skb is stored in the echo skb array. > > IMO there's no need for a separate helper function. Maybe a macro which > should have something with 'payload' in its name - to determine the tx > byte statistics based on CAN_RTR_FLAG ... Good point! 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
