From owner-freebsd-current@FreeBSD.ORG Thu Dec 12 19:17:27 2013 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id DEFC46DD; Thu, 12 Dec 2013 19:17:27 +0000 (UTC) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [IPv6:2001:470:1f11:75::1]) (using TLSv1 with cipher ADH-CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 98CE91F58; Thu, 12 Dec 2013 19:17:27 +0000 (UTC) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 81161B9CA; Thu, 12 Dec 2013 14:17:26 -0500 (EST) From: John Baldwin To: Justin Hibbits Subject: Re: Request for testing an alternate branch Date: Thu, 12 Dec 2013 14:15:47 -0500 User-Agent: KMail/1.13.5 (FreeBSD/8.4-CBSD-20130906; KDE/4.5.5; amd64; ; ) References: <20131204222113.39fb23dd@zhabar.gateway.2wire.net> <201312111626.12035.jhb@freebsd.org> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <201312121415.47440.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Thu, 12 Dec 2013 14:17:26 -0500 (EST) Cc: FreeBSD Current , FreeBSD PowerPC ML X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 12 Dec 2013 19:17:27 -0000 On Wednesday, December 11, 2013 5:40:30 pm Justin Hibbits wrote: > On Wed, Dec 11, 2013 at 1:26 PM, John Baldwin wrote: > > On Thursday, December 05, 2013 1:21:13 am Justin Hibbits wrote: > >> I've been working on the projects/pmac_pmu branch for some time now to > >> add suspend/resume as well as CPU speed change for certain PowerPC > >> machines, about a year since I created the branch, and now it's stable > >> enough that I want to merge it into HEAD, hence this request. However, > >> it does touch several drivers, turning them into "early drivers", such > >> that they can be initialized, and suspended and resumed at a different > >> time. Saying that, I do need testing from other architectures, to make > >> sure I haven't broken anything. > >> > >> The technical details: > >> > >> To get proper ordering, I've extended the bus_generic_suspend() and > >> bus_generic_resume() to do multiple passes. Devices which cannot be > >> enabled or disabled at the current pass level would return an EAGAIN. > >> This could possibly cause problems, since it's an addition to an > >> existing API rather than a new API to run along side it, so it needs a > >> great deal of testing. It works fine on PowerPC, but I don't have any > >> i386/amd64 or sparc64 hardware to test it on, so would like others who > >> do to test it. I don't think that it would impact x86 at all (testing > >> is obviously required), because the nexus is not an EARLY_DRIVER_MODULE, > >> so all devices would be handled at the same pass. But, I do know the > >> sparc64 has an EARLY_DRIVER_MODULE() nexus, so that will likely be > >> impacted. > > > > I have patches to change many x86 drivers to use EARLY_DRIVER_MODULE() > > FWIW. > > > > Also, I'm still not a fan of the EAGAIN approach. I'd rather have a method > > in bus_if.m to suspend or resume a single device and to track that a device > > is suspended or resumed via a device_t flag or some such. (I think I had > > suggested this previously as it would also allow us to have a tool to > > suspend/resume individual drivers at runtime apart from a full suspend/resume > > request). > > > > -- > > John Baldwin > > Understood. You had mentioned something along those lines before. Is > it safe/sane to partially merge a branch into HEAD? If so, I can > merge only the changes necessary for PMU cpufreq, and work on the > suspend/resume separately. I put them together because most of the > low level code involved is the same between them. Yes, you can split them up. However, the way to do that is to generate a diff and patch that into a head checkout and commit. There's not a good way to have 'svn merge' do it AFAIK. > I do like your idea of a device_t flag, but I'm not sure what the best > way to go with that would be. I do already use a device_t flag to > determine if the device is suspended, but only bus_generic_* access > that in this patch. > > Would a better way be: > * root_suspend instead of suspending its children, instead traverses > and suspends each descendent in reverse order. > * While doing this, insert each device upon successful suspend into a list. > * For root_resume(), traverse the list back, and suspend each device > in the reverse order. I would rather do it more the way that multipass attach now works where you do scans of the entire device tree as you walk the pass number down (during suspend) suspending any devices that are not yet suspended if their pass number is >= current pass number, then on resume you do scans of the entire tree raising the pass number back up using similar logic to attach where you resume any suspended devices if the driver's pass number is <= current pass number. > With this, add a new method, called device_suspend_child() to suspend > a specific child if it hasn't already been suspended. Well, I would call it 'bus_suspend_child' and 'bus_resume_child' as these would be bus methods in bus_if.m. > * This could require modifying the PCI driver to move the device > child suspend/resume into those functions, instead of doing that logic > in the device_suspend/device_resume itself. Correct. bus_generic_suspend() and bus_suspend_resume() would turn into loops that look a lot like bus_generic_new_pass() (so the logic for honoring pass numbers would happen in these routines and they would invoke bus_suspend_child() and bus_resume_child() to change the state of individual drivers). device_suspend() and device_resume() for the root device would look like bus_set_pass() with a similar loop that walked through the pass levels and invoked bus_generic_suspend/resume after each pass change to start the pass across the device tree. > I guess, in short, I'm asking: Is it fine if I merge only the code > necessary for this cpufreq? That would require making other changes > to this before merging in, to isolate that code, but it's very doable. > > - Justin > -- John Baldwin