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

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

[Vincent Mailhol]

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


Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
Hi Marc,

Because you already added the series to the testing branch of
linux-can-next, I am only resending the last patch. Please let me know
if you prefer me to resend the full series.


** Changelog **

v3 -> v4:

  - Tag the union in struct can_ctrlmode as packed.

  - Remove patch 1, 2 and 3 from the series because those were already
    added to the testing branch of linux-can-next (and no changes
    occured on those first three patches).

v2 -> v3:

  - Make can_set_static_ctrlmode() return an error and adjust the
    drivers which use this helper function accordingly.

v1 -> v2:

  - Add a first patch to replace can_priv::ctrlmode_static by the
    inline function can_get_static_ctrlmode()

  - Add a second patch to reorder the fields of struct can_priv for
    better packing (save eight bytes on x86_64 \o/)

  - Rewrite the comments of the third patch "can: netlink: report the
    CAN controller mode supported flags" (no changes on the code
    itself).

---
drivers/net/can/dev/netlink.c    | 5 ++++-
 include/uapi/linux/can/netlink.h | 5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 26c336808be5..32e1eb63ee7d 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -475,7 +475,10 @@ static int can_tdc_fill_info(struct sk_buff *skb, const struct net_device *dev)
 static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
 {
 	struct can_priv *priv = netdev_priv(dev);
-	struct can_ctrlmode cm = {.flags = priv->ctrlmode};
+	struct can_ctrlmode cm = {
+		.supported = priv->ctrlmode_supported,
+		.flags = priv->ctrlmode
+	};
 	struct can_berr_counter bec = { };
 	enum can_state state = priv->state;
 
diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
index 75b85c60efb2..091bb78b0406 100644
--- a/include/uapi/linux/can/netlink.h
+++ b/include/uapi/linux/can/netlink.h
@@ -88,7 +88,10 @@ struct can_berr_counter {
  * CAN controller mode
  */
 struct can_ctrlmode {
-	__u32 mask;
+	union {
+		__u32 mask;		/* Userland to kernel */
+		__u32 supported;	/* Kernel to userland */
+	} __attribute__((packed));	/* Prevent ABI from adding padding */
 	__u32 flags;
 };
 
-- 
2.32.0

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

[Marc Kleine-Budde]

[-- 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 --]

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

[Marc Kleine-Budde]

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

On 26.10.2021 21:16:51, Vincent Mailhol wrote:
> Because you already added the series to the testing branch of
> linux-can-next, I am only resending the last patch. Please let me know
> if you prefer me to resend the full series.

I'll include the other patches in my next pull request, no need to
repost them.

regards,
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 --]

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

[Vincent MAILHOL]

On Fri. 29 Oct 2021 at 21:44, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> 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?

Backward compatibility (new kernel, old iproute2) should be OK: the
kernel will report the value but it will not be consumed.

> 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
> |                 },

I missed that, nice catch!

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

Adding a new nested entry only for one u32 seemed overkill to
me. This is why I tried to do the change as tiny as possible.

I would like to use this IFLA_CAN_CTRLMODE_EXT as a last
resort. I gave it a second thought and I have another idea: we
could keep the exact same kernel code and just have the userland
to discard the can_ctrlmode::supported if it is zero. The caveat
would be that it will be impossible to report the static features
of a controller which do have ctrlmode_static features but no
ctrlmode_supported features. Other use cases would be
supported. As a matter of fact, the two drivers which rely on the
static features (m_can and rcar_canfd) do also have supported
modes. So discarding can_ctrlmode::supported when it is zero would
introduce no issue for all existing drivers. The only remaining
risk is for the yet to be introduced drivers.

So if we are ready to accept the limitation that we would not be
able to report the static features of such hypothetical drivers,
then we can keep the current patch (maybe add a comment) and
introduce an if switch in iproute2 to discard zero value.

The probability for such a driver to ever exist is already low. I
think that this limitation is acceptable.

What do you think?