Re: [PATCH v4 4/5] can: bcm: add support for CAN FD frames

[Marc Kleine-Budde <mkl@pengutronix.de>]

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

On 06/04/2016 09:26 PM, Oliver Hartkopp wrote:
> The programming API of the CAN_BCM depends on struct can_frame which is
> given as array directly behind the bcm_msg_head structure. To follow this
> schema for the CAN FD frames a new flag 'CAN_FD_FRAME' in the bcm_msg_head
> flags indicates that the concatenated CAN frame structures behind the
> bcm_msg_head are defined as struct canfd_frame.
> 
> This patch adds the support to handle CAN and CAN FD frames on a per BCM-op
> base. Main changes:
> 
> - generally use struct canfd_frames instead if struct can_frames
> - use canfd_frame.flags instead of can_frame.can_dlc for private BCM flags
> - make all CAN frame sizes depending on the new CAN_FD_FRAME flags
> - separate between CAN and CAN FD when sending/receiving frames
> 
> Due to the dependence of the CAN_FD_FRAME flag the former binary interface
> for classic CAN frames remains stable.
> 
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> ---
>  include/uapi/linux/can/bcm.h |   1 +
>  net/can/bcm.c                | 223 ++++++++++++++++++++++++++-----------------
>  2 files changed, 134 insertions(+), 90 deletions(-)
> 
> diff --git a/include/uapi/linux/can/bcm.h b/include/uapi/linux/can/bcm.h
> index 7a291dc..cefb304 100644
> --- a/include/uapi/linux/can/bcm.h
> +++ b/include/uapi/linux/can/bcm.h
> @@ -99,5 +99,6 @@ enum {
>  #define RX_ANNOUNCE_RESUME  0x0100
>  #define TX_RESET_MULTI_IDX  0x0200
>  #define RX_RTR_FRAME        0x0400
> +#define CAN_FD_FRAME        0x0800
>  
>  #endif /* !_UAPI_CAN_BCM_H */
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index f3bf387..31571b6 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -1,7 +1,7 @@
>  /*
>   * bcm.c - Broadcast Manager to filter/send (cyclic) CAN content
>   *
> - * Copyright (c) 2002-2007 Volkswagen Group Electronic Research
> + * Copyright (c) 2002-2016 Volkswagen Group Electronic Research
>   * All rights reserved.
>   *
>   * Redistribution and use in source and binary forms, with or without
> @@ -67,27 +67,27 @@
>   */
>  #define MAX_NFRAMES 256
>  
> -/* use of last_frames[index].can_dlc */
> +/* use of last_frames[index].flags */
>  #define RX_RECV    0x40 /* received data for this element */
>  #define RX_THR     0x80 /* element not been sent due to throttle feature */
> -#define BCM_CAN_DLC_MASK 0x0F /* clean private flags in can_dlc by masking */
> +#define BCM_CAN_FLAGS_MASK 0x3F /* to clean private flags after usage */
>  
>  /* get best masking value for can_rx_register() for a given single can_id */
>  #define REGMASK(id) ((id & CAN_EFF_FLAG) ? \
>  		     (CAN_EFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG) : \
>  		     (CAN_SFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG))
>  
> -#define CAN_BCM_VERSION CAN_VERSION
> +#define CAN_BCM_VERSION "20160531"
>  
>  MODULE_DESCRIPTION("PF_CAN broadcast manager protocol");
>  MODULE_LICENSE("Dual BSD/GPL");
>  MODULE_AUTHOR("Oliver Hartkopp <oliver.hartkopp@volkswagen.de>");
>  MODULE_ALIAS("can-proto-2");
>  
> -/* easy access to CAN frame payload */
> -static inline u64 GET_U64(const struct can_frame *cp)
> +/* easy access to the first 64 bit of can(fd)_frame payload */
> +static inline u64 GET_U64(const struct canfd_frame *cp, int offset)
>  {
> -	return *(u64 *)cp->data;
> +	return *(u64 *)(cp->data + offset);

Can you ensure that offset is always a multiple of 8? Unaligned pointer
access will be quite slow on some platforms. Can you please make this
function lower-case (as it's a function not a macro).

>  }
>  

Marc

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


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