From owner-svn-src-all@freebsd.org Mon Jun 3 11:16:37 2019 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 4654D15AD611; Mon, 3 Jun 2019 11:16:37 +0000 (UTC) (envelope-from zeising@freebsd.org) Received: from mail.daemonic.se (mail.daemonic.se [IPv6:2607:f740:d:20::25]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 7602A72F8C; Mon, 3 Jun 2019 11:16:36 +0000 (UTC) (envelope-from zeising@freebsd.org) Received: from cid.daemonic.se (localhost [IPv6:::1]) by mail.daemonic.se (Postfix) with ESMTP id 45HXYv4BK3z3c7W; Mon, 3 Jun 2019 11:16:35 +0000 (UTC) X-Virus-Scanned: amavisd-new at daemonic.se Received: from mail.daemonic.se ([127.0.0.1]) (using TLS with cipher ECDHE-RSA-AES128-GCM-SHA256) by cid.daemonic.se (mailscanner.daemonic.se [127.0.0.1]) (amavisd-new, port 10587) with ESMTPS id ydPFxNBXKKOr; Mon, 3 Jun 2019 11:16:34 +0000 (UTC) Received: from garnet.daemonic.se (host-90-236-177-23.mobileonline.telia.com [90.236.177.23]) by mail.daemonic.se (Postfix) with ESMTPSA id 45HXYt3PW3z3c7V; Mon, 3 Jun 2019 11:16:34 +0000 (UTC) Subject: Re: svn commit: r348355 - head/sys/dev/iicbus To: Ian Lepore , Andriy Gapon , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org References: <201905290908.x4T98L89066643@repo.freebsd.org> Cc: jmd@freebsd.org From: Niclas Zeising Message-ID: Date: Mon, 3 Jun 2019 13:16:33 +0200 User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Rspamd-Queue-Id: 7602A72F8C X-Spamd-Bar: -- Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-2.99 / 15.00]; local_wl_from(0.00)[freebsd.org]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; NEURAL_HAM_SHORT(-0.99)[-0.993,0]; ASN(0.00)[asn:36236, ipnet:2607:f740:d::/48, country:US]; NEURAL_HAM_LONG(-1.00)[-1.000,0] X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 03 Jun 2019 11:16:37 -0000 On 2019-05-29 22:52, Ian Lepore wrote: > On Wed, 2019-05-29 at 15:12 +0300, Andriy Gapon wrote: >> On 29/05/2019 14:54, Niclas Zeising wrote: >>> On 2019-05-29 11:08, Andriy Gapon wrote: >>>> Author: avg >>>> Date: Wed May 29 09:08:20 2019 >>>> New Revision: 348355 >>>> URL: https://svnweb.freebsd.org/changeset/base/348355 >>>> >>>> Log: >>>> revert r273728 and parts of r306589, iicbus no-stop by default feature >>>> Since drm2 removal, there has not been any consumer of the feature in the >>>> tree. I am also unaware of any out-of-tree consumer. >>>> More importantly, the feature has been broken from the very start, both >>>> before and after r306589, because the ivar was set on a device that does >>>> not support it and it was read from another device that also does not >>>> support it. >>>> A bus-wide no-stop flag cannot be implemented as an ivar as iicbus >>>> attaches as a child of various drivers. Implementing the ivar in each >>>> and every I2C driver is just impractical. >>>> If we ever want to implement this feature properly, then probably the >>>> easiest way to do it would be via a flag in the softc of iicbus. >>>> In fact, we might have to do that in the stable branches if we want to >>>> fix the code for them. >>>> Reported by: ian (long time ago) >>>> MFC after: 1 month (maybe) >>>> X-MFC-note: cannot just merge the change, must keep drm2 happy >>>> >>> >>> Hi! >>> Just a note, be aware that drm2 lives on in ports as drm-legacy-kmod. I haven't >>> tested, but, from the description above I worry that it will affect the port. >>> What do you think? >> >> Oh, I forgot about that one... >> I think that it could be affected if it still uses FreeBSD iic code. >> I guess I might have to revert the change. >> >> > > I don't think so, because I don't think this change ever worked. I'm > not sure how anybody convinced themselves that it did. It attempts to > retrieve ivars from a device that doesn't have them, so the net effect > is that the nostop variable is initialized from stack garbage. Maybe > whoever wrote and tested it was lucky enough to have that accidentally > be consistently zero or non-zero, so their testing appeared to work. > > Looking at the drm2 code that is the only user of this, it appears that > the nostop value is only used in the case where the driver falls back > to using the builtin intel_iicbb_driver. That driver relies on > iicbus_transfer_gen() which is where the nostop kludge was added. > That's the fundamental problem in all of this: the right thing to do, > IMO, would have been to implement the iicbus_transfer method directly > in the intel_iicbb_driver (probably by just cut-and-pasting the code > from iicconf.c then doing whatever is necessary to ignore stops). And > we can still do that, pretty trivially, if necessary. > > -- Ian > Hi! It seems like things broke after all, latest pkg build (on head-amd64) reports this: /wrkdirs/usr/ports/graphics/drm-legacy-kmod/work/drm-legacy-12bd551/src/dev/drm2/i915/intel_iic.c:570:2: error: implicit declaration of function 'iicbus_set_nostop' is invalid in C99 [-Werror,-Wimplicit-function-declaration] iicbus_set_nostop(idev, true); ^ /wrkdirs/usr/ports/graphics/drm-legacy-kmod/work/drm-legacy-12bd551/src/dev/drm2/i915/intel_iic.c:570:2: error: this function declaration is not a prototype [-Werror,-Wstrict-prototypes] 2 errors generated. Full log: http://beefy12.nyi.freebsd.org/data/head-amd64-default/p503023_s348376/logs/drm-legacy-kmod-g20190523.log Regards -- Niclas Zeising