[PATCH v2 0/3] report the controller capabilities through the netlink interface

The main purpose of this series is to report the CAN controller
capabilities. The proposed method reuses the existing struct
can_ctrlmode and thus do not need a new IFLA_CAN_* entry.

While doing so, I also realized that can_priv::ctrlmode_static could
actually be derived from the other ctrlmode fields. So I added two
extra patches to the series: one to replace that field with a function
and one to repack struct can_priv and fill the hole created after
removing can_priv::ctrlmode_priv.

Please note that the first two patches are not required by the third
one. I am just grouping everything in the same series because the
patches all revolve around the controller modes.


** Changelog **

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


Vincent Mailhol (3):
  can: dev: replace can_priv::ctrlmode_static by
    can_get_static_ctrlmode()
  can: dev: reorder struct can_priv members for better packing
  can: netlink: report the CAN controller mode supported flags

 drivers/net/can/dev/dev.c        |  5 +++--
 drivers/net/can/dev/netlink.c    |  7 +++++--
 include/linux/can/dev.h          | 18 +++++++++++++-----
 include/uapi/linux/can/netlink.h |  5 ++++-
 4 files changed, 25 insertions(+), 10 deletions(-)

-- 
2.32.0

The statically enabled features of a CAN controller can be retrieved
using below formula:

| u32 ctrlmode_static = priv->ctrlmode & ~priv->ctrlmode_supported;

As such, there is no need to store this information. This patch remove
the field ctrlmode_static of struct can_priv and provides, in
replacement, the inline function can_get_static_ctrlmode() which
returns the same value.

A condition sine qua non for this to work is that the controller
static modes should never be set in can_priv::ctrlmode_supported. This
is already the case for existing drivers, however, we added a warning
message in can_set_static_ctrlmode() to check that.

Signed-off-by: Vincent Mailhol <redacted>
---
 drivers/net/can/dev/dev.c     |  5 +++--
 drivers/net/can/dev/netlink.c |  2 +-
 include/linux/can/dev.h       | 12 ++++++++++--
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/net/can/dev/dev.c b/drivers/net/can/dev/dev.c
index e3d840b81357..59c79f92fccc 100644
--- a/drivers/net/can/dev/dev.c
+++ b/drivers/net/can/dev/dev.c

