Date: Sat, 14 Dec 2013 17:22:41 -0800 From: Justin Hibbits <chmeeedalf@gmail.com> 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: <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> <CAHSQbTAPG6hUBJOept3DcWPJrzCYNDUHUimR7wXkYzh03v51pw@mail.gmail.com> <201312121415.47440.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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. - Justin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20131214172241.1f3e3ee3>