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

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 (local)

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 |