Date: Wed, 11 Dec 2013 14:40:30 -0800 From: Justin Hibbits <jhibbits@freebsd.org> To: John Baldwin <jhb@freebsd.org> Cc: FreeBSD Current <freebsd-current@freebsd.org>, FreeBSD PowerPC ML <freebsd-ppc@freebsd.org> Subject: Re: Request for testing an alternate branch Message-ID: <CAHSQbTAPG6hUBJOept3DcWPJrzCYNDUHUimR7wXkYzh03v51pw@mail.gmail.com> In-Reply-To: <201312111626.12035.jhb@freebsd.org> References: <20131204222113.39fb23dd@zhabar.gateway.2wire.net> <201312111626.12035.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Dec 11, 2013 at 1:26 PM, John Baldwin <jhb@freebsd.org> 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. 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. With this, add a new method, called device_suspend_child() to suspend a specific child if it hasn't already been suspended. * 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. 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAHSQbTAPG6hUBJOept3DcWPJrzCYNDUHUimR7wXkYzh03v51pw>