* [PATCH v2 0/3] report the controller capabilities through the netlink interface
@ 2021-10-09 13:13 Vincent Mailhol
2021-10-09 13:13 ` [PATCH v2 1/3] can: dev: replace can_priv::ctrlmode_static by can_get_static_ctrlmode() Vincent Mailhol
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Vincent Mailhol @ 2021-10-09 13:13 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can; +Cc: netdev, linux-kernel, Vincent Mailhol
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 1/3] can: dev: replace can_priv::ctrlmode_static by can_get_static_ctrlmode()
2021-10-09 13:13 [PATCH v2 0/3] report the controller capabilities through the netlink interface Vincent Mailhol
@ 2021-10-09 13:13 ` Vincent Mailhol
2021-10-24 18:30 ` Marc Kleine-Budde
2021-10-09 13:13 ` [PATCH v2 2/3] can: dev: reorder struct can_priv members for better packing Vincent Mailhol
2021-10-09 13:13 ` [PATCH v2 3/3] can: netlink: report the CAN controller mode supported flags Vincent Mailhol
2 siblings, 1 reply; 8+ messages in thread
From: Vincent Mailhol @ 2021-10-09 13:13 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 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 <mailhol.vincent@wanadoo.fr>
---
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 2/3] can: dev: reorder struct can_priv members for better packing
2021-10-09 13:13 [PATCH v2 0/3] report the controller capabilities through the netlink interface Vincent Mailhol
2021-10-09 13:13 ` [PATCH v2 1/3] can: dev: replace can_priv::ctrlmode_static by can_get_static_ctrlmode() Vincent Mailhol
@ 2021-10-09 13:13 ` Vincent Mailhol
2021-10-09 13:13 ` [PATCH v2 3/3] can: netlink: report the CAN controller mode supported flags Vincent Mailhol
2 siblings, 0 replies; 8+ messages in thread
From: Vincent Mailhol @ 2021-10-09 13:13 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 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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 3/3] can: netlink: report the CAN controller mode supported flags
2021-10-09 13:13 [PATCH v2 0/3] report the controller capabilities through the netlink interface Vincent Mailhol
2021-10-09 13:13 ` [PATCH v2 1/3] can: dev: replace can_priv::ctrlmode_static by can_get_static_ctrlmode() Vincent Mailhol
2021-10-09 13:13 ` [PATCH v2 2/3] can: dev: reorder struct can_priv members for better packing Vincent Mailhol
@ 2021-10-09 13:13 ` Vincent Mailhol
2 siblings, 0 replies; 8+ messages in thread
From: Vincent Mailhol @ 2021-10-09 13:13 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 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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/3] can: dev: replace can_priv::ctrlmode_static by can_get_static_ctrlmode()
2021-10-09 13:13 ` [PATCH v2 1/3] can: dev: replace can_priv::ctrlmode_static by can_get_static_ctrlmode() Vincent Mailhol
@ 2021-10-24 18:30 ` Marc Kleine-Budde
2021-10-25 17:22 ` Vincent MAILHOL
0 siblings, 1 reply; 8+ messages in thread
From: Marc Kleine-Budde @ 2021-10-24 18:30 UTC (permalink / raw)
To: Vincent Mailhol; +Cc: linux-can, netdev, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1153 bytes --]
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 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/3] can: dev: replace can_priv::ctrlmode_static by can_get_static_ctrlmode()
2021-10-24 18:30 ` Marc Kleine-Budde
@ 2021-10-25 17:22 ` Vincent MAILHOL
2021-10-25 19:06 ` Marc Kleine-Budde
0 siblings, 1 reply; 8+ messages in thread
From: Vincent MAILHOL @ 2021-10-25 17:22 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: linux-can, netdev, open list
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 <mkl@pengutronix.de> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/3] can: dev: replace can_priv::ctrlmode_static by can_get_static_ctrlmode()
2021-10-25 17:22 ` Vincent MAILHOL
@ 2021-10-25 19:06 ` Marc Kleine-Budde
2021-10-26 1:36 ` Vincent MAILHOL
0 siblings, 1 reply; 8+ messages in thread
From: Marc Kleine-Budde @ 2021-10-25 19:06 UTC (permalink / raw)
To: Vincent MAILHOL; +Cc: linux-can, netdev, open list
[-- Attachment #1: Type: text/plain, Size: 2100 bytes --]
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 <mkl@pengutronix.de> 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 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/3] can: dev: replace can_priv::ctrlmode_static by can_get_static_ctrlmode()
2021-10-25 19:06 ` Marc Kleine-Budde
@ 2021-10-26 1:36 ` Vincent MAILHOL
0 siblings, 0 replies; 8+ messages in thread
From: Vincent MAILHOL @ 2021-10-26 1:36 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: linux-can, netdev, open list
On Tue. 26 Oct 2021 at 04:06, Marc Kleine-Budde <mkl@pengutronix.de> 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 <mkl@pengutronix.de> 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/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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-10-26 1:36 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-09 13:13 [PATCH v2 0/3] report the controller capabilities through the netlink interface Vincent Mailhol
2021-10-09 13:13 ` [PATCH v2 1/3] can: dev: replace can_priv::ctrlmode_static by can_get_static_ctrlmode() Vincent Mailhol
2021-10-24 18:30 ` Marc Kleine-Budde
2021-10-25 17:22 ` Vincent MAILHOL
2021-10-25 19:06 ` Marc Kleine-Budde
2021-10-26 1:36 ` Vincent MAILHOL
2021-10-09 13:13 ` [PATCH v2 2/3] can: dev: reorder struct can_priv members for better packing Vincent Mailhol
2021-10-09 13:13 ` [PATCH v2 3/3] can: netlink: report the CAN controller mode supported flags Vincent Mailhol
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.