Date: Tue, 17 Dec 2013 14:36:40 -0500 From: John Baldwin <jhb@freebsd.org> To: Justin Hibbits <chmeeedalf@gmail.com> Cc: FreeBSD Current <freebsd-current@freebsd.org>, FreeBSD PowerPC ML <freebsd-ppc@freebsd.org> Subject: Re: Request for testing an alternate branch Message-ID: <201312171436.40282.jhb@freebsd.org> In-Reply-To: <20131214172241.1f3e3ee3@zhabar.gateway.2wire.net> References: <20131204222113.39fb23dd@zhabar.gateway.2wire.net> <201312121415.47440.jhb@freebsd.org> <20131214172241.1f3e3ee3@zhabar.gateway.2wire.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Saturday, December 14, 2013 8:22:41 pm Justin Hibbits wrote: > On Thu, 12 Dec 2013 14:15:47 -0500 > John Baldwin <jhb@freebsd.org> wrote: > > > On Wednesday, December 11, 2013 5:40:30 pm Justin Hibbits wrote: > > > 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. > > > > 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. Yes, that is effectively what I am advocating. The bus-specific logic for how to manage power, etc. would move out of the bus's device_suspend method and into the bus_suspend_child method. Bus's would generally always use bus_generic_suspend/resume and those would change to look more like bus_generic_new_pass(). -- John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201312171436.40282.jhb>