Re: [PATCH v13 09/12] ACPI: x86: s2idle: Add a function to get constraints for a device

[Rafael J. Wysocki]

On Fri, Aug 18, 2023 at 4:04 PM Mario Limonciello
<mario.limonciello@xxxxxxx> 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@xxxxxxx> 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.