[RFC PATCH v1 0/3] iproute2-next: iplink_can: report the controller capabilities

[RFC PATCH v1 0/3] iproute2-next: iplink_can: report the controller capabilities

[Vincent Mailhol]

The patch series serve one purpose: allow the user to check both the
supported and the static capabilities.

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...)

Here, we introduce a way to directly report the supported features as
well as the statically enabled features.

The first patch of the series only does some clean up. The second
patch is the real thing. The last patch contains the needed
modification to the uapi headers and is only there for convenience.

Vincent Mailhol (3):
  iplink_can: code refactoring of print_ctrlmode()
  iplink_can: add ctrlmode_{supported,_static} to the "--details --json"
    output
  uapi: can: netlink: add new field to struct can_ctrlmode to report
    capabilities

 include/uapi/linux/can/netlink.h |  5 ++-
 ip/iplink_can.c                  | 54 +++++++++++++++++++-------------
 2 files changed, 36 insertions(+), 23 deletions(-)

-- 
2.32.0

[RFC PATCH v1 1/3] iplink_can: code refactoring of print_ctrlmode()

[Vincent Mailhol]

We do some code refactoring of print_ctrlmode() in prevision of the
upcoming patch:

  - add a new function argument: enum output_type t in order to
    specify the output type (i.e. PRINT_{FP,JSON,ANY}).

  - add a new function argument: const char *key in order to specify
    the name of the json array (e.g. "ctrlmode"). This will be use in
    the upcoming patch to specify other entries: "ctrlmode_supported"
    and "ctrlmode_static".

  - replace the _PF() macro with the print_flag() function to increase
    readability.

  - directly return if none of the flags are set (previously, this
    check was done before calling the function).

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
---
 ip/iplink_can.c | 50 ++++++++++++++++++++++++++++---------------------
 1 file changed, 29 insertions(+), 21 deletions(-)

diff --git a/ip/iplink_can.c b/ip/iplink_can.c
index 6a26f3ff..c70c420d 100644
--- a/ip/iplink_can.c
+++ b/ip/iplink_can.c
@@ -88,27 +88,36 @@ static void set_ctrlmode(char *name, char *arg,
 	cm->mask |= flags;
 }
 
