Re: [RFC PATCH 2/5] can: canxl: introduce ETH_P_CANXL ethernet protocol handling

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 12.07.22 03:23, Vincent Mailhol wrote:
> On Tue. 12 Jul. 2022 at 03:44, Oliver Hartkopp <socketcan@hartkopp.net> 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@hartkopp.net>
>> ---
>>   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()

> {
>         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()

> {
>         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;
> 
>         return cfd->len;
> }
> 

Best regards,
Oliver