diff mbox series

[v4,5/7] can: netlink: add interface for CAN-FD Transmitter Delay Compensation (TDC)

Message ID 20210814091750.73931-6-mailhol.vincent@wanadoo.fr
State New
Headers show
Series add the netlink interface for CAN-FD Transmitter Delay Compensation (TDC) | expand

Commit Message

Vincent MAILHOL Aug. 14, 2021, 9:17 a.m. UTC 
Add the netlink interface for TDC parameters of struct can_tdc_const
and can_tdc.

Contrary to the can_bittiming(_const) structures for which there is
just a single IFLA_CAN(_DATA)_BITTMING(_CONST) entry per structure,
here, we create a nested entry IFLA_CAN_TDC. Within this nested entry,
additional IFLA_CAN_TDC_TDC* entries are added for each of the TDC
parameters of the newly introduced struct can_tdc_const and struct
can_tdc.

For struct can_tdc_const, these are:
        IFLA_CAN_TDC_TDCV_MIN
        IFLA_CAN_TDC_TDCV_MAX
        IFLA_CAN_TDC_TDCO_MIN
        IFLA_CAN_TDC_TDCO_MAX
        IFLA_CAN_TDC_TDCF_MIN
        IFLA_CAN_TDC_TDCF_MAX

For struct can_tdc, these are:
        IFLA_CAN_TDC_TDCV
        IFLA_CAN_TDC_TDCO
        IFLA_CAN_TDC_TDCF

This is done so that changes can be applied in the future to the
structures without breaking the netlink interface.

All the new parameters are defined as u32. This arbitrary choice is
done to mimic the other bittiming values with are also all of type
u32. An u16 would have been sufficient to hold the TDC values.

This patch completes below series:
  - commit 289ea9e4ae59 ("can: add new CAN FD bittiming parameters:
    Transmitter Delay Compensation (TDC)")
  - commit c25cc7993243 ("can: bittiming: add calculation for CAN FD
    Transmitter Delay Compensation (TDC)")

Reference: https://lore.kernel.org/linux-can/20210224002008.4158-1-mailhol.vincent@wanadoo.fr/T/#t

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/net/can/dev/netlink.c    | 181 ++++++++++++++++++++++++++++++-
 include/uapi/linux/can/netlink.h |  29 ++++-
 2 files changed, 204 insertions(+), 6 deletions(-)

Comments

Marc Kleine-Budde Aug. 14, 2021, 11:12 a.m. UTC | #1 
On 14.08.2021 18:17:48, Vincent Mailhol wrote:
[...]

>  static int can_changelink(struct net_device *dev, struct nlattr *tb[],
>  			  struct nlattr *data[],
>  			  struct netlink_ext_ack *extack)
>  {
>  	struct can_priv *priv = netdev_priv(dev);
> +	u32 tdc_mask = 0;
>  	int err;
>  
>  	/* We need synchronization with dev->stop() */
> @@ -107,6 +179,7 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
>  		struct can_ctrlmode *cm;
>  		u32 ctrlstatic;
>  		u32 maskedflags;
> +		u32 tdc_flags;
>  
>  		/* Do not allow changing controller mode while running */
>  		if (dev->flags & IFF_UP)
> @@ -138,7 +211,18 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
>  			dev->mtu = CAN_MTU;
>  			memset(&priv->data_bittiming, 0,
>  			       sizeof(priv->data_bittiming));
> +			memset(&priv->tdc, 0, sizeof(priv->tdc));
> +			priv->ctrlmode &= ~CAN_CTRLMODE_TDC_MASK;
>  		}
> +
> +		tdc_flags = cm->flags & CAN_CTRLMODE_TDC_MASK;
> +		/* CAN_CTRLMODE_TDC_{AUTO,MANUAL} are mutually exclusive */
> +		if (tdc_flags == CAN_CTRLMODE_TDC_MASK)
> +			return -EOPNOTSUPP;
> +		/* If one of CAN_CTRLMODE_TDC_* is set then TDC must be set */
> +		if (tdc_flags && !data[IFLA_CAN_TDC])
> +			return -EOPNOTSUPP;

These don't need information form the can_priv, right? So these checks
can be moved to can_validate()?

> +		tdc_mask = cm->mask & CAN_CTRLMODE_TDC_MASK;
>  	}

Marc
Vincent MAILHOL Aug. 15, 2021, 3:38 a.m. UTC | #2 
On Sat. 14 Aug 2021 at 20:12, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 14.08.2021 18:17:48, Vincent Mailhol wrote:

> [...]

>

> >  static int can_changelink(struct net_device *dev, struct nlattr *tb[],

> >                         struct nlattr *data[],

> >                         struct netlink_ext_ack *extack)

> >  {

> >       struct can_priv *priv = netdev_priv(dev);

> > +     u32 tdc_mask = 0;

> >       int err;

> >

> >       /* We need synchronization with dev->stop() */

> > @@ -107,6 +179,7 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],

> >               struct can_ctrlmode *cm;

> >               u32 ctrlstatic;

> >               u32 maskedflags;

> > +             u32 tdc_flags;

> >

> >               /* Do not allow changing controller mode while running */

> >               if (dev->flags & IFF_UP)

> > @@ -138,7 +211,18 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],

