Re: [PATCH v12 9/9] ACPI: x86: s2idle: Enforce LPS0 constraints for PCI devices

[Rafael J. Wysocki]

On Thu, Aug 17, 2023 at 9:40 PM Limonciello, Mario
<mario.limonciello@xxxxxxx> wrote:
>
>
>
> On 8/17/2023 2:37 PM, Rafael J. Wysocki wrote:
> > On Thu, Aug 17, 2023 at 9:30 PM Limonciello, Mario
> > <mario.limonciello@xxxxxxx> wrote:
> >>
> >>
> >>
> >> On 8/17/2023 2:25 PM, Bjorn Helgaas wrote:
> >>> On Wed, Aug 16, 2023 at 03:41:43PM -0500, Mario Limonciello wrote:
> >>>> Since commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
> >>>> PCIe ports from modern machines (>=2015) are allowed to be put into D3 by
> >>>> storing a value to the `bridge_d3` variable in the `struct pci_dev`
> >>>> structure.
> >>>> ...
> >>>
> >>>> +static void lpi_check_pci_dev(struct lpi_constraints *entry, struct pci_dev *pdev)
> >>>> +{
> >>>> +    pci_power_t target = entry->enabled ? entry->min_dstate : PCI_D0;
> >>>> +
> >>>> +    if (pdev->current_state == target)
> >>>> +            return;
> >>>> +
> >>>> +    /* constraint of ACPI D3hot means PCI D3hot _or_ D3cold */
> >>>> +    if (target == ACPI_STATE_D3_HOT &&
> >>>
> >>> ACPI_STATE_D3_HOT is not a valid pci_power_t value.
> >>
> >> Based on this, kernel robot sparse complaints and your comments on v11's
> >> last patch I am going to split off to another function that returns the
> >> pci_power_t state based upon the situation and better comment the reason
> >> for the D0 when not enabled.
> >>
> >>>
> >>>> +        (pdev->current_state == PCI_D3hot ||
> >>>> +         pdev->current_state == PCI_D3cold))
> >>>> +            return;
> >>>> +
> >>>> +    if (pm_debug_messages_on)
> >>>> +            acpi_handle_info(entry->handle,
> >>>> +                             "LPI: PCI device in %s, not in %s\n",
> >>>> +                             acpi_power_state_string(pdev->current_state),
> >>>> +                             acpi_power_state_string(target));
> >>>> +
> >>>> +    /* don't try with things that PCI core hasn't touched */
> >>>> +    if (pdev->current_state == PCI_UNKNOWN) {
> >>>> +            entry->handle = NULL;
> >>>> +            return;
> >>>> +    }
> >>>> +
> >>>> +    pci_set_power_state(pdev, target);
> >>>
> >>> It doesn't seem logical for a "check_constraints()" function that
> >>> takes no parameters and returns nothing to actively set the PCI power
> >>> state.
> >>>
> >>> lpi_check_constraints() returns nothing, and from the fact that it was
> >>> previously only called when "pm_debug_messages_on", I infer that it
> >>> should have no side effects.
> >>>
> >>> IMHO "lpi_check_constraints" is not a great name because "check"
> >>> doesn't suggest anything specific about what it does.
> >>> "dump_constraints()" -- fine.  "log_unmet_constraints()" -- fine
> >>> (seems like the original intention of 726fb6b4f2a8 ("ACPI / PM: Check
> >>> low power idle constraints for debug only"), which added it.
> >>>
> >>
> >> Great feedback, thanks.  I'm thinking to instead change it to:
> >>
> >> lpi_enforce_constraints()
> >
> > Don't even try to go this way, please.
> >
> > Originally, the LPI constraints are there to indicate to Windows
> > whether or not it should attempt to enter Connected/Modern Standby.
> >
> > Because Linux doesn't do Modern Standby, it doesn't use the LPI
> > constraints the way Windows does and it really shouldn't do that.
> >
> > I think that the exercise here is to use the information from the
> > constraints list as an indication whether or not a given PCI Root Port
> > is supposed to be put into D3hot/cold on suspend-to-idle and this has
> > nothing to do with enforcement.
>
> What do you think about me making the changes to pci_prepare_to_sleep()?
>
> Something like this:
>
> @@ -2733,11 +2742,17 @@ int pci_prepare_to_sleep(struct pci_dev *dev)
>   {
>          bool wakeup = device_may_wakeup(&dev->dev);
>          pci_power_t target_state = pci_target_state(dev, wakeup);
> +       pci_power_t constraint;
>          int error;
>
>          if (target_state == PCI_POWER_ERROR)
>                  return -EIO;
>
> +       /* if platform indicates device constraint for suspend, use it */
> +       constraint = platform_check_constraint(dev, target_state);
> +       if (constraint != PCI_POWER_ERROR)
> +               target_state = constraint;
> +
>          pci_enable_wake(dev, target_state, wakeup);
>
>          error = pci_set_power_state(dev, target_state);

I think that this is going to regress things in the field.

I agree with replacing and/or amending the dmi_get_bios_year() check
in pci_bridge_d3_possible() with the information from the constraints
list, but I don't agree with using it for pretty much anything else
that may affect functionality.