On Monday 23 March 2009, Jeremy Fitzhardinge wrote:
> Rafael J. Wysocki wrote:
> > On Monday 23 March 2009, Tian, Kevin wrote:
> >
> >> And then Xen jumps in to finish remaining steps. From this angle,
> >> Xen is not a completely new platform and, well, S3 is more like a
> >> 'S1' type from dom0's p.o.v with a different trigger method. Then is
> >> it overkilled to introduce a new set of ops with 99% content
> >> duplicated?
> >>
> >
> > IMO, no, it isn't.
>
> Hm. Well, lets take acpi_suspend_enter() as a specific example. The
> Xen change here is:
>
> @@ -240,11 +240,20 @@ static int acpi_suspend_enter(suspend_state_t pm_state)
> barrier();
> status = acpi_enter_sleep_state(acpi_state);
> break;
>
> case ACPI_STATE_S3:
> - do_suspend_lowlevel();
> + if (!xen_pv_domain())
> + do_suspend_lowlevel();
> + else {
> + /*
> + * Xen will save and restore CPU context, so
> + * we can skip that and just go straight to
> + * the suspend.
> + */
> + acpi_enter_sleep_state(acpi_state);
> + }
> break;
> }
Hmm, in that case it may be more appropriate to modify
do_suspend_lowlevel(). Have you considered doing that?
> /* If ACPI is not enabled by the BIOS, we need to enable it here. */
> if (set_sci_en_on_resume)
>
>
> Which is, functionally, adding one if() and a new line of code, in the
> middle of a ~70 line function.
>
> Are you suggesting that it would be best to copy this whole function so
> that I can put one line of Xen-specific code in the middle, rather than
> just making this change?
No.
> Some other functions, the Xen vs. non-Xen changes are larger;
> acpi_sleep_prepare() could reasonably have a Xen-specific variant
> because a big chunk of it is setting up the wakeup vector (which is
> unnecessary under Xen), and the rest can be easily pulled into common
> code. But unfortunately acpi_sleep_prepare isn't itself an operation,
> and is only called at the bottom of 2-3 level deep callchains.
>
> I think that rather than having a separate xen-acpi
> platform_suspend_ops, it would make more sense to have a acpi_ops within
> acpi/sleep.c and handle the differences that way. I'll see how it turns
> out.
Yes, that may be better.
Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
