Mailing List Archive

[PATCH v2 2/2] can: netlink: add interface for CAN-FD Transmitter Delay Compensation (TDC)
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_MAX
IFLA_CAN_TDC_TDCO_MAX
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 is the third and last one to introduce TDC in the
kernel. It 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 | 138 ++++++++++++++++++++++++++++++-
include/uapi/linux/can/netlink.h | 26 +++++-
2 files changed, 159 insertions(+), 5 deletions(-)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 32c603a09809..6134bbf69c10 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -19,6 +19,16 @@ 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_MAX] = { .type = NLA_U32 },
+ [IFLA_CAN_TDC_TDCO_MAX] = { .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 +56,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,6 +64,65 @@ 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)
+ 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 (tdcv && !tdc_const->tdcv_max)
+ return -EOPNOTSUPP;
+
+ if (tdcv > tdc_const->tdcv_max)
+ return -EINVAL;
+
+ tdc->tdcv = tdcv;
+ }
+
+ if (tb[IFLA_CAN_TDC_TDCO]) {
+ u32 tdco = nla_get_u32(tb[IFLA_CAN_TDC_TDCO]);
+
+ if (tdco && !tdc_const->tdco_max)
+ return -EOPNOTSUPP;
+
+ if (tdco > tdc_const->tdco_max)
+ return -EINVAL;
+
+ tdc->tdco = tdco;
+ }
+
+ if (tb[IFLA_CAN_TDC_TDCF]) {
+ u32 tdcf = nla_get_u32(tb[IFLA_CAN_TDC_TDCF]);
+
+ if (tdcf && !tdc_const->tdcf_max)
+ return -EOPNOTSUPP;
+
+ if (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)
@@ -220,9 +289,37 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
priv->termination = termval;
}

