Re: [RFC PATCH v5 2/2] can: reject CAN FD content when disabled on CAN XL interfaces

[Vincent Mailhol <mailhol@kernel.org>]

+cc: Stéphane

On 15/09/2025 at 19:55, Vincent Mailhol wrote:
> On 11/09/2025 at 05:12, Oliver Hartkopp wrote:
>> On 10.09.25 18:19, Vincent Mailhol wrote:
>>> On 10/09/2025 at 17:48, Oliver Hartkopp wrote:
>>>> On 10.09.25 09:40, Vincent Mailhol wrote:
>>>>> On 10/09/2025 at 16:27, Oliver Hartkopp wrote:
>>>>
>>>>>>>       /* CAN XL is allowed on virtual interfaces if it fits the MTU */
>>>>>>>          if (!priv)
>>>>>>>              return dev->mtu == CANXL_MTU;
>>>>>>
>>>>>>           return can_is_canxl_dev_mtu(mtu);
>>>>>>
>>>>>> The MTU of CAN XL interfaces might vary.
>>>>>
>>>>> Maybe this is something that we discussed before, I do not remember, but how is
>>>>> it that the MTU can vary?
>>>>>
>>>>> MTU is the *Maximum* Transmission Unit. I understand that the size of a CAN XL
>>>>> frame is variable, but the MTU should be constant, right? Why can it vary?
>>>>
>>>> Depending on the realtime requirements the length of the CAN frames (and
>>>> therefore the time the bus is blocked) the MTU can be reduced. This is (like the
>>>> bitrate settings) a network architects decision which is enforced by setting the
>>>> MTU accordingly.
>>>
>>> Is this an extension we offer in Socket CAN?
>>
>> Yes.
>>
>>> The standard says nothing about
>>> having the MTU configurable.
>>>
>>> For CAN FD, we forcefully set the MTU in netlink.c
>>>
>>> https://elixir.bootlin.com/linux/v6.16/source/drivers/net/can/dev/netlink.c#L228
>>
>> Oh. I did not realize before that we can either modify the MTU with setting fd
>> on/off and via setting the MTU in can_change_mtu()
>>
>> https://elixir.bootlin.com/linux/v6.16/source/drivers/net/can/dev/dev.c#L313
>>
>> ?!?
>>
>> The two APIs problem for changing the MTU?!?
>>
>> I expected the default MTU for CAN FD capable interfaces to be CANFD_MTU which
>> is obviously not the case.
>>
>>> I will have to think of what are the implication for CAN XL.
>>
>> I would define a default CANXL MTU (CANXL_MTU 2060) which might be changed with
>> can_change_mtu().
>>
>> And when ever we switch xl on this value is selected as device MTU.
>>
>> Or the user can change the MTU as he needs it.
>> And when xl on is selected and the MTU is a can_is_canxl_dev_mtu() this value is
>> used. When can_is_canxl_dev_mtu() is not true we take CANXL_MTU.
>>
>> Something like this.
> 
> Yes. I was thinking of something similar. This is what I added locally at the
> moment:
> 
> 	if ((priv->ctrlmode & CAN_CTRLMODE_XL) &&
> 	    !can_is_canxl_dev_mtu(dev->mtu)) {
>   		/* Set CAN XL MTU to its max unless if already set by user */
> 		dev->mtu = CANXL_MAX_MTU;
> 	}
> 
> But I am still testing it.

I am looking at the code of can_change_mtu() but I can not understand the intent.

Back then, commit bc05a8944a34 ("can: allow to change the device mtu for CAN FD
capable devices") stated that:

  The configuration can be done either with the 'fd { on | off }' option in the
  'ip' tool from iproute2 or by setting the CAN netdevice MTU to CAN_MTU (16) or
  to CANFD_MTU (72).

  Link: https://git.kernel.org/torvalds/c/bc05a8944a34

But if I do something like:

  ip link set can0 mtu 72 type can bitrate 500000

the command is accepted and I am then left with a device which is in an
incoherent status (FD on, but no databittiming ?!)

I tested this on a device and it is just throwing me errors.

The same goes on when setting the mtu back to 16:

  ip link set can0 type can bitrate 500000 fd on dbitrate 5000000
  ip link set can0 mtu 16

and now I have a device in Classical CAN mode but iproute2 is still reporting me
the databittiming values (although this time, the device does not send me errors).

So, whatever I try, can_change_mtu() put the device in some incoherent state
with some more or less serious impact (either device errors or bad reporting).
And the only method to remediate is to use the 'fd { on | off }' option.

Am I missing something? If the feature is just broken, then I would like to
rewrite it but this time using the net_dev infrastructure by populating
net_device->min_mtu and net_device->max_mtu. IMHO, this should give us a way
more robust interface.


Yours sincerely,
Vincent Mailhol