On 12.07.2022 09:12:49, Oliver Hartkopp wrote:
>
>
> On 11.07.22 21:41, Marc Kleine-Budde wrote:
> > On 11.07.2022 20:34:23, Oliver Hartkopp 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;
> > > + }
> > > +}
> > > +
> > > #endif /* !_CAN_SKB_H */
> > > diff --git a/include/uapi/linux/if_ether.h b/include/uapi/linux/if_ether.h
> > > index d370165bc621..69e0457eb200 100644
> > > --- a/include/uapi/linux/if_ether.h
> > > +++ b/include/uapi/linux/if_ether.h
> > > @@ -136,10 +136,11 @@
> > > #define ETH_P_WAN_PPP 0x0007 /* Dummy type for WAN PPP frames*/
> > > #define ETH_P_PPP_MP 0x0008 /* Dummy type for PPP MP frames */
> > > #define ETH_P_LOCALTALK 0x0009 /* Localtalk pseudo type */
> > > #define ETH_P_CAN 0x000C /* CAN: Controller Area Network */
> > > #define ETH_P_CANFD 0x000D /* CANFD: CAN flexible data rate*/
> > > +#define ETH_P_CANXL 0x000E /* CANXL: eXtended frame Length */
> > > #define ETH_P_PPPTALK 0x0010 /* Dummy type for Atalk over PPP*/
> > > #define ETH_P_TR_802_2 0x0011 /* 802.2 frames */
> > > #define ETH_P_MOBITEX 0x0015 /* Mobitex (kaz@xxxxxxxx) */
> > > #define ETH_P_CONTROL 0x0016 /* Card specific control frames */
> > > #define ETH_P_IRDA 0x0017 /* Linux-IrDA */
> >
> > This file doesn't change that often I suppose. Or does it make sense to
> > send this change mainline as soon as possible?
>
> AFAIK you only can reserve these values when you have a reference that uses
> it. I don't seen any additional pressure here.
There have been added other types that are not used in the kernel (yet).
> > > diff --git a/net/can/af_can.c b/net/can/af_can.c
> > > index 1fb49d51b25d..2c9f48aa5f1f 100644
> > > --- a/net/can/af_can.c
> > > +++ b/net/can/af_can.c
> > > @@ -197,31 +197,32 @@ static int can_create(struct net *net, struct socket *sock, int protocol,
> > > * -EINVAL when the skb->data does not contain a valid CAN frame
> > > */
> > > int can_send(struct sk_buff *skb, int loop)
> > > {
> > > struct sk_buff *newskb = NULL;
> > > - struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> > > + unsigned int len = can_get_data_len(skb);
> > > struct can_pkg_stats *pkg_stats = dev_net(skb->dev)->can.pkg_stats;
> > > int err = -EINVAL;
> > > if (skb->len == CAN_MTU) {
> > > skb->protocol = htons(ETH_P_CAN);
> > > - if (unlikely(cfd->len > CAN_MAX_DLEN))
> > > + if (unlikely(len > CAN_MAX_DLEN))
> > > goto inval_skb;
> > > } else if (skb->len == CANFD_MTU) {
> > > skb->protocol = htons(ETH_P_CANFD);
> > > - if (unlikely(cfd->len > CANFD_MAX_DLEN))
> > > + if (unlikely(len > CANFD_MAX_DLEN))
> > > + goto inval_skb;
> > > + } else if (skb->len == CANXL_MTU) {
> > > + skb->protocol = htons(ETH_P_CANXL);
> > > + if (unlikely(len > CANXL_MAX_DLEN || len == 0))
> >
> > Do we need a helper for the > CANXL_MAX_DLEN || == 0 check?
>
> Some can_xl_is_valid_data_size(unsigned int len) ?
In other location you're using "canxl" not "can_xl".
What about: canxl_valid_data_size()
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
