On Thu. 14 Jul. 2022 at 05:27, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote:
> On 13.07.22 09:15, Vincent Mailhol wrote:
> > On Wed. 13 Jul. 2022 at 16:02, Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> wrote:
> >> On 13.07.2022 08:58:26, Vincent Mailhol wrote:
> >>> On Wed. 13 Jul. 2022 at 05:20, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote:
> >>>> On 12.07.22 03:23, Vincent Mailhol wrote:
> >>>>> On Tue. 12 Jul. 2022 at 03:44, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> >>>>>> ---
> >>>>>> 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 confuse 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()
Hmm, you did not fully convince me but at the same time, your solution
is okayish to me.
My main point was to correctly manage the length according to the RTR
flag and this concern is now fully addressed. I am fine to go on with
your naming suggestions.
Yours sincerely,
Vincent Mailhol
