From owner-freebsd-current@FreeBSD.ORG Sun Dec 15 01:22:48 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 53C146F5; Sun, 15 Dec 2013 01:22:48 +0000 (UTC) Received: from mail-yh0-x230.google.com (mail-yh0-x230.google.com [IPv6:2607:f8b0:4002:c01::230]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id E488C1D23; Sun, 15 Dec 2013 01:22:47 +0000 (UTC) Received: by mail-yh0-f48.google.com with SMTP id f73so2687878yha.7 for ; Sat, 14 Dec 2013 17:22:47 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-type:content-transfer-encoding; bh=WcsbpuAsqsk8qvOm3q6gfEuHD0LawMy6IcXkQx7IeaU=; b=XBcHlBb+e7fuIUV82w8UnFgY7aB8KyAVxOTa337kwCeyQTvVQKLDEX4Mu8bTR1r/aX lScut+8EmXVgTLhimRd9QMGTlNsz8AEs+RdZWdkXtnxzkwecy2EoNnLMdKgvxrJ3kcfv sfuhWQUkqkUccMfTJfTP2Lg7GczinqhmZ3/b8VPvd8pmRf090u8i7yNKIXNwUwVnnA3J PwLoKmibg8NRjHkkuA7YY75QzHaQYYiuJm+hp9xSp2Rx7XU9u4B+uM3eMIYsUKH7ybNZ qagqzCk6RM8VBjGt2cKqcY6TXsmY7SVQjE84EAYh0U1JYV2EEOhZjbSm7UvhBztrlYe0 8K5A== X-Received: by 10.236.42.231 with SMTP id j67mr156926yhb.131.1387070567195; Sat, 14 Dec 2013 17:22:47 -0800 (PST) Received: from zhabar.gateway.2wire.net (76-253-2-5.lightspeed.sntcca.sbcglobal.net. [76.253.2.5]) by mx.google.com with ESMTPSA id h23sm11100393yhc.0.2013.12.14.17.22.45 for (version=SSLv3 cipher=RC4-SHA bits=128/128); Sat, 14 Dec 2013 17:22:46 -0800 (PST) Date: Sat, 14 Dec 2013 17:22:41 -0800 From: Justin Hibbits To: John Baldwin Subject: Re: Request for testing an alternate branch Message-ID: <20131214172241.1f3e3ee3@zhabar.gateway.2wire.net> In-Reply-To: <201312121415.47440.jhb@freebsd.org> References: <20131204222113.39fb23dd@zhabar.gateway.2wire.net> <201312111626.12035.jhb@freebsd.org> <201312121415.47440.jhb@freebsd.org> X-Mailer: Claws Mail 3.9.2 (GTK+ 2.24.19; powerpc64-portbld-freebsd11.0) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit 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: Sun, 15 Dec 2013 01:22:48 -0000 On Thu, 12 Dec 2013 14:15:47 -0500 John Baldwin wrote: > 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, Would it make sense, then, to replace all the child suspend logic in drivers (i.e. busses), with the bus_generic_suspend(), and let all the drivers suspend only themselves? And the same with the resume code? If all the suspend logic is to be moved into the bus_generic, I think this makes sense. Then you simply call bus_suspend_child(bus, device), which will drill down and take care of all the recursion, calling BUS_SUSPEND_CHILD() on each child. To me, this seems the most sensible, and would only cause problems if a driver cares about grandchildren, not only children. But, then, that also seems a bit ridiculous anyway. - Justin