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

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 16.09.25 15:17, Vincent Mailhol wrote:
> On 16/09/2025 at 18:14, Oliver Hartkopp wrote:
>> On 15.09.25 20:54, Vincent Mailhol wrote:
>>> On 16/09/2025 at 03:08, Oliver Hartkopp wrote:
>>
>>>> I think the interface to set the MTU lacks a clear separation of how to set the
>>>> MTU for real (hardware) CAN interfaces and virtual CAN interfaces.
>>>
>>> Ack.
>>>
>>>> 1. IMO we should be able to set the MTUs on virtual and real interfaces when the
>>>> interface is down (as those MTUs have no effect at this time).
>>>
>>> Mostly agreed. It should not be possible to switch between the Classical CAN,
>>> CAN FD or CAN XL MTUs. But I do not yet see an issue to change the MTU to
>>> something in between CANXL_MIN_MTU and CANXL_MAX_MTU while a CAN XL node is
>>> running.
>>
>> No, not while it is running (== up).
>> My point was that you can set the MTU as long as the interface is not "up".
>> Together with the default initial values (see below) this makes perfect sense to
>> me.
>>
>>> I want to first study the other interfaces (e.g. ethernet) and the core net
>>> infrastructure in order to make my mind. For the moment, I am just undecided.
>>
>> I'm not sure if ethernet is a good example for our use-case with different CAN
>> protocols types (CC/FD/XL) which is more than having ethernet frames of
>> different length.
> 
> I agree that we should not switch between MTUs of different protocols while the
> interface is up.
> 
> My point is just to allow switching the MTU to anything between CANXL_MIN_MTU
> and CANXL_MAX_MTU on a CAN XL interface which is up. So the use case is also
> just one protocol type (CAN XL). And that, I think, is comparable to Ethernet
> frames.

Ok. I did not know that it is possible to change the ethernet MTU on the 
fly when the interface is up. But this would indeed match the CAN XL 
MIN/MAX MTU pattern then. I'm fine with it.

>>>> 2. When a virtual interface is set to "up" the MTU is used and fixed.
>>>
>>> Same as above, mostly agreed aside from the CAN XL on which I do not yet have my
>>> final opinion.
>>>
>>>> 3. When a real interface is set to up the mtu is set to ...
>>>>     a. mtu = CAN_MTU when fd off and xl off
>>>>     b. mtu = CANFD_MTU when fd on and xl off
>>>>     c. mtu = the configured CAN XL MTU (*) when xl on
>>>>
>>>> (*) when the configured mtu is not in the range of CANXL_MIN_MTU and
>>>> CANXL_MAX_MTU the mtu is set to CANXL_MAX_MTU.
>>>>
>>>> By default the initial MTU of virtual CAN interfaces should be set to CANXL_MTU.
>>>>
>>>> By default the initial MTU of real CAN interfaces should be set to the maximum
>>>> value which the real CAN interface is capable too:
>>>>    a. CAN_CTRLMODE_XL supported -> CANXL_MTU.
>>>>    b. CAN_CTRLMODE_FD supported -> CANFD_MTU.
>>>>    c. default CAN_MTU
>>>
>>> I was thinking of the opposite:
>>>
>>>     a. if the device is CAN FD static it is CANFD_MTU
>>>     b. if the device is CAN XL static it is CANXL_MTU
>>>     c. otherwise, it is the CAN_MTU by default
>>>
>>> which, if you remove point b., happens to be the current logic. I do not see a
>>> need to change this.
>>
>> I like that approach of having the supported MTUs as default and later reduce
>> the MTU based on fd=off and xl=off, because it would be similar with the virtual
>> CAN configuration then.
> 
> vcan's MTU is set to CANFD_MTU. You still need to manually increase it to enable
> xl. But fair enough, we could modify the vcan default to CANXL_MTU so that it
> works as you just described.

Yes. I changed to initial value from CC to FD here:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=97edec3a11cf6f73f2e45c3035b5ff8e4c3543dd

Therefore it makes sense to do this for CAN XL again.

> But I do not see the parallel. On vcan, FD is turned on by default. On real
> interfaces, CAN_CTRLMODE_FD is turned off by default. Why should we be trying to
> spot similarities on something which is different in the first place?

We should generally separate the configuration of interfaces with 
netlink ctrlmode support (real CAN interfaces) from virtual CAN 
interfaces that don't know about  CAN_CTRLMODE_FD. See my personal 
summary below.

> What I dislike the most is that there is an existing logic for the real
> interfaces. The MTU is set by default to CAN_MTU here:
> 
>    https://elixir.bootlin.com/linux/v6.16/source/drivers/net/can/dev/dev.c#L242
> 
> and is modified only if the device is CAN FD static here:
> 
>    https://elixir.bootlin.com/linux/v6.16/source/include/linux/can/dev.h#L144
> 
> Here, we are not adding a new feature, but extending an existing one. So you
> would need a strong argument to justify a change going in the opposite direction
> of the current logic.

Ok. You convinced me. Please follow the current approach.
Every CAN interface starts with CAN_MTU unless they are static FD/XL 
interfaces.

