Re: [PATCH v2] can: raw: fix CAN FD frame transmissions over CAN XL devices

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 1/31/23 12:06, Marc Kleine-Budde wrote:
> On 31.01.2023 10:18:24, Oliver Hartkopp wrote:
>> A CAN XL device is always capable to process CAN FD frames. The former
>> check when sending CAN FD frames relied on the existence of a CAN FD
>> device and did not check for a CAN XL device that would be correct too.
>>
>> With this patch the CAN FD feature is enabled automatically when CAN XL
>> is switched on - and CAN FD cannot be switch off while CAN XL is enabled.
>>
>> This precondition also leads to a clean up and reduction of checks in the
>> hot path in raw_rcv() and raw_sendmsg().
>>
>> Fixes: 626332696d75 ("can: raw: add CAN XL support")
>> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
>> ---
>>
>> V2: fixed typo: devive -> device
>>
>> net/can/raw.c | 45 ++++++++++++++++++++++++++++++---------------
>>   1 file changed, 30 insertions(+), 15 deletions(-)
>>
>> diff --git a/net/can/raw.c b/net/can/raw.c
>> index 81071cdb0301..c505d315a7a9 100644
>> --- a/net/can/raw.c
>> +++ b/net/can/raw.c
>> @@ -130,11 +130,11 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
>>   	/* check the received tx sock reference */
>>   	if (!ro->recv_own_msgs && oskb->sk == sk)
>>   		return;
>>   
>>   	/* make sure to not pass oversized frames to the socket */
>> -	if ((can_is_canfd_skb(oskb) && !ro->fd_frames && !ro->xl_frames) ||
>> +	if ((can_is_canfd_skb(oskb) && !ro->fd_frames) ||
>>   	    (can_is_canxl_skb(oskb) && !ro->xl_frames))
>>   		return;
>>   
>>   	/* eliminate multiple filter matches for the same skb */
>>   	if (this_cpu_ptr(ro->uniq)->skb == oskb &&
>> @@ -668,19 +668,27 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
>>   			return -EINVAL;
>>   
>>   		if (copy_from_sockptr(&ro->fd_frames, optval, optlen))
>>   			return -EFAULT;
>>   
>> +		/* Enabling CAN XL includes CAN FD */
>> +		if (ro->xl_frames && !ro->fd_frames) {
>> +			ro->fd_frames = ro->xl_frames;
>> +			return -EINVAL;
>> +		}
> 
> This looks a bit weird. Maybe copy to a local variable first, then
> check, then assign ->fd_frames or exit with error (instead of rolling
> back)?
>

Changed in V4.

Thanks & BR,
Oliver