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] [day] [month] [year] [list]

Date:   Tue, 24 Nov 2020 16:05:01 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Vladimir Oltean <vladimir.oltean@....com>
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, 24 Nov 2020 21:00:04 +0000 Vladimir Oltean wrote:
> 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.

Thanks for explaining!