Re: Request for testing an alternate branch

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.

- Justin