* [PATCH v3 1/4] can: dev: replace can_priv::ctrlmode_static by can_get_static_ctrlmode()
2021-10-25 17:22 [PATCH v3 0/4] report the controller capabilities through the netlink interface Vincent Mailhol
@ 2021-10-25 17:22 ` Vincent Mailhol
2021-10-25 17:22 ` [PATCH v3 2/4] can: dev: add sanity check in can_set_static_ctrlmode() Vincent Mailhol
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Vincent Mailhol @ 2021-10-25 17:22 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can; +Cc: netdev, linux-kernel, Vincent Mailhol
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
removes the field ctrlmode_static of struct can_priv and provides, in
replacement, the inline function can_get_static_ctrlmode() which
returns the same value.
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
drivers/net/can/dev/dev.c | 5 +++--
drivers/net/can/dev/netlink.c | 2 +-
include/linux/can/dev.h | 7 +++++--
3 files changed, 9 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 95cca4e5251f..26c336808be5 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -211,7 +211,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 45f19d9db5ca..92e2d69462f0 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;
@@ -139,13 +138,17 @@ static inline void can_set_static_ctrlmode(struct net_device *dev,
/* alloc_candev() succeeded => netdev_priv() is valid at this point */
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v3 2/4] can: dev: add sanity check in can_set_static_ctrlmode()
2021-10-25 17:22 [PATCH v3 0/4] report the controller capabilities through the netlink interface Vincent Mailhol
2021-10-25 17:22 ` [PATCH v3 1/4] can: dev: replace can_priv::ctrlmode_static by can_get_static_ctrlmode() Vincent Mailhol
@ 2021-10-25 17:22 ` Vincent Mailhol
2021-10-25 17:22 ` [PATCH v3 3/4] can: dev: reorder struct can_priv members for better packing Vincent Mailhol
2021-10-25 17:22 ` [PATCH v3 4/4] can: netlink: report the CAN controller mode supported flags Vincent Mailhol
3 siblings, 0 replies; 6+ messages in thread
From: Vincent Mailhol @ 2021-10-25 17:22 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can; +Cc: netdev, linux-kernel, Vincent Mailhol
Previous patch removed can_priv::ctrlmode_static to replace it with
can_get_static_ctrlmode().
A condition sine qua non for this to work is that the controller
static modes should never be set in can_priv::ctrlmode_supported
(c.f. the comment on can_priv::ctrlmode_supported which states that it
is for "options that can be *modified* by netlink"). Also, this
condition is already correctly fulfilled by all existing drivers
which rely on the ctrlmode_static feature.
Nonetheless, we added an extra safeguard in can_set_static_ctrlmode()
to return an error value and to warn the developer who would be
adventurous enough to set to static a given feature that is already
set to supported.
The drivers which rely on the static controller mode are then updated
to check the return value of can_set_static_ctrlmode().
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
Some few comments on how the rcar_canfd and m_can drivers free their
allocated resources when an error occurs during probing.
The function rcar_canfd_channel_probe() is quite inconsistent with the
way it handles errors. After the call to alloc_candev, there are
several "goto fail" statements that would directly exit without
calling free_candev()!
Nonetheless, later on the driver will check the return value of
rcar_canfd_channel_probe() and call rcar_canfd_channel_remove() which
will correctly call free_candev(). Even if this is inconsistent, there
is no sign of a memory leak. So I just applied the change the
can_set_static_ctrlmode() without bothering more (N.B. I do not own
that device so I am not willing to take the risk of making bigger
changes because I can not test).
On the other hand, m_can_dev_setup() is fine: the return value is
checked by the caller and necessary actions are taken.
As such, for both driver, we did a minimal change.
---
drivers/net/can/m_can/m_can.c | 10 +++++++---
drivers/net/can/rcar/rcar_canfd.c | 4 +++-
include/linux/can/dev.h | 11 +++++++++--
3 files changed, 19 insertions(+), 6 deletions(-)
diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 2470c47b2e31..ab4371aa4ff1 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1463,7 +1463,7 @@ static bool m_can_niso_supported(struct m_can_classdev *cdev)
static int m_can_dev_setup(struct m_can_classdev *cdev)
{
struct net_device *dev = cdev->net;
- int m_can_version;
+ int m_can_version, err;
m_can_version = m_can_check_core_release(cdev);
/* return if unsupported version */
@@ -1493,13 +1493,17 @@ static int m_can_dev_setup(struct m_can_classdev *cdev)
switch (cdev->version) {
case 30:
/* CAN_CTRLMODE_FD_NON_ISO is fixed with M_CAN IP v3.0.x */
- can_set_static_ctrlmode(dev, CAN_CTRLMODE_FD_NON_ISO);
+ err = can_set_static_ctrlmode(dev, CAN_CTRLMODE_FD_NON_ISO);
+ if (err)
+ return err;
cdev->can.bittiming_const = &m_can_bittiming_const_30X;
cdev->can.data_bittiming_const = &m_can_data_bittiming_const_30X;
break;
case 31:
/* CAN_CTRLMODE_FD_NON_ISO is fixed with M_CAN IP v3.1.x */
- can_set_static_ctrlmode(dev, CAN_CTRLMODE_FD_NON_ISO);
+ err = can_set_static_ctrlmode(dev, CAN_CTRLMODE_FD_NON_ISO);
+ if (err)
+ return err;
cdev->can.bittiming_const = &m_can_bittiming_const_31X;
cdev->can.data_bittiming_const = &m_can_data_bittiming_const_31X;
break;
diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
index ff9d0f5ae0dd..e225c1c03812 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -1706,7 +1706,9 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch,
&rcar_canfd_data_bittiming_const;
/* Controller starts in CAN FD only mode */
- can_set_static_ctrlmode(ndev, CAN_CTRLMODE_FD);
+ err = can_set_static_ctrlmode(ndev, CAN_CTRLMODE_FD);
+ if (err)
+ goto fail;
priv->can.ctrlmode_supported = CAN_CTRLMODE_BERR_REPORTING;
} else {
/* Controller starts in Classical CAN only mode */
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 92e2d69462f0..fff3f70df697 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -131,17 +131,24 @@ static inline s32 can_get_relative_tdco(const struct can_priv *priv)
}
/* helper to define static CAN controller features at device creation time */
-static inline void can_set_static_ctrlmode(struct net_device *dev,
- u32 static_mode)
+static inline int __must_check can_set_static_ctrlmode(struct net_device *dev,
+ u32 static_mode)
{
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 -EINVAL;
+ }
priv->ctrlmode = static_mode;
/* override MTU which was set by default in can_setup()? */
if (static_mode & CAN_CTRLMODE_FD)
dev->mtu = CANFD_MTU;
+
+ return 0;
}
static inline u32 can_get_static_ctrlmode(struct can_priv *priv)
--
2.32.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v3 3/4] can: dev: reorder struct can_priv members for better packing
2021-10-25 17:22 [PATCH v3 0/4] report the controller capabilities through the netlink interface Vincent Mailhol
2021-10-25 17:22 ` [PATCH v3 1/4] can: dev: replace can_priv::ctrlmode_static by can_get_static_ctrlmode() Vincent Mailhol
2021-10-25 17:22 ` [PATCH v3 2/4] can: dev: add sanity check in can_set_static_ctrlmode() Vincent Mailhol
@ 2021-10-25 17:22 ` Vincent Mailhol
2021-10-25 17:22 ` [PATCH v3 4/4] can: netlink: report the CAN controller mode supported flags Vincent Mailhol
3 siblings, 0 replies; 6+ messages in thread
From: Vincent Mailhol @ 2021-10-25 17:22 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can; +Cc: netdev, linux-kernel, Vincent Mailhol
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 <mailhol.vincent@wanadoo.fr>
---
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 fff3f70df697..c2ea47f30046 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 */
@@ -83,9 +86,6 @@ struct can_priv {
struct can_berr_counter *bec);
int (*do_get_auto_tdcv)(const struct net_device *dev, u32 *tdcv);
- 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v3 4/4] can: netlink: report the CAN controller mode supported flags
2021-10-25 17:22 [PATCH v3 0/4] report the controller capabilities through the netlink interface Vincent Mailhol
` (2 preceding siblings ...)
2021-10-25 17:22 ` [PATCH v3 3/4] can: dev: reorder struct can_priv members for better packing Vincent Mailhol
@ 2021-10-25 17:22 ` Vincent Mailhol
2021-10-26 3:29 ` Vincent MAILHOL
3 siblings, 1 reply; 6+ messages in thread
From: Vincent Mailhol @ 2021-10-25 17:22 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can; +Cc: netdev, linux-kernel, Vincent Mailhol
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 <mailhol.vincent@wanadoo.fr>
---
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 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v3 4/4] can: netlink: report the CAN controller mode supported flags
2021-10-25 17:22 ` [PATCH v3 4/4] can: netlink: report the CAN controller mode supported flags Vincent Mailhol
@ 2021-10-26 3:29 ` Vincent MAILHOL
0 siblings, 0 replies; 6+ messages in thread
From: Vincent MAILHOL @ 2021-10-26 3:29 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can; +Cc: netdev, open list
On Tue. 26 Oct 2021 at 02:22, Vincent Mailhol
<mailhol.vincent@wanadoo.fr> 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 <mailhol.vincent@wanadoo.fr>
> ---
> 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 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/212c8bc3-89f9-9c33-ed1b-b50ac04e7532@hartkopp.net/T/#u
[2] https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html
> __u32 flags;
> };
Yours sincerely,
Vincent Mailhol
^ permalink raw reply [flat|nested] 6+ messages in thread