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

[Warner Losh]

On Fri, Oct 25, 2019, 6:51 PM Rodney W. Grimes <freebsd at gndrsh.dnsmgr.net>
wrote:

> > 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?
>

No. The values won't com close to overflowing even at the slowest transfer
speeds. The macros are most needed for nanosecond stuff, and these are all
at most tens of microseconds...

Warner

> >
> > >
> >
> > 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
>