On Thu, 2013-03-21 at 11:24 +0000, Grant Likely wrote:
> Device tree nodes are already treated as objects, and we already want to
> expose them to userspace which is done using the /proc filesystem today.
> Right now the kernel has to do a lot of work to keep the /proc view in
> sync with the in-kernel representation. If device_nodes are switched to
> be kobjects then the device tree code can be a whole lot simpler. It
> also turns out that switching to using /sysfs from /proc results in
> smaller code and data size, and the userspace ABI won't change if
> /proc/device-tree symlinks to /sys/device-tree
Here you say /sys/device-tree
> +What: /sys/firmware/ofw/../device-tree/
Here you say /sys/firmware/../device-tree/ ... (wtf are those .. ?)
And further down:
proc_symlink("device-tree", NULL, "/sys/firmware/ofw/device-tree-0");
Some confusion here ... at least _I_ am confused :-)
Then, you do this:
> +static bool of_init_complete = false;
The above requires some explanations
> +static int __of_node_add(struct device_node *np)
> +{
> +
> + const char *name;
> + struct property *pp;
> + static int extra = 0;
> + int rc;
> +
> + np->kobj.kset = of_kset;
> + if (!np->parent) {
> + /* Nodes without parents are new top level trees */
> + rc = kobject_add(&np->kobj, NULL, "device-tree-%i", extra++);
> +#if !defined(CONFIG_PROC_DEVICETREE)
> + /* Symlink to the new tree when PROC_DEVICETREE is disabled */
> + if (!rc && extra == 1)
> + proc_symlink("device-tree", NULL,
> "/sys/firmware/ofw/device-tree-0");
> +#endif /* CONFIG_PROC_DEVICETREE */
WTF is this business of having multiple top level trees ? Also that
local static extra is gross. What is this all about ?
> + } else {
> + name = kbasename(np->full_name);
> + if (!name || !name[0])
> + return -EINVAL;
> + rc = kobject_add(&np->kobj, &np->parent->kobj, "%s", name);
> + }
> + if (rc)
> + return rc;
> +
> + for_each_property_of_node(np, pp) {
> + /* Important: Don't leak passwords */
> + bool secure = strncmp(pp->name, "security-", 9) == 0;
> +
> + pp->attr.attr.name = pp->name;
> + pp->attr.attr.mode = secure ? S_IRUSR : S_IRUGO;
> + pp->attr.size = secure ? 0 : pp->length;
> + pp->attr.read = of_node_property_read;
> + rc = sysfs_create_bin_file(&np->kobj, &pp->attr);
> + WARN(rc, "error creating device node attribute\n");
Might want some better message (attribute name, node path, ...)
We have mechanisms to deal with collisions in proc devicetree that you
don't seem to have here (or am I missing something ?). The main source
of pain is a property and a child node having the same name (happens
regulary with l2-cache on macs for example).
Cheers,
Ben.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
