On 12.07.2022 10:02:28, Oliver Hartkopp wrote:
> > > > > --- 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).
> >
>
> Ok?!?
>
> I remembered some discussion about PF_FLEXRAY which bounced some years ago.
>
> But I think it should be fine if we get this small patchset discussed and
> upstreamed to net-next in this round, right?
ACK
> > > > > 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".
...but that's the struct canxl_frame and alloc_canxl_skb().
> I just thought about can_blablabla() as common namespace for our functions.
ACK it is. There are:
include/linux/can/length.h:160:u8 can_fd_dlc2len(u8 dlc);
include/linux/can/length.h:163:u8 can_fd_len2dlc(u8 len);
> But canxl_whatever() is also safe ;-)
>
> > What about: canxl_valid_data_size()
Sorry for the noise,
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