> >                       dev->mtu = CAN_MTU;

> >                       memset(&priv->data_bittiming, 0,

> >                              sizeof(priv->data_bittiming));

> > +                     memset(&priv->tdc, 0, sizeof(priv->tdc));

> > +                     priv->ctrlmode &= ~CAN_CTRLMODE_TDC_MASK;

> >               }

> > +

> > +             tdc_flags = cm->flags & CAN_CTRLMODE_TDC_MASK;

> > +             /* CAN_CTRLMODE_TDC_{AUTO,MANUAL} are mutually exclusive */

> > +             if (tdc_flags == CAN_CTRLMODE_TDC_MASK)

> > +                     return -EOPNOTSUPP;

> > +             /* If one of CAN_CTRLMODE_TDC_* is set then TDC must be set */

> > +             if (tdc_flags && !data[IFLA_CAN_TDC])

> > +                     return -EOPNOTSUPP;

>

> These don't need information form the can_priv, right? So these checks

> can be moved to can_validate()?


ACK. Actually, this comment applies not only to can_changelink()
but also to can_tdc_changelink().  I just sent a v5 where I made
sure to move all the checks that do not rely on can_priv to
can_validate().


Yours sincerely,
Vincent
diff mbox series

Patch

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 80425636049d..735b9e36202e 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -2,6 +2,7 @@ 
 /* Copyright (C) 2005 Marc Kleine-Budde, Pengutronix
  * Copyright (C) 2006 Andrey Volkov, Varma Electronics
  * Copyright (C) 2008-2009 Wolfgang Grandegger <wg@grandegger.com>
+ * Copyright (C) 2021 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
  */
 
 #include <linux/can/dev.h>
