Re: [RFC PATCH 2/5] can: canxl: introduce ETH_P_CANXL ethernet protocol handling

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 3889 bytes --]

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@cafe.net)	*/
> > > > >    #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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]