| Message ID | 20230818051319.551-10-mario.limonciello@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix wakeup problems on some AMD platforms | expand |
On Fri, Aug 18, 2023 at 7:15 AM Mario Limonciello <mario.limonciello@amd.com> wrote: > > Other parts of the kernel may use constraints information to make > decisions on what power state to put a device into. I would say more about what the constraints are in this changelog, or it becomes cryptic to people who are not familiar with the S0ix/LPS0 terminology. Something like "Some platforms may achieve additional energy conservation in deep idle states provided that the states of all devices on the platform meet specific requirements. In particular, they each may need to be put into a specific minimum low-power state (or a deeper one) for this purpose. The table of Low-Power S0 Idle (LPS0) constraints provided by the platform firmware on some platforms using ACPI specifies that minimum low-power state for each device. That information may help to make decisions on what power state to put a given device into when it is suspended, so introduce a function allowing its caller to retrieve the minimum LPS0 low-power state for a given device." > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- > v12->v13: > * Drop checking for enabled, just return constraints > v11->v12: > * Use for_each_lpi_constraint instead > * use CONFIG_SUSPEND instead of CONFIG_ACPI_SLEEP > v9->v10: > * split from other patches > * kerneldoc fixes > * move debug statement to this function > --- > drivers/acpi/x86/s2idle.c | 24 ++++++++++++++++++++++++ > include/linux/acpi.h | 6 ++++++ > 2 files changed, 30 insertions(+) > > diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c > index 1aa3cd5677bd8..dd961f3a60577 100644 > --- a/drivers/acpi/x86/s2idle.c > +++ b/drivers/acpi/x86/s2idle.c > @@ -299,6 +299,30 @@ static void lpi_device_get_constraints(void) > ACPI_FREE(out_obj); > } > > +/** > + * acpi_get_lps0_constraint - get any LPS0 constraint for a device "get the minimum LPS0 low-power state for a device". > + * @dev: device to get constraints for > + * > + * Returns: > + * - the value for constraint. > + * - Otherwise, -ENODEV. Why not ACPI_STATE_UNKNOWN? > + */ > +int acpi_get_lps0_constraint(struct device *dev) I think that some overhead would be reduced below if this were taking a struct acpi_device pointer as the argument. > +{ > + struct lpi_constraints *entry; > + > + for_each_lpi_constraint(entry) { > + if (!device_match_acpi_handle(dev, entry->handle)) > + continue; > + acpi_handle_debug(entry->handle, > + "ACPI device constraint: %d\n", entry->min_dstate); > + return entry->min_dstate; > + } > + dev_dbg(dev, "No ACPI device constraint specified\n"); > + > + return -ENODEV; ACPI_STATE_UNKNOWN? > +} > + > static void lpi_check_constraints(void) > { > struct lpi_constraints *entry; > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index f1552c04a2856..6745535444c76 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -1109,6 +1109,12 @@ struct acpi_s2idle_dev_ops { > }; > int acpi_register_lps0_dev(struct acpi_s2idle_dev_ops *arg); > void acpi_unregister_lps0_dev(struct acpi_s2idle_dev_ops *arg); > +int acpi_get_lps0_constraint(struct device *dev); > +#else /* CONFIG_SUSPEND && CONFIG_X86 */ > +static inline int acpi_get_lps0_constraint(struct device *dev) > +{ > + return -ENODEV; > +} > #endif /* CONFIG_SUSPEND && CONFIG_X86 */ > #ifndef CONFIG_IA64 > void arch_reserve_mem_area(acpi_physical_address addr, size_t size); > -- > 2.34.1 >
On Fri, Aug 18, 2023 at 10:31:03AM +0200, Rafael J. Wysocki wrote: > On Fri, Aug 18, 2023 at 7:15 AM Mario Limonciello > <mario.limonciello@amd.com> wrote: ... > > +int acpi_get_lps0_constraint(struct device *dev) > > I think that some overhead would be reduced below if this were taking > a struct acpi_device pointer as the argument. Hmm... Either you need a pointer to handle, which involves pointer arithmetics or something else. I would believe if you tell that ACPI handle should be passed, but current suggestion is not obvious to me how it may help. > > +{ > > + struct lpi_constraints *entry; > > + > > + for_each_lpi_constraint(entry) { > > + if (!device_match_acpi_handle(dev, entry->handle)) Here we retrieve handle... > > + continue; > > + acpi_handle_debug(entry->handle, > > + "ACPI device constraint: %d\n", entry->min_dstate); > > + return entry->min_dstate; > > + } > > + dev_dbg(dev, "No ACPI device constraint specified\n"); ...and here we are using dev directly (otherwise acpi_handle_dbg() should be used). > > + return -ENODEV; > > ACPI_STATE_UNKNOWN? > > > +}
On 8/18/2023 05:47, Andy Shevchenko wrote: > On Fri, Aug 18, 2023 at 10:31:03AM +0200, Rafael J. Wysocki wrote: >> On Fri, Aug 18, 2023 at 7:15 AM Mario Limonciello >> <mario.limonciello@amd.com> wrote: > > ... > >>> +int acpi_get_lps0_constraint(struct device *dev) >> >> I think that some overhead would be reduced below if this were taking >> a struct acpi_device pointer as the argument. > > Hmm... Either you need a pointer to handle, which involves pointer arithmetics > or something else. I would believe if you tell that ACPI handle should be passed, > but current suggestion is not obvious to me how it may help. To Rafael's point about overhead there are potentially "less" calls into acpi_get_lps0_constraint if it's a 'struct acpi_device' pointer because it won't be called by caller for any devices that don't have an ACPI companion. > >>> +{ >>> + struct lpi_constraints *entry; >>> + >>> + for_each_lpi_constraint(entry) { >>> + if (!device_match_acpi_handle(dev, entry->handle)) > > Here we retrieve handle... > >>> + continue; >>> + acpi_handle_debug(entry->handle, >>> + "ACPI device constraint: %d\n", entry->min_dstate); >>> + return entry->min_dstate; >>> + } > >>> + dev_dbg(dev, "No ACPI device constraint specified\n"); > > ...and here we are using dev directly (otherwise acpi_handle_dbg() should be used). I'll just move the debugging statements into the caller of acpi_get_lps0_constraint(). > >>> + return -ENODEV; >> >> ACPI_STATE_UNKNOWN? Much better, thanks. >> >>> +} >
On Fri, Aug 18, 2023 at 4:04 PM Mario Limonciello <mario.limonciello@amd.com> wrote: > > On 8/18/2023 05:47, Andy Shevchenko wrote: > > On Fri, Aug 18, 2023 at 10:31:03AM +0200, Rafael J. Wysocki wrote: > >> On Fri, Aug 18, 2023 at 7:15 AM Mario Limonciello > >> <mario.limonciello@amd.com> wrote: > > > > ... > > > >>> +int acpi_get_lps0_constraint(struct device *dev) > >> > >> I think that some overhead would be reduced below if this were taking > >> a struct acpi_device pointer as the argument. > > > > Hmm... Either you need a pointer to handle, which involves pointer arithmetics > > or something else. I would believe if you tell that ACPI handle should be passed, > > but current suggestion is not obvious to me how it may help. > > To Rafael's point about overhead there are potentially "less" calls into > acpi_get_lps0_constraint if it's a 'struct acpi_device' pointer because > it won't be called by caller for any devices that don't have an ACPI > companion. > > > > >>> +{ > >>> + struct lpi_constraints *entry; > >>> + > >>> + for_each_lpi_constraint(entry) { > >>> + if (!device_match_acpi_handle(dev, entry->handle)) > > > > Here we retrieve handle... Which uses ACPI_HANDLE() to retrieve the companion ACPI handle for dev. This checks dev against NULL and then passes it to ACPI_COMPANION() which resolves to to_acpi_device_node() on the dev's fwnode field and that involves an is_acpi_device_node() check and some pointer arithmetic via container_of(). Of course, this needs to be done in every iteration of the loop until a matching handle is found (or not), and because dev is always the same, the result of this will be the same every time. If this is not pointless overhead then I'm not quite sure how to call it. Now, if struct acpi_device *adev is passed as the argument, the check above reduces to (adev->handle == entry->handle) which is much more straightforward IMV. > > > >>> + continue; > >>> + acpi_handle_debug(entry->handle, > >>> + "ACPI device constraint: %d\n", entry->min_dstate); > >>> + return entry->min_dstate; > >>> + } > > > >>> + dev_dbg(dev, "No ACPI device constraint specified\n"); > > > > ...and here we are using dev directly (otherwise acpi_handle_dbg() should be used). > > I'll just move the debugging statements into the caller of > acpi_get_lps0_constraint(). Yeah, that works too.
On Fri, Aug 18, 2023 at 05:38:15PM +0200, Rafael J. Wysocki wrote: > On Fri, Aug 18, 2023 at 4:04 PM Mario Limonciello > <mario.limonciello@amd.com> wrote: > > On 8/18/2023 05:47, Andy Shevchenko wrote: > > > On Fri, Aug 18, 2023 at 10:31:03AM +0200, Rafael J. Wysocki wrote: > > >> On Fri, Aug 18, 2023 at 7:15 AM Mario Limonciello > > >> <mario.limonciello@amd.com> wrote: ... > > >> I think that some overhead would be reduced below if this were taking > > >> a struct acpi_device pointer as the argument. > > > > > > Hmm... Either you need a pointer to handle, which involves pointer arithmetics > > > or something else. I would believe if you tell that ACPI handle should be passed, > > > but current suggestion is not obvious to me how it may help. > > > > To Rafael's point about overhead there are potentially "less" calls into > > acpi_get_lps0_constraint if it's a 'struct acpi_device' pointer because > > it won't be called by caller for any devices that don't have an ACPI > > companion. > > > > >>> + struct lpi_constraints *entry; > > >>> + > > >>> + for_each_lpi_constraint(entry) { > > >>> + if (!device_match_acpi_handle(dev, entry->handle)) > > > > > > Here we retrieve handle... > > Which uses ACPI_HANDLE() to retrieve the companion ACPI handle for > dev. This checks dev against NULL and then passes it to > ACPI_COMPANION() which resolves to to_acpi_device_node() on the dev's > fwnode field and that involves an is_acpi_device_node() check and some > pointer arithmetic via container_of(). Of course, this needs to be > done in every iteration of the loop until a matching handle is found > (or not), and because dev is always the same, the result of this will > be the same every time. If this is not pointless overhead then I'm not > quite sure how to call it. > > Now, if struct acpi_device *adev is passed as the argument, the check > above reduces to (adev->handle == entry->handle) which is much more > straightforward IMV. Yes, this is fine, and if we move out dev_dbg() call, the suggestion makes even more sense. I agree with your arguments.
On Fri, Aug 18, 2023 at 5:38 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Fri, Aug 18, 2023 at 4:04 PM Mario Limonciello > <mario.limonciello@amd.com> wrote: > > > > On 8/18/2023 05:47, Andy Shevchenko wrote: > > > On Fri, Aug 18, 2023 at 10:31:03AM +0200, Rafael J. Wysocki wrote: > > >> On Fri, Aug 18, 2023 at 7:15 AM Mario Limonciello > > >> <mario.limonciello@amd.com> wrote: > > > > > > ... > > > > > >>> +int acpi_get_lps0_constraint(struct device *dev) > > >> > > >> I think that some overhead would be reduced below if this were taking > > >> a struct acpi_device pointer as the argument. Besides, I don't think that the constraints should be checked directly from pci_bridge_d3_possible(). I'd rather check them from acpi_pci_bridge_d3() which knows the ACPI companion of the given device already and can use it directly.
diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c index 1aa3cd5677bd8..dd961f3a60577 100644 --- a/drivers/acpi/x86/s2idle.c +++ b/drivers/acpi/x86/s2idle.c @@ -299,6 +299,30 @@ static void lpi_device_get_constraints(void) ACPI_FREE(out_obj); } +/** + * acpi_get_lps0_constraint - get any LPS0 constraint for a device + * @dev: device to get constraints for + * + * Returns: + * - the value for constraint. + * - Otherwise, -ENODEV. + */ +int acpi_get_lps0_constraint(struct device *dev) +{ + struct lpi_constraints *entry; + + for_each_lpi_constraint(entry) { + if (!device_match_acpi_handle(dev, entry->handle)) + continue; + acpi_handle_debug(entry->handle, + "ACPI device constraint: %d\n", entry->min_dstate); + return entry->min_dstate; + } + dev_dbg(dev, "No ACPI device constraint specified\n"); + + return -ENODEV; +} + static void lpi_check_constraints(void) { struct lpi_constraints *entry; diff --git a/include/linux/acpi.h b/include/linux/acpi.h index f1552c04a2856..6745535444c76 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -1109,6 +1109,12 @@ struct acpi_s2idle_dev_ops { }; int acpi_register_lps0_dev(struct acpi_s2idle_dev_ops *arg); void acpi_unregister_lps0_dev(struct acpi_s2idle_dev_ops *arg); +int acpi_get_lps0_constraint(struct device *dev); +#else /* CONFIG_SUSPEND && CONFIG_X86 */ +static inline int acpi_get_lps0_constraint(struct device *dev) +{ + return -ENODEV; +} #endif /* CONFIG_SUSPEND && CONFIG_X86 */ #ifndef CONFIG_IA64 void arch_reserve_mem_area(acpi_physical_address addr, size_t size);
Other parts of the kernel may use constraints information to make decisions on what power state to put a device into. Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- v12->v13: * Drop checking for enabled, just return constraints v11->v12: * Use for_each_lpi_constraint instead * use CONFIG_SUSPEND instead of CONFIG_ACPI_SLEEP v9->v10: * split from other patches * kerneldoc fixes * move debug statement to this function --- drivers/acpi/x86/s2idle.c | 24 ++++++++++++++++++++++++ include/linux/acpi.h | 6 ++++++ 2 files changed, 30 insertions(+)