On 8/17/14, 5:49, "Grant Likely" [off-list ref] wrote:

Hi Mika and Rafael,

Comments below...

On Sun, 17 Aug 2014 09:04:13 +0300, Mika Westerberg
[off-list ref] wrote:

quoted

From: "Rafael J. Wysocki" <redacted>

Add a uniform interface by which device drivers can request device
properties from the platform firmware by providing a property name
and the corresponding data type.

Three general helper functions, device_property_get(),
device_property_read() and device_property_read_array() are provided.
The first one allows the raw value of a given device property to be
accessed by the driver.  The remaining two allow the value of a numeric
or string property and multiple numeric or string values of one array
property to be acquired, respectively.

The interface is supposed to cover both ACPI and Device Trees,
although the ACPI part is only implemented at this time.

Nit:
s/at this time/in this change. The DT component is implemented in a
subsequent patch./

I almost complained that the DT portion must be implemented before this
series can even be considered, but then I found it in patch 4/9.  :-)

quoted

Signed-off-by: Rafael J. Wysocki <redacted>
Signed-off-by: Aaron Lu <redacted>
Reviewed-by: Darren Hart <redacted>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/acpi/glue.c      |   4 +-
 drivers/acpi/property.c  | 179
++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/acpi/scan.c      |  12 +++-
 drivers/base/Makefile    |   2 +-
 drivers/base/property.c  |  48 +++++++++++++
 include/acpi/acpi_bus.h  |   1 +
 include/linux/device.h   |   3 +
 include/linux/property.h |  50 +++++++++++++
 8 files changed, 293 insertions(+), 6 deletions(-)
 create mode 100644 drivers/base/property.c
 create mode 100644 include/linux/property.h

...

quoted

 static void acpi_device_del(struct acpi_device *device)
 {
 	mutex_lock(&acpi_device_lock);
-	if (device->parent)
+	if (device->parent) {
 		list_del(&device->node);
+		device->parent->child_count--;

I worry about housekeeping like this. It is easy for bugs to slip in.
Unless returning the child count is in a critical path (which it
shouldn't be), I would drop the child_count member and simply count the
number of items in the list when required.

This of course is not my subsystem, so I won't ACK or NACK the patch over
this.

This would be consistent with of_get_child_count() and is unlikely to be
called much more than once per device. The maintenance however is limited
to the add/del functions, so it doesn't seem difficult to maintain. I have
no objections to just walking the list though if that is preferable.

...

quoted

@@ -701,6 +702,7 @@ struct acpi_dev_node {
  * @archdata:	For arch-specific additions.
  * @of_node:	Associated device tree node.
  * @acpi_node:	Associated ACPI device node.
+ * @property_ops: Firmware interface for device properties
  * @devt:	For creating the sysfs "dev".
  * @id:		device instance
  * @devres_lock: Spinlock to protect the resource of the device.

@@ -777,6 +779,7 @@ struct device {
 
 	struct device_node	*of_node; /* associated device tree node */
 	struct acpi_dev_node	acpi_node; /* associated ACPI device node */
+	struct dev_prop_ops	*property_ops;

There are only 2 users of this interface. I don't think adding an ops
pointer to each and every struct device is warrented when the wrapper
function can check if of_node or acpi_node is set and call the
appropriate helper. It is unlikely anything else will use this hook. It
will result in smaller memory footprint. Also smaller code when only one
of
CONFIG_OF and CONFIG_ACPI are selected, which is almost always. :-)

It can be refactored later if that ever changes.

Our intent was to eliminate the #ifdefery in every one of the accessors.
It was my understanding the ops structures were preferable in such
situations. For a 64-bit machine with 1000 devices (all of which use
device properties) with one or the other of ACPI/OF enabled, the
additional memory requirement here is what... Something like (8*1000 + 4)
~= 8KB ? That seems worth the arguably more maintainable code to me. Is
there more to it than this, am I missing some more significant impact?

  

quoted

 	dev_t			devt;	/* dev_t, creates the sysfs "dev" */
 	u32			id;	/* device instance */

diff --git a/include/linux/property.h b/include/linux/property.h
new file mode 100644
index 000000000000..52ea7fe7fe09
--- /dev/null
+++ b/include/linux/property.h

@@ -0,0 +1,50 @@
+/*
+ * property.h - Unified device property interface.
+ *
+ * Copyright (C) 2014, Intel Corporation
+ * Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef _LINUX_PROPERTY_H_
+#define _LINUX_PROPERTY_H_
+
+#include <linux/device.h>
+#include <linux/acpi.h>
+#include <linux/of.h>
+
+enum dev_prop_type {
+	DEV_PROP_U8,
+	DEV_PROP_U16,
+	DEV_PROP_U32,
+	DEV_PROP_U64,
+	DEV_PROP_STRING,
+	DEV_PROP_MAX,
+};
+
+struct dev_prop_ops {
+	int (*get)(struct device *dev, const char *propname, void **valptr);
+	int (*read)(struct device *dev, const char *propname,
+		    enum dev_prop_type proptype, void *val);
+	int (*read_array)(struct device *dev, const char *propname,
+			  enum dev_prop_type proptype, void *val, size_t nval);

The associated DT functions that implement property reads
(of_property_read_*) were created in part to provide some type safety
when reading properties. This proposed API throws that away by accepting
a void* for the data field, which I don't want to do. This API either
needs to have a separate accessor for each data type, or it needs some
other mechanism (accessor macros?) to ensure the right type is passed
in.

OK, this would increase the memory impact by 6-fold for 8,16,32,and 64
byte integers, strings, and device references if additional typed function
pointers were added to the dev_prop_ops. The macros could mitigate this I
suppose.

Type checking and validation was something we had discussed as being
important to this mechanism. I believe there was some interest in seeing
this done at the ASL/FDT + Schema level - but that's not justification for
maintaining it in the kernel interface as well.

Could this be addressed by adding an enum dev_prop_type to the get/read
calls similar to that of the read_array call? That would keep the call
count down, maintain the smaller memory footprint, and still allow for
type verification within the device properties API.

-- 
Darren Hart					Open Source Technology Center
darren.hart@intel.com				            Intel Corporation