Re: [PATCH v3 5/5] can: allow to change the device mtu for CAN FD capable devices

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 18.02.2014 14:08, Oliver Hartkopp wrote:
>
>
> On 02/18/2014 12:09 PM, Marc Kleine-Budde wrote:
>> On 02/15/2014 06:29 PM, Oliver Hartkopp wrote:
>>>> On 02/15/2014 05:32 PM, Oliver Hartkopp wrote:
>>>>> The configuration for CAN FD depends on CAN_CTRLMODE_FD enabled in the
>>>>> driver
>>>>> specific ctrlmode_supported capabilities.
>>>>>
>>>>> 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).
>>>>
>>>> Please don't add two possibilities to activate canfd on a CAN network
>>>> card. There should only be one.
>>>
>>> Yes it /should/ be like this.
>>> I thought about it some time but I didn't find a better solution.
>>>
>>> The problem is that dev->mtu is the switch for the skb->len whether CAN frames
>>> or CAN FD frames are generated or received.
>>>
>>> The MTU can be modified by 'ip' (independently from CAN specific settings).
>>
>> It _can_ be modified (directly by the userspace) _if_ the change mtu
>> callback is implemented. Maybe we don't need the callback?

See:
http://lxr.free-electrons.com/source/net/core/dev.c#L5324

When we do not define our own callback function the mtu is set to any value 
the user specifies. This is definitely not a good idea %-)

The question is if we define our own can_change_mtu() callback to control the 
mtu access - but only allow the FD switching via ip tool.

Or if we allow to modify the FD switch via 'ip' and mtu setting which was the 
original patch idea?!?

>
> We need the callback to check if the setting is allowed.
> E.g. to set the MTU to 72 is only allowed for CAN FD capable devices.
>
> Or do you think about *only* switching the MTU via
>
> ip link set can0 type can fd on
> ip link set can0 type can fd off
>
> ??
>
> This could be an idea too.
>
> Currently the vcan can only be switched via mtu setting to identify CAN or CAN
> FD. But this could be a single and only exception to do it like this - as the
> vcan does not use the CAN driver infrastructure.
>
>>
>>> OTOH the only way the CAN devices provide their capabilities is the
>>> ctrlmode_supported bitfield - and it does not make sense to provide
>>> CAN_CTRLMODE_FD in ctrlmode_supported and do not provide the configuration
>>> for it.
>>>
>>> If you would skip the CAN_CTRLMODE_FD bit in ctrlmode_supported there's no way
>>> to identify a CAN FD capable device. You could only try to set the MTU to
>>> CANFD_MTU and see, if it fails. Not a good solution either.
>>>
>>>
>>>>> +EXPORT_SYMBOL_GPL(can_change_mtu);
>>>>
>>>> Who will be the user of this function? The individual network drivers?
>>>>
>>>
>>> Yes. It manages the MTU and CAN_CTRLMODE_FD consistency and only allows the
>>> CANFD_MTU when the driver supports it.
>>
>> And it has to be assigned by a FD capable driver to the net_device_ops,
>> right?
>>
>
> Yes.
>
> Regards,
> Oliver
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html