Re: Request for testing an alternate branch

On Saturday, December 14, 2013 8:22:41 pm Justin Hibbits wrote:
> On Thu, 12 Dec 2013 14:15:47 -0500
> John Baldwin <jhb_at_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_at_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