Questions about of/base.c:of_update_property()

Questions about of/base.c:of_update_property()

[Guenter Roeck]

Hi folks,

looking into drivers/of/base.c, I notices a couple of oddities in
of_update_property(). Both were interoduced with commit 75b57ecf
(of: Make device nodes kobjects so they show up in sysfs).
In both cases, there is an unnecessary value check. Sending
a patch to remove the checks would be simple, but I want to
make sure that the checks are not supposed to address some other
condition.

1) Unnecessary check if rc != 0

Here are all the uses of the 'rc' variable in of_update_property():

         rc = of_property_notify(OF_RECONFIG_UPDATE_PROPERTY, np, newprop);
         if (rc)
                 return rc;
	...
	while (*next) {
		...
	}
	raw_spin_unlock_irqrestore(&devtree_lock, flags);
         if (rc)
                 return rc;

What is the purpose of the second check ? Can it be removed,
or is there some other condition which should have been checked
instead ? Should the check possibly be "if (!found)" ?

2) Unnecessary check if oldprop != NULL

	oldprop = of_find_property(np, newprop->name, NULL);
         if (!oldprop)
                 return of_add_property(np, newprop);
	...
	/* Update the sysfs attribute */
         if (oldprop)
                 sysfs_remove_bin_file(&np->kobj, &oldprop->attr);

oldprop is not modified between the two checks. Can the check be
removed, or is there some other condition which should have been
checked instead ?

3) At the very end of the function, there is a check

        if (!found)
                 return -ENODEV;

This is _after_ the sysfs entry for the old property was removed,
and the sysfs entry for the new property was created. Yet, '!found'
suggests that there is some inconsistency in the property, so I wonder
if the check should happen earlier, before sysfs entries are touched.

Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Questions about of/base.c:of_update_property()

[Rob Herring]

On Thu, Apr 10, 2014 at 8:18 PM, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
> Hi folks,
>
> looking into drivers/of/base.c, I notices a couple of oddities in
> of_update_property(). Both were interoduced with commit 75b57ecf
> (of: Make device nodes kobjects so they show up in sysfs).
> In both cases, there is an unnecessary value check. Sending
> a patch to remove the checks would be simple, but I want to
> make sure that the checks are not supposed to address some other
> condition.
>
> 1) Unnecessary check if rc != 0
>
> Here are all the uses of the 'rc' variable in of_update_property():
>
>         rc = of_property_notify(OF_RECONFIG_UPDATE_PROPERTY, np, newprop);
>         if (rc)
>                 return rc;
>         ...
>         while (*next) {
>                 ...
>         }
>         raw_spin_unlock_irqrestore(&devtree_lock, flags);
>         if (rc)
>                 return rc;
>
> What is the purpose of the second check ? Can it be removed,
> or is there some other condition which should have been checked
> instead ? Should the check possibly be "if (!found)" ?

Looks like cut 'n paste from of_add_property. I believe the second one
should be removed and !found should be checked before updating sysfs.

>
> 2) Unnecessary check if oldprop != NULL
>
>         oldprop = of_find_property(np, newprop->name, NULL);
>         if (!oldprop)
>                 return of_add_property(np, newprop);
>         ...
>         /* Update the sysfs attribute */
>         if (oldprop)
>                 sysfs_remove_bin_file(&np->kobj, &oldprop->attr);
>
> oldprop is not modified between the two checks. Can the check be
> removed, or is there some other condition which should have been
> checked instead ?

Yes, it can be removed.

> 3) At the very end of the function, there is a check
>
>        if (!found)
>                 return -ENODEV;
>
> This is _after_ the sysfs entry for the old property was removed,
> and the sysfs entry for the new property was created. Yet, '!found'
> suggests that there is some inconsistency in the property, so I wonder
> if the check should happen earlier, before sysfs entries are touched.

Right.

Can you send a patch fixing these.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html