On Sat, 2017-08-12 at 16:27 +0200, Rafael J. Wysocki wrote:
[...]
> > +
> > +struct lpi_constraints {
> > + char *name;
> > + int min_dstate;
> If you store the handle here as well, you won't need to
> look it up every time _check_constraints() is called.
The reason I didn't keep handle here, I thought handle can be stale or
change for PnP device on plug in and out. Is this not true?
> >
> > +};
> > +
> > +static struct lpi_constraints *lpi_constraints_table;
> > +static int lpi_constraints_table_size;
> > +
> > +static void lpi_device_get_constraints(void)
> > +{
> > + union acpi_object *out_obj;
> > + int i;
> > +
> > + out_obj = acpi_evaluate_dsm_typed(lps0_device_handle,
> > &lps0_dsm_guid,
> > + 1,
> > ACPI_LPS0_GET_DEVICE_CONSTRAINTS,
> > + NULL,
> > ACPI_TYPE_PACKAGE);
> > +
> > + acpi_handle_debug(lps0_device_handle, "_DSM function 1
> > eval %s\n",
> > + out_obj ? "successful" : "failed");
> > +
> > + if (!out_obj)
> > + return;
> > +
> > + lpi_constraints_table = kcalloc(out_obj->package.count,
> > + sizeof(*lpi_constraints_ta
> > ble),
> > + GFP_KERNEL);
> > + if (!lpi_constraints_table)
> > + goto free_acpi_buffer;
> > +
> > + pr_debug("LPI: constraints dump begin\n");
> Please add an empty line after this. Also something like
> "constraints
> list begin" would sound better IMO.
>
OK.
> >
> > + for (i = 0; i < out_obj->package.count; i++) {
> > + union acpi_object *package = &out_obj-
> > >package.elements[i];
> > + struct lpi_device_info info = { };
> > + int package_count = 0, j;
> > +
> > + if (!package)
> > + continue;
> > +
> > + for (j = 0; j < package->package.count; ++j) {
> > + union acpi_object *element =
> > + &(package-
> > >package.elements[j]);
> > +
> > + switch (element->type) {
> > + case ACPI_TYPE_INTEGER:
> > + info.enabled = element-
> > >integer.value;
> > + break;
> > + case ACPI_TYPE_STRING:
> > + info.name = element-
> > >string.pointer;
> > + break;
> > + case ACPI_TYPE_PACKAGE:
> > + package_count = element-
> > >package.count;
> > + info.package = element-
> > >package.elements;
> > + break;
> > + }
> > + }
> > +
> > + if (!info.enabled || !info.package || !info.name)
> > + continue;
> > +
> I would evaluate acpi_get_handle() here and store the handle in the
> constraints table (if persent). And you can skip the entry
> altogether
> if not present, because you won't be printing it anyway.
>
> >
> > + lpi_constraints_table[lpi_constraints_table_size].
> > name =
> > + kstrdup(info.name,
> > GFP_KERNEL);
> > + if
> > (!lpi_constraints_table[lpi_constraints_table_size].name)
> > + goto free_constraints;
> > +
> > + pr_debug("index:%d Name:%s\n", i, info.name);
> > +
> > + for (j = 0; j < package_count; ++j) {
> > + union acpi_object *info_obj =
> > &info.package[j];
> > + union acpi_object *cnstr_pkg;
> > + union acpi_object *obj;
> > + struct lpi_device_constraint dev_info;
> > +
> > + switch (info_obj->type) {
> > + case ACPI_TYPE_INTEGER:
> > + /* version */
> > + break;
> > + case ACPI_TYPE_PACKAGE:
> > + if (info_obj->package.count < 2)
> > + break;
> > +
> > + cnstr_pkg = info_obj-
> > >package.elements;
> > + obj = &cnstr_pkg[0];
> > + dev_info.uid = obj->integer.value;
> > + obj = &cnstr_pkg[1];
> > + dev_info.min_dstate = obj-
> > >integer.value;
> > + pr_debug("uid %d min_dstate %d\n",
> > + dev_info.uid,
> > + dev_info.min_dstate);
> > + lpi_constraints_table[
> > + lpi_constraints_table_size
> > ].min_dstate =
> > + dev_info.min_dstat
> > e;
> > + break;
> > + }
> > + }
> > +
> > + lpi_constraints_table_size++;
> > + }
> > +
> > + pr_debug("LPI: constraints dump end\n");
> > +free_acpi_buffer:
> > + ACPI_FREE(out_obj);
> > + return;
> > +
> > +free_constraints:
> > + ACPI_FREE(out_obj);
> > + for (i = 0; i < lpi_constraints_table_size; ++i)
> > + kfree(lpi_constraints_table[i].name);
> > + kfree(lpi_constraints_table);
> > + lpi_constraints_table_size = 0;
> > +}
> > +
> > +static void lpi_check_constraints(void)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < lpi_constraints_table_size; ++i) {
> > + acpi_handle handle;
> > + struct acpi_device *adev;
> > + int state, ret;
> > +
> > + if (ACPI_FAILURE(acpi_get_handle(NULL,
> > + lpi_constraints_t
> > able[i].name,
> > + &handle)))
> > + continue;
> So if you store the handle, the above won't be necessary.
>
> >
> > +
> > + if (acpi_bus_get_device(handle, &adev))
> > + continue;
> > +
> > + ret = acpi_device_get_power(adev, &state);
> > + if (ret)
> > + pr_debug("LPI: %s required min power state
> > %d, current power state %d, real power state [ERROR]\n",
> > + lpi_constraints_table[i].name,
> > + lpi_constraints_table[i].min_dsta
> > te,
> > + adev->power.state);
> > + else
> > + pr_debug("LPI: %s required min power state
> > %d, current power state %d, real power state %d\n",
> > + lpi_constraints_table[i].name,
> > + lpi_constraints_table[i].min_dsta
> > te,
> > + adev->power.state, state);
> I'm not convinced about the value of the above TBH.
>
> Also in theory _PSC may go and access things like PCI config spaces
> of devices
> which isn't a good idea for devices in D3_cold, so maybe skip this?
OK.
>
> >
> > +
> > + if (adev->flags.power_manageable && adev-
> > >power.state <
> > + lpi_constraints_table[i].m
> > in_dstate)
> > + pr_info("LPI: Constraint [%s] not
> > matched\n",
> "Constrant [%s] not met"?
>
> Also I'd use acpi_handle_info(adev->handle, ...) to print this.
>
> >
> > + lpi_constraints_table[i].name);
> I would print a message if !flags.power_manageable, because it means
> we
> can't get the constraint right in general.
>
> Also I would print both power.state and min_state (possibly using
> acpi_power_state_string()) in this message as that is valuable for
> debugging.
OK.
Thanks,
Srinivas
--
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