@@ -19,6 +20,19 @@  static const struct nla_policy can_policy[IFLA_CAN_MAX + 1] = {
 	[IFLA_CAN_DATA_BITTIMING] = { .len = sizeof(struct can_bittiming) },
 	[IFLA_CAN_DATA_BITTIMING_CONST]	= { .len = sizeof(struct can_bittiming_const) },
 	[IFLA_CAN_TERMINATION] = { .type = NLA_U16 },
+	[IFLA_CAN_TDC] = { .type = NLA_NESTED },
+};
+
+static const struct nla_policy can_tdc_policy[IFLA_CAN_TDC_MAX + 1] = {
+	[IFLA_CAN_TDC_TDCV_MIN] = { .type = NLA_U32 },
+	[IFLA_CAN_TDC_TDCV_MAX] = { .type = NLA_U32 },
+	[IFLA_CAN_TDC_TDCO_MIN] = { .type = NLA_U32 },
+	[IFLA_CAN_TDC_TDCO_MAX] = { .type = NLA_U32 },
+	[IFLA_CAN_TDC_TDCF_MIN] = { .type = NLA_U32 },
+	[IFLA_CAN_TDC_TDCF_MAX] = { .type = NLA_U32 },
+	[IFLA_CAN_TDC_TDCV] = { .type = NLA_U32 },
+	[IFLA_CAN_TDC_TDCO] = { .type = NLA_U32 },
+	[IFLA_CAN_TDC_TDCF] = { .type = NLA_U32 },
 };
 
 static int can_validate(struct nlattr *tb[], struct nlattr *data[],
@@ -46,7 +60,7 @@  static int can_validate(struct nlattr *tb[], struct nlattr *data[],
 			return -EOPNOTSUPP;
 	}
 
-	if (data[IFLA_CAN_DATA_BITTIMING]) {
+	if (data[IFLA_CAN_DATA_BITTIMING] || data[IFLA_CAN_TDC]) {
 		if (!is_can_fd)
 			return -EOPNOTSUPP;
 	}
@@ -54,11 +68,69 @@  static int can_validate(struct nlattr *tb[], struct nlattr *data[],
 	return 0;
 }
 
+static int can_tdc_changelink(struct net_device *dev, const struct nlattr *nla,
+			      struct netlink_ext_ack *extack)
+{
+	struct nlattr *tb[IFLA_CAN_TDC_MAX + 1];
+	struct can_priv *priv = netdev_priv(dev);
+	struct can_tdc *tdc = &priv->tdc;
+	const struct can_tdc_const *tdc_const = priv->tdc_const;
+	int err;
+
+	if (!tdc_const || !can_tdc_is_enabled(priv))
+		return -EOPNOTSUPP;
+
+	if (dev->flags & IFF_UP)
+		return -EBUSY;
+
+	err = nla_parse_nested(tb, IFLA_CAN_TDC_MAX, nla,
+			       can_tdc_policy, extack);
+	if (err)
+		return err;
+
+	if (tb[IFLA_CAN_TDC_TDCV]) {
+		u32 tdcv = nla_get_u32(tb[IFLA_CAN_TDC_TDCV]);
+
+		if (priv->ctrlmode & CAN_CTRLMODE_TDC_AUTO)
+			return -EOPNOTSUPP;
+
+		if (tdcv < tdc_const->tdcv_min || tdcv > tdc_const->tdcv_max)
+			return -EINVAL;
+
+		tdc->tdcv = tdcv;
+	} else if (priv->ctrlmode & CAN_CTRLMODE_TDC_MANUAL) {
+		return -EOPNOTSUPP;
+	}
+
+	if (tb[IFLA_CAN_TDC_TDCO]) {
+		u32 tdco = nla_get_u32(tb[IFLA_CAN_TDC_TDCO]);
+
+		if (tdco < tdc_const->tdco_min || tdco > tdc_const->tdco_max)
+			return -EINVAL;
+
+		tdc->tdco = tdco;
+	} else {
+		return -EOPNOTSUPP;
+	}
+
+	if (tb[IFLA_CAN_TDC_TDCF]) {
+		u32 tdcf = nla_get_u32(tb[IFLA_CAN_TDC_TDCF]);
+
+		if (tdcf < tdc_const->tdcf_min || tdcf > tdc_const->tdcf_max)
+			return -EINVAL;
+
+		tdc->tdcf = tdcf;
+	}
+
+	return 0;
+}
+
 static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 			  struct nlattr *data[],
 			  struct netlink_ext_ack *extack)
 {
 	struct can_priv *priv = netdev_priv(dev);
+	u32 tdc_mask = 0;
 	int err;
 
 	/* We need synchronization with dev->stop() */
@@ -107,6 +179,7 @@  static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 		struct can_ctrlmode *cm;
 		u32 ctrlstatic;
 		u32 maskedflags;
+		u32 tdc_flags;
 
 		/* Do not allow changing controller mode while running */
 		if (dev->flags & IFF_UP)
@@ -138,7 +211,18 @@  static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 			dev->mtu = CAN_MTU;
 			memset(&priv->data_bittiming, 0,
 			       sizeof(priv->data_bittiming));
+			memset(&priv->tdc, 0, sizeof(priv->tdc));
+			priv->ctrlmode &= ~CAN_CTRLMODE_TDC_MASK;
 		}
+
+		tdc_flags = cm->flags & CAN_CTRLMODE_TDC_MASK;
+		/* CAN_CTRLMODE_TDC_{AUTO,MANUAL} are mutually exclusive */
+		if (tdc_flags == CAN_CTRLMODE_TDC_MASK)
+			return -EOPNOTSUPP;
+		/* If one of CAN_CTRLMODE_TDC_* is set then TDC must be set */
+		if (tdc_flags && !data[IFLA_CAN_TDC])
+			return -EOPNOTSUPP;
+		tdc_mask = cm->mask & CAN_CTRLMODE_TDC_MASK;
 	}
 
 	if (data[IFLA_CAN_RESTART_MS]) {
@@ -189,8 +273,6 @@  static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 
 		memcpy(&priv->data_bittiming, &dbt, sizeof(dbt));
 
-		can_calc_tdco(dev);
-
 		if (priv->do_set_data_bittiming) {
 			/* Finally, set the bit-timing registers */
 			err = priv->do_set_data_bittiming(dev);
@@ -223,9 +305,52 @@  static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 		priv->termination = termval;
 	}
 
+	if (data[IFLA_CAN_TDC]) {
+		/* Use the provided TDC parameters */
+		err = can_tdc_changelink(dev, data[IFLA_CAN_TDC], extack);
+		if (err)
+			return err;
+	} else if (!tdc_mask) {
+		/* both TDC parameters and flags not provided: do calculation */
+		can_calc_tdco(dev);
+	} /* else: both CAN_CTRLMODE_TDC_{AUTO,MANUAL} are explicitly
+	   * turned off. TDC is disabled
+	   */
+
 	return 0;
 }
 
