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".
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 <[email protected]>
---
Please refer to below link for the iproute2-next counterpart of this
patch:
https://lore.kernel.org/linux-can/[email protected]/T/#t
---
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..b846922ac18f 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 */
+ };
__u32 flags;
};
--
2.32.0
On Tue. 26 Oct 2021 at 02:22, Vincent Mailhol
<[email protected]> 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".
>
> 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 <[email protected]>
> ---
> Please refer to below link for the iproute2-next counterpart of this
> patch:
>
> https://lore.kernel.org/linux-can/[email protected]/T/#t
> ---
> 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..b846922ac18f 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 */
> + };
While daydreaming during my lunch break, I suddenly remembered
this thread [1] and was concerned that introducing the union
might break the UAPI.
As a matter of fact, the C standard allows the compiler to add
padding at the end of an union. c.f. 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."
For example, if the kernel were to be compiled with the
-mstructure-size-boundary=64 ARM option in GCC [2], 32 bits of
padding would be introduced after the union, thus breaking the
alignment of the next field: can_ctrlmode::flags.
As far as my knowledge goes, I am not sure whether or not
-mstructure-size-boundary=64 (or similar options on other
architectures) is actually used. Nonetheless, I think it is safer
to declare the union as __attribute__((packed)) to prevent such
padding from occuring.
I will send a v4 later today to address this.
[1] https://lore.kernel.org/linux-can/[email protected]/T/#u
[2] https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html
> __u32 flags;
> };
Yours sincerely,
Vincent Mailhol