Hi Grant, On Nov 14, 2013, at 2:44 AM, Grant Likely wrote: > On Wed, 13 Nov 2013 10:03:37 +0100, Pantelis Antoniou <panto@xxxxxxxxxxxxxxxxxxxxxxx> wrote: >> On Nov 13, 2013, at 2:34 AM, Grant Likely wrote: >>> On Tue, 12 Nov 2013 11:39:08 +0100, Pantelis Antoniou <panto@xxxxxxxxxxxxxxxxxxxxxxx> wrote: >>>>> On Tue, 5 Nov 2013 19:50:16 +0200, Pantelis Antoniou <panto@xxxxxxxxxxxxxxxxxxxxxxx> wrote: >>>>>> + } else { >>>>>> + pr_warn("%s: node %p cannot be freed; memory is gone\n", >>>>>> + __func__, node); >>>>>> + } >>>>>> +} >>>>> >>>>> All of the above is potentially dangerous. There is no way to determine >>>>> if anything still holds a reference to a node. The proper way to handle >>>>> removal of properties is to have a release method when the last >>>>> of_node_put is called. >>>>> >>>> >>>> This is safe, and expected to be called only on a dynamically created tree, >>>> that's what all the checks against OF_DYNAMIC guard against. >>>> >>>> It is not ever meant to be called on an arbitrary tree, created by unflattening >>>> a blob. >>> >>> I am talking about when being used on a dynamic tree. The problem is >>> when a driver or other code holds a reference to a dynamic nodes, but >>> doesn't release it correctly. The memory must not be freed until all of >>> the references are relased. OF_DYNAMIC doesn't actually help in that >>> case, and it is the reason for of_node_get()/of_node_put() >>> >> >> I know, but even that is not enough. of_node_get()/of_node_put() handles the >> case of references to the nodes, but not what happens with references to >> properties. deadprops is mitigating the problem somewhat, but if we're going >> to go to all the trouble of kobjectification let's do the props as well. >> >> of_get_property could be modified to return a devm_kmalloced copy of the real >> property and that would deal with most of the callers. Of course for >> the small sized scalar data we can avoid the copy. >> >> By using the devm_* interface we also avoid having to mess too much with the callers. >> >> I.e. what about something like devm_of_get_property()? > > Reference counting is already a horrible pain to keep correct. I don't > see a better way to handle it in the dynamic case, so we're stuck with > it, but I don't want to make it any harder. Adding ref counting to > properties will make it harder than it already is to get the code right. > I'm absolutely fine with a little bit of wasted memory in the form of > deadprops when the alternative is so horrible. References at the node > level is enough granularity. > > I don't think kduping the property is the solution either. I strongly > suspect that will be far more expensive than the deadprop solution. > As long as we can live with deadprops all is fine. Perhaps a devm_of_get_property() makes sense for new drivers though? What do you think? Perhaps copying to a user supplied buffer as well? >>>>>> +{ >>>>>> + struct device_node *node; >>>>>> + >>>>>> + node = kzalloc(sizeof(*node), flags); >>>>>> + if (node == NULL) >>>>>> + return NULL; >>>>>> + >>>>>> + node->name = kstrdup(name, flags); >>>>>> + if (node->name == NULL) >>>>>> + goto err_return; >>>>>> + >>>>>> + node->type = kstrdup(type, flags); >>>>>> + if (node->type == NULL) >>>>>> + goto err_return; >>>>>> + >>>>>> + node->full_name = kstrdup(full_name, flags); >>>>>> + if (node->type == NULL) >>>>>> + goto err_return; >>>>> >>>>> Again, who do you expect the user of this function to be? If it is part >>>>> of unflattening an overlay tree, is there a reason that the passed in >>>>> names cannot be used directly instead of kmallocing them? >>>>> >>>> >>>> I want to be able to get rid of the blob eventually; I don't need to keep >>>> dragging it around. >>> >>> Why? It really doesn't hurt and it means data does not need to be >>> copied. >> >> Copying data lead to less problems that having to drag that blob around. >> That's just preference, so not a big issue. > > Can you elaborate? What problems do you foresee being created by keeping > the blob? It's a kind of drag. That means you get handed a device_node pointer you are not able to free it without having the blob along with the container/accessor of it. I.e. For the normal case where the blob comes from a request_firmware() call You have to keep the firmware structure around. Depending on what other method you're going to use tends to make the code a little bit messier. > > g. Regards -- Pantelis -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html