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.
>
> > > {
> > > 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.
There also is
| unsigned int can_skb_get_frame_len(const struct sk_buff *skb)
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.
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Attachment:
signature.asc
Description: PGP signature
