| Message ID | 20211026121651.1814251-1-mailhol.vincent@wanadoo.fr (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v4] can: netlink: report the CAN controller mode supported flags | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Series ignored based on subject |
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 |
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
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?
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; };
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(-)