Re: HPA Resume patch

[Jens Axboe <axboe@kernel.dk>]

On Mon, Aug 28 2006, Lee Trager wrote:
> Jens Axboe wrote:
> > On Sun, Aug 27 2006, Pavel Machek wrote:
> >   
> >> Hi!
> >>
> >>     
> >>> This patch fixes a problem with computers that have HPA on their hard
> >>> drive and not being able to come out of resume from RAM or disk. I've
> >>> tested this patch on 2.6.17.x and 2.6.18-rc4 and it works great on both
> >>> of these. This patch also fixes the bug #6840. This is my first patch to
> >>> the kernel and I was told to e-mail the above people to get my patch
> >>> into the kernel.
> >>>       
> >> Congratulations for a first patch.
> >>
> >>     
> >>> If I made a mistake please be gentle and correct me ;)
> >>>       
> >> We'll need signed-off-by: line next time.
> >>
> >> Stefan, can we get this some testing? Or anyone else with thinkpad
> >> with host-protected area still enabled?
> >>     
> >
> > It has design issues, at someone else already noticed. hpa restore needs
> > to be a driver private step, included in the resume state machine. The
> > current patch is a gross layering violation.
> >
> > But thanks to Lee for taking a stab at this, I hope he'll continue and
> > get it polished :-)
> >
> >   
> Ok I redid the patch following exactly what Sergey and Randy said. This
> problem happens on any computer that has HPA on their drive when they
> come back from resume so I don't think you have to only test this with
> Thinkpad users. Anyway my only question is how to I get my patched
> signed off by someone?
> 
> Thanks for all your help!

While this is a _lot_ better than your previous patch, I don't think you
quite understood my suggestion. Which is probably fault, so I'll try to
be a little more verbose.

If you look at the suspend state machine, it goes through a (small)
sequence of steps (see drivers/ide/ide-io.c:ide_complete_power_step())
and get the device suspended. Same thing for resume, just not that many
steps there. My suggestion was to continue using this infrastructure and
just add the HPA restore as a resume state. Right now it does:

        ide_pm_state_start_resume (== idedisk_pm_idle)
                complete that
        ide_pm_restore_dma
                complete that

and we are done. Your patch basically puts more actions into a single
resume state switch, not ideal. What you want to do is have the HPA
restore as an additional state.

Is that clearer? If not, let me know...

-- 
Jens Axboe