Re: HPA Resume patch

[Lee Trager <Lee@PicturesInMotion.net>]

Jens Axboe wrote:
> 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...
>
>   
Ok I've been looking through the source to see how to do this properly.
I can't figure out where ide_generic_resume() is called from. It doesn't
look like its called from ide_complete_power_step() I want to know
because I want the HPA stuff to be done right after ide_generic_resume()
is called. I'll do some testing to see if it works to get called before
and after the DMA stuff.

Thanks,

Lee

-- 
VGER BF report: U 0.5