| Message ID | 20220408184844.22829-4-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v6,1/5] device property: Allow error pointer to be passed to fwnode APIs | expand |
On Wed, Apr 13, 2022 at 05:35:46PM +0300, Sakari Ailus wrote: > On Sun, Apr 10, 2022 at 05:10:23PM +0300, Andy Shevchenko wrote: > > On Sat, Apr 9, 2022 at 2:35 AM Sakari Ailus > > <sakari.ailus@linux.intel.com> wrote: > > > On Fri, Apr 08, 2022 at 09:48:43PM +0300, Andy Shevchenko wrote: > > > > As to_of_node() suggests and the way the code in the OF and software node > > > > back ends actually uses the fwnode handle, it may be constified. Do this > > > > for good. > > > > > > How? > > > > > > If the fwnode is const, then the struct it contains must be presumed to be > > > const, too. > > > > Why? The idea is that we are not updating the fwnode, but the container. > > The container may or may not be const. It's orthogonal, no? > > As you wrote: may or may not. The stricter requirement, i.e. const, must be > thus followed. I think it would be fine (after adding a comment on what is > being done) if you *know* the container struct is not const. But that is > not the case here. But even with the original code one may not guarantee that. How the original code works or prevents of using a const container against non-const fwnode pointer?
On Wed, Apr 13, 2022 at 08:10:22PM +0200, Rafael J. Wysocki wrote: > On Fri, Apr 8, 2022 at 8:49 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: ... > > -struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode) > > +struct fwnode_handle *fwnode_handle_get(const struct fwnode_handle *fwnode) > > { > > if (!fwnode_has_op(fwnode, get)) > > return fwnode; ^^^^, so it needs a casting, but then we have to comment why is so. > Why is 0-day complaining about this one?
On Thu, Apr 14, 2022 at 3:09 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Apr 14, 2022 at 12:47:20AM +0300, Sakari Ailus wrote: > > On Wed, Apr 13, 2022 at 09:23:17PM +0300, Andy Shevchenko wrote: > > > On Wed, Apr 13, 2022 at 09:19:28PM +0300, Andy Shevchenko wrote: > > > > On Wed, Apr 13, 2022 at 08:10:22PM +0200, Rafael J. Wysocki wrote: > > > > > On Fri, Apr 8, 2022 at 8:49 PM Andy Shevchenko > > > > > <andriy.shevchenko@linux.intel.com> wrote: > > ... > > > > > > > -struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode) > > > > > > +struct fwnode_handle *fwnode_handle_get(const struct fwnode_handle *fwnode) > > > > > > { > > > > > > > > > > if (!fwnode_has_op(fwnode, get)) > > > > > > return fwnode; > > > > > > > > ^^^^, so it needs a casting, but then we have to comment why is so. > > > > > > Note, it means that the fwnode parameter either invalid or has no given option. > > > It's not a problem to drop casting in the first case, but the second one should > > > be justified and Sakari wants to be sure that the initial container is not > > > const, which seems can't be achieved even with the original code. > > > > I wonder if I'm missing something. The fwnode argument originally was not > > const here. > > Yes, and our discussion went to the direction of what const qualifier implies > here. I assume that the const means that we do not modify the fwnode object, > while its container is another story which we have no influence on. You, if > I read your messages correctly, insisting that const here implies that the > container object is const as well. > > Reading current implementation I see now, that with children APIs we have > two pointers passed, while with parent APIs only a single one. In children > API due to above is easy to use const qualifier for the first argument. > Parent APIs missed that and hence have this problem that we can't constify > their parameters. > > to_of_node() expects const parameter while returns non-const container. > Is it a subtle issue there? (I believe it should be consistent then) This is fine AFAICS. The const parameter means that to_of_node() will not update the memory pointed to by it, which is correct. The value returned by it may be used by its caller in whatever way they like, because the caller has no obligation to preserve the memory pointed to by it, unless they've also received that pointer with the const qualifier, but then they need to know how it is related to the to_of_node() return value and what to do with it. IOW, to_of_node() has no information on its caller's obligations with respect to the memory pointed to by its argument, so it is OK for it to return a non-const result. Moreover, if it had done otherwise, it might have created an obligation for the caller that didn't exist before. > This patch and the followed one can be moved without understanding why > we need the non-const parameter there. I'm not sure what you mean here, sorry.
diff --git a/drivers/base/property.c b/drivers/base/property.c index 36401cfe432c..1ad4b37cd312 100644 --- a/drivers/base/property.c +++ b/drivers/base/property.c @@ -776,7 +776,7 @@ EXPORT_SYMBOL_GPL(device_get_named_child_node); * * Returns the fwnode handle. */ -struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode) +struct fwnode_handle *fwnode_handle_get(const struct fwnode_handle *fwnode) { if (!fwnode_has_op(fwnode, get)) return fwnode; diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c index b0bbd1805970..84a11008ffb8 100644 --- a/drivers/base/swnode.c +++ b/drivers/base/swnode.c @@ -363,7 +363,7 @@ EXPORT_SYMBOL_GPL(property_entries_free); /* -------------------------------------------------------------------------- */ /* fwnode operations */ -static struct fwnode_handle *software_node_get(struct fwnode_handle *fwnode) +static struct fwnode_handle *software_node_get(const struct fwnode_handle *fwnode) { struct swnode *swnode = to_swnode(fwnode); diff --git a/drivers/of/property.c b/drivers/of/property.c index 9a50ad25906e..8d06282af8e4 100644 --- a/drivers/of/property.c +++ b/drivers/of/property.c @@ -858,7 +858,7 @@ struct device_node *of_graph_get_remote_node(const struct device_node *node, } EXPORT_SYMBOL(of_graph_get_remote_node); -static struct fwnode_handle *of_fwnode_get(struct fwnode_handle *fwnode) +static struct fwnode_handle *of_fwnode_get(const struct fwnode_handle *fwnode) { return of_fwnode_handle(of_node_get(to_of_node(fwnode))); } diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h index 9a81c4410b9f..a94bd47192a3 100644 --- a/include/linux/fwnode.h +++ b/include/linux/fwnode.h @@ -108,7 +108,7 @@ struct fwnode_reference_args { * zero on success, a negative error code otherwise. */ struct fwnode_operations { - struct fwnode_handle *(*get)(struct fwnode_handle *fwnode); + struct fwnode_handle *(*get)(const struct fwnode_handle *fwnode); void (*put)(struct fwnode_handle *fwnode); bool (*device_is_available)(const struct fwnode_handle *fwnode); const void *(*device_get_match_data)(const struct fwnode_handle *fwnode, diff --git a/include/linux/property.h b/include/linux/property.h index fc24d45632eb..c631ee7fd161 100644 --- a/include/linux/property.h +++ b/include/linux/property.h @@ -121,7 +121,7 @@ struct fwnode_handle *fwnode_get_named_child_node( struct fwnode_handle *device_get_named_child_node(struct device *dev, const char *childname); -struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode); +struct fwnode_handle *fwnode_handle_get(const struct fwnode_handle *fwnode); void fwnode_handle_put(struct fwnode_handle *fwnode); int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index);
As to_of_node() suggests and the way the code in the OF and software node back ends actually uses the fwnode handle, it may be constified. Do this for good. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- v6: new patch drivers/base/property.c | 2 +- drivers/base/swnode.c | 2 +- drivers/of/property.c | 2 +- include/linux/fwnode.h | 2 +- include/linux/property.h | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-)