+static size_t can_tdc_get_size(const struct net_device *dev)
+{
+	struct can_priv *priv = netdev_priv(dev);
+	size_t size;
+
+	if (!priv->tdc_const)
+		return 0;
+
+	size = nla_total_size(0);			/* nest IFLA_CAN_TDC */
+	if (priv->ctrlmode_supported & CAN_CTRLMODE_TDC_MANUAL) {
+		size += nla_total_size(sizeof(u32));	/* IFLA_CAN_TDCV_MIN */
+		size += nla_total_size(sizeof(u32));	/* IFLA_CAN_TDCV_MAX */
+	}
+	size += nla_total_size(sizeof(u32));		/* IFLA_CAN_TDCO_MIN */
+	size += nla_total_size(sizeof(u32));		/* IFLA_CAN_TDCO_MAX */
+	if (priv->tdc_const->tdcf_max) {
+		size += nla_total_size(sizeof(u32));	/* IFLA_CAN_TDCF_MIN */
+		size += nla_total_size(sizeof(u32));	/* IFLA_CAN_TDCF_MAX */
+	}
+
+	if (can_tdc_is_enabled(priv)) {
+		if (priv->ctrlmode & CAN_CTRLMODE_TDC_MANUAL)
+			size += nla_total_size(sizeof(u32));	/* IFLA_CAN_TDCV */
+		size += nla_total_size(sizeof(u32));		/* IFLA_CAN_TDCO */
+		if (priv->tdc_const->tdcf_max)
+			size += nla_total_size(sizeof(u32));	/* IFLA_CAN_TDCF */
+	}
+
+	return size;
+}
+
 static size_t can_get_size(const struct net_device *dev)
 {
 	struct can_priv *priv = netdev_priv(dev);
@@ -257,10 +382,56 @@  static size_t can_get_size(const struct net_device *dev)
 		size += nla_total_size(sizeof(*priv->data_bitrate_const) *
 				       priv->data_bitrate_const_cnt);
 	size += sizeof(priv->bitrate_max);			/* IFLA_CAN_BITRATE_MAX */
+	size += can_tdc_get_size(dev);				/* IFLA_CAN_TDC */
 
 	return size;
 }
 
