[PATCH 0/2] Stop showing data bittiming if FD is turned off

[PATCH 0/2] Stop showing data bittiming if FD is turned off

[Vincent Mailhol]

While doing tests on TDC, I witnessed a strange behavior: the data
bittiming are still displayed after turning fd to off.

$ ip link set can0 type can bitrate 500000 dbitrate 2000000 fd on
$ ip link set can0 type can bitrate 500000 fd off
$ ip --details link show can0
1:  can0: <NOARP,ECHO> mtu 16 qdisc pfifo_fast state DOWN mode DEFAULT group default qlen 10
    link/can  promiscuity 0 minmtu 0 maxmtu 0 
    can state STOPPED restart-ms 0 
	  bitrate 500000 sample-point 0.875
	  tq 12 prop-seg 69 phase-seg1 70phase-seg2 20  sjw 1 brp 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 dbrp 1
	  tdcv 0 tdco 30 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 

This series comports two patches: one for the data bittiming parameters, one of the tdc parameters.

If possible, I wish to have the second patch squashed in the netlink
TDC series. I put comments after the --- cutter.


Vincent Mailhol (2):
  can: netlink: clear data_bittiming if fd is turned off
  can: netlink: clear tdc if fd is turned off

 drivers/net/can/dev/netlink.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

-- 
2.31.1

[PATCH 1/2] can: netlink: clear data_bittiming if fd is turned off

[Vincent Mailhol]

When the FD is turned off through the netlink interface, the value
still remain in data_bittiming and are displayed despite of the
feature being disabled.

Example:

$ ip link set can0 type can bitrate 500000 dbitrate 2000000 fd on
$ ip --details link show can0
1:  can0: <NOARP,ECHO> mtu 72 qdisc pfifo_fast state DOWN mode DEFAULT group default qlen 10
    link/can  promiscuity 0 minmtu 0 maxmtu 0
    can <FD> state STOPPED restart-ms 0
	  bitrate 500000 sample-point 0.875
	  tq 12 prop-seg 69 phase-seg1 70 phase-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 dbrp-inc 1
	  clock 80000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535

$ ip link set can0 type can bitrate 500000 fd off
$ ip --details link show can0
1:  can0: <NOARP,ECHO> mtu 16 qdisc pfifo_fast state DOWN mode DEFAULT group default qlen 10
    link/can  promiscuity 0 minmtu 0 maxmtu 0
    can state STOPPED restart-ms 0
	  bitrate 500000 sample-point 0.875
	  tq 12 prop-seg 69 phase-seg1 70 phase-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 dbrp-inc 1
	  clock 80000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535

Remark: once FD is turned off, it is not possible to just turn fd back
on and reuse the previously input data bittiming values:

$ ip link set can0 type can bitrate 500000 fd on
RTNETLINK answers: Operation not supported

This means that the user will need to overwrite data bittiming with
fresh values in order to turn fd on again.

Because old data bittiming values can not be reused, this patch just
clears priv->data_bittiming whenever FD is turned off. This way, the
data bittiming variables are not displayed anymore.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
Hi Marc,

I suggest to rebase this patch before the netlink TDC series.
---
 drivers/net/can/dev/netlink.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index e8c38d0df626..b8d531e49540 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -201,10 +201,13 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 		priv->ctrlmode |= maskedflags;
 
 		/* CAN_CTRLMODE_FD can only be set when driver supports FD */
-		if (priv->ctrlmode & CAN_CTRLMODE_FD)
+		if (priv->ctrlmode & CAN_CTRLMODE_FD) {
 			dev->mtu = CANFD_MTU;
-		else
+		} else {
 			dev->mtu = CAN_MTU;
+			memset(&priv->data_bittiming, 0,
+			       sizeof(priv->data_bittiming));
+		}
 	}
 
 	if (data[IFLA_CAN_RESTART_MS]) {
-- 
2.31.1

[PATCH 2/2] can: netlink: clear tdc if fd is turned off

[Vincent Mailhol]

When the FD is turned off through the netlink interface, the value
still remain in struct can_tdc and are displayed despite of the
feature being disabled.

This patch clears priv->tdc whenever FD is set to off. This way, the
TDC variables are not displayed anymore.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
Hi Marc,

I would suggest to squash that into commit 5eb2cd8e2ded ("can:
netlink: add interface for CAN-FD Transmitter Delay Compensation
(TDC)").
---
 drivers/net/can/dev/netlink.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index b8d531e49540..b33e6da6ca5a 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -207,6 +207,7 @@ 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));
 		}
 	}
 
-- 
2.31.1

Re: [PATCH 1/2] can: netlink: clear data_bittiming if fd is turned off

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 3069 bytes --]

On 18.06.2021 17:19:03, Vincent Mailhol wrote:
> When the FD is turned off through the netlink interface, the value

values

