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