Re: [PATCH v4] can: netlink: report the CAN controller mode supported flags

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 3162 bytes --]

On 26.10.2021 21:16:51, Vincent Mailhol wrote:
> This patch introduces a method for the user to check both the
> supported and the static capabilities. The proposed method reuses the
> existing struct can_ctrlmode and thus do not need a new IFLA_CAN_*
> entry.
>
> Currently, the CAN netlink interface provides no easy ways to check
> the capabilities of a given controller. The only method from the
> command line is to try each CAN_CTRLMODE_* individually to check
> whether the netlink interface returns an -EOPNOTSUPP error or not
> (alternatively, one may find it easier to directly check the source
> code of the driver instead...)
>
> It appears that can_ctrlmode::mask is only used in one direction: from
> the userland to the kernel. So we can just reuse this field in the
> other direction (from the kernel to userland). But, because the
> semantic is different, we use a union to give this field a proper
> name: "supported".
>
> The union is tagged as packed to prevent any ABI from adding
> padding. In fact, any padding added after the union would change the
> offset of can_ctrlmode::flags within the structure and thus break the
> UAPI backward compatibility. References:
>
>   - ISO/IEC 9899-1999, section 6.7.2.1 "Structure and union
>     specifiers", clause 15: "There may be unnamed padding at the end
>     of a structure or union."
>
>   - The -mstructure-size-boundary=64 ARM option in GCC:
>     https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html
>
>   - A similar issue which occurred on struct can_frame:
>     https://lore.kernel.org/linux-can/212c8bc3-89f9-9c33-ed1b-b50ac04e7532@hartkopp.net
>
> Below table explains how the two fields can_ctrlmode::supported and
> can_ctrlmode::flags, when masked with any of the CAN_CTRLMODE_* bit
> flags, allow us to identify both the supported and the static
> capabilities:
>
>  supported &	flags &		Controller capabilities
>  CAN_CTRLMODE_*	CAN_CTRLMODE_*
>  -----------------------------------------------------------------------
>  false		false		Feature not supported (always disabled)
>  false		true		Static feature (always enabled)
>  true		false		Feature supported but disabled
>  true		true		Feature supported and enabled

What about forwards and backwards compatibility?

Using the new ip (or any other user space app) on an old kernel, it
looks like enabled features are static features. For example the ip
output on a mcp251xfd with enabled CAN-FD, which is _not_ static.

|         "linkinfo": {
|             "info_kind": "can",
|             "info_data": {
|                 "ctrlmode": [ "FD" ],
|                 "ctrlmode_static": [ "FD" ],
|                 "state": "ERROR-ACTIVE",
|                 "berr_counter": {
|                     "tx": 0,
|                     "rx": 0
|                 },

Is it worth and add a new IFLA_CAN_CTRLMODE_EXT that doesn't pass a
struct, but is a NLA_NESTED type?

Marc

--
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]