svn commit: r354076 - head/sys/dev/ow

[Rodney W. Grimes]

> On Fri, 2019-10-25 at 19:03 +0300, Andriy Gapon wrote:
> > On 25/10/2019 18:56, Ian Lepore wrote:
> > > On Fri, 2019-10-25 at 18:51 +0300, Andriy Gapon wrote:
> > > > On 25/10/2019 18:46, Ian Lepore wrote:
> > > > > On Fri, 2019-10-25 at 15:38 +0000, Andriy Gapon wrote:
> > > > > > Author: avg
> > > > > > Date: Fri Oct 25 15:38:09 2019
> > > > > > New Revision: 354076
> > > > > > URL: https://svnweb.freebsd.org/changeset/base/354076
> > > > > > 
> > > > > > Log:
> > > > > >   owc_gpiobus_read_data: compare times in sbintime_t units
> > > > > >   
> > > > > >   Previously the code used sbttous() before microseconds
> > > > > > comparison
> > > > > > in one
> > > > > >   place, sbttons() and nanoseconds in another, division by
> > > > > > SBT_1US
> > > > > > and
> > > > > >   microseconds in yet another.
> > > > > >   
> > > > > >   Now the code consistently uses multiplication by SBT_1US to
> > > > > > convert
> > > > > >   microseconds to sbintime_t before comparing them with
> > > > > > periods
> > > > > > between
> > > > > >   calls to sbinuptime().  This is fast, this is precise
> > > > > > enough
> > > > > > (below
> > > > > >   0.03%) and the periods defined by the protocol cannot
> > > > > > overflow.
> > > > > >   
> > > > > >   Reviewed by:	imp (D22108)
> > > > > >   MFC after:	2 weeks
> > > > > > 
> > > > > > Modified:
> > > > > >   head/sys/dev/ow/owc_gpiobus.c
> > > > > > 
> > > > > > Modified: head/sys/dev/ow/owc_gpiobus.c
> > > > > > =============================================================
> > > > > > ====
> > > > > > ====
> > > > > > =========
> > > > > > --- head/sys/dev/ow/owc_gpiobus.c	Fri Oct 25 15:02:50
> > > > > > 2019	(
> > > > > > r354
> > > > > > 075)
> > > > > > +++ head/sys/dev/ow/owc_gpiobus.c	Fri Oct 25 15:38:09
> > > > > > 2019	(
> > > > > > r354
> > > > > > 076)
> > > > > > @@ -296,10 +296,10 @@ owc_gpiobus_read_data(device_t dev,
> > > > > > struct
> > > > > > ow_timing *
> > > > > >  	do {
> > > > > >  		now = sbinuptime();
> > > > > >  		GETPIN(sc, &sample);
> > > > > > -	} while (sbttous(now - then) < t->t_rdv + 2 && sample
> > > > > > == 0);
> > > > > > +	} while (now - then < (t->t_rdv + 2) * SBT_1US &&
> > > > > > sample == 0);
> > > > > >  	critical_exit();
> > > > > >  
> > > > > > -	if (sbttons(now - then) < t->t_rdv * 1000)
> > > > > > +	if (now - then < t->t_rdv * SBT_1US)
> > > > > >  		*bit = 1;
> > > > > >  	else
> > > > > >  		*bit = 0;
> > > > > > @@ -307,7 +307,7 @@ owc_gpiobus_read_data(device_t dev,
> > > > > > struct
> > > > > > ow_timing *
> > > > > >  	/* Wait out the rest of t_slot */
> > > > > >  	do {
> > > > > >  		now = sbinuptime();
> > > > > > -	} while ((now - then) / SBT_1US < t->t_slot);
> > > > > > +	} while (now - then < t->t_slot * SBT_1US);
> > > > > >  
> > > > > >  	RELBUS(sc);
> > > > > >  
> > > > > 
> > > > > Unit conversions with sbt times should be done using the macros
> > > > > that
> > > > > carefully avoid roundoff errors.  I don't understand why you've
> > > > > changed
> > > > > the code that correctly did use those macros to inline math.
> > > > 
> > > > I think that the commit message explains it:
> > > > This is fast, this is precise enough (below 0.03%) and the
> > > > periods
> > > > defined by
> > > > the protocol cannot overflow.
> > > > 
> > > > Do you disagree?
> > > > Could you please explain in which of the three lines changed the
> > > > new
> > > > code is
> > > > worse and why?
> > > > 
> > > 
> > > I absolutely disagree (or I wouldn't have replied).  Unit
> > > conversions
> > > using sbt times should use the predefined macros, NOT incline
> > > multiplication and division.  I don't know how to say it more
> > > clearly
> > > than that.  The conversion macros are idiomatic (at least, they
> > > would
> > > be if people stopped writing inline conversion expressions).
> > 
> > I can agree that I should have used ustosbt() instead of
> > multiplication by
> > SBT_1US, but I don't agree with your original message that I changed
> > the code
> > that correctly used the macros.
> > 
> > But again, I know the times being converted, they are fixed by the
> > protocol and
> > I do not see why I have to use this:
> > 
> > static __inline sbintime_t
> > ustosbt(int64_t _us)
> > {
> >         sbintime_t sb = 0;
> > 
> > #ifdef KASSERT
> >         KASSERT(_us >= 0, ("Negative values illegal for ustosbt:
> > %jd", _us));
> > #endif
> >         if (_us >= SBT_1S) {
> >                 sb = (_us / 1000000) * SBT_1S;
> >                 _us = _us % 1000000;
> >         }
> >         /* 9223372036855 = ceil(2^63 / 1000000) */
> >         sb += ((_us * 9223372036855ull) + 0x7fffffff) >> 31;
> >         return (sb);
> > }
> > 
> > instead of this
> > x * (((sbintime_t)1 << 32) / 1000000)
> > 
> > The times:
> > static struct ow_timing timing_regular = {
> >         .t_slot = 60,           /* 60 to 120 */
> >         .t_low0 = 60,           /* really 60 to 120 */
> >         .t_low1 = 1,            /* really 1 to 15 */
> >         .t_release = 45,        /* <= 45us */
> >         .t_rec = 1,             /* at least 1us */
> >         .t_rdv = 15,            /* 15us */
> >         .t_rstl = 480,          /* 480us or more */
> >         .t_rsth = 480,          /* 480us or more */
> >         .t_pdl = 60,            /* 60us to 240us */
> >         .t_pdh = 60,            /* 15us to 60us */
> >         .t_lowr = 1,            /* 1us */
> > };

Should this chunk of code now have a bit fat NB: if you
modify these be careful, there is other code that assumes
the values here can not cause overflow?

> > 
> > 
> 
> The fact that you could say all that means that there is nothing I can
> say that is going to change your mind.  I will say if you ever do
> anything like this to code I wrote, I will revert it immediately.
> 
> -- Ian
> 
> 

-- 
Rod Grimes                                                 rgrimes at freebsd.org