Questions about of/base.c:of_update_property()
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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 25 lines · Guenter Roeck
Hi folks,
looking into
drivers/of/base.c, I notices a couple of oddities inof_update_property(). Both were interoduced with commit75b57ecf(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;
Quoted 3 lines
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.
Quoted 11 lines
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);
Quoted 3 lines
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.
Quoted 9 lines
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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html