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

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 12.07.22 09:17, Marc Kleine-Budde wrote:
> 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@hartkopp.net>
>>>> ---
>>>>    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@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?

>>>> 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".

I just thought about can_blablabla() as common namespace for our functions.

But canxl_whatever() is also safe ;-)

> What about: canxl_valid_data_size()

Fine for me.

Thanks,
Oliver