> still remain in data_bittiming and are displayed despite of the
> feature being disabled.
> 
> Example:
> 
> $ ip link set can0 type can bitrate 500000 dbitrate 2000000 fd on
> $ ip --details link show can0
> 1:  can0: <NOARP,ECHO> mtu 72 qdisc pfifo_fast state DOWN mode DEFAULT group default qlen 10
>     link/can  promiscuity 0 minmtu 0 maxmtu 0
>     can <FD> state STOPPED restart-ms 0
> 	  bitrate 500000 sample-point 0.875
> 	  tq 12 prop-seg 69 phase-seg1 70 phase-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 dbrp-inc 1
> 	  clock 80000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535
> 
> $ ip link set can0 type can bitrate 500000 fd off
> $ ip --details link show can0
> 1:  can0: <NOARP,ECHO> mtu 16 qdisc pfifo_fast state DOWN mode DEFAULT group default qlen 10
>     link/can  promiscuity 0 minmtu 0 maxmtu 0
>     can state STOPPED restart-ms 0
> 	  bitrate 500000 sample-point 0.875
> 	  tq 12 prop-seg 69 phase-seg1 70 phase-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 dbrp-inc 1
> 	  clock 80000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535
> 
> Remark: once FD is turned off, it is not possible to just turn fd back
> on and reuse the previously input data bittiming values:

> $ ip link set can0 type can bitrate 500000 fd on
> RTNETLINK answers: Operation not supported
> 
> This means that the user will need to overwrite data bittiming with
                                        ^^^^^^^^^
set

At least with your change it's more a "set again" than to overwrite.

> fresh values in order to turn fd on again.
> 
> Because old data bittiming values can not be reused, this patch just
> clears priv->data_bittiming whenever FD is turned off. This way, the
> data bittiming variables are not displayed anymore.
> 
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> ---
> Hi Marc,
> 
> I suggest to rebase this patch before the netlink TDC series.

makes sense - If you're OK with the changes, I'll add them while
applying.

Your patch makes the interface consistent, another option would be to
allow FD mode if the data bit timing values have been set before.
Opinions?

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 --]

Re: [PATCH 2/2] can: netlink: clear tdc if fd is turned off

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 871 bytes --]

On 18.06.2021 17:19:04, Vincent Mailhol wrote:
> When the FD is turned off through the netlink interface, the value

values

> still remain in struct can_tdc and are displayed despite of the
> feature being disabled.
> 
> This patch clears priv->tdc whenever FD is set to off. This way, the
> TDC variables are not displayed anymore.
> 
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> ---
> Hi Marc,
> 
> I would suggest to squash that into commit 5eb2cd8e2ded ("can:
> netlink: add interface for CAN-FD Transmitter Delay Compensation
> (TDC)").

Will do.

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 --]

Re: [PATCH 1/2] can: netlink: clear data_bittiming if fd is turned off

[Vincent MAILHOL]

On Fri. 18 juin 2021 at 18:10, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> On 18.06.2021 17:19:03, Vincent Mailhol wrote:
> > When the FD is turned off through the netlink interface, the value
>
> values
>
> > still remain in data_bittiming and are displayed despite of the
> > feature being disabled.
> >
> > Example:
> >
> > $ ip link set can0 type can bitrate 500000 dbitrate 2000000 fd on
> > $ ip --details link show can0
> > 1:  can0: <NOARP,ECHO> mtu 72 qdisc pfifo_fast state DOWN mode DEFAULT group default qlen 10
> >     link/can  promiscuity 0 minmtu 0 maxmtu 0
> >     can <FD> state STOPPED restart-ms 0
> >         bitrate 500000 sample-point 0.875
> >         tq 12 prop-seg 69 phase-seg1 70 phase-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 dbrp-inc 1
> >         clock 80000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535
> >
> > $ ip link set can0 type can bitrate 500000 fd off
> > $ ip --details link show can0
> > 1:  can0: <NOARP,ECHO> mtu 16 qdisc pfifo_fast state DOWN mode DEFAULT group default qlen 10
> >     link/can  promiscuity 0 minmtu 0 maxmtu 0
> >     can state STOPPED restart-ms 0
> >         bitrate 500000 sample-point 0.875
> >         tq 12 prop-seg 69 phase-seg1 70 phase-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 dbrp-inc 1
> >         clock 80000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535
> >
> > Remark: once FD is turned off, it is not possible to just turn fd back
> > on and reuse the previously input data bittiming values:
>
> > $ ip link set can0 type can bitrate 500000 fd on
> > RTNETLINK answers: Operation not supported
> >
> > This means that the user will need to overwrite data bittiming with
>                                         ^^^^^^^^^
> set
>
> At least with your change it's more a "set again" than to overwrite.
>
> > fresh values in order to turn fd on again.
> >
> > Because old data bittiming values can not be reused, this patch just
> > clears priv->data_bittiming whenever FD is turned off. This way, the
> > data bittiming variables are not displayed anymore.
> >
> > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > ---
> > Hi Marc,
> >
> > I suggest to rebase this patch before the netlink TDC series.
>
> makes sense - If you're OK with the changes, I'll add them while
> applying.

I am OK with the changes.

> Your patch makes the interface consistent, another option would be to
> allow FD mode if the data bit timing values have been set before.
> Opinions?

One part of me says it would make sense but let's also try to be
realistic. The only reason I spotted this issue is because I was
actively trying to find defects while testing my TDC
implementation. Under normal usage, I never had have such needs.

How many people will need to set fd on, then off, then on again?
Too few I think. Let's force those few people to always provide
the data bittiming values. The overhead which would be needed to
implement this in drivers/net/can/dev/netlink.c is just not worth it.

> 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 |