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.
> > {
> > 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()
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.
> > {
> > 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;
> > }
Yours sincerely,
Vincent Mailhol
