Date: Fri, 25 Oct 2019 17:51:20 -0700 (PDT) From: "Rodney W. Grimes" <freebsd@gndrsh.dnsmgr.net> To: Ian Lepore <ian@freebsd.org> Cc: Andriy Gapon <avg@freebsd.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r354076 - head/sys/dev/ow Message-ID: <201910260051.x9Q0pKl3058676@gndrsh.dnsmgr.net> In-Reply-To: <8baac891351521c9e7148a0c0db9978645158dce.camel@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
> 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@freebsd.org
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201910260051.x9Q0pKl3058676>