+static int can_tdc_fill_info(struct sk_buff *skb, const struct net_device *dev)
+{
+	struct nlattr *nest;
+	struct can_priv *priv = netdev_priv(dev);
+	struct can_tdc *tdc = &priv->tdc;
+	const struct can_tdc_const *tdc_const = priv->tdc_const;
+
+	if (!tdc_const)
+		return 0;
+
+	nest = nla_nest_start(skb, IFLA_CAN_TDC);
+	if (!nest)
+		return -EMSGSIZE;
+
+	if (priv->ctrlmode_supported & CAN_CTRLMODE_TDC_MANUAL &&
+	    (nla_put_u32(skb, IFLA_CAN_TDC_TDCV_MIN, tdc_const->tdcv_min) ||
+	     nla_put_u32(skb, IFLA_CAN_TDC_TDCV_MAX, tdc_const->tdcv_max)))
+		goto err_cancel;
+	if (nla_put_u32(skb, IFLA_CAN_TDC_TDCO_MIN, tdc_const->tdco_min) ||
+	    nla_put_u32(skb, IFLA_CAN_TDC_TDCO_MAX, tdc_const->tdco_max))
+		goto err_cancel;
+	if (tdc_const->tdcf_max &&
+	    (nla_put_u32(skb, IFLA_CAN_TDC_TDCF_MIN, tdc_const->tdcf_min) ||
+	     nla_put_u32(skb, IFLA_CAN_TDC_TDCF_MAX, tdc_const->tdcf_max)))
+		goto err_cancel;
+
+	if (can_tdc_is_enabled(priv)) {
+		if (priv->ctrlmode & CAN_CTRLMODE_TDC_MANUAL &&
+		    nla_put_u32(skb, IFLA_CAN_TDC_TDCV, tdc->tdcv))
+			goto err_cancel;
+		if (nla_put_u32(skb, IFLA_CAN_TDC_TDCO, tdc->tdco))
+			goto err_cancel;
+		if (tdc_const->tdcf_max &&
+		    nla_put_u32(skb, IFLA_CAN_TDC_TDCF, tdc->tdcf))
+			goto err_cancel;
+	}
+
+	nla_nest_end(skb, nest);
+	return 0;
+
+err_cancel:
+	nla_nest_cancel(skb, nest);
+	return -EMSGSIZE;
+}
+
 static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
 {
 	struct can_priv *priv = netdev_priv(dev);
@@ -318,7 +489,9 @@  static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
 
 	    (nla_put(skb, IFLA_CAN_BITRATE_MAX,
 		     sizeof(priv->bitrate_max),
-		     &priv->bitrate_max))
+		     &priv->bitrate_max)) ||
+
+	    (can_tdc_fill_info(skb, dev))
 	    )
 
 		return -EMSGSIZE;
diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
index 004cd09a7d49..75b85c60efb2 100644
--- a/include/uapi/linux/can/netlink.h
+++ b/include/uapi/linux/can/netlink.h
@@ -136,10 +136,35 @@  enum {
 	IFLA_CAN_BITRATE_CONST,
 	IFLA_CAN_DATA_BITRATE_CONST,
 	IFLA_CAN_BITRATE_MAX,
-	__IFLA_CAN_MAX
+	IFLA_CAN_TDC,
+
+	/* add new constants above here */
+	__IFLA_CAN_MAX,
+	IFLA_CAN_MAX = __IFLA_CAN_MAX - 1
 };
 
-#define IFLA_CAN_MAX	(__IFLA_CAN_MAX - 1)
+/*
+ * CAN FD Transmitter Delay Compensation (TDC)
+ *
+ * Please refer to struct can_tdc_const and can_tdc in
+ * include/linux/can/bittiming.h for further details.
+ */
+enum {
+	IFLA_CAN_TDC_UNSPEC,
+	IFLA_CAN_TDC_TDCV_MIN,	/* u32 */
+	IFLA_CAN_TDC_TDCV_MAX,	/* u32 */
+	IFLA_CAN_TDC_TDCO_MIN,	/* u32 */
+	IFLA_CAN_TDC_TDCO_MAX,	/* u32 */
+	IFLA_CAN_TDC_TDCF_MIN,	/* u32 */
+	IFLA_CAN_TDC_TDCF_MAX,	/* u32 */
+	IFLA_CAN_TDC_TDCV,	/* u32 */
+	IFLA_CAN_TDC_TDCO,	/* u32 */
+	IFLA_CAN_TDC_TDCF,	/* u32 */
+
+	/* add new constants above here */
+	__IFLA_CAN_TDC,
+	IFLA_CAN_TDC_MAX = __IFLA_CAN_TDC - 1
+};
 
 /* u16 termination range: 1..65535 Ohms */
 #define CAN_TERMINATION_DISABLED 0