netdev - RE: [PATCH v3 net] enetc: Let the hardware auto-advance the taprio base-time of 0

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <VE1PR04MB6496784CA12CA642867F372A92FA0@VE1PR04MB6496.eurprd04.prod.outlook.com>
Date:   Wed, 25 Nov 2020 02:28:27 +0000
From:   Po Liu <po.liu@....com>
To:     Vladimir Oltean <vladimir.oltean@....com>,
        Jakub Kicinski <kuba@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Claudiu Manoil <claudiu.manoil@....com>
CC:     Vinicius Costa Gomes <vinicius.gomes@...el.com>
Subject: RE: [PATCH v3 net] enetc: Let the hardware auto-advance the taprio
 base-time of 0

It makes sense to me for this patch. Thanks!

> -----Original Message-----
> From: Vladimir Oltean <vladimir.oltean@....com>
> Sent: 2020年11月25日 6:03
> To: Jakub Kicinski <kuba@...nel.org>; netdev@...r.kernel.org; Po Liu
> <po.liu@....com>; Claudiu Manoil <claudiu.manoil@....com>
> Cc: Vinicius Costa Gomes <vinicius.gomes@...el.com>
> Subject: [PATCH v3 net] enetc: Let the hardware auto-advance the taprio
> base-time of 0
> 
> The tc-taprio base time indicates the beginning of the tc-taprio schedule,
> which is cyclic by definition (where the length of the cycle in nanoseconds
> is called the cycle time). The base time is a 64-bit PTP time in the TAI
> domain.
> 
> Logically, the base-time should be a future time. But that imposes some
> restrictions to user space, which has to retrieve the current PTP time from
> the NIC first, then calculate a base time that will still be larger than the
> base time by the time the kernel driver programs this value into the
> hardware. Actually ensuring that the programmed base time is in the
> future is still a problem even if the kernel alone deals with this.
> 
> Luckily, the enetc hardware already advances a base-time that is in the
> past into a congruent time in the immediate future, according to the same
> formula that can be found in the software implementation of taprio (in
> taprio_get_start_time):
> 
> 	/* Schedule the start time for the beginning of the next
> 	 * cycle.
> 	 */
> 	n = div64_s64(ktime_sub_ns(now, base), cycle);
> 	*start = ktime_add_ns(base, (n + 1) * cycle);
> 
> There's only one problem: the driver doesn't let the hardware do that.
> It interferes with the base-time passed from user space, by special-casing
> the situation when the base-time is zero, and replaces that with the
> current PTP time. This changes the intended effective base-time of the
> schedule, which will in the end have a different phase offset than if the
> base-time of 0.000000000 was to be advanced by an integer multiple of
> the cycle-time.
> 
> Fixes: 34c6adf1977b ("enetc: Configure the Time-Aware Scheduler via tc-
> taprio offload")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---
> Changes in v3:
> - Removed an obsolete phrase from commit message.
> 
> Changes in v2:
> - Now letting the hardware completely deal with advancing base times in
>   the past.
> 
>  drivers/net/ethernet/freescale/enetc/enetc_qos.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_qos.c
> b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
> index aeb21dc48099..a9aee219fb58 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_qos.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
> @@ -92,18 +92,8 @@ static int enetc_setup_taprio(struct net_device
> *ndev,
>  	gcl_config->atc = 0xff;
>  	gcl_config->acl_len = cpu_to_le16(gcl_len);
> 
> -	if (!admin_conf->base_time) {
> -		gcl_data->btl =
> -			cpu_to_le32(enetc_rd(&priv->si->hw,
> ENETC_SICTR0));
> -		gcl_data->bth =
> -			cpu_to_le32(enetc_rd(&priv->si->hw,
> ENETC_SICTR1));
> -	} else {
> -		gcl_data->btl =
> -			cpu_to_le32(lower_32_bits(admin_conf-
> >base_time));
> -		gcl_data->bth =
> -			cpu_to_le32(upper_32_bits(admin_conf-
> >base_time));
> -	}
> -
> +	gcl_data->btl = cpu_to_le32(lower_32_bits(admin_conf-
> >base_time));
> +	gcl_data->bth = cpu_to_le32(upper_32_bits(admin_conf-
> >base_time));
>  	gcl_data->ct = cpu_to_le32(admin_conf->cycle_time);
>  	gcl_data->cte = cpu_to_le32(admin_conf->cycle_time_extension);
> 
> --
> 2.25.1