All of lore.kernel.org
 help / color / mirror / Atom feed
* [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.