@@ -300,6 +300,7 @@ EXPORT_SYMBOL_GPL(free_candev);
 int can_change_mtu(struct net_device *dev, int new_mtu)
 {
 	struct can_priv *priv = netdev_priv(dev);
+	u32 ctrlmode_static = can_get_static_ctrlmode(priv);
 
 	/* Do not allow changing the MTU while running */
 	if (dev->flags & IFF_UP)

@@ -309,7 +310,7 @@ int can_change_mtu(struct net_device *dev, int new_mtu)
 	switch (new_mtu) {
 	case CAN_MTU:
 		/* 'CANFD-only' controllers can not switch to CAN_MTU */
-		if (priv->ctrlmode_static & CAN_CTRLMODE_FD)
+		if (ctrlmode_static & CAN_CTRLMODE_FD)
 			return -EINVAL;
 
 		priv->ctrlmode &= ~CAN_CTRLMODE_FD;

@@ -318,7 +319,7 @@ int can_change_mtu(struct net_device *dev, int new_mtu)
 	case CANFD_MTU:
 		/* check for potential CANFD ability */
 		if (!(priv->ctrlmode_supported & CAN_CTRLMODE_FD) &&
-		    !(priv->ctrlmode_static & CAN_CTRLMODE_FD))
+		    !(ctrlmode_static & CAN_CTRLMODE_FD))
 			return -EINVAL;
 
 		priv->ctrlmode |= CAN_CTRLMODE_FD;

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 80425636049d..6c9906e8040c 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c

@@ -112,7 +112,7 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 		if (dev->flags & IFF_UP)
 			return -EBUSY;
 		cm = nla_data(data[IFLA_CAN_CTRLMODE]);
-		ctrlstatic = priv->ctrlmode_static;
+		ctrlstatic = can_get_static_ctrlmode(priv);
 		maskedflags = cm->flags & cm->mask;
 
 		/* check whether provided bits are allowed to be passed */

diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 2413253e54c7..2381ad9e0814 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h

@@ -69,7 +69,6 @@ struct can_priv {
 	/* CAN controller features - see include/uapi/linux/can/netlink.h */
 	u32 ctrlmode;		/* current options setting */
 	u32 ctrlmode_supported;	/* options that can be modified by netlink */
-	u32 ctrlmode_static;	/* static enabled options for driver/hardware */
 
 	int restart_ms;
 	struct delayed_work restart_work;

@@ -104,14 +103,23 @@ static inline void can_set_static_ctrlmode(struct net_device *dev,
 	struct can_priv *priv = netdev_priv(dev);
 
 	/* alloc_candev() succeeded => netdev_priv() is valid at this point */
+	if (priv->ctrlmode_supported & static_mode) {
+		netdev_warn(dev,
+			    "Controller features can not be supported and static at the same time\n");
+		return;
+	}
 	priv->ctrlmode = static_mode;
-	priv->ctrlmode_static = static_mode;
 
 	/* override MTU which was set by default in can_setup()? */
 	if (static_mode & CAN_CTRLMODE_FD)
 		dev->mtu = CANFD_MTU;
 }
 
+static inline u32 can_get_static_ctrlmode(struct can_priv *priv)
+{
+	return priv->ctrlmode & ~priv->ctrlmode_supported;
+}
+
 void can_setup(struct net_device *dev);
 
 struct net_device *alloc_candev_mqs(int sizeof_priv, unsigned int echo_skb_max,

-- 
2.32.0

Save eight bytes of holes on x86-64 architectures by reordering the
members of struct can_priv.

Before:

| $ pahole -C can_priv drivers/net/can/dev/dev.o
| struct can_priv {
| 	struct net_device *        dev;                  /*     0     8 */
| 	struct can_device_stats    can_stats;            /*     8    24 */
| 	const struct can_bittiming_const  * bittiming_const; /*    32     8 */
| 	const struct can_bittiming_const  * data_bittiming_const; /*    40     8 */
| 	struct can_bittiming       bittiming;            /*    48    32 */
| 	/* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */
| 	struct can_bittiming       data_bittiming;       /*    80    32 */
| 	const struct can_tdc_const  * tdc_const;         /*   112     8 */
| 	struct can_tdc             tdc;                  /*   120    12 */
| 	/* --- cacheline 2 boundary (128 bytes) was 4 bytes ago --- */
| 	unsigned int               bitrate_const_cnt;    /*   132     4 */
| 	const u32  *               bitrate_const;        /*   136     8 */
| 	const u32  *               data_bitrate_const;   /*   144     8 */
| 	unsigned int               data_bitrate_const_cnt; /*   152     4 */
| 	u32                        bitrate_max;          /*   156     4 */
| 	struct can_clock           clock;                /*   160     4 */
| 	unsigned int               termination_const_cnt; /*   164     4 */
| 	const u16  *               termination_const;    /*   168     8 */
| 	u16                        termination;          /*   176     2 */
|
| 	/* XXX 6 bytes hole, try to pack */
|
| 	struct gpio_desc *         termination_gpio;     /*   184     8 */
| 	/* --- cacheline 3 boundary (192 bytes) --- */
| 	u16                        termination_gpio_ohms[2]; /*   192     4 */
| 	enum can_state             state;                /*   196     4 */
| 	u32                        ctrlmode;             /*   200     4 */
| 	u32                        ctrlmode_supported;   /*   204     4 */
| 	int                        restart_ms;           /*   208     4 */
|
| 	/* XXX 4 bytes hole, try to pack */
|
| 	struct delayed_work        restart_work;         /*   216    88 */
|
| 	/* XXX last struct has 4 bytes of padding */
|
| 	/* --- cacheline 4 boundary (256 bytes) was 48 bytes ago --- */
| 	int                        (*do_set_bittiming)(struct net_device *); /*   304     8 */
| 	int                        (*do_set_data_bittiming)(struct net_device *); /*   312     8 */
| 	/* --- cacheline 5 boundary (320 bytes) --- */
| 	int                        (*do_set_mode)(struct net_device *, enum can_mode); /*   320     8 */
| 	int                        (*do_set_termination)(struct net_device *, u16); /*   328     8 */
| 	int                        (*do_get_state)(const struct net_device  *, enum can_state *); /*   336     8 */
| 	int                        (*do_get_berr_counter)(const struct net_device  *, struct can_berr_counter *); /*   344     8 */
| 	unsigned int               echo_skb_max;         /*   352     4 */
|
| 	/* XXX 4 bytes hole, try to pack */
|
| 	struct sk_buff * *         echo_skb;             /*   360     8 */
|
| 	/* size: 368, cachelines: 6, members: 32 */
| 	/* sum members: 354, holes: 3, sum holes: 14 */
| 	/* paddings: 1, sum paddings: 4 */
| 	/* last cacheline: 48 bytes */
| };

After:

| $ pahole -C can_priv drivers/net/can/dev/dev.o
| struct can_priv {
| 	struct net_device *        dev;                  /*     0     8 */
| 	struct can_device_stats    can_stats;            /*     8    24 */
| 	const struct can_bittiming_const  * bittiming_const; /*    32     8 */
| 	const struct can_bittiming_const  * data_bittiming_const; /*    40     8 */
| 	struct can_bittiming       bittiming;            /*    48    32 */
| 	/* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */
| 	struct can_bittiming       data_bittiming;       /*    80    32 */
| 	const struct can_tdc_const  * tdc_const;         /*   112     8 */
| 	struct can_tdc             tdc;                  /*   120    12 */
| 	/* --- cacheline 2 boundary (128 bytes) was 4 bytes ago --- */
| 	unsigned int               bitrate_const_cnt;    /*   132     4 */
| 	const u32  *               bitrate_const;        /*   136     8 */
| 	const u32  *               data_bitrate_const;   /*   144     8 */
| 	unsigned int               data_bitrate_const_cnt; /*   152     4 */
| 	u32                        bitrate_max;          /*   156     4 */
| 	struct can_clock           clock;                /*   160     4 */
| 	unsigned int               termination_const_cnt; /*   164     4 */
| 	const u16  *               termination_const;    /*   168     8 */
| 	u16                        termination;          /*   176     2 */
|
| 	/* XXX 6 bytes hole, try to pack */
|
| 	struct gpio_desc *         termination_gpio;     /*   184     8 */
| 	/* --- cacheline 3 boundary (192 bytes) --- */
| 	u16                        termination_gpio_ohms[2]; /*   192     4 */
| 	unsigned int               echo_skb_max;         /*   196     4 */
| 	struct sk_buff * *         echo_skb;             /*   200     8 */
| 	enum can_state             state;                /*   208     4 */
| 	u32                        ctrlmode;             /*   212     4 */
| 	u32                        ctrlmode_supported;   /*   216     4 */
| 	int                        restart_ms;           /*   220     4 */
| 	struct delayed_work        restart_work;         /*   224    88 */
|
| 	/* XXX last struct has 4 bytes of padding */
|
| 	/* --- cacheline 4 boundary (256 bytes) was 56 bytes ago --- */
| 	int                        (*do_set_bittiming)(struct net_device *); /*   312     8 */
| 	/* --- cacheline 5 boundary (320 bytes) --- */
| 	int                        (*do_set_data_bittiming)(struct net_device *); /*   320     8 */
| 	int                        (*do_set_mode)(struct net_device *, enum can_mode); /*   328     8 */
| 	int                        (*do_set_termination)(struct net_device *, u16); /*   336     8 */
| 	int                        (*do_get_state)(const struct net_device  *, enum can_state *); /*   344     8 */
| 	int                        (*do_get_berr_counter)(const struct net_device  *, struct can_berr_counter *); /*   352     8 */
|
| 	/* size: 360, cachelines: 6, members: 32 */
| 	/* sum members: 354, holes: 1, sum holes: 6 */
| 	/* paddings: 1, sum paddings: 4 */
| 	/* last cacheline: 40 bytes */
| };

Signed-off-by: Vincent Mailhol <redacted>
---
 include/linux/can/dev.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 2381ad9e0814..78e49b1e001c 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h

@@ -64,6 +64,9 @@ struct can_priv {
 	struct gpio_desc *termination_gpio;
 	u16 termination_gpio_ohms[CAN_TERMINATION_GPIO_MAX];
 
+	unsigned int echo_skb_max;
+	struct sk_buff **echo_skb;
+
 	enum can_state state;
 
 	/* CAN controller features - see include/uapi/linux/can/netlink.h */

@@ -82,9 +85,6 @@ struct can_priv {
 	int (*do_get_berr_counter)(const struct net_device *dev,
 				   struct can_berr_counter *bec);
 
-	unsigned int echo_skb_max;
-	struct sk_buff **echo_skb;
-
 #ifdef CONFIG_CAN_LEDS
 	struct led_trigger *tx_led_trig;
 	char tx_led_trig_name[CAN_LED_NAME_SZ];

-- 
2.32.0

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 <redacted>
---
Please refer to below link for the iproute2-next counterpart of this
patch:

https://lore.kernel.org/linux-can/20211003050147.569044-1-mailhol.vincent@wanadoo.fr/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 6c9906e8040c..86521836fd6d 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c

@@ -264,7 +264,10 @@ static size_t can_get_size(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 f730d443b918..2847ed0dcac3 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 09.10.2021 22:13:02, Vincent Mailhol wrote:

The statically enabled features of a CAN controller can be retrieved
using below formula:

| u32 ctrlmode_static = priv->ctrlmode & ~priv->ctrlmode_supported;

As such, there is no need to store this information. This patch remove
the field ctrlmode_static of struct can_priv and provides, in
replacement, the inline function can_get_static_ctrlmode() which
returns the same value.

A condition sine qua non for this to work is that the controller
static modes should never be set in can_priv::ctrlmode_supported. This
is already the case for existing drivers, however, we added a warning
message in can_set_static_ctrlmode() to check that.

Please make the can_set_static_ctrlmode to return an error in case of a
problem. Adjust the drivers using the function is this patch, too.

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 |

Hi Marc,

Welcome back on the mailing list, hope you had some nice
holidays! And also thanks a lot for your support over the last
few months on my other series to introduce the TDC netlink
interface :)

Le lun. 25 oct. 2021 à 03:30, Marc Kleine-Budde [off-list ref] a écrit :

On 09.10.2021 22:13:02, Vincent Mailhol wrote:

quoted

The statically enabled features of a CAN controller can be retrieved
using below formula:

| u32 ctrlmode_static = priv->ctrlmode & ~priv->ctrlmode_supported;

As such, there is no need to store this information. This patch remove
the field ctrlmode_static of struct can_priv and provides, in
replacement, the inline function can_get_static_ctrlmode() which
returns the same value.

A condition sine qua non for this to work is that the controller
static modes should never be set in can_priv::ctrlmode_supported. This
is already the case for existing drivers, however, we added a warning
message in can_set_static_ctrlmode() to check that.

Please make the can_set_static_ctrlmode to return an error in case of a
problem. Adjust the drivers using the function is this patch, too.

I didn't do so initially because this is more a static
configuration issue that should only occur during
development. Nonetheless, what you suggest is really simple.

I will just split the patch in two: one of the setter and one for
the getter and address your comments.


Yours sincerely,
Vincent Mailhol

On 26.10.2021 02:22:36, Vincent MAILHOL wrote:

Welcome back on the mailing list, hope you had some nice
holidays!

Thanks was really nice, good weather, 1000km of cycling and hanging
around in Vienna. :D

And also thanks a lot for your support over the last
few months on my other series to introduce the TDC netlink
interface :)

The pleasure is on my side, working with you!

Le lun. 25 oct. 2021 à 03:30, Marc Kleine-Budde [off-list ref] a écrit :

quoted

On 09.10.2021 22:13:02, Vincent Mailhol wrote:

quoted

The statically enabled features of a CAN controller can be retrieved
using below formula:

| u32 ctrlmode_static = priv->ctrlmode & ~priv->ctrlmode_supported;

As such, there is no need to store this information. This patch remove
the field ctrlmode_static of struct can_priv and provides, in
replacement, the inline function can_get_static_ctrlmode() which
returns the same value.

A condition sine qua non for this to work is that the controller
static modes should never be set in can_priv::ctrlmode_supported. This
is already the case for existing drivers, however, we added a warning
message in can_set_static_ctrlmode() to check that.

Please make the can_set_static_ctrlmode to return an error in case of a
problem. Adjust the drivers using the function is this patch, too.

I didn't do so initially because this is more a static
configuration issue that should only occur during
development. Nonetheless, what you suggest is really simple.

I will just split the patch in two: one of the setter and one for
the getter and address your comments.

Fine with me. Most important thing is, that the kernel compiles after
each patch.

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 |

On Tue. 26 Oct 2021 at 04:06, Marc Kleine-Budde [off-list ref] wrote:

On 26.10.2021 02:22:36, Vincent MAILHOL wrote:

quoted

Welcome back on the mailing list, hope you had some nice
holidays!

Thanks was really nice, good weather, 1000km of cycling and hanging
around in Vienna. :D

quoted

And also thanks a lot for your support over the last
few months on my other series to introduce the TDC netlink
interface :)

The pleasure is on my side, working with you!

quoted

Le lun. 25 oct. 2021 à 03:30, Marc Kleine-Budde [off-list ref] a écrit :

quoted

On 09.10.2021 22:13:02, Vincent Mailhol wrote:

quoted

The statically enabled features of a CAN controller can be retrieved
using below formula:

| u32 ctrlmode_static = priv->ctrlmode & ~priv->ctrlmode_supported;

As such, there is no need to store this information. This patch remove
the field ctrlmode_static of struct can_priv and provides, in
replacement, the inline function can_get_static_ctrlmode() which
returns the same value.

A condition sine qua non for this to work is that the controller
static modes should never be set in can_priv::ctrlmode_supported. This
is already the case for existing drivers, however, we added a warning
message in can_set_static_ctrlmode() to check that.

Please make the can_set_static_ctrlmode to return an error in case of a
problem. Adjust the drivers using the function is this patch, too.

I didn't do so initially because this is more a static
configuration issue that should only occur during
development. Nonetheless, what you suggest is really simple.

I will just split the patch in two: one of the setter and one for
the getter and address your comments.

Fine with me. Most important thing is, that the kernel compiles after
each patch.

Yep, the v3 [1] compiles fine after each patch. The only limitation is
that I could not do a runtime test for rcar_fd and m_can (second
patch of the v3 [2]) because I do not own the hardware.

[1] https://lore.kernel.org/linux-can/20211025172247.1774451-1-mailhol.vincent@wanadoo.fr/T/#t
[2] https://lore.kernel.org/linux-can/20211025172247.1774451-3-mailhol.vincent@wanadoo.fr/


Yours sincerely,
Vincent Mailhol