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

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 13.07.22 09:15, Vincent Mailhol wrote:
> On Wed. 13 Jul. 2022 at 16:02, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>> On 13.07.2022 08:58:26, Vincent Mailhol wrote:
>>> On Wed. 13 Jul. 2022 at 05:20, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>>>> 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()
>>>
>>> ACK.
> 
> So, according to Marc's comments (c.f. below):
> can_skb_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()
>>>
>>> I anticipate that most of the time, developers do not want to get the
>>> RTR length but the actual length (e.g. to memcpy data[] or to increase
>>> statistics). People will get confused between can_get_data_len() and
>>> can_get_len() due to the similar names. So I would suggest a more
>>> explicit name to point out that this one is probably not the one you
>>> want to use.
>>> Candidates name I can think of:
>>>   * can_get_raw_len()
>>>   * can_get_advertised_len()
>>>   * can_get_rtr_len()

But these three functions still bconfuse me.

IMO we need two values:

- the data byte length (RTR aware)
- the (raw) length value

My suggestion for a naming would be:

- can_skb_get_data_len()
- can_skb_get_len_value()

This also fits to the existing
can_skb_get_frame_len().

> So according to Marc's comments (c.f. below), candidates become:
>    * can_skb_get_raw_len()
>    * can_skb_get_advertised_len()
>    * can_skb_get_rtr_len()

Right.

>>> The only time you want to access the raw len (with real RTR value) is
>>> in the TX path when you fill your device's structures. But here the
>>> can_get_cc_dlc() is a better helper function which is already RTR
>>> aware.
>>
>> There also is
>>
>> | unsigned int can_skb_get_frame_len(const struct sk_buff *skb)
> 
> I totally forgot about that one, despite the fact that I co-developed
> it with you.
> 
>> It gives the length of the frame on the wire in bytes. We should use a
>> common naming scheme. Let's always use can_skb_ as a prefix or drop the
>> skb_ from this function.
> 
> You are right. The idea was to use the can_skb_ prefix because the
> argument of the function was a struct skb_buff. Same applies here.

Yes, that's fine!

Best regards,
Oliver