lkml.org 
[lkml]   [2013]   [Mar]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH V2 1/2] of: Make device nodes kobjects so they show up in sysfs
    From
    Date
    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.




    \
     
     \ /
      Last update: 2013-03-21 14:42    [W:3.276 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site