From owner-svn-src-head@freebsd.org Fri Oct 25 16:06:48 2019 Return-Path: Delivered-To: svn-src-head@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id DE2BF174867 for ; Fri, 25 Oct 2019 16:06:48 +0000 (UTC) (envelope-from ian@freebsd.org) Received: from outbound3d.ore.mailhop.org (outbound3d.ore.mailhop.org [54.186.57.195]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) server-signature RSA-PSS (4096 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 4708BJ4sF7z3D1V for ; Fri, 25 Oct 2019 16:06:48 +0000 (UTC) (envelope-from ian@freebsd.org) ARC-Seal: i=1; a=rsa-sha256; t=1572019606; cv=none; d=outbound.mailhop.org; s=arc-outbound20181012; b=L+toUX5s4CxFvqvE31bwLoKhKNqjCrMPwpOxNFU8yXEmO8lIRLcmwOl/Jb8xWbwkNq1YaeFnUK7HI L4e2503DggNGnpLBx7ETPQTIpl4fLCG1bITAPSMJbHBoOSeTfkx/OjYzmwvkIyktFzFme9hyoUyVH+ B5uKWRS9mAkzpqtwCHwwiDE9a+ETDjT7M1N94f2fl8WYe/rWrestsXLwIIj993zVthqoSRQDapgMTK JeHTN9dy+KhcrMv1/Kbm+pNImR3gGTNLxKOhbiKgYzlAFzk8nuIOc48PNMMpTj6XXYzk8htc5juCP8 M3/lpkhO2nsWjWS71ZoVsG3K2NM3Iqg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=outbound.mailhop.org; s=arc-outbound20181012; h=content-transfer-encoding:mime-version:content-type:references:in-reply-to: date:to:from:subject:message-id:dkim-signature:from; bh=gpoP4SXFH0cauPXFsSGBf/VsBP5Eh/wKP4kGxvzq/NU=; b=NZYlRmC0Jf/Uy3jD0yWpl+wiznlqreyeCKttZRh4+YwpeQM2xhPARtmmjsr7l555Ccn4eNHF6IIxn lc60WJUfiIkmQdcpFqwBtnBc76CMv380RvQhR/H2VvEklQU+m9dxbxquupS89h+LWxyUhOzoxMG3M1 kVc3WiZgSAn11Z/LghKFLaA2FP/FUQqCwJA/mgsxggm7y/RofG/NnKRM6LakmLjedx9EeHvGt8F4wp IILZvze3odV72/oKlvkohG1sjneVLIRjdfW7/+bjpO269qYVPKaKJVHqoE8/sL51+BgGxUaYr4LGpP wRXrRbBoldbiPScdh9ytiEDkkHYbUFg== ARC-Authentication-Results: i=1; outbound3.ore.mailhop.org; spf=softfail smtp.mailfrom=freebsd.org smtp.remote-ip=67.177.211.60; dmarc=none header.from=freebsd.org; arc=none header.oldest-pass=0; DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=outbound.mailhop.org; s=dkim-high; h=content-transfer-encoding:mime-version:content-type:references:in-reply-to: date:to:from:subject:message-id:from; bh=gpoP4SXFH0cauPXFsSGBf/VsBP5Eh/wKP4kGxvzq/NU=; b=cn4Xn7hFiTGGWvz6HuEdG3aDtUWWA9w1UZTnKjsb7A4kWuvvb8lKiSd3WwbgkVh6YRDXQxcscd900 zwl8P1R8iBSJwQ6KNlWES41DZAhINcFGdFanxyJpv0PiVgLHZU+eZt+9fpS5frNYrdCJvnM+xIExpj Tsxl1MnUG6sje6A0BbnZxhv4GsuV0iUWcXeBwqCM8NbkvFsnOjBKLIujsrB3mBNd10FjfhXsymWTaM IC7uVfROOFJFHCR2z+i6AhjYKJ3ekw7XZyRIYluX06b8JqhGx1WLtK9nH0STSlCrcR/ktqNwJYUygD COjkocGmnk3WIh51/4pLgo+fT2TBk8Q== X-MHO-RoutePath: aGlwcGll X-MHO-User: 7078f7db-f741-11e9-b80b-052b4a66b6b2 X-Report-Abuse-To: https://support.duocircle.com/support/solutions/articles/5000540958-duocircle-standard-smtp-abuse-information X-Originating-IP: 67.177.211.60 X-Mail-Handler: DuoCircle Outbound SMTP Received: from ilsoft.org (unknown [67.177.211.60]) by outbound3.ore.mailhop.org (Halon) with ESMTPSA id 7078f7db-f741-11e9-b80b-052b4a66b6b2; Fri, 25 Oct 2019 16:06:45 +0000 (UTC) Received: from rev (rev [172.22.42.240]) by ilsoft.org (8.15.2/8.15.2) with ESMTP id x9PG6iFH049768; Fri, 25 Oct 2019 10:06:44 -0600 (MDT) (envelope-from ian@freebsd.org) Message-ID: <8baac891351521c9e7148a0c0db9978645158dce.camel@freebsd.org> Subject: Re: svn commit: r354076 - head/sys/dev/ow From: Ian Lepore To: Andriy Gapon , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Date: Fri, 25 Oct 2019 10:06:44 -0600 In-Reply-To: <01c62f31-1b10-6856-eb33-9238dcb0d9c6@FreeBSD.org> References: <201910251538.x9PFc9ii028313@repo.freebsd.org> <2fde5b7b-6186-0969-08a5-83524b6aa274@FreeBSD.org> <0877c65ce11b0509dcc9e3ee491a37cc2a488f93.camel@freebsd.org> <01c62f31-1b10-6856-eb33-9238dcb0d9c6@FreeBSD.org> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.28.5 FreeBSD GNOME Team Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-Rspamd-Queue-Id: 4708BJ4sF7z3D1V X-Spamd-Bar: - Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-1.86 / 15.00]; local_wl_from(0.00)[freebsd.org]; NEURAL_HAM_MEDIUM(-0.86)[-0.861,0]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; ASN(0.00)[asn:16509, ipnet:54.186.0.0/15, country:US] X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 25 Oct 2019 16:06:48 -0000 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 */ > }; > > 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