| Message ID | 20241128053937.4076797-1-dmitry.torokhov@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] device property: do not leak child nodes when using NULL/error pointers | expand |
On Wed, Nov 27, 2024 at 09:39:34PM -0800, Dmitry Torokhov wrote: > The documentation to various API calls that locate children for a given > fwnode (such as fwnode_get_next_available_child_node() or > device_get_next_child_node()) states that the reference to the node > passed in "child" argument is dropped unconditionally, however the > change that added checks for the main node to be NULL or error pointer > broke this promise. > > Add missing fwnode_handle_put() calls to restore the documented > behavior. > > Fixes: 002752af7b89 ("device property: Allow error pointer to be passed to fwnode APIs") > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > drivers/base/property.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/base/property.c b/drivers/base/property.c > index 837d77e3af2b..696ba43b8e8a 100644 > --- a/drivers/base/property.c > +++ b/drivers/base/property.c > @@ -759,6 +759,12 @@ struct fwnode_handle * > fwnode_get_next_child_node(const struct fwnode_handle *fwnode, > struct fwnode_handle *child) > { > + if (IS_ERR_OR_NULL(fwnode) || > + !fwnode_has_op(fwnode, get_next_child_node)) { > + fwnode_handle_put(child); > + return NULL; > + } > + > return fwnode_call_ptr_op(fwnode, get_next_child_node, child); > } > EXPORT_SYMBOL_GPL(fwnode_get_next_child_node); > @@ -778,9 +784,6 @@ fwnode_get_next_available_child_node(const struct fwnode_handle *fwnode, > { > struct fwnode_handle *next_child = child; > > - if (IS_ERR_OR_NULL(fwnode)) > - return NULL; > - > do { > next_child = fwnode_get_next_child_node(fwnode, next_child); > if (!next_child) > @@ -806,8 +809,10 @@ struct fwnode_handle *device_get_next_child_node(const struct device *dev, > const struct fwnode_handle *fwnode = dev_fwnode(dev); > struct fwnode_handle *next; > > - if (IS_ERR_OR_NULL(fwnode)) > + if (IS_ERR_OR_NULL(fwnode)) { > + fwnode_handle_put(child); > return NULL; > + } > > /* Try to find a child in primary fwnode */ > next = fwnode_get_next_child_node(fwnode, child); > -- > 2.47.0.338.g60cca15819-goog > Hi, This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him a patch that has triggered this response. He used to manually respond to these common problems, but in order to save his sanity (he kept writing the same thing over and over, yet to different people), I was created. Hopefully you will not take offence and will fix the problem in your patch and resubmit it so that it can be accepted into the Linux kernel tree. You are receiving this message because of the following common error(s) as indicated below: - You have marked a patch with a "Fixes:" tag for a commit that is in an older released kernel, yet you do not have a cc: stable line in the signed-off-by area at all, which means that the patch will not be applied to any older kernel releases. To properly fix this, please follow the documented rules in the Documentation/process/stable-kernel-rules.rst file for how to resolve this. If you wish to discuss this problem further, or you have questions about how to resolve this issue, please feel free to respond to this email and Greg will reply once he has dug out from the pending patches received from other developers. thanks, greg k-h's patch email bot
On Wed, Nov 27, 2024 at 09:39:34PM -0800, Dmitry Torokhov wrote: > The documentation to various API calls that locate children for a given > fwnode (such as fwnode_get_next_available_child_node() or > device_get_next_child_node()) states that the reference to the node > passed in "child" argument is dropped unconditionally, however the > change that added checks for the main node to be NULL or error pointer > broke this promise. This commit message doesn't explain a use case. Hence it might be just a documentation issue, please elaborate. > Add missing fwnode_handle_put() calls to restore the documented > behavior. ... While at it, please fix the kernel-doc (missing Return section). > { > + if (IS_ERR_OR_NULL(fwnode) || Unneeded check as fwnode_has_op() has it already. > + !fwnode_has_op(fwnode, get_next_child_node)) { > + fwnode_handle_put(child); > + return NULL; > + } > return fwnode_call_ptr_op(fwnode, get_next_child_node, child); Now it's useless to call the macro, you can simply take the direct call. > } ... > @@ struct fwnode_handle *device_get_next_child_node(const struct device *dev, > const struct fwnode_handle *fwnode = dev_fwnode(dev); > struct fwnode_handle *next; > - if (IS_ERR_OR_NULL(fwnode)) > + if (IS_ERR_OR_NULL(fwnode)) { > + fwnode_handle_put(child); > return NULL; > + } > /* Try to find a child in primary fwnode */ > next = fwnode_get_next_child_node(fwnode, child); So, why not just moving the original check (w/o dropping the reference) here? Wouldn't it have the same effect w/o explicit call to the fwnode_handle_put()?
On Thu, Nov 28, 2024 at 03:13:16PM +0200, Andy Shevchenko wrote: > On Wed, Nov 27, 2024 at 09:39:34PM -0800, Dmitry Torokhov wrote: > > The documentation to various API calls that locate children for a given > > fwnode (such as fwnode_get_next_available_child_node() or > > device_get_next_child_node()) states that the reference to the node > > passed in "child" argument is dropped unconditionally, however the > > change that added checks for the main node to be NULL or error pointer > > broke this promise. > > This commit message doesn't explain a use case. Hence it might be just > a documentation issue, please elaborate. I do not have a specific use case in mind, however the implementation behavior does not match the stated one, and so it makes sense to get it fixed. Otherwise callers would have to add checks to conditionally drop the reference to "child" argument in certain cases, which will complicate caller's code. > > > Add missing fwnode_handle_put() calls to restore the documented > > behavior. > > ... > > While at it, please fix the kernel-doc (missing Return section). OK. > > > { > > + if (IS_ERR_OR_NULL(fwnode) || > > Unneeded check as fwnode_has_op() has it already. Yes, it has, but that is not obvious nor it is a documented behavior of fwnode_has_op(). It also different semantics: it checks whether a fwnode implements a given operation, not whether fwnode is valid. That check is incidental in fwnode_has_op(). They all are macros so compiler should collapse duplicate checks, but if you feel really strongly about it I can drop IS_ERR_OR_NULL() check. > > > + !fwnode_has_op(fwnode, get_next_child_node)) { > > + fwnode_handle_put(child); > > + return NULL; > > + } > > > return fwnode_call_ptr_op(fwnode, get_next_child_node, child); > > Now it's useless to call the macro, you can simply take the direct call. OK, will change to a direct call. > > > } > > ... > > > @@ struct fwnode_handle *device_get_next_child_node(const struct device *dev, > > const struct fwnode_handle *fwnode = dev_fwnode(dev); > > struct fwnode_handle *next; > > > - if (IS_ERR_OR_NULL(fwnode)) > > + if (IS_ERR_OR_NULL(fwnode)) { > > + fwnode_handle_put(child); > > return NULL; > > + } > > > /* Try to find a child in primary fwnode */ > > next = fwnode_get_next_child_node(fwnode, child); > > So, why not just moving the original check (w/o dropping the reference) here? > Wouldn't it have the same effect w/o explicit call to the fwnode_handle_put()? Because if you rely on check in fwnode_get_next_child_node() you would not know if it returned NULL because there are no more children or because the node is invalid. In the latter case you can't dereference fwnode->secondary. Thanks.
On Thu, Nov 28, 2024 at 03:04:50PM -0800, Dmitry Torokhov wrote: > On Thu, Nov 28, 2024 at 03:13:16PM +0200, Andy Shevchenko wrote: > > On Wed, Nov 27, 2024 at 09:39:34PM -0800, Dmitry Torokhov wrote: > > > The documentation to various API calls that locate children for a given > > > fwnode (such as fwnode_get_next_available_child_node() or > > > device_get_next_child_node()) states that the reference to the node > > > passed in "child" argument is dropped unconditionally, however the > > > change that added checks for the main node to be NULL or error pointer > > > broke this promise. > > > > This commit message doesn't explain a use case. Hence it might be just > > a documentation issue, please elaborate. > > I do not have a specific use case in mind, however the implementation > behavior does not match the stated one, and so it makes sense to get it > fixed. Otherwise callers would have to add checks to conditionally drop > the reference to "child" argument in certain cases, which will > complicate caller's code. Perhaps this should be somewhere between the cover letter / commit message? > > > Add missing fwnode_handle_put() calls to restore the documented > > > behavior. ... > > > { > > > + if (IS_ERR_OR_NULL(fwnode) || > > > > Unneeded check as fwnode_has_op() has it already. > > Yes, it has, but that is not obvious nor it is a documented behavior of > fwnode_has_op(). Would like to document that then? > It also different semantics: it checks whether a fwnode > implements a given operation, not whether fwnode is valid. That check is > incidental in fwnode_has_op(). I kinda disagree on this. The invalid fwnode may not have any operations, so it's implied and will always be like that. > They all are macros so compiler should collapse duplicate checks, but if > you feel really strongly about it I can drop IS_ERR_OR_NULL() check. Yes, please drop it and rather we want fwnode_has_op() to be documented with main purpose and guaranteed side effect (the latter makes no need of duplication that I pointed out). > > > + !fwnode_has_op(fwnode, get_next_child_node)) { > > > + fwnode_handle_put(child); > > > + return NULL; > > > + } ... > > > @@ struct fwnode_handle *device_get_next_child_node(const struct device *dev, > > > const struct fwnode_handle *fwnode = dev_fwnode(dev); > > > struct fwnode_handle *next; > > > > > - if (IS_ERR_OR_NULL(fwnode)) > > > + if (IS_ERR_OR_NULL(fwnode)) { > > > + fwnode_handle_put(child); > > > return NULL; > > > + } > > > > > /* Try to find a child in primary fwnode */ > > > next = fwnode_get_next_child_node(fwnode, child); > > > > So, why not just moving the original check (w/o dropping the reference) here? > > Wouldn't it have the same effect w/o explicit call to the fwnode_handle_put()? > > Because if you rely on check in fwnode_get_next_child_node() you would > not know if it returned NULL because there are no more children or > because the node is invalid. In the latter case you can't dereference > fwnode->secondary. Yes, so, how does it contradict my proposal?
On Fri, Nov 29, 2024 at 04:50:15PM +0200, Andy Shevchenko wrote: > On Thu, Nov 28, 2024 at 03:04:50PM -0800, Dmitry Torokhov wrote: > > On Thu, Nov 28, 2024 at 03:13:16PM +0200, Andy Shevchenko wrote: > > > On Wed, Nov 27, 2024 at 09:39:34PM -0800, Dmitry Torokhov wrote: > > > > The documentation to various API calls that locate children for a given > > > > fwnode (such as fwnode_get_next_available_child_node() or > > > > device_get_next_child_node()) states that the reference to the node > > > > passed in "child" argument is dropped unconditionally, however the > > > > change that added checks for the main node to be NULL or error pointer > > > > broke this promise. > > > > > > This commit message doesn't explain a use case. Hence it might be just > > > a documentation issue, please elaborate. > > > > I do not have a specific use case in mind, however the implementation > > behavior does not match the stated one, and so it makes sense to get it > > fixed. Otherwise callers would have to add checks to conditionally drop > > the reference to "child" argument in certain cases, which will > > complicate caller's code. > > Perhaps this should be somewhere between the cover letter / commit message? OK, I thought that it was pretty obvious, but I will expand the commit message to include this. > > > > > Add missing fwnode_handle_put() calls to restore the documented > > > > behavior. > > ... > > > > > { > > > > + if (IS_ERR_OR_NULL(fwnode) || > > > > > > Unneeded check as fwnode_has_op() has it already. > > > > Yes, it has, but that is not obvious nor it is a documented behavior of > > fwnode_has_op(). > > Would like to document that then? > > > It also different semantics: it checks whether a fwnode > > implements a given operation, not whether fwnode is valid. That check is > > incidental in fwnode_has_op(). > > I kinda disagree on this. The invalid fwnode may not have any operations, > so it's implied and will always be like that. Yeah, it is clear that we disagree. I agree that invalid fwnode will not have an operation defined, still checking whether an operation is supported and whether a node is invalid or not are 2 different operations to me. But we do not need to argue further. > > > They all are macros so compiler should collapse duplicate checks, but if > > you feel really strongly about it I can drop IS_ERR_OR_NULL() check. > > Yes, please drop it and rather we want fwnode_has_op() to be documented with > main purpose and guaranteed side effect (the latter makes no need of > duplication that I pointed out). OK. > > > > > + !fwnode_has_op(fwnode, get_next_child_node)) { > > > > + fwnode_handle_put(child); > > > > + return NULL; > > > > + } > > ... > > > > > @@ struct fwnode_handle *device_get_next_child_node(const struct device *dev, > > > > const struct fwnode_handle *fwnode = dev_fwnode(dev); > > > > struct fwnode_handle *next; > > > > > > > - if (IS_ERR_OR_NULL(fwnode)) > > > > + if (IS_ERR_OR_NULL(fwnode)) { > > > > + fwnode_handle_put(child); > > > > return NULL; > > > > + } > > > > > > > /* Try to find a child in primary fwnode */ > > > > next = fwnode_get_next_child_node(fwnode, child); > > > > > > So, why not just moving the original check (w/o dropping the reference) here? > > > Wouldn't it have the same effect w/o explicit call to the fwnode_handle_put()? > > > > Because if you rely on check in fwnode_get_next_child_node() you would > > not know if it returned NULL because there are no more children or > > because the node is invalid. In the latter case you can't dereference > > fwnode->secondary. > > Yes, so, how does it contradict my proposal? I guess I misunderstood your proposal then. Could you please explain it in more detail? Thanks.
On Fri, Nov 29, 2024 at 11:16:54PM -0800, Dmitry Torokhov wrote: > On Fri, Nov 29, 2024 at 04:50:15PM +0200, Andy Shevchenko wrote: > > On Thu, Nov 28, 2024 at 03:04:50PM -0800, Dmitry Torokhov wrote: > > > On Thu, Nov 28, 2024 at 03:13:16PM +0200, Andy Shevchenko wrote: > > > > On Wed, Nov 27, 2024 at 09:39:34PM -0800, Dmitry Torokhov wrote: ... > > > > > @@ struct fwnode_handle *device_get_next_child_node(const struct device *dev, > > > > > const struct fwnode_handle *fwnode = dev_fwnode(dev); > > > > > struct fwnode_handle *next; > > > > > > > > > - if (IS_ERR_OR_NULL(fwnode)) > > > > > + if (IS_ERR_OR_NULL(fwnode)) { > > > > > + fwnode_handle_put(child); > > > > > return NULL; > > > > > + } > > > > > > > > > /* Try to find a child in primary fwnode */ > > > > > next = fwnode_get_next_child_node(fwnode, child); > > > > > > > > So, why not just moving the original check (w/o dropping the reference) here? > > > > Wouldn't it have the same effect w/o explicit call to the fwnode_handle_put()? > > > > > > Because if you rely on check in fwnode_get_next_child_node() you would > > > not know if it returned NULL because there are no more children or > > > because the node is invalid. In the latter case you can't dereference > > > fwnode->secondary. > > > > Yes, so, how does it contradict my proposal? > > I guess I misunderstood your proposal then. Could you please explain it > in more detail? Current code (in steps): if (IS_ERR_OR_NULL()) check trying primary trying secondary if previous is NULL My proposal trying primary return if not NULL if (IS_ERR_OR_NULL()) check in its current form (no put op) trying secondary After your first patch IIUC this is possible as trying primary will put child uncoditionally.
On Sat, Nov 30, 2024 at 11:44:04PM +0200, Andy Shevchenko wrote: > On Fri, Nov 29, 2024 at 11:16:54PM -0800, Dmitry Torokhov wrote: > > On Fri, Nov 29, 2024 at 04:50:15PM +0200, Andy Shevchenko wrote: > > > On Thu, Nov 28, 2024 at 03:04:50PM -0800, Dmitry Torokhov wrote: > > > > On Thu, Nov 28, 2024 at 03:13:16PM +0200, Andy Shevchenko wrote: > > > > > On Wed, Nov 27, 2024 at 09:39:34PM -0800, Dmitry Torokhov wrote: > > ... > > > > > > > @@ struct fwnode_handle *device_get_next_child_node(const struct device *dev, > > > > > > const struct fwnode_handle *fwnode = dev_fwnode(dev); > > > > > > struct fwnode_handle *next; > > > > > > > > > > > - if (IS_ERR_OR_NULL(fwnode)) > > > > > > + if (IS_ERR_OR_NULL(fwnode)) { > > > > > > + fwnode_handle_put(child); > > > > > > return NULL; > > > > > > + } > > > > > > > > > > > /* Try to find a child in primary fwnode */ > > > > > > next = fwnode_get_next_child_node(fwnode, child); > > > > > > > > > > So, why not just moving the original check (w/o dropping the reference) here? > > > > > Wouldn't it have the same effect w/o explicit call to the fwnode_handle_put()? > > > > > > > > Because if you rely on check in fwnode_get_next_child_node() you would > > > > not know if it returned NULL because there are no more children or > > > > because the node is invalid. In the latter case you can't dereference > > > > fwnode->secondary. > > > > > > Yes, so, how does it contradict my proposal? > > > > I guess I misunderstood your proposal then. Could you please explain it > > in more detail? > > > Current code (in steps): > if (IS_ERR_OR_NULL()) check > trying primary > trying secondary if previous is NULL > > > My proposal > > trying primary > return if not NULL > if (IS_ERR_OR_NULL()) check in its current form (no put op) > trying secondary > > After your first patch IIUC this is possible as trying primary will put child uncoditionally. Ah, I see. No, I do not think this is a good idea: it will make the code harder to understand for a casual reader: "Why do we check node validity only after we used it for the first time?" For the code not in a hot path there is a lot of value in simplicity. Thanks.
On Mon, Dec 02, 2024 at 09:49:06PM -0800, Dmitry Torokhov wrote: > On Sat, Nov 30, 2024 at 11:44:04PM +0200, Andy Shevchenko wrote: > > On Fri, Nov 29, 2024 at 11:16:54PM -0800, Dmitry Torokhov wrote: > > > On Fri, Nov 29, 2024 at 04:50:15PM +0200, Andy Shevchenko wrote: > > > > On Thu, Nov 28, 2024 at 03:04:50PM -0800, Dmitry Torokhov wrote: > > > > > On Thu, Nov 28, 2024 at 03:13:16PM +0200, Andy Shevchenko wrote: > > > > > > On Wed, Nov 27, 2024 at 09:39:34PM -0800, Dmitry Torokhov wrote: ... > > > > > > > @@ struct fwnode_handle *device_get_next_child_node(const struct device *dev, > > > > > > > const struct fwnode_handle *fwnode = dev_fwnode(dev); > > > > > > > struct fwnode_handle *next; > > > > > > > > > > > > > - if (IS_ERR_OR_NULL(fwnode)) > > > > > > > + if (IS_ERR_OR_NULL(fwnode)) { > > > > > > > + fwnode_handle_put(child); > > > > > > > return NULL; > > > > > > > + } > > > > > > > > > > > > > /* Try to find a child in primary fwnode */ > > > > > > > next = fwnode_get_next_child_node(fwnode, child); > > > > > > > > > > > > So, why not just moving the original check (w/o dropping the reference) here? > > > > > > Wouldn't it have the same effect w/o explicit call to the fwnode_handle_put()? > > > > > > > > > > Because if you rely on check in fwnode_get_next_child_node() you would > > > > > not know if it returned NULL because there are no more children or > > > > > because the node is invalid. In the latter case you can't dereference > > > > > fwnode->secondary. > > > > > > > > Yes, so, how does it contradict my proposal? > > > > > > I guess I misunderstood your proposal then. Could you please explain it > > > in more detail? > > > > > > Current code (in steps): > > if (IS_ERR_OR_NULL()) check > > trying primary > > trying secondary if previous is NULL > > > > > > My proposal > > > > trying primary > > return if not NULL > > if (IS_ERR_OR_NULL()) check in its current form (no put op) > > trying secondary > > > > After your first patch IIUC this is possible as trying primary will put child uncoditionally. > > Ah, I see. No, I do not think this is a good idea: it will make the code > harder to understand for a casual reader: "Why do we check node validity > only after we used it for the first time?" Theare a re already a few API calls there that are hard to understand, I spent some time on them to get it through and still got it wrong as this series shows. So, I don't think we anyhow change this. > For the code not in a hot path there is a lot of value in simplicity. If you really want to go to this rabbit hole, think how we can get rid of repetitive checks of the secondary or more if any in the future nodes in the list. So the basic idea is to have this all hidden (to some extent) behind the macro or alike. In the code it would be something as for node in primary, secondary, ... call the API if (okay) return result return error This will indeed help.
On Tue, Dec 03, 2024 at 03:27:31PM +0200, Andy Shevchenko wrote: > On Mon, Dec 02, 2024 at 09:49:06PM -0800, Dmitry Torokhov wrote: > > On Sat, Nov 30, 2024 at 11:44:04PM +0200, Andy Shevchenko wrote: > > > On Fri, Nov 29, 2024 at 11:16:54PM -0800, Dmitry Torokhov wrote: > > > > On Fri, Nov 29, 2024 at 04:50:15PM +0200, Andy Shevchenko wrote: > > > > > On Thu, Nov 28, 2024 at 03:04:50PM -0800, Dmitry Torokhov wrote: > > > > > > On Thu, Nov 28, 2024 at 03:13:16PM +0200, Andy Shevchenko wrote: > > > > > > > On Wed, Nov 27, 2024 at 09:39:34PM -0800, Dmitry Torokhov wrote: > > ... > > > > > > > > > @@ struct fwnode_handle *device_get_next_child_node(const struct device *dev, > > > > > > > > const struct fwnode_handle *fwnode = dev_fwnode(dev); > > > > > > > > struct fwnode_handle *next; > > > > > > > > > > > > > > > - if (IS_ERR_OR_NULL(fwnode)) > > > > > > > > + if (IS_ERR_OR_NULL(fwnode)) { > > > > > > > > + fwnode_handle_put(child); > > > > > > > > return NULL; > > > > > > > > + } > > > > > > > > > > > > > > > /* Try to find a child in primary fwnode */ > > > > > > > > next = fwnode_get_next_child_node(fwnode, child); > > > > > > > > > > > > > > So, why not just moving the original check (w/o dropping the reference) here? > > > > > > > Wouldn't it have the same effect w/o explicit call to the fwnode_handle_put()? > > > > > > > > > > > > Because if you rely on check in fwnode_get_next_child_node() you would > > > > > > not know if it returned NULL because there are no more children or > > > > > > because the node is invalid. In the latter case you can't dereference > > > > > > fwnode->secondary. > > > > > > > > > > Yes, so, how does it contradict my proposal? > > > > > > > > I guess I misunderstood your proposal then. Could you please explain it > > > > in more detail? > > > > > > > > > Current code (in steps): > > > if (IS_ERR_OR_NULL()) check > > > trying primary > > > trying secondary if previous is NULL > > > > > > > > > My proposal > > > > > > trying primary > > > return if not NULL > > > if (IS_ERR_OR_NULL()) check in its current form (no put op) > > > trying secondary > > > > > > After your first patch IIUC this is possible as trying primary will put child uncoditionally. > > > > Ah, I see. No, I do not think this is a good idea: it will make the code > > harder to understand for a casual reader: "Why do we check node validity > > only after we used it for the first time?" > > Theare a re already a few API calls there that are hard to understand, I spent > some time on them to get it through and still got it wrong as this series > shows. So, I don't think we anyhow change this. The fact that some code is confusing does not mean that we should add more confusing code. We will not fix everything at once, but we can make things better bit by bit. Look, the check where it is now makes total sense, you added it there yourself! It checks that we are dealing with a valid node and returns early. The intent is very easy to understand and the only thing that is missing is that "put" operation to satisfy the documented behavior. Anything more just makes things more complex for no good reason. > > > For the code not in a hot path there is a lot of value in simplicity. > > If you really want to go to this rabbit hole, think how we can get rid of > repetitive checks of the secondary or more if any in the future nodes in the > list. > > So the basic idea is to have this all hidden (to some extent) behind the macro > or alike. In the code it would be something as > > for node in primary, secondary, ... > call the API > if (okay) > return result > > return error > > This will indeed help. I think this will indeed help if we ever going to have more than primary and secondary nodes. It is also tricky if you want to transition seamlessly between different types of nodes (i.e. you have ACPI primary with OF overlay secondary with swnode as tertiary etc). And you probably want to add support for references between different typesof nodes (i.e. swnode being able to reference OF device node for example). This kind of rework is however out of scope of what I have time to do at the moment. Thanks.
On Tue, Dec 03, 2024 at 02:45:49PM -0800, Dmitry Torokhov wrote: > On Tue, Dec 03, 2024 at 03:27:31PM +0200, Andy Shevchenko wrote: > > On Mon, Dec 02, 2024 at 09:49:06PM -0800, Dmitry Torokhov wrote: > > > On Sat, Nov 30, 2024 at 11:44:04PM +0200, Andy Shevchenko wrote: > > > > On Fri, Nov 29, 2024 at 11:16:54PM -0800, Dmitry Torokhov wrote: > > > > > On Fri, Nov 29, 2024 at 04:50:15PM +0200, Andy Shevchenko wrote: > > > > > > On Thu, Nov 28, 2024 at 03:04:50PM -0800, Dmitry Torokhov wrote: > > > > > > > On Thu, Nov 28, 2024 at 03:13:16PM +0200, Andy Shevchenko wrote: > > > > > > > > On Wed, Nov 27, 2024 at 09:39:34PM -0800, Dmitry Torokhov wrote: ... > > > > > > > > > @@ struct fwnode_handle *device_get_next_child_node(const struct device *dev, > > > > > > > > > const struct fwnode_handle *fwnode = dev_fwnode(dev); > > > > > > > > > struct fwnode_handle *next; > > > > > > > > > > > > > > > > > - if (IS_ERR_OR_NULL(fwnode)) > > > > > > > > > + if (IS_ERR_OR_NULL(fwnode)) { > > > > > > > > > + fwnode_handle_put(child); > > > > > > > > > return NULL; > > > > > > > > > + } > > > > > > > > > > > > > > > > > /* Try to find a child in primary fwnode */ > > > > > > > > > next = fwnode_get_next_child_node(fwnode, child); > > > > > > > > > > > > > > > > So, why not just moving the original check (w/o dropping the reference) here? > > > > > > > > Wouldn't it have the same effect w/o explicit call to the fwnode_handle_put()? > > > > > > > > > > > > > > Because if you rely on check in fwnode_get_next_child_node() you would > > > > > > > not know if it returned NULL because there are no more children or > > > > > > > because the node is invalid. In the latter case you can't dereference > > > > > > > fwnode->secondary. > > > > > > > > > > > > Yes, so, how does it contradict my proposal? > > > > > > > > > > I guess I misunderstood your proposal then. Could you please explain it > > > > > in more detail? > > > > > > > > > > > > Current code (in steps): > > > > if (IS_ERR_OR_NULL()) check > > > > trying primary > > > > trying secondary if previous is NULL > > > > > > > > > > > > My proposal > > > > > > > > trying primary > > > > return if not NULL > > > > if (IS_ERR_OR_NULL()) check in its current form (no put op) > > > > trying secondary > > > > > > > > After your first patch IIUC this is possible as trying primary will put child uncoditionally. > > > > > > Ah, I see. No, I do not think this is a good idea: it will make the code > > > harder to understand for a casual reader: "Why do we check node validity > > > only after we used it for the first time?" > > > > Theare a re already a few API calls there that are hard to understand, I spent > > some time on them to get it through and still got it wrong as this series > > shows. So, I don't think we anyhow change this. > > The fact that some code is confusing does not mean that we should add > more confusing code. We will not fix everything at once, but we can make > things better bit by bit. > > Look, the check where it is now makes total sense, you added it there > yourself! It checks that we are dealing with a valid node and returns > early. The intent is very easy to understand and the only thing that is > missing is that "put" operation to satisfy the documented behavior. > Anything more just makes things more complex for no good reason. Right, that's why I think we need to go away from open coding the iteration over the list of nodes (primary, secondary, etc). > > > For the code not in a hot path there is a lot of value in simplicity. > > > > If you really want to go to this rabbit hole, think how we can get rid of > > repetitive checks of the secondary or more if any in the future nodes in the > > list. > > > > So the basic idea is to have this all hidden (to some extent) behind the macro > > or alike. In the code it would be something as > > > > for node in primary, secondary, ... > > call the API > > if (okay) > > return result > > > > return error > > > > This will indeed help. > > I think this will indeed help if we ever going to have more than primary > and secondary nodes. It is also tricky if you want to transition > seamlessly between different types of nodes (i.e. you have ACPI primary > with OF overlay secondary with swnode as tertiary etc). And you probably > want to add support for references between different typesof nodes > (i.e. swnode being able to reference OF device node for example). > > This kind of rework is however out of scope of what I have time to do at > the moment. I am not asking you to invest into big rework, the idea is to try to fold the iterations to a kind of loop. Is it feasible? Or maybe it can be partially done, so the above becomes something like call_prmary_op(fwnode, ...) if (IS_ERR_OR_NULL(fwnode)) ... else call_op() call_secondary_op(fwnode, ...) if (IS_ERR_OR_NULL(fwnode)) ... else call_op() (with the potential of collapsing one into the other) and then the above next = call_primary_op(wnode, ...); if (next) return next; return call_secondary_op(fwnode, ...);
On Wed, Dec 04, 2024 at 03:16:34AM +0200, Andy Shevchenko wrote: > On Tue, Dec 03, 2024 at 02:45:49PM -0800, Dmitry Torokhov wrote: > > On Tue, Dec 03, 2024 at 03:27:31PM +0200, Andy Shevchenko wrote: > > > On Mon, Dec 02, 2024 at 09:49:06PM -0800, Dmitry Torokhov wrote: > > > > On Sat, Nov 30, 2024 at 11:44:04PM +0200, Andy Shevchenko wrote: > > > > > On Fri, Nov 29, 2024 at 11:16:54PM -0800, Dmitry Torokhov wrote: > > > > > > On Fri, Nov 29, 2024 at 04:50:15PM +0200, Andy Shevchenko wrote: > > > > > > > On Thu, Nov 28, 2024 at 03:04:50PM -0800, Dmitry Torokhov wrote: > > > > > > > > On Thu, Nov 28, 2024 at 03:13:16PM +0200, Andy Shevchenko wrote: > > > > > > > > > On Wed, Nov 27, 2024 at 09:39:34PM -0800, Dmitry Torokhov wrote: > > ... > > > > > > > > > > > @@ struct fwnode_handle *device_get_next_child_node(const struct device *dev, > > > > > > > > > > const struct fwnode_handle *fwnode = dev_fwnode(dev); > > > > > > > > > > struct fwnode_handle *next; > > > > > > > > > > > > > > > > > > > - if (IS_ERR_OR_NULL(fwnode)) > > > > > > > > > > + if (IS_ERR_OR_NULL(fwnode)) { > > > > > > > > > > + fwnode_handle_put(child); > > > > > > > > > > return NULL; > > > > > > > > > > + } > > > > > > > > > > > > > > > > > > > /* Try to find a child in primary fwnode */ > > > > > > > > > > next = fwnode_get_next_child_node(fwnode, child); > > > > > > > > > > > > > > > > > > So, why not just moving the original check (w/o dropping the reference) here? > > > > > > > > > Wouldn't it have the same effect w/o explicit call to the fwnode_handle_put()? > > > > > > > > > > > > > > > > Because if you rely on check in fwnode_get_next_child_node() you would > > > > > > > > not know if it returned NULL because there are no more children or > > > > > > > > because the node is invalid. In the latter case you can't dereference > > > > > > > > fwnode->secondary. > > > > > > > > > > > > > > Yes, so, how does it contradict my proposal? > > > > > > > > > > > > I guess I misunderstood your proposal then. Could you please explain it > > > > > > in more detail? > > > > > > > > > > > > > > > Current code (in steps): > > > > > if (IS_ERR_OR_NULL()) check > > > > > trying primary > > > > > trying secondary if previous is NULL > > > > > > > > > > > > > > > My proposal > > > > > > > > > > trying primary > > > > > return if not NULL > > > > > if (IS_ERR_OR_NULL()) check in its current form (no put op) > > > > > trying secondary > > > > > > > > > > After your first patch IIUC this is possible as trying primary will put child uncoditionally. > > > > > > > > Ah, I see. No, I do not think this is a good idea: it will make the code > > > > harder to understand for a casual reader: "Why do we check node validity > > > > only after we used it for the first time?" > > > > > > Theare a re already a few API calls there that are hard to understand, I spent > > > some time on them to get it through and still got it wrong as this series > > > shows. So, I don't think we anyhow change this. > > > > The fact that some code is confusing does not mean that we should add > > more confusing code. We will not fix everything at once, but we can make > > things better bit by bit. > > > > Look, the check where it is now makes total sense, you added it there > > yourself! It checks that we are dealing with a valid node and returns > > early. The intent is very easy to understand and the only thing that is > > missing is that "put" operation to satisfy the documented behavior. > > Anything more just makes things more complex for no good reason. > > Right, that's why I think we need to go away from open coding the iteration > over the list of nodes (primary, secondary, etc). > > > > > For the code not in a hot path there is a lot of value in simplicity. > > > > > > If you really want to go to this rabbit hole, think how we can get rid of > > > repetitive checks of the secondary or more if any in the future nodes in the > > > list. > > > > > > So the basic idea is to have this all hidden (to some extent) behind the macro > > > or alike. In the code it would be something as > > > > > > for node in primary, secondary, ... > > > call the API > > > if (okay) > > > return result > > > > > > return error > > > > > > This will indeed help. > > > > I think this will indeed help if we ever going to have more than primary > > and secondary nodes. It is also tricky if you want to transition > > seamlessly between different types of nodes (i.e. you have ACPI primary > > with OF overlay secondary with swnode as tertiary etc). And you probably > > want to add support for references between different typesof nodes > > (i.e. swnode being able to reference OF device node for example). > > > > This kind of rework is however out of scope of what I have time to do at > > the moment. > > I am not asking you to invest into big rework, the idea is to try to fold the > iterations to a kind of loop. Is it feasible? We could potentially do something like below. BTW, do you know why fwnode_property_get_reference_args() returns -ENOENT for NULL or error fwnode instead of -EINVAL as the rest of them? And would you object to unifying this? Thanks.
On Thu, Dec 05, 2024 at 12:57:38PM -0800, Dmitry Torokhov wrote: > On Wed, Dec 04, 2024 at 03:16:34AM +0200, Andy Shevchenko wrote: > > On Tue, Dec 03, 2024 at 02:45:49PM -0800, Dmitry Torokhov wrote: > > > On Tue, Dec 03, 2024 at 03:27:31PM +0200, Andy Shevchenko wrote: > > > > On Mon, Dec 02, 2024 at 09:49:06PM -0800, Dmitry Torokhov wrote: > > > > > On Sat, Nov 30, 2024 at 11:44:04PM +0200, Andy Shevchenko wrote: > > > > > > On Fri, Nov 29, 2024 at 11:16:54PM -0800, Dmitry Torokhov wrote: > > > > > > > On Fri, Nov 29, 2024 at 04:50:15PM +0200, Andy Shevchenko wrote: > > > > > > > > On Thu, Nov 28, 2024 at 03:04:50PM -0800, Dmitry Torokhov wrote: > > > > > > > > > On Thu, Nov 28, 2024 at 03:13:16PM +0200, Andy Shevchenko wrote: > > > > > > > > > > On Wed, Nov 27, 2024 at 09:39:34PM -0800, Dmitry Torokhov wrote: ... > > > > > > > > > > > @@ struct fwnode_handle *device_get_next_child_node(const struct device *dev, > > > > > > > > > > > const struct fwnode_handle *fwnode = dev_fwnode(dev); > > > > > > > > > > > struct fwnode_handle *next; > > > > > > > > > > > > > > > > > > > > > - if (IS_ERR_OR_NULL(fwnode)) > > > > > > > > > > > + if (IS_ERR_OR_NULL(fwnode)) { > > > > > > > > > > > + fwnode_handle_put(child); > > > > > > > > > > > return NULL; > > > > > > > > > > > + } > > > > > > > > > > > > > > > > > > > > > /* Try to find a child in primary fwnode */ > > > > > > > > > > > next = fwnode_get_next_child_node(fwnode, child); > > > > > > > > > > > > > > > > > > > > So, why not just moving the original check (w/o dropping the reference) here? > > > > > > > > > > Wouldn't it have the same effect w/o explicit call to the fwnode_handle_put()? > > > > > > > > > > > > > > > > > > Because if you rely on check in fwnode_get_next_child_node() you would > > > > > > > > > not know if it returned NULL because there are no more children or > > > > > > > > > because the node is invalid. In the latter case you can't dereference > > > > > > > > > fwnode->secondary. > > > > > > > > > > > > > > > > Yes, so, how does it contradict my proposal? > > > > > > > > > > > > > > I guess I misunderstood your proposal then. Could you please explain it > > > > > > > in more detail? > > > > > > > > > > > > > > > > > > Current code (in steps): > > > > > > if (IS_ERR_OR_NULL()) check > > > > > > trying primary > > > > > > trying secondary if previous is NULL > > > > > > > > > > > > > > > > > > My proposal > > > > > > > > > > > > trying primary > > > > > > return if not NULL > > > > > > if (IS_ERR_OR_NULL()) check in its current form (no put op) > > > > > > trying secondary > > > > > > > > > > > > After your first patch IIUC this is possible as trying primary will put child uncoditionally. > > > > > > > > > > Ah, I see. No, I do not think this is a good idea: it will make the code > > > > > harder to understand for a casual reader: "Why do we check node validity > > > > > only after we used it for the first time?" > > > > > > > > Theare a re already a few API calls there that are hard to understand, I spent > > > > some time on them to get it through and still got it wrong as this series > > > > shows. So, I don't think we anyhow change this. > > > > > > The fact that some code is confusing does not mean that we should add > > > more confusing code. We will not fix everything at once, but we can make > > > things better bit by bit. > > > > > > Look, the check where it is now makes total sense, you added it there > > > yourself! It checks that we are dealing with a valid node and returns > > > early. The intent is very easy to understand and the only thing that is > > > missing is that "put" operation to satisfy the documented behavior. > > > Anything more just makes things more complex for no good reason. > > > > Right, that's why I think we need to go away from open coding the iteration > > over the list of nodes (primary, secondary, etc). > > > > > > > For the code not in a hot path there is a lot of value in simplicity. > > > > > > > > If you really want to go to this rabbit hole, think how we can get rid of > > > > repetitive checks of the secondary or more if any in the future nodes in the > > > > list. > > > > > > > > So the basic idea is to have this all hidden (to some extent) behind the macro > > > > or alike. In the code it would be something as > > > > > > > > for node in primary, secondary, ... > > > > call the API > > > > if (okay) > > > > return result > > > > > > > > return error > > > > > > > > This will indeed help. > > > > > > I think this will indeed help if we ever going to have more than primary > > > and secondary nodes. It is also tricky if you want to transition > > > seamlessly between different types of nodes (i.e. you have ACPI primary > > > with OF overlay secondary with swnode as tertiary etc). And you probably > > > want to add support for references between different typesof nodes > > > (i.e. swnode being able to reference OF device node for example). > > > > > > This kind of rework is however out of scope of what I have time to do at > > > the moment. > > > > I am not asking you to invest into big rework, the idea is to try to fold the > > iterations to a kind of loop. Is it feasible? > > We could potentially do something like below. > > BTW, do you know why fwnode_property_get_reference_args() returns > -ENOENT for NULL or error fwnode instead of -EINVAL as the rest of them? I think we need to ask author, but I believe it's due to the OF analogue. (Haven't checked myself, though) > And would you object to unifying this? ... > +#define FWNODE_ITERATE(n, result, cont_val, op, ...) \ for_each_fwnode() or alike. > +({ \ > + int __ret = -EINVAL; \ > + typeof(result) __r; \ > + \ > + for (const struct fwnode_handle *__node = n; \ > + !IS_ERR_OR_NULL(__node); \ > + __node = __node->secondary) { \ > + if (!__node->ops || !__node->ops->op) { \ > + __ret = -ENXIO; \ > + continue; \ > + } \ > + __r = __node->ops->op(__node, ## __VA_ARGS__); \ > + if (__r != cont_val) { \ > + result = __r; \ > + __ret = 0; \ > + break; \ > + } \ > + } \ > + __ret; \ With a bit of polishing this may be the way to go. > +})
diff --git a/drivers/base/property.c b/drivers/base/property.c index 837d77e3af2b..696ba43b8e8a 100644 --- a/drivers/base/property.c +++ b/drivers/base/property.c @@ -759,6 +759,12 @@ struct fwnode_handle * fwnode_get_next_child_node(const struct fwnode_handle *fwnode, struct fwnode_handle *child) { + if (IS_ERR_OR_NULL(fwnode) || + !fwnode_has_op(fwnode, get_next_child_node)) { + fwnode_handle_put(child); + return NULL; + } + return fwnode_call_ptr_op(fwnode, get_next_child_node, child); } EXPORT_SYMBOL_GPL(fwnode_get_next_child_node); @@ -778,9 +784,6 @@ fwnode_get_next_available_child_node(const struct fwnode_handle *fwnode, { struct fwnode_handle *next_child = child; - if (IS_ERR_OR_NULL(fwnode)) - return NULL; - do { next_child = fwnode_get_next_child_node(fwnode, next_child); if (!next_child) @@ -806,8 +809,10 @@ struct fwnode_handle *device_get_next_child_node(const struct device *dev, const struct fwnode_handle *fwnode = dev_fwnode(dev); struct fwnode_handle *next; - if (IS_ERR_OR_NULL(fwnode)) + if (IS_ERR_OR_NULL(fwnode)) { + fwnode_handle_put(child); return NULL; + } /* Try to find a child in primary fwnode */ next = fwnode_get_next_child_node(fwnode, child);
The documentation to various API calls that locate children for a given fwnode (such as fwnode_get_next_available_child_node() or device_get_next_child_node()) states that the reference to the node passed in "child" argument is dropped unconditionally, however the change that added checks for the main node to be NULL or error pointer broke this promise. Add missing fwnode_handle_put() calls to restore the documented behavior. Fixes: 002752af7b89 ("device property: Allow error pointer to be passed to fwnode APIs") Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/base/property.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)