Re: Request for testing an alternate branch

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 Baldwin