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. -- Dmitry index 0ca3c0908b0c..3b4c394138e2 100644 --- a/drivers/base/property.c +++ b/drivers/base/property.c @@ -18,6 +18,28 @@ #include <linux/string.h> #include <linux/types.h> +#define FWNODE_ITERATE(n, result, cont_val, op, ...) \ +({ \ + 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; \ +}) + struct fwnode_handle *__dev_fwnode(struct device *dev) { return IS_ENABLED(CONFIG_OF) && dev->of_node ? @@ -57,16 +79,14 @@ EXPORT_SYMBOL_GPL(device_property_present); bool fwnode_property_present(const struct fwnode_handle *fwnode, const char *propname) { + int error; bool ret; - if (IS_ERR_OR_NULL(fwnode)) + error = FWNODE_ITERATE(fwnode, ret, false, property_present, propname); + if (error) return false; - ret = fwnode_call_bool_op(fwnode, property_present, propname); - if (ret) - return ret; - - return fwnode_call_bool_op(fwnode->secondary, property_present, propname); + return ret; } EXPORT_SYMBOL_GPL(fwnode_property_present); @@ -259,18 +279,15 @@ static int fwnode_property_read_int_array(const struct fwnode_handle *fwnode, unsigned int elem_size, void *val, size_t nval) { + int error; int ret; - if (IS_ERR_OR_NULL(fwnode)) - return -EINVAL; - - ret = fwnode_call_int_op(fwnode, property_read_int_array, propname, - elem_size, val, nval); - if (ret != -EINVAL) - return ret; + error = FWNODE_ITERATE(fwnode, ret, -EINVAL, property_read_int_array, + propname, elem_size, val, nval); + if (error) + return error; - return fwnode_call_int_op(fwnode->secondary, property_read_int_array, propname, - elem_size, val, nval); + return ret; } /** @@ -414,18 +431,15 @@ int fwnode_property_read_string_array(const struct fwnode_handle *fwnode, const char *propname, const char **val, size_t nval) { + int error; int ret; - if (IS_ERR_OR_NULL(fwnode)) - return -EINVAL; - - ret = fwnode_call_int_op(fwnode, property_read_string_array, propname, - val, nval); - if (ret != -EINVAL) - return ret; + error = FWNODE_ITERATE(fwnode, ret, -EINVAL, property_read_string_array, + propname, val, nval); + if (error) + return error; - return fwnode_call_int_op(fwnode->secondary, property_read_string_array, propname, - val, nval); + return ret; } EXPORT_SYMBOL_GPL(fwnode_property_read_string_array);