netdev - Re: [PATCH net] enetc: Advance the taprio base time in the future

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20201124210003.kyzqkaudfjl7q3dw@skbuf>
Date:   Tue, 24 Nov 2020 21:00:04 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     Po Liu <po.liu@....com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Vinicius Costa Gomes <vinicius.gomes@...el.com>
Subject: Re: [PATCH net] enetc: Advance the taprio base time in the future

On Tue, Nov 24, 2020 at 09:58:12AM -0800, Jakub Kicinski wrote:
> > This is the right way for calculation. For the ENETC,  hardware also
> > do the same calculation before send to Operation State Machine.
> > For some TSN IP, like Felix and DesignWare TSN in RT1170 and IMX8MP
> > require the basetime limite the range not less than the current time
> > 8 cycles, software may do calculation before setting to the
> > hardware.
> > Actually, I do suggest this calculation to sch_taprio.c, but I found
> > same calculation only for the TXTIME by taprio_get_start_time().
> > Which means:
> > If (currenttime < basetime)
> >        Admin_basetime = basetime;
> > Else
> >        Admin_basetime =  basetime + (n+1)* cycletime;
> > N is the minimal value which make Admin_basetime is larger than the
> > currenttime.
> >
> > User space never to get the current time. Just set a value as offset
> > OR future time user want.
> > For example: set basetime = 1000000ns, means he want time align to
> > 1000000ns, and on the other device, also set the basetime =
> > 1000000ns, then the two devices are aligned cycle.
> > If user want all the devices start at 11.24.2020 11:00 then set
> > basetime = 1606273200.0 s.
> >
> > > - the sja1105 offload does it via future_base_time()
> > > - the ocelot/felix offload does it via vsc9959_new_base_time()
> > >
> > > As for the obvious question: doesn't the hardware just "do the right thing"
> > > if passed a time in the past? I've tested and it doesn't look like it. I cannot
> >
> > So hardware already do calculation same way.
>
> So the patch is unnecessary? Or correct? Not sure what you're saying..

He's not saying the patch is unnecessary. What the enetc driver
currently does for the case where the base_time is zero is bogus anyway.

What Po is saying is that calling future_base_time() should not be
needed. Instead, he is suggesting we could program directly the
admin_conf->base_time into the hardware, which will do the right thing
as long as the driver doesn't mangle it in various ways, such as replace
the base_time with the current time.

And what I said in the commit message is that I've been there before and
there were some still apparent issues with the schedule's phase. I had
some issues at the application layer as well. In the meantime I sorted
those out, and after re-applying the simple kernel change and giving the
system some thorough testing, it looks like Po is right.