-static void print_ctrlmode(FILE *f, __u32 cm)
+static void print_flag(enum output_type t, __u32 *flags, __u32 flag,
+		       const char* name)
 {
-	open_json_array(PRINT_ANY, is_json_context() ? "ctrlmode" : "<");
-#define _PF(cmflag, cmname)						\
-	if (cm & cmflag) {						\
-		cm &= ~cmflag;						\
-		print_string(PRINT_ANY, NULL, cm ? "%s," : "%s", cmname); \
+	if (*flags & flag) {
+		*flags &= ~flag;
+		print_string(t, NULL, *flags ? "%s, " : "%s", name);
 	}
-	_PF(CAN_CTRLMODE_LOOPBACK, "LOOPBACK");
-	_PF(CAN_CTRLMODE_LISTENONLY, "LISTEN-ONLY");
-	_PF(CAN_CTRLMODE_3_SAMPLES, "TRIPLE-SAMPLING");
-	_PF(CAN_CTRLMODE_ONE_SHOT, "ONE-SHOT");
-	_PF(CAN_CTRLMODE_BERR_REPORTING, "BERR-REPORTING");
-	_PF(CAN_CTRLMODE_FD, "FD");
-	_PF(CAN_CTRLMODE_FD_NON_ISO, "FD-NON-ISO");
-	_PF(CAN_CTRLMODE_PRESUME_ACK, "PRESUME-ACK");
-	_PF(CAN_CTRLMODE_CC_LEN8_DLC, "CC-LEN8-DLC");
-#undef _PF
-	if (cm)
-		print_hex(PRINT_ANY, NULL, "%x", cm);
-	close_json_array(PRINT_ANY, "> ");
+}
+
+static void print_ctrlmode(enum output_type t, __u32 flags, const char* key)
+{
+	if (!flags)
+		return;
+
+	open_json_array(t, is_json_context() ? key : "<");
+
+	print_flag(t, &flags, CAN_CTRLMODE_LOOPBACK, "LOOPBACK");
+	print_flag(t, &flags, CAN_CTRLMODE_LISTENONLY, "LISTEN-ONLY");
+	print_flag(t, &flags, CAN_CTRLMODE_3_SAMPLES, "TRIPLE-SAMPLING");
+	print_flag(t, &flags, CAN_CTRLMODE_ONE_SHOT, "ONE-SHOT");
+	print_flag(t, &flags, CAN_CTRLMODE_BERR_REPORTING, "BERR-REPORTING");
+	print_flag(t, &flags, CAN_CTRLMODE_FD, "FD");
+	print_flag(t, &flags, CAN_CTRLMODE_FD_NON_ISO, "FD-NON-ISO");
+	print_flag(t, &flags, CAN_CTRLMODE_PRESUME_ACK, "PRESUME-ACK");
+	print_flag(t, &flags, CAN_CTRLMODE_CC_LEN8_DLC, "CC-LEN8-DLC");
+
+	if (flags)
+		print_hex(t, NULL, "%x", flags);
+
+	close_json_array(t, "> ");
 }
 
 static int can_parse_opt(struct link_util *lu, int argc, char **argv,
@@ -282,8 +291,7 @@ static void can_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 	if (tb[IFLA_CAN_CTRLMODE]) {
 		struct can_ctrlmode *cm = RTA_DATA(tb[IFLA_CAN_CTRLMODE]);
 
-		if (cm->flags)
-			print_ctrlmode(f, cm->flags);
+		print_ctrlmode(PRINT_ANY, cm->flags, "ctrlmode");
 	}
 
 	if (tb[IFLA_CAN_STATE]) {
-- 
2.32.0

[RFC PATCH v1 2/3] iplink_can: add ctrlmode_{supported,_static} to the "--details --json" output

[Vincent Mailhol]

This patch is the userland counterpart of [1].  Indeed, [1] allows the
can netlink interface to report the CAN controller capabilities.

Previously, only the options which were switched on were reported
(i.e. can_priv::ctrlmode). Here, we add two additional pieces of
information to the json report:

  - ctrlmode_supported: the options that can be modified by netlink

  - ctrlmode_static: option which are statically enabled by the driver
    (i.e. can not be turned off)

For your information, we borrowed the naming convention from struct
can_priv[2].

Contrary to the ctrlmode, the ctrlmode_{supported,_static} are only
reported in the json context. The reason is that this newly added
information can quickly become very verbose and we do not want to
overload the default output. You can think of the --details output as
the verbose mode and the --details --json output as the *very* verbose
mode.

*Example:*

This is how the output would look like for a dummy driver which would
have:
  - FCAN_CTRLMODE_FD_NON_ISO statically enabled by the driver
  - CAN_CTRLMODE_CC_LEN8_DLC turned on by the user
  - CAN_CTRLMODE_LOOPBACK, CAN_CTRLMODE_LISTENONLY,
    CAN_CTRLMODE_3_SAMPLES, CAN_CTRLMODE_FD, CAN_CTRLMODE_CC_LEN8_DLC
    supported by the driver

| $ ip link set can0 type can cc-len8-dlc on
| $ ip --details --json --pretty link show can0
| [ {
|         "ifindex": 1,
|         "ifname": "can0",
|         "flags": [ "NOARP","ECHO" ],
|         "mtu": 16,
|         "qdisc": "noop",
|         "operstate": "DOWN",
|         "linkmode": "DEFAULT",
|         "group": "default",
|         "txqlen": 10,
|         "link_type": "can",
|         "promiscuity": 0,
|         "min_mtu": 0,
|         "max_mtu": 0,
|         "linkinfo": {
|             "info_kind": "can",
|             "info_data": {
|                 "ctrlmode": [ "FD-NON-ISO","CC-LEN8-DLC" ],
|                 "ctrlmode_supported": [ "LOOPBACK","LISTEN-ONLY","TRIPLE-SAMPLING","FD","CC-LEN8-DLC" ],
|                 "ctrlmode_static": [ "FD-NON-ISO" ],
|                 "state": "STOPPED",
|                 "restart_ms": 0,
|                 "bittiming_const": {
|                     "name": "DUMMY_CAN_DEV",
|                     "tseg1": {
|                         "min": 2,
|                         "max": 256
|                     },
|                     "tseg2": {
|                         "min": 2,
|                         "max": 128
|                     },
|                     "sjw": {
|                         "min": 1,
|                         "max": 128
|                     },
|                     "brp": {
|                         "min": 1,
|                         "max": 512
|                     },
|                     "brp_inc": 1
|                 },
|                 "data_bittiming_const": {
|                     "name": "DUMMY_CAN_DEV",
|                     "tseg1": {
|                         "min": 2,
|                         "max": 32
|                     },
|                     "tseg2": {
|                         "min": 1,
|                         "max": 16
|                     },
|                     "sjw": {
|                         "min": 1,
|                         "max": 8
|                     },
|                     "brp": {
|                         "min": 1,
|                         "max": 32
|                     },
|                     "brp_inc": 1
|                 },
|                 "clock": 80000000
|             }
|         },
|         "num_tx_queues": 1,
|         "num_rx_queues": 1,
|         "gso_max_size": 65536,
|         "gso_max_segs": 65535,
|         "parentbus": "usb",
|         "parentdev": "1-9:1.1"
|     } ]

As mentioned above, the default output remains unchanged:

| $ ./ip --details link show can0
| 1: can0: <NOARP,ECHO> mtu 16 qdisc noop state DOWN mode DEFAULT group default qlen 10
|     link/can  promiscuity 0 minmtu 0 maxmtu 0
|     can <FD-NON-ISO, CC-LEN8-DLC> state STOPPED restart-ms 0
| 	  DUMMY_CAN_DEV: tseg1 2..256 tseg2 2..128 sjw 1..128 brp 1..512 brp-inc 1
| 	  DUMMY_CAN_DEV: dtseg1 2..32 dtseg2 1..16 dsjw 1..8 dbrp 1..32 dbrp-inc 1
| 	  clock 80000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 parentbus usb parentdev 1-9:1.1

[1] https://lore.kernel.org/linux-can/20211003044049.568441-1-mailhol.vincent@wanadoo.fr/T/#u
[2] https://elixir.bootlin.com/linux/v5.14/source/include/linux/can/dev.h#61

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
I tried to think a lot of what would be the best way to report the
ctrlmode_{supported,_static} in the default (non-json) context and
just concluded that this was too much info.

That said, I warmly welcome any ideas of how to add
ctrlmode_{supported,_static} to the default output while keeping
things sexy.
---
 ip/iplink_can.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/ip/iplink_can.c b/ip/iplink_can.c
index c70c420d..516dfd23 100644
--- a/ip/iplink_can.c
+++ b/ip/iplink_can.c
@@ -124,7 +124,7 @@ static int can_parse_opt(struct link_util *lu, int argc, char **argv,
 			 struct nlmsghdr *n)
 {
 	struct can_bittiming bt = {}, dbt = {};
-	struct can_ctrlmode cm = {0, 0};
+	struct can_ctrlmode cm = { 0 };
 
 	while (argc > 0) {
 		if (matches(*argv, "bitrate") == 0) {
@@ -292,6 +292,8 @@ static void can_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 		struct can_ctrlmode *cm = RTA_DATA(tb[IFLA_CAN_CTRLMODE]);
 
 		print_ctrlmode(PRINT_ANY, cm->flags, "ctrlmode");
+		print_ctrlmode(PRINT_JSON, cm->supported, "ctrlmode_supported");
+		print_ctrlmode(PRINT_JSON, cm->flags & ~cm->supported, "ctrlmode_static");
 	}
 
 	if (tb[IFLA_CAN_STATE]) {
-- 
2.32.0

[RFC PATCH v1 3/3] uapi: can: netlink: add new field to struct can_ctrlmode to report capabilities

[Vincent Mailhol]

This patch is for your convinience. iproute2-next normally directly
pulls the uapi changes but I think it will be more convinient for
people who want to test to just be able to directly apply this series
and have things working.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
Please do not pull!
---
 include/uapi/linux/can/netlink.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
index 00c763df..fa1cab72 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