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

From: Marc Kleine-Budde <mkl@pengutronix.de>
To: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Cc: linux-can@vger.kernel.org
Subject: Re: [PATCH 1/2] can: netlink: clear data_bittiming if fd is turned off
Date: Fri, 18 Jun 2021 11:10:15 +0200	[thread overview]
Message-ID: <20210618091015.kioemxe3j3cbx6qg@pengutronix.de> (raw)
In-Reply-To: <20210618081904.141114-2-mailhol.vincent@wanadoo.fr>

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