diff mbox series

[v6,4/5] device property: Constify fwnode_handle_get()

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

Commit Message

Andy Shevchenko April 8, 2022, 6:48 p.m. UTC
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(-)

Comments

Sakari Ailus April 8, 2022, 9:36 p.m. UTC | #1
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
>
kernel test robot April 9, 2022, 2:38 a.m. UTC | #2
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
kernel test robot April 9, 2022, 3:39 a.m. UTC | #3
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
Andy Shevchenko April 10, 2022, 2:10 p.m. UTC | #4
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?
Sakari Ailus April 13, 2022, 2:35 p.m. UTC | #5
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.
Andy Shevchenko April 13, 2022, 4:54 p.m. UTC | #6
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?
Rafael J. Wysocki April 13, 2022, 6:10 p.m. UTC | #7
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?
Andy Shevchenko April 13, 2022, 6:19 p.m. UTC | #8
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?
Rafael J. Wysocki April 13, 2022, 6:21 p.m. UTC | #9
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.
Andy Shevchenko April 13, 2022, 6:23 p.m. UTC | #10
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?
Sakari Ailus April 13, 2022, 9:47 p.m. UTC | #11
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.
Andy Shevchenko April 14, 2022, 1:09 p.m. UTC | #12
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.
Rafael J. Wysocki April 15, 2022, 3:40 p.m. UTC | #13
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 mbox series

Patch

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);