Re: [Xen-devel] Re: [PATCH] pvops: Resume devices when suspend is failed

[Frank Pan <frankpzh@xxxxxxxxx>]

Yes, that's what I doubt.

I write dpm_resume_end() inside the if clause just because this reason.

What if we call clock_was_set() before dpm_resume_end() ?
Is there any device need timer intr during resume?

IOW:

  if (dpm_suspend(..))
    goto out_thaw;
  xs_suspend()
  if (dpm_suspend_noirq())
    goto out_resume;
  xen_suspend()
  dpm_resume_noirq()

out_resume:
  xen_arch_resume()
  xs_resume()
  clock_was_set()

out_thaw:
  dpm_resume_end()
  return

On Wed, Mar 9, 2011 at 5:09 AM, Shriram Rajagopalan <rshriram@xxxxxxxxx> wrote:
> On Tue, Mar 8, 2011 at 2:32 AM, Ian Campbell <Ian.Campbell@xxxxxxxxxx>
> wrote:
>>
>> On Mon, 2011-03-07 at 17:22 +0000, Stefano Stabellini wrote:
>> > On Fri, 4 Mar 2011, Frank Pan wrote:
>> > > Recent pvops kernel does not call dpm_resume_end when
>> > > dpm_suspend_start is failed. This makes some device remain suspended
>> > > after the unsuccessful call of do_suspend from xenbus.
>> > >
>> > > In my test, a PV-on-HVM guest printed the following message after
>> > > received a suspend request through xenbus, and then stucked due to
>> > > disk access.
>> > >
>> > > [41577.764748] sd 0:0:0:0: [sda] Stopping disk
>> > > [41577.765273] PM: Device input2 failed to suspend: error -22
>> > > [41577.765275] xen suspend: dpm_suspend_start -22
>> > >
>> > > The following patch fixes this by calling dpm_suspend_start after the
>> > > failure of dpm_resume_end.
>>
>> > Thanks for spotting this issue and for the patch!
>> > However I think it would be better to move out_thaw before
>> > dpm_resume_end instead.
>>
> FYI, the code flow after Frank's patch
> Âif (dpm_suspend(..)) {
> Â dpm_resume_end()
> Â goto out_thaw;
> }
> xs_suspend()
> if (dpm_suspend_noirq())
> Â goto out_resume;
> xen_suspend()
> dpm_resume_noirq()
>
> out_resume:
> xen_arch_resume()
> xs_resume()
> dpm_resume_end()
> clock_was_set()
>
> out_thaw:
> ÂÂÂÂÂ return
>
> I am not sure if the clock_was_set() call is harmless (retriggering the
> timers),
> even if no dpm_suspend_noirq() calls were issued.
>>
>> ACK.
>>
>> This will likely interact with Shriram's recent changes to the PMSG_*
>> types used by Xen save/restore. Ideally this patch would be against
>> those.
>>
>> Also the choice of PMSG_* passed to dpm_resume_end will likely depend on
>> whether dpm_suspend_start succeeded or failed. Based on looking at
>> hibernate.c I think, after Shriram's patches, it needs to be
>> PMSG_RECOVER in the error case, PMSG_THAW in the checkpoint case and
>> PMSG_RESTORE in the normal resume case but you should check the
>> descriptions in pm.h to be sure.
>>
>> Ian.
>>
>> > Would you be OK to do that and send a patch?
>> >
>> > _______________________________________________
>> > Xen-devel mailing list
>> > Xen-devel@xxxxxxxxxxxxxxxxxxx
>> > http://lists.xensource.com/xen-devel
>>
>>
>
>



-- 
æéç, Frank Pan

Computer Science and Technology
Tsinghua University

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel