| Message ID | 20220408184844.22829-4-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v6,1/5] device property: Allow error pointer to be passed to fwnode APIs | expand |
Hi Andy, 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. > > 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(-) > > 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); > -- > 2.35.1 >
Hi Andy,
I love your patch! Yet something to improve:
[auto build test ERROR on driver-core/driver-core-testing]
[also build test ERROR on rafael-pm/linux-next robh/for-next linus/master v5.18-rc1 next-20220408]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/device-property-Allow-error-pointer-to-be-passed-to-fwnode-APIs/20220409-025056
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git 3123109284176b1532874591f7c81f3837bbdc17
config: hexagon-randconfig-r041-20220408 (https://download.01.org/0day-ci/archive/20220409/202204091049.rHkbqiFm-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project c29a51b3a257908aebc01cd7c4655665db317d66)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/477683439b5ee0954b08970d8c356b4cdaca8bc0
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Andy-Shevchenko/device-property-Allow-error-pointer-to-be-passed-to-fwnode-APIs/20220409-025056
git checkout 477683439b5ee0954b08970d8c356b4cdaca8bc0
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/base/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> drivers/base/property.c:782:10: error: returning 'const struct fwnode_handle *' from a function with result type 'struct fwnode_handle *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
return fwnode;
^~~~~~
1 error generated.
vim +782 drivers/base/property.c
613e97218ccbd7 Adam Thomson 2016-06-21 772
e7887c284969a2 Sakari Ailus 2017-03-28 773 /**
e7887c284969a2 Sakari Ailus 2017-03-28 774 * fwnode_handle_get - Obtain a reference to a device node
e7887c284969a2 Sakari Ailus 2017-03-28 775 * @fwnode: Pointer to the device node to obtain the reference to.
cf89a31ca55272 Sakari Ailus 2017-09-19 776 *
cf89a31ca55272 Sakari Ailus 2017-09-19 777 * Returns the fwnode handle.
e7887c284969a2 Sakari Ailus 2017-03-28 778 */
477683439b5ee0 Andy Shevchenko 2022-04-08 779 struct fwnode_handle *fwnode_handle_get(const struct fwnode_handle *fwnode)
e7887c284969a2 Sakari Ailus 2017-03-28 780 {
cf89a31ca55272 Sakari Ailus 2017-09-19 781 if (!fwnode_has_op(fwnode, get))
cf89a31ca55272 Sakari Ailus 2017-09-19 @782 return fwnode;
cf89a31ca55272 Sakari Ailus 2017-09-19 783
cf89a31ca55272 Sakari Ailus 2017-09-19 784 return fwnode_call_ptr_op(fwnode, get);
e7887c284969a2 Sakari Ailus 2017-03-28 785 }
e7887c284969a2 Sakari Ailus 2017-03-28 786 EXPORT_SYMBOL_GPL(fwnode_handle_get);
e7887c284969a2 Sakari Ailus 2017-03-28 787
Hi Andy,
I love your patch! Perhaps something to improve:
[auto build test WARNING on driver-core/driver-core-testing]
[also build test WARNING on rafael-pm/linux-next robh/for-next linus/master v5.18-rc1 next-20220408]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/device-property-Allow-error-pointer-to-be-passed-to-fwnode-APIs/20220409-025056
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git 3123109284176b1532874591f7c81f3837bbdc17
config: um-i386_defconfig (https://download.01.org/0day-ci/archive/20220409/202204091133.KMBmLNSx-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.2.0-19) 11.2.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/477683439b5ee0954b08970d8c356b4cdaca8bc0
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Andy-Shevchenko/device-property-Allow-error-pointer-to-be-passed-to-fwnode-APIs/20220409-025056
git checkout 477683439b5ee0954b08970d8c356b4cdaca8bc0
# save the config file to linux build tree
mkdir build_dir
make W=1 O=build_dir ARCH=um SUBARCH=i386 SHELL=/bin/bash drivers/base/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/base/property.c: In function 'fwnode_handle_get':
>> drivers/base/property.c:782:24: warning: return discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
782 | return fwnode;
| ^~~~~~
vim +/const +782 drivers/base/property.c
613e97218ccbd7f Adam Thomson 2016-06-21 772
e7887c284969a23 Sakari Ailus 2017-03-28 773 /**
e7887c284969a23 Sakari Ailus 2017-03-28 774 * fwnode_handle_get - Obtain a reference to a device node
e7887c284969a23 Sakari Ailus 2017-03-28 775 * @fwnode: Pointer to the device node to obtain the reference to.
cf89a31ca55272e Sakari Ailus 2017-09-19 776 *
cf89a31ca55272e Sakari Ailus 2017-09-19 777 * Returns the fwnode handle.
e7887c284969a23 Sakari Ailus 2017-03-28 778 */
477683439b5ee09 Andy Shevchenko 2022-04-08 779 struct fwnode_handle *fwnode_handle_get(const struct fwnode_handle *fwnode)
e7887c284969a23 Sakari Ailus 2017-03-28 780 {
cf89a31ca55272e Sakari Ailus 2017-09-19 781 if (!fwnode_has_op(fwnode, get))
cf89a31ca55272e Sakari Ailus 2017-09-19 @782 return fwnode;
cf89a31ca55272e Sakari Ailus 2017-09-19 783
cf89a31ca55272e Sakari Ailus 2017-09-19 784 return fwnode_call_ptr_op(fwnode, get);
e7887c284969a23 Sakari Ailus 2017-03-28 785 }
e7887c284969a23 Sakari Ailus 2017-03-28 786 EXPORT_SYMBOL_GPL(fwnode_handle_get);
e7887c284969a23 Sakari Ailus 2017-03-28 787
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?
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.
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 Fri, Apr 8, 2022 at 8:49 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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. > > 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(-) > > 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); > -- Why is 0-day complaining about this one?
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 Wed, Apr 13, 2022 at 6:58 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > 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? I don't think that this is the point here. If const struct fwnode_handle *fwnode is passed to the ->get() callback, the callback itself (and any function called by it) shouldn't attempt to update the memory pointed to by fwnode. It need not be the memory starting at the fwnode address IIUC, so that would cover the whole object the fwnode is embedded in. This way the caller of ->get() can assume the immutability of the memory passed to it for read access. The question is whether or not ->get() needs to update the memory in question. If it doesn't, making fwnode const is correct.
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. > > Why is 0-day complaining about this one?
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.
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 patch and the followed one can be moved without understanding why we need the non-const parameter there.
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(-)