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
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 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
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 <[email protected]>
---
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 <[email protected]>
---
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
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 <[email protected]> a écrit :
>
> 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.
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 <[email protected]> a écrit :
> >
> > 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.
>
> 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 <[email protected]> wrote:
> 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 <[email protected]> a écrit :
> > >
> > > 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.
> >
> > 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/[email protected]/T/#t
[2] https://lore.kernel.org/linux-can/[email protected]/
Yours sincerely,
Vincent Mailhol