> I see that can_change_mtu() is currently broken, so that's a strong enough
> reason to change it. But the default MTU logic looks coherent to me so I do not
> get why that should change.
> 
>> I assume the initial MTU isn't looked at by the users anyway.
> 
> The user may look at it. It is better not to make assumptions here.
> 
> The design must be such that the netlink always reports a coherent configuration
> at any point in time.
> 
>>> If we set CANXL_MTU by default on XL capable devices, it means that at a moment
>>> in time, we have a device with the CAN_CTRLMODE_XL off but with a CAN XL MTU.
>>
>> ???
>>
>> Maybe I was not clear enough:
>>
>> You intitialize the MTU to CANXL_MTU when CAN_CTRLMODE_XL is a "supported mode".
>> The interface is not "up" at this time and therefore the MTU is not on active
>> service.
>>
>> Then you configure the interface with bitrates and xl/fd on/off.
>>
>> And then you set the interface to "up" and in this process the MTU is set as a
>> valid and activated value with a MTU based on the xl/fd on/off settings. This
>> was my idea.
> 
> Then, why do we need to set an MTU in the first place? The user must call the
> netlink interface at least once.
> 
> If I follow your point, we might as well set it to zero at the beginning to just
> signal that the interface is not ready and that the MTU is unknown.
> 
> Also, consider the following. If I connect a real device and do right away a:
> 
>    ip link set can0 type can xl off
> 
> then, what is supposed to happen?

Nothing. There is no bitrate configured.

> This is supposed to be NOP (the CAN_CTRLMODE_XL is off at the beginning).
> 
> But then, should the MTU change? If yes, should it change to the CANFD_MTU or to
> the CAN_MTU?
> 
> That's why I am saying that having a disconnection between the MTU and the
> control modes is bad. Your logic creates some edge cases for no valid reason.
> 
> The simple logic would be this:
> 
> 		fd off, xl off		fd on, xl off		fd any, xl on
>    ---------------------------------------------------------------------------
>    default mtu	CAN_MTU			CANFD_MTU		CANXL_MTU
>    min mtu	CAN_MTU			CANFD_MTU		CANXL_MIN_MTU
>    max mtu	CAN_MTU			CANFD_MTU		CANXL_MAX_MTU
> 
> Each time the user touches the fd and xl flags, the MTU, MTU MIN and MTU_MAX
> triplet is modified accordingly in can_changelink() to the values in above table
> and this way, you are always in a coherent state in which the MTU matches the
> control mode flags.
> 

Yes, that's fine.

Will the user defined MTU for CAN XL survive the settings when xl is set 
to off and then set to on again?

>>> And this is inconsistent. For me, the MTU should always match the control mode
>>> flags. Because all control modes are off at the beginning, the MTU is thus the
>>> Classical CAN one.
>>>
>>>> I think this should make it clearer and fix the current inconsistency.
>>>>
>>>> Setting the CANFD_MTU via the ip set mtu feature and expect "fd on" being set at
>>>> the same time is bad.
>>>
>>> OK. Aside of a few details, I think we agree on the big lines. The good thing is
>>> that the current can_change_mtu() only targets the real interfaces. The virtual
>>> ones already have their own functions and so will not get impacted.
>>
>> Right. The virtual CAN stuff can stay as-is.
>>
>>> So I am just thinking of doing a full rewrite of can_change_mtu(). The old logic
>>> of being able to implicitly set the fd flag by providing an MTU will go to the
>>> trash can.
>>
>> Yes. That was not consistent and clear in the usage.
>>
>> With my suggestion the can_change_mtu() will be just a simple setting of values
>> which is the same for real and virtual interfaces.
>> For real interfaces we might make some additional checks against the "supported
>> modes" for FD and XL.
>>
>> And when the real interface is set to "up" the MTU is adjusted to the real cc/
>> fd/xl configuration values. I hope this makes it clear now.
>>
>>> The new logic will try to follow as much as possible the intended MTU
>>> logic of the core net framework (which I am still studying).
>>
>> Don't expect too much for our use-case there ;-)
> 
> There is one feature that I would really like to use:
> 
>    net_device->min_mtu
> 
> and
> 
>    net_device->max_mtu
> 
> Once you set those, you automatically get:
> 
>    - validation by the core infrastructure that the user's inputs are in range
>    - reporting of those min and max MTU values through the netlink interface
> 
> The first point is a nice to have. This way, the devices are not forced anymore
> to populate net_device_ops->ndo_change_mtu.
> 
> The second point is what looks the most important to me. I am incapable of
> remembering what are the actual values of CANXL_MIN_MTU and CANXL_MAX_MTU. If we
> advertise those through netlink, then I can easily confirm the range.

Ok. Nice.

To sum it up for myself:

1. Setting the MTU from user space is only relevant for virtual CAN 
interfaces and CAN XL interfaces for values between CANXL_MIN_MTU and 
CANXL_MAX_MTU.

2. Usually the MTU is set automatically by the netlink configuration 
process when fd/xl on/off are set.

Right?

Best regards,
Oliver