netdev - Re: [PATCH 4/6] net: ethernet: ti: cpts: add ptp pps support

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: <20161130221738.GA13099@localhost.localdomain>
Date:   Wed, 30 Nov 2016 23:17:38 +0100
From:   Richard Cochran <richardcochran@...il.com>
To:     Grygorii Strashko <grygorii.strashko@...com>
Cc:     Murali Karicheri <m-karicheri2@...com>,
        Wingman Kwok <w-kwok2@...com>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        Mugunthan V N <mugunthanvnm@...com>,
        Sekhar Nori <nsekhar@...com>, linux-kernel@...r.kernel.org,
        linux-omap@...r.kernel.org, Rob Herring <robh+dt@...nel.org>,
        devicetree@...r.kernel.org
Subject: Re: [PATCH 4/6] net: ethernet: ti: cpts: add ptp pps support

On Wed, Nov 30, 2016 at 02:43:57PM -0600, Grygorii Strashko wrote:
> > In order to produce the PPS edge correctly, you would have to adjust
> > the comparison value whenever cc.mult changes, 
> 
> yes. And that is done in cpts_ptp_adjfreq()
> 	if (cpts->ts_comp_enabled)
> 		cpts->ts_comp_one_sec_cycs = cpts_cc_ns2cyc(cpts, NSEC_PER_SEC);
> 	^^^ re-calculate reload value for 
>  
> 	cpts_ts_comp_settime(cpts, ns);
> 	^^^ adjust the ts_comp

And it races with the pulse itself.  You forgot about this part:

> @@ -172,14 +232,31 @@ static int cpts_ptp_adjfreq(struct ptp_clock_info *ptp, s32 ppb)
>  	adj *= ppb;
>  	diff = div_u64(adj, 1000000000ULL);
>  
> +	mutex_lock(&cpts->ptp_clk_mutex);
> +
>  	spin_lock_irqsave(&cpts->lock, flags);
> +	if (cpts->ts_comp_enabled) {
> +		cpts_ts_comp_disable(cpts);

Sorry, but this is a train wreck.

> > but of course this is unworkable.
> > 
> 
> Sry, but this is questionable - code for pps comes from TI internal
> branches (SDK releases) where it survived for a pretty long time.

That doesn't mean the code is any good.  If you adjust at the right
moment, then no pulse occurs at all!

> I'm, of course, agree that without HW support for freq adjustment
> this PPS feature is not super precise and has some limitation,
> but that is what we agree to live with. 

I do NOT agree to live with this.  I am one who is going to have to
explain to the world why their beagle bone PPS sucks.
 
Thanks,
Richard