+ if (data[IFLA_CAN_TDC]) {
+ err = can_tdc_changelink(dev, data[IFLA_CAN_TDC], extack);
+ if (err)
+ return err;
+ }
+
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 */
+ size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCV_MAX */
+ size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCO_MAX */
+ size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCF_MAX */
+
+ if (priv->tdc.tdco) {
+ size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCV */
+ size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCO */
+ 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);
@@ -254,10 +351,43 @@ 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 (nla_put_u32(skb, IFLA_CAN_TDC_TDCV_MAX, tdc_const->tdcv_max) ||
+ nla_put_u32(skb, IFLA_CAN_TDC_TDCO_MAX, tdc_const->tdco_max) ||
+ nla_put_u32(skb, IFLA_CAN_TDC_TDCF_MAX, tdc_const->tdcf_max))
+ goto err_cancel;
+
+ if (priv->tdc.tdco)
+ if (nla_put_u32(skb, IFLA_CAN_TDC_TDCV, tdc->tdcv) ||
+ nla_put_u32(skb, IFLA_CAN_TDC_TDCO, tdc->tdco) ||
+ 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);
@@ -315,7 +445,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 f730d443b918..3e81cad069a1 100644
--- a/include/uapi/linux/can/netlink.h
+++ b/include/uapi/linux/can/netlink.h
@@ -134,10 +134,32 @@ 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_MAX, /* u32 */
+ IFLA_CAN_TDC_TDCO_MAX, /* 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
--
2.31.1
Re: [PATCH v2 2/2] can: netlink: add interface for CAN-FD Transmitter Delay Compensation (TDC) [ In reply to ]
On 04.06.2021 00:15:50, Vincent Mailhol wrote:
[...]

> +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 */
> + size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCV_MAX */
> + size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCO_MAX */
> + size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCF_MAX */
> +
> + if (priv->tdc.tdco) {

Naively I'd say, iff the device has tdc_const give the user space the
tdc parameters, regardless if some value is 0 or not.

What do you think?

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 |
Re: [PATCH v2 2/2] can: netlink: add interface for CAN-FD Transmitter Delay Compensation (TDC) [ In reply to ]
On Wed. 16 Jun 2021 at 18:46, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 04.06.2021 00:15:50, Vincent Mailhol wrote:
> [...]
>
> > +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 */
> > + size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCV_MAX */
> > + size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCO_MAX */
> > + size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCF_MAX */
> > +
> > + if (priv->tdc.tdco) {
>
> Naively I'd say, iff the device has tdc_const give the user space the
> tdc parameters, regardless if some value is 0 or not.
>
> What do you think?

I thought about that.
The first important remark is that if tdc.tdco is zero, then TDC
is off (c.f. documentation of struct can_tdc::tdco).

Let me illustrate my vision through examples.


** Case 1: link is not configured at all. **
Here, only the constant values are displayed.

# ip --details link show can0
1: can0: <NOARP,ECHO> mtu 16 qdisc noop state DOWN mode DEFAULT group
default qlen 10
link/can promiscuity 0 minmtu 0 maxmtu 0
can state STOPPED (berr-counter tx 0 rx 0) restart-ms 0
ES582.1/ES584.1: tseg1 2..256 tseg2 2..128 sjw 1..128 brp 1..512 brp_inc 1
ES582.1/ES584.1: dtseg1 2..32 dtseg2 1..16 dsjw 1..8 dbrp 1..32 brp_inc 1
tdcv_max 0 tdco_max 127 tdcf_max 127
clock 80000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536
gso_max_segs 65535


** Case 2: only the nominal bitrate is configured. **
The data bittiming variables (including TDC) are not shown.

# ip --details link show can0
1: can0: <NOARP,ECHO> mtu 16 qdisc noop state DOWN mode DEFAULT group
default qlen 10
link/can promiscuity 0 minmtu 0 maxmtu 0
can state STOPPED (berr-counter tx 0 rx 0) restart-ms 0
bitrate 500000 sample-point 0.875
tq 12 prop-seg 69 phase-seg1 70phase-seg2 20 sjw 1
ES582.1/ES584.1: tseg1 2..256 tseg2 2..128 sjw 1..128 brp 1..512 brp_inc 1
ES582.1/ES584.1: dtseg1 2..32 dtseg2 1..16 dsjw 1..8 dbrp 1..32 brp_inc 1
tdcv_max 0 tdco_max 127 tdcf_max 127
clock 80000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536
gso_max_segs 65535


** Case 3: both nominal and data bitrates are configured (but not TDC). **
Only the TDC variables are not shown.

# ip --details link show can0
1: can0: <NOARP,ECHO> mtu 72 qdisc noop state DOWN mode DEFAULT group
default qlen 10
link/can promiscuity 0 minmtu 0 maxmtu 0
can <FD> state STOPPED (berr-counter tx 0 rx 0) restart-ms 0
bitrate 500000 sample-point 0.875
tq 12 prop-seg 69 phase-seg1 70phase-seg2 20 sjw 1
ES582.1/ES584.1: tseg1 2..256 tseg2 2..128 sjw 1..128 brp 1..512 brp_inc 1
dbitrate 2000000 dsample-point 0.750
dtq 12 dprop-seg 14 dphase-seg1 15 dphase-seg2 10 dsjw 1
ES582.1/ES584.1: dtseg1 2..32 dtseg2 1..16 dsjw 1..8 dbrp 1..32 brp_inc 1
tdcv_max 0 tdco_max 127 tdcf_max 127
clock 80000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536
gso_max_segs 65535


** Case 4: nominal and data bitrates and TDC are configured. **
Everything is shown.

# ip --details link show can0
1: can0: <NOARP,ECHO> mtu 72 qdisc noop state DOWN mode DEFAULT group
default qlen 10
link/can promiscuity 0 minmtu 0 maxmtu 0
can <FD> state STOPPED (berr-counter tx 0 rx 0) restart-ms 0
bitrate 1000000 sample-point 0.750
tq 12 prop-seg 29 phase-seg1 30phase-seg2 20 sjw 1
ES582.1/ES584.1: tseg1 2..256 tseg2 2..128 sjw 1..128 brp 1..512 brp_inc 1
dbitrate 5000000 dsample-point 0.750
dtq 12 dprop-seg 5 dphase-seg1 6 dphase-seg2 4 dsjw 1
tdcv 0 tdco 12 tdcf 0
ES582.1/ES584.1: dtseg1 2..32 dtseg2 1..16 dsjw 1..8 dbrp 1..32 brp_inc 1
tdcv_max 0 tdco_max 127 tdcf_max 127
clock 80000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536
gso_max_segs 65535


I think that we can agree on Cases 1, 2 (it would not make sense
to display TDC without the data bittiming variables) and 4.

The edge case is Case 3. It depends if we consider the TDC as a
separate set or not. It is not silly to display the TDC whenever
fd is on. I prefer to keep it the way I did it. But I would not
object to changing this if you insist. That would mean:
- if (priv->tdc.tdco) {
+ if (priv->data_bittiming.bitrate) {


Finally, I have one side comment. It seems to me that you did not
understand that the intent of
| if (priv->tdc.tdco)
was to actually check whether TDC was on or off. In other words, my
code was unclear.

I am now thinking to introduce an helper macro:
static bool can_tdc_is_enabled(const struct can_priv *priv)
|{
| return !!priv->tdc.tdco;
|}

The code would look more clear like that.
- if (priv->tdc.tdco) {
+ if (can_tdc_is_enabled(priv) {


Yours sincerely,
Vincent
Re: [PATCH v2 2/2] can: netlink: add interface for CAN-FD Transmitter Delay Compensation (TDC) [ In reply to ]
On 16.06.2021 22:53:02, Vincent MAILHOL wrote:
> On Wed. 16 Jun 2021 at 18:46, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > On 04.06.2021 00:15:50, Vincent Mailhol wrote:
> > [...]
> >
> > > +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 */
> > > + size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCV_MAX */
> > > + size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCO_MAX */
> > > + size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCF_MAX */
> > > +
> > > + if (priv->tdc.tdco) {
> >
> > Naively I'd say, iff the device has tdc_const give the user space the
> > tdc parameters, regardless if some value is 0 or not.
> >
> > What do you think?
>
> I thought about that.
> The first important remark is that if tdc.tdco is zero, then TDC
> is off (c.f. documentation of struct can_tdc::tdco).
>
> Let me illustrate my vision through examples.

[...]

examples makes sense \o/

[...]

> Finally, I have one side comment. It seems to me that you did not
> understand that the intent of
> | if (priv->tdc.tdco)
> was to actually check whether TDC was on or off. In other words, my
> code was unclear.
>
> I am now thinking to introduce an helper macro:
> static bool can_tdc_is_enabled(const struct can_priv *priv)
> |{
> | return !!priv->tdc.tdco;
> |}
>
> The code would look more clear like that.
> - if (priv->tdc.tdco) {
> + if (can_tdc_is_enabled(priv) {

Sounds good, I'm squashing this patch:

| diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
| index 6134bbf69c10..d48be574eae7 100644
| --- a/drivers/net/can/dev/netlink.c
| +++ b/drivers/net/can/dev/netlink.c
| @@ -311,7 +311,7 @@ static size_t can_tdc_get_size(const struct net_device *dev)
| size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCO_MAX */
| size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCF_MAX */
|
| - if (priv->tdc.tdco) {
| + if (can_tdc_is_enabled(priv)) {
| size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCV */
| size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCO */
| size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCF */
| @@ -352,6 +352,7 @@ static size_t can_get_size(const struct net_device *dev)
| 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;
| }
|
| @@ -374,7 +375,7 @@ static int can_tdc_fill_info(struct sk_buff *skb, const struct net_device *dev)
| nla_put_u32(skb, IFLA_CAN_TDC_TDCF_MAX, tdc_const->tdcf_max))
| goto err_cancel;
|
| - if (priv->tdc.tdco)
| + if (can_tdc_is_enabled(priv)) {
| if (nla_put_u32(skb, IFLA_CAN_TDC_TDCV, tdc->tdcv) ||
| nla_put_u32(skb, IFLA_CAN_TDC_TDCO, tdc->tdco) ||
| nla_put_u32(skb, IFLA_CAN_TDC_TDCF, tdc->tdcf))
| diff --git a/include/linux/can/bittiming.h b/include/linux/can/bittiming.h
| index 9de6e9053e34..b6d1db1e7258 100644
| --- a/include/linux/can/bittiming.h
| +++ b/include/linux/can/bittiming.h
| @@ -83,6 +83,11 @@ struct can_tdc_const {
| u32 tdcf_max;
| };
|
| +static inline bool can_tdc_is_enabled(const struct can_priv *priv)
| +{
| + return !!priv->tdc.tdco;
| +}
| +
| #ifdef CONFIG_CAN_CALC_BITTIMING
| int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
| const struct can_bittiming_const *btc);

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 |
Re: [PATCH v2 2/2] can: netlink: add interface for CAN-FD Transmitter Delay Compensation (TDC) [ In reply to ]
On Wed. 16 Jun 2021 à 23:29, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 16.06.2021 22:53:02, Vincent MAILHOL wrote:
> > On Wed. 16 Jun 2021 at 18:46, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > > On 04.06.2021 00:15:50, Vincent Mailhol wrote:
> > > [...]
> > >
> > > > +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 */
> > > > + size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCV_MAX */
> > > > + size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCO_MAX */
> > > > + size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCF_MAX */
> > > > +
> > > > + if (priv->tdc.tdco) {
> > >
> > > Naively I'd say, iff the device has tdc_const give the user space the
> > > tdc parameters, regardless if some value is 0 or not.
> > >
> > > What do you think?
> >
> > I thought about that.
> > The first important remark is that if tdc.tdco is zero, then TDC
> > is off (c.f. documentation of struct can_tdc::tdco).
> >
> > Let me illustrate my vision through examples.
>
> [...]
>
> examples makes sense \o/

Great!

> [...]
>
> > Finally, I have one side comment. It seems to me that you did not
> > understand that the intent of
> > | if (priv->tdc.tdco)
> > was to actually check whether TDC was on or off. In other words, my
> > code was unclear.
> >
> > I am now thinking to introduce an helper macro:
> > static bool can_tdc_is_enabled(const struct can_priv *priv)
> > |{
> > | return !!priv->tdc.tdco;
> > |}
> >
> > The code would look more clear like that.
> > - if (priv->tdc.tdco) {
> > + if (can_tdc_is_enabled(priv) {
>
> Sounds good, I'm squashing this patch:
>
> | diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
> | index 6134bbf69c10..d48be574eae7 100644
> | --- a/drivers/net/can/dev/netlink.c
> | +++ b/drivers/net/can/dev/netlink.c
> | @@ -311,7 +311,7 @@ static size_t can_tdc_get_size(const struct net_device *dev)
> | size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCO_MAX */
> | size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCF_MAX */
> |
> | - if (priv->tdc.tdco) {
> | + if (can_tdc_is_enabled(priv)) {
> | size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCV */
> | size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCO */
> | size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCF */
> | @@ -352,6 +352,7 @@ static size_t can_get_size(const struct net_device *dev)
> | 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;
> | }
> |
> | @@ -374,7 +375,7 @@ static int can_tdc_fill_info(struct sk_buff *skb, const struct net_device *dev)
> | nla_put_u32(skb, IFLA_CAN_TDC_TDCF_MAX, tdc_const->tdcf_max))
> | goto err_cancel;
> |
> | - if (priv->tdc.tdco)
> | + if (can_tdc_is_enabled(priv)) {
> | if (nla_put_u32(skb, IFLA_CAN_TDC_TDCV, tdc->tdcv) ||
> | nla_put_u32(skb, IFLA_CAN_TDC_TDCO, tdc->tdco) ||
> | nla_put_u32(skb, IFLA_CAN_TDC_TDCF, tdc->tdcf))
> | diff --git a/include/linux/can/bittiming.h b/include/linux/can/bittiming.h
> | index 9de6e9053e34..b6d1db1e7258 100644
> | --- a/include/linux/can/bittiming.h
> | +++ b/include/linux/can/bittiming.h
> | @@ -83,6 +83,11 @@ struct can_tdc_const {
> | u32 tdcf_max;
> | };
> |
> | +static inline bool can_tdc_is_enabled(const struct can_priv *priv)

Did you try to compile? I am not sure if bittiming.h is able to
see struct can_priv which is defined in dev.h.


Yours sincerely,
Vincent

> | +{
> | + return !!priv->tdc.tdco;
> | +}
> | +
> | #ifdef CONFIG_CAN_CALC_BITTIMING
> | int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
> | const struct can_bittiming_const *btc);
Re: [PATCH v2 2/2] can: netlink: add interface for CAN-FD Transmitter Delay Compensation (TDC) [ In reply to ]
On 16.06.2021 23:43:35, Vincent MAILHOL wrote:
> > Sounds good, I'm squashing this patch:
> >
> > | diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
> > | index 6134bbf69c10..d48be574eae7 100644
> > | --- a/drivers/net/can/dev/netlink.c
> > | +++ b/drivers/net/can/dev/netlink.c
> > | @@ -311,7 +311,7 @@ static size_t can_tdc_get_size(const struct net_device *dev)
> > | size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCO_MAX */
> > | size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCF_MAX */
> > |
> > | - if (priv->tdc.tdco) {
> > | + if (can_tdc_is_enabled(priv)) {
> > | size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCV */
> > | size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCO */
> > | size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCF */
> > | @@ -352,6 +352,7 @@ static size_t can_get_size(const struct net_device *dev)
> > | 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;
> > | }
> > |
> > | @@ -374,7 +375,7 @@ static int can_tdc_fill_info(struct sk_buff *skb, const struct net_device *dev)
> > | nla_put_u32(skb, IFLA_CAN_TDC_TDCF_MAX, tdc_const->tdcf_max))
> > | goto err_cancel;
> > |
> > | - if (priv->tdc.tdco)
> > | + if (can_tdc_is_enabled(priv)) {
> > | if (nla_put_u32(skb, IFLA_CAN_TDC_TDCV, tdc->tdcv) ||
> > | nla_put_u32(skb, IFLA_CAN_TDC_TDCO, tdc->tdco) ||
> > | nla_put_u32(skb, IFLA_CAN_TDC_TDCF, tdc->tdcf))
> > | diff --git a/include/linux/can/bittiming.h b/include/linux/can/bittiming.h
> > | index 9de6e9053e34..b6d1db1e7258 100644
> > | --- a/include/linux/can/bittiming.h
> > | +++ b/include/linux/can/bittiming.h
> > | @@ -83,6 +83,11 @@ struct can_tdc_const {
> > | u32 tdcf_max;
> > | };
> > |
> > | +static inline bool can_tdc_is_enabled(const struct can_priv *priv)
>
> Did you try to compile?

Not before sending that mail :)

> I am not sure if bittiming.h is able to see struct can_priv which is
> defined in dev.h.

Nope it doesn't, I moved the can_tdc_is_enabled() to
include/linux/can/dev.h

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 |
Re: [PATCH v2 2/2] can: netlink: add interface for CAN-FD Transmitter Delay Compensation (TDC) [ In reply to ]
On Wed. 16 Jun 2021 at 23:46, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 16.06.2021 23:43:35, Vincent MAILHOL wrote:
> > > Sounds good, I'm squashing this patch:
> > >
> > > | diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
> > > | index 6134bbf69c10..d48be574eae7 100644
> > > | --- a/drivers/net/can/dev/netlink.c
> > > | +++ b/drivers/net/can/dev/netlink.c
> > > | @@ -311,7 +311,7 @@ static size_t can_tdc_get_size(const struct net_device *dev)
> > > | size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCO_MAX */
> > > | size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCF_MAX */
> > > |
> > > | - if (priv->tdc.tdco) {
> > > | + if (can_tdc_is_enabled(priv)) {
> > > | size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCV */
> > > | size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCO */
> > > | size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCF */
> > > | @@ -352,6 +352,7 @@ static size_t can_get_size(const struct net_device *dev)
> > > | 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;
> > > | }
> > > |
> > > | @@ -374,7 +375,7 @@ static int can_tdc_fill_info(struct sk_buff *skb, const struct net_device *dev)
> > > | nla_put_u32(skb, IFLA_CAN_TDC_TDCF_MAX, tdc_const->tdcf_max))
> > > | goto err_cancel;
> > > |
> > > | - if (priv->tdc.tdco)
> > > | + if (can_tdc_is_enabled(priv)) {
> > > | if (nla_put_u32(skb, IFLA_CAN_TDC_TDCV, tdc->tdcv) ||
> > > | nla_put_u32(skb, IFLA_CAN_TDC_TDCO, tdc->tdco) ||
> > > | nla_put_u32(skb, IFLA_CAN_TDC_TDCF, tdc->tdcf))
> > > | diff --git a/include/linux/can/bittiming.h b/include/linux/can/bittiming.h
> > > | index 9de6e9053e34..b6d1db1e7258 100644
> > > | --- a/include/linux/can/bittiming.h
> > > | +++ b/include/linux/can/bittiming.h
> > > | @@ -83,6 +83,11 @@ struct can_tdc_const {
> > > | u32 tdcf_max;
> > > | };
> > > |
> > > | +static inline bool can_tdc_is_enabled(const struct can_priv *priv)
> >
> > Did you try to compile?
>
> Not before sending that mail :)
>
> > I am not sure if bittiming.h is able to see struct can_priv which is
> > defined in dev.h.
>
> Nope it doesn't, I moved the can_tdc_is_enabled() to
> include/linux/can/dev.h

Ack. It seems to be the only solution…

Moving forward, I will do one more round of tests and send the
patch for iproute2-next (warning, the RFC I sent last month has
some issues, if you wish to test it on your side, please wait).

I will also apply can_tdc_is_enabled() to the etas_es58x driver.

Could you push the recent changes on the testing branch of linux-can-next? It
would be really helpful for me!


Yours sincerely,
Vincent
Re: [PATCH v2 2/2] can: netlink: add interface for CAN-FD Transmitter Delay Compensation (TDC) [ In reply to ]
On 17.06.2021 00:44:12, Vincent MAILHOL wrote:
> > > Did you try to compile?
> >
> > Not before sending that mail :)
> >
> > > I am not sure if bittiming.h is able to see struct can_priv which is
> > > defined in dev.h.
> >
> > Nope it doesn't, I moved the can_tdc_is_enabled() to
> > include/linux/can/dev.h
>
> Ack. It seems to be the only solution…
>
> Moving forward, I will do one more round of tests and send the
> patch for iproute2-next (warning, the RFC I sent last month has
> some issues, if you wish to test it on your side, please wait).
>
> I will also apply can_tdc_is_enabled() to the etas_es58x driver.
>
> Could you push the recent changes on the testing branch of linux-can-next? It
> would be really helpful for me!

done

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 |
Re: [PATCH v2 2/2] can: netlink: add interface for CAN-FD Transmitter Delay Compensation (TDC) [ In reply to ]
On 04.06.2021 00:15:50, Vincent Mailhol wrote:
> 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_MAX
> IFLA_CAN_TDC_TDCO_MAX
> IFLA_CAN_TDC_TDCF_MAX
>
> For struct can_tdc, these are:
> IFLA_CAN_TDC_TDCV
> IFLA_CAN_TDC_TDCO
> IFLA_CAN_TDC_TDCF

I just noticed in the mcp2518fd data sheet:

| bit 14-8 TDCO[6:0]: Transmitter Delay Compensation Offset bits;
| Secondary Sample Point (SSP) Two’s complement; offset can be positive,
| zero, or negative.
|
| 011 1111 = 63 x TSYSCLK
| ...
| 000 0000 = 0 x TSYSCLK
| ...
| 111 1111 = –64 x TSYSCLK

Have you takes this into account?

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 |
Re: [PATCH v2 2/2] can: netlink: add interface for CAN-FD Transmitter Delay Compensation (TDC) [ In reply to ]
On Fri. 18 Jun 2021 at 18:34, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 04.06.2021 00:15:50, Vincent Mailhol wrote:
> > 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_MAX
> > IFLA_CAN_TDC_TDCO_MAX
> > IFLA_CAN_TDC_TDCF_MAX
> >
> > For struct can_tdc, these are:
> > IFLA_CAN_TDC_TDCV
> > IFLA_CAN_TDC_TDCO
> > IFLA_CAN_TDC_TDCF
>
> I just noticed in the mcp2518fd data sheet:
>
> | bit 14-8 TDCO[6:0]: Transmitter Delay Compensation Offset bits;
> | Secondary Sample Point (SSP) Two’s complement; offset can be positive,
> | zero, or negative.
> |
> | 011 1111 = 63 x TSYSCLK
> | ...
> | 000 0000 = 0 x TSYSCLK
> | ...
> | 111 1111 = –64 x TSYSCLK
>
> Have you takes this into account?

I have not. And I fail to understand what would be the physical
meaning if TDCO is zero or negative.

TDCV indicates the position of the bit start on the RX pin. If
TDCO is zero, the measurement occurs on the bit start when all
the ringing occurs. That is a really bad choice to do the
measurement. If it is negative, it means that you are measuring
the previous bit o_O !?

Maybe I am missing something but I just do not get it.

I believe you started to implement the mcp2518fd. Can you force a
zero and a negative value and tell me if the bus is stable?


Yours sincerely,
Vincent Mailhol
Re: [PATCH v2 2/2] can: netlink: add interface for CAN-FD Transmitter Delay Compensation (TDC) [ In reply to ]
On Fri. 18 Jun 2021 at 19:23, Vincent MAILHOL
<mailhol.vincent@wanadoo.fr> wrote:
>
> On Fri. 18 Jun 2021 at 18:34, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > On 04.06.2021 00:15:50, Vincent Mailhol wrote:
> > > 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_MAX
> > > IFLA_CAN_TDC_TDCO_MAX
> > > IFLA_CAN_TDC_TDCF_MAX
> > >
> > > For struct can_tdc, these are:
> > > IFLA_CAN_TDC_TDCV
> > > IFLA_CAN_TDC_TDCO
> > > IFLA_CAN_TDC_TDCF
> >
> > I just noticed in the mcp2518fd data sheet:
> >
> > | bit 14-8 TDCO[6:0]: Transmitter Delay Compensation Offset bits;
> > | Secondary Sample Point (SSP) Two’s complement; offset can be positive,
> > | zero, or negative.
> > |
> > | 011 1111 = 63 x TSYSCLK
> > | ...
> > | 000 0000 = 0 x TSYSCLK
> > | ...
> > | 111 1111 = –64 x TSYSCLK
> >
> > Have you takes this into account?
>
> I have not. And I fail to understand what would be the physical
> meaning if TDCO is zero or negative.
>
> TDCV indicates the position of the bit start on the RX pin. If
> TDCO is zero, the measurement occurs on the bit start when all
> the ringing occurs. That is a really bad choice to do the
> measurement. If it is negative, it means that you are measuring
> the previous bit o_O !?
>
> Maybe I am missing something but I just do not get it.
>
> I believe you started to implement the mcp2518fd. Can you force a
> zero and a negative value and tell me if the bus is stable?

Actually, ISO 11898-1 specifies that the "SSP position should be
at least 0 to 63 minimum time quanta". This means that we can
have SSP = TDCV + TDCO = 0. In my implementation, I used 0 as a
reserved value for TDCV and TDCO. To comply with the standard, I
now need to allow both TDCV and TDCO to be zero and add a new
field in struct tdc to manage the automatic/manual options.

That said, these zero values still make no sense to me. Why would
someone do the measurement on the bit edge?

Concerning the negative values, the ISO standard says nothing
about it. If you are using the automatic measurement, a negative
TDCO is impossible to use. TDCV is measured on every bit. When
the measurement is done, it is too late to subtract from it (or
maybe the mcp2518fd has a time machine built in?). If you are
using the manual mode for TDCV, just choose two positive values
so that TDCV + TDCO = SSF.