diff mbox series

[v2,1/5] device property: Keep dev_fwnode() and dev_fwnode_const() separate

Message ID 20220928105746.51208-2-andriy.shevchenko@linux.intel.com
State Superseded
Headers show
Series device property: Consitify a few APIs and correct dev_fwnode() | expand

Commit Message

Andy Shevchenko Sept. 28, 2022, 10:57 a.m. UTC
It's not fully correct to take a const parameter pointer to a struct
and return a non-const pointer to a member of that struct.

Instead, introduce a const version of the dev_fwnode() API which takes
and returns const pointers and use it where it's applicable.

Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Fixes: aade55c86033 ("device property: Add const qualifier to device_get_match_data() parameter")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/base/property.c  | 11 +++++++++--
 include/linux/property.h |  3 ++-
 2 files changed, 11 insertions(+), 3 deletions(-)

Comments

Sakari Ailus Sept. 30, 2022, 2:30 p.m. UTC | #1
Hi Greg,

On Wed, Sep 28, 2022 at 01:05:20PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Sep 28, 2022 at 01:57:42PM +0300, Andy Shevchenko wrote:
> > It's not fully correct to take a const parameter pointer to a struct
> > and return a non-const pointer to a member of that struct.
> > 
> > Instead, introduce a const version of the dev_fwnode() API which takes
> > and returns const pointers and use it where it's applicable.
> > 
> > Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Fixes: aade55c86033 ("device property: Add const qualifier to device_get_match_data() parameter")
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/base/property.c  | 11 +++++++++--
> >  include/linux/property.h |  3 ++-
> >  2 files changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/base/property.c b/drivers/base/property.c
> > index 4d6278a84868..699f1b115e0a 100644
> > --- a/drivers/base/property.c
> > +++ b/drivers/base/property.c
> > @@ -17,13 +17,20 @@
> >  #include <linux/property.h>
> >  #include <linux/phy.h>
> >  
> > -struct fwnode_handle *dev_fwnode(const struct device *dev)
> > +struct fwnode_handle *dev_fwnode(struct device *dev)
> >  {
> >  	return IS_ENABLED(CONFIG_OF) && dev->of_node ?
> >  		of_fwnode_handle(dev->of_node) : dev->fwnode;
> >  }
> >  EXPORT_SYMBOL_GPL(dev_fwnode);
> >  
> > +const struct fwnode_handle *dev_fwnode_const(const struct device *dev)
> > +{
> > +	return IS_ENABLED(CONFIG_OF) && dev->of_node ?
> > +		of_fwnode_handle(dev->of_node) : dev->fwnode;
> > +}
> > +EXPORT_SYMBOL_GPL(dev_fwnode_const);
> 
> Ick, no, this is a mess.
> 
> Either always return a const pointer, or don't.  Ideally always return a
> const pointer, so all we really need is:
> 
> const struct fwnode_handle *dev_fwnode(const struct device *dev);
> 
> right?
> 
> Yes, it will take some unwinding backwards to get there, but please do
> that instead of having 2 different functions where the parameter type is
> part of the function name.  This isn't the 1980's...

The problem with this approach is that sometimes non-const fwnode_handles
are needed. On OF, for instance, anything that has something to do with
refcounting requires this. Software nodes as well.

One option which I suggested earlier was to turn dev_fwnode() into a macro
and use C11 _Generic() to check whether the device is const or not.

Being able to turn struct device pointers const is certainly not worth
violating constness properties.
Greg Kroah-Hartman Sept. 30, 2022, 2:43 p.m. UTC | #2
On Fri, Sep 30, 2022 at 02:30:53PM +0000, Sakari Ailus wrote:
> Hi Greg,
> 
> On Wed, Sep 28, 2022 at 01:05:20PM +0200, Greg Kroah-Hartman wrote:
> > On Wed, Sep 28, 2022 at 01:57:42PM +0300, Andy Shevchenko wrote:
> > > It's not fully correct to take a const parameter pointer to a struct
> > > and return a non-const pointer to a member of that struct.
> > > 
> > > Instead, introduce a const version of the dev_fwnode() API which takes
> > > and returns const pointers and use it where it's applicable.
> > > 
> > > Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > Fixes: aade55c86033 ("device property: Add const qualifier to device_get_match_data() parameter")
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > >  drivers/base/property.c  | 11 +++++++++--
> > >  include/linux/property.h |  3 ++-
> > >  2 files changed, 11 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/base/property.c b/drivers/base/property.c
> > > index 4d6278a84868..699f1b115e0a 100644
> > > --- a/drivers/base/property.c
> > > +++ b/drivers/base/property.c
> > > @@ -17,13 +17,20 @@
> > >  #include <linux/property.h>
> > >  #include <linux/phy.h>
> > >  
> > > -struct fwnode_handle *dev_fwnode(const struct device *dev)
> > > +struct fwnode_handle *dev_fwnode(struct device *dev)
> > >  {
> > >  	return IS_ENABLED(CONFIG_OF) && dev->of_node ?
> > >  		of_fwnode_handle(dev->of_node) : dev->fwnode;
> > >  }
> > >  EXPORT_SYMBOL_GPL(dev_fwnode);
> > >  
> > > +const struct fwnode_handle *dev_fwnode_const(const struct device *dev)
> > > +{
> > > +	return IS_ENABLED(CONFIG_OF) && dev->of_node ?
> > > +		of_fwnode_handle(dev->of_node) : dev->fwnode;
> > > +}
> > > +EXPORT_SYMBOL_GPL(dev_fwnode_const);
> > 
> > Ick, no, this is a mess.
> > 
> > Either always return a const pointer, or don't.  Ideally always return a
> > const pointer, so all we really need is:
> > 
> > const struct fwnode_handle *dev_fwnode(const struct device *dev);
> > 
> > right?
> > 
> > Yes, it will take some unwinding backwards to get there, but please do
> > that instead of having 2 different functions where the parameter type is
> > part of the function name.  This isn't the 1980's...
> 
> The problem with this approach is that sometimes non-const fwnode_handles
> are needed. On OF, for instance, anything that has something to do with
> refcounting requires this. Software nodes as well.

If they are writable, then yes, let's keep them writable, and not create
two function paths where we have to pick and choose.

> One option which I suggested earlier was to turn dev_fwnode() into a macro
> and use C11 _Generic() to check whether the device is const or not.

As much fun as that would be, I don't think it would work well.

Although, maybe it would, have an example of how that would look?

I ask as I just went through a large refactoring of the kobject layer to
mark many things const * and I find it a bit "sad" that functions like
this:
	static inline struct device *kobj_to_dev(const struct kobject *kobj)
	{
		return container_of(kobj, struct device, kobj);
	}
have the ability to take a read-only pointer and spit out a writable one
thanks to the pointer math in container_of() with no one being the
wiser.

> Being able to turn struct device pointers const is certainly not worth
> violating constness properties.

Agreed, but we can do better...

thanks,

greg k-h
Sakari Ailus Oct. 3, 2022, 11:02 a.m. UTC | #3
Hi Greg,

On Fri, Sep 30, 2022 at 04:43:19PM +0200, Greg Kroah-Hartman wrote:
> On Fri, Sep 30, 2022 at 02:30:53PM +0000, Sakari Ailus wrote:
> > Hi Greg,
> > 
> > On Wed, Sep 28, 2022 at 01:05:20PM +0200, Greg Kroah-Hartman wrote:
> > > On Wed, Sep 28, 2022 at 01:57:42PM +0300, Andy Shevchenko wrote:
> > > > It's not fully correct to take a const parameter pointer to a struct
> > > > and return a non-const pointer to a member of that struct.
> > > > 
> > > > Instead, introduce a const version of the dev_fwnode() API which takes
> > > > and returns const pointers and use it where it's applicable.
> > > > 
> > > > Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > Fixes: aade55c86033 ("device property: Add const qualifier to device_get_match_data() parameter")
> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > ---
> > > >  drivers/base/property.c  | 11 +++++++++--
> > > >  include/linux/property.h |  3 ++-
> > > >  2 files changed, 11 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/base/property.c b/drivers/base/property.c
> > > > index 4d6278a84868..699f1b115e0a 100644
> > > > --- a/drivers/base/property.c
> > > > +++ b/drivers/base/property.c
> > > > @@ -17,13 +17,20 @@
> > > >  #include <linux/property.h>
> > > >  #include <linux/phy.h>
> > > >  
> > > > -struct fwnode_handle *dev_fwnode(const struct device *dev)
> > > > +struct fwnode_handle *dev_fwnode(struct device *dev)
> > > >  {
> > > >  	return IS_ENABLED(CONFIG_OF) && dev->of_node ?
> > > >  		of_fwnode_handle(dev->of_node) : dev->fwnode;
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(dev_fwnode);
> > > >  
> > > > +const struct fwnode_handle *dev_fwnode_const(const struct device *dev)
> > > > +{
> > > > +	return IS_ENABLED(CONFIG_OF) && dev->of_node ?
> > > > +		of_fwnode_handle(dev->of_node) : dev->fwnode;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(dev_fwnode_const);
> > > 
> > > Ick, no, this is a mess.
> > > 
> > > Either always return a const pointer, or don't.  Ideally always return a
> > > const pointer, so all we really need is:
> > > 
> > > const struct fwnode_handle *dev_fwnode(const struct device *dev);
> > > 
> > > right?
> > > 
> > > Yes, it will take some unwinding backwards to get there, but please do
> > > that instead of having 2 different functions where the parameter type is
> > > part of the function name.  This isn't the 1980's...
> > 
> > The problem with this approach is that sometimes non-const fwnode_handles
> > are needed. On OF, for instance, anything that has something to do with
> > refcounting requires this. Software nodes as well.
> 
> If they are writable, then yes, let's keep them writable, and not create
> two function paths where we have to pick and choose.
> 
> > One option which I suggested earlier was to turn dev_fwnode() into a macro
> > and use C11 _Generic() to check whether the device is const or not.
> 
> As much fun as that would be, I don't think it would work well.
> 
> Although, maybe it would, have an example of how that would look?

Similar to what container_of() could be, see below.

We could also partially revert aade55c86033bee868a93e4bf3843c9c99e84526
which (also) made dev_fwnode() argument const (which is the source of the
issue).

> 
> I ask as I just went through a large refactoring of the kobject layer to
> mark many things const * and I find it a bit "sad" that functions like
> this:
> 	static inline struct device *kobj_to_dev(const struct kobject *kobj)
> 	{
> 		return container_of(kobj, struct device, kobj);
> 	}
> have the ability to take a read-only pointer and spit out a writable one
> thanks to the pointer math in container_of() with no one being the
> wiser.

Yeah, container_of() is dangerous, especially in macros. It could of course
be made safer. Something like this:

<URL:https://lore.kernel.org/linux-kernel/1495195570-5249-1-git-send-email-sakari.ailus@linux.intel.com/>

I can respin it, back in 2017 I got no replies.

> 
> > Being able to turn struct device pointers const is certainly not worth
> > violating constness properties.
> 
> Agreed, but we can do better...
Rafael J. Wysocki Oct. 3, 2022, 11:54 a.m. UTC | #4
On Fri, Sep 30, 2022 at 4:43 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Fri, Sep 30, 2022 at 02:30:53PM +0000, Sakari Ailus wrote:
> > Hi Greg,
> >
> > On Wed, Sep 28, 2022 at 01:05:20PM +0200, Greg Kroah-Hartman wrote:
> > > On Wed, Sep 28, 2022 at 01:57:42PM +0300, Andy Shevchenko wrote:
> > > > It's not fully correct to take a const parameter pointer to a struct
> > > > and return a non-const pointer to a member of that struct.
> > > >
> > > > Instead, introduce a const version of the dev_fwnode() API which takes
> > > > and returns const pointers and use it where it's applicable.
> > > >
> > > > Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > Fixes: aade55c86033 ("device property: Add const qualifier to device_get_match_data() parameter")
> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > ---
> > > >  drivers/base/property.c  | 11 +++++++++--
> > > >  include/linux/property.h |  3 ++-
> > > >  2 files changed, 11 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/base/property.c b/drivers/base/property.c
> > > > index 4d6278a84868..699f1b115e0a 100644
> > > > --- a/drivers/base/property.c
> > > > +++ b/drivers/base/property.c
> > > > @@ -17,13 +17,20 @@
> > > >  #include <linux/property.h>
> > > >  #include <linux/phy.h>
> > > >
> > > > -struct fwnode_handle *dev_fwnode(const struct device *dev)
> > > > +struct fwnode_handle *dev_fwnode(struct device *dev)
> > > >  {
> > > >   return IS_ENABLED(CONFIG_OF) && dev->of_node ?
> > > >           of_fwnode_handle(dev->of_node) : dev->fwnode;
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(dev_fwnode);
> > > >
> > > > +const struct fwnode_handle *dev_fwnode_const(const struct device *dev)
> > > > +{
> > > > + return IS_ENABLED(CONFIG_OF) && dev->of_node ?
> > > > +         of_fwnode_handle(dev->of_node) : dev->fwnode;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(dev_fwnode_const);
> > >
> > > Ick, no, this is a mess.
> > >
> > > Either always return a const pointer, or don't.  Ideally always return a
> > > const pointer, so all we really need is:
> > >
> > > const struct fwnode_handle *dev_fwnode(const struct device *dev);
> > >
> > > right?
> > >
> > > Yes, it will take some unwinding backwards to get there, but please do
> > > that instead of having 2 different functions where the parameter type is
> > > part of the function name.  This isn't the 1980's...
> >
> > The problem with this approach is that sometimes non-const fwnode_handles
> > are needed. On OF, for instance, anything that has something to do with
> > refcounting requires this. Software nodes as well.
>
> If they are writable, then yes, let's keep them writable, and not create
> two function paths where we have to pick and choose.
>
> > One option which I suggested earlier was to turn dev_fwnode() into a macro
> > and use C11 _Generic() to check whether the device is const or not.
>
> As much fun as that would be, I don't think it would work well.
>
> Although, maybe it would, have an example of how that would look?
>
> I ask as I just went through a large refactoring of the kobject layer to
> mark many things const * and I find it a bit "sad" that functions like
> this:
>         static inline struct device *kobj_to_dev(const struct kobject *kobj)
>         {
>                 return container_of(kobj, struct device, kobj);
>         }
> have the ability to take a read-only pointer and spit out a writable one
> thanks to the pointer math in container_of() with no one being the
> wiser.

Well, is this really a problem?

After all, if an immutable structure is embedded in another one, that
doesn't automatically imply that the containing structure has to be
immutable too.  Hence, a const pointer to the inner structure doesn't
automatically yield a const pointer to the outer one.

> > Being able to turn struct device pointers const is certainly not worth
> > violating constness properties.
>
> Agreed, but we can do better...
Sakari Ailus Oct. 3, 2022, noon UTC | #5
Hi Rafael,

On Mon, Oct 03, 2022 at 01:54:37PM +0200, Rafael J. Wysocki wrote:
> > I ask as I just went through a large refactoring of the kobject layer to
> > mark many things const * and I find it a bit "sad" that functions like
> > this:
> >         static inline struct device *kobj_to_dev(const struct kobject *kobj)
> >         {
> >                 return container_of(kobj, struct device, kobj);
> >         }
> > have the ability to take a read-only pointer and spit out a writable one
> > thanks to the pointer math in container_of() with no one being the
> > wiser.
> 
> Well, is this really a problem?
> 
> After all, if an immutable structure is embedded in another one, that
> doesn't automatically imply that the containing structure has to be
> immutable too.  Hence, a const pointer to the inner structure doesn't
> automatically yield a const pointer to the outer one.

I think in that case it'd be better, to at least make an informed decision
on that instead of just dropping the const qualifier.
Greg Kroah-Hartman Oct. 3, 2022, 3:05 p.m. UTC | #6
On Mon, Oct 03, 2022 at 01:54:37PM +0200, Rafael J. Wysocki wrote:
> On Fri, Sep 30, 2022 at 4:43 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Fri, Sep 30, 2022 at 02:30:53PM +0000, Sakari Ailus wrote:
> > > Hi Greg,
> > >
> > > On Wed, Sep 28, 2022 at 01:05:20PM +0200, Greg Kroah-Hartman wrote:
> > > > On Wed, Sep 28, 2022 at 01:57:42PM +0300, Andy Shevchenko wrote:
> > > > > It's not fully correct to take a const parameter pointer to a struct
> > > > > and return a non-const pointer to a member of that struct.
> > > > >
> > > > > Instead, introduce a const version of the dev_fwnode() API which takes
> > > > > and returns const pointers and use it where it's applicable.
> > > > >
> > > > > Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > Fixes: aade55c86033 ("device property: Add const qualifier to device_get_match_data() parameter")
> > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > > Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > ---
> > > > >  drivers/base/property.c  | 11 +++++++++--
> > > > >  include/linux/property.h |  3 ++-
> > > > >  2 files changed, 11 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/base/property.c b/drivers/base/property.c
> > > > > index 4d6278a84868..699f1b115e0a 100644
> > > > > --- a/drivers/base/property.c
> > > > > +++ b/drivers/base/property.c
> > > > > @@ -17,13 +17,20 @@
> > > > >  #include <linux/property.h>
> > > > >  #include <linux/phy.h>
> > > > >
> > > > > -struct fwnode_handle *dev_fwnode(const struct device *dev)
> > > > > +struct fwnode_handle *dev_fwnode(struct device *dev)
> > > > >  {
> > > > >   return IS_ENABLED(CONFIG_OF) && dev->of_node ?
> > > > >           of_fwnode_handle(dev->of_node) : dev->fwnode;
> > > > >  }
> > > > >  EXPORT_SYMBOL_GPL(dev_fwnode);
> > > > >
> > > > > +const struct fwnode_handle *dev_fwnode_const(const struct device *dev)
> > > > > +{
> > > > > + return IS_ENABLED(CONFIG_OF) && dev->of_node ?
> > > > > +         of_fwnode_handle(dev->of_node) : dev->fwnode;
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(dev_fwnode_const);
> > > >
> > > > Ick, no, this is a mess.
> > > >
> > > > Either always return a const pointer, or don't.  Ideally always return a
> > > > const pointer, so all we really need is:
> > > >
> > > > const struct fwnode_handle *dev_fwnode(const struct device *dev);
> > > >
> > > > right?
> > > >
> > > > Yes, it will take some unwinding backwards to get there, but please do
> > > > that instead of having 2 different functions where the parameter type is
> > > > part of the function name.  This isn't the 1980's...
> > >
> > > The problem with this approach is that sometimes non-const fwnode_handles
> > > are needed. On OF, for instance, anything that has something to do with
> > > refcounting requires this. Software nodes as well.
> >
> > If they are writable, then yes, let's keep them writable, and not create
> > two function paths where we have to pick and choose.
> >
> > > One option which I suggested earlier was to turn dev_fwnode() into a macro
> > > and use C11 _Generic() to check whether the device is const or not.
> >
> > As much fun as that would be, I don't think it would work well.
> >
> > Although, maybe it would, have an example of how that would look?
> >
> > I ask as I just went through a large refactoring of the kobject layer to
> > mark many things const * and I find it a bit "sad" that functions like
> > this:
> >         static inline struct device *kobj_to_dev(const struct kobject *kobj)
> >         {
> >                 return container_of(kobj, struct device, kobj);
> >         }
> > have the ability to take a read-only pointer and spit out a writable one
> > thanks to the pointer math in container_of() with no one being the
> > wiser.
> 
> Well, is this really a problem?
> 
> After all, if an immutable structure is embedded in another one, that
> doesn't automatically imply that the containing structure has to be
> immutable too.  Hence, a const pointer to the inner structure doesn't
> automatically yield a const pointer to the outer one.

That is true, but it's a _huge_ hint that we are throwing away here,
sometimes without even really realizing it.

Ideally, if you have a const * passed into container_of() you would get
a const * back, and then, if you _really_ know what you are doing with
it, feel free to cast it away.  That cast would be a huge sign that
"hey, something is happening here" and allow people to at least notice
it, while today, we loose all of that.

Let me play around with this a bit.  In talking with the Rust Linux
developers, a lot of "how do we know if this pointer is immutable or
not" discussions happen.  With many of our apis, right now we don't know
that, and perhaps that should change as it would make things not
necessarily more "safe", but more "obvious" as to what both the intent
is, and what is actually happening to pointers at times.

Especially in the mess that is kobjects and struct device where we cast
pointers around with abandon :)

thanks,

greg k-h
Greg Kroah-Hartman Oct. 3, 2022, 3:07 p.m. UTC | #7
On Mon, Oct 03, 2022 at 11:02:19AM +0000, Sakari Ailus wrote:
> Hi Greg,
> 
> On Fri, Sep 30, 2022 at 04:43:19PM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Sep 30, 2022 at 02:30:53PM +0000, Sakari Ailus wrote:
> > > Hi Greg,
> > > 
> > > On Wed, Sep 28, 2022 at 01:05:20PM +0200, Greg Kroah-Hartman wrote:
> > > > On Wed, Sep 28, 2022 at 01:57:42PM +0300, Andy Shevchenko wrote:
> > > > > It's not fully correct to take a const parameter pointer to a struct
> > > > > and return a non-const pointer to a member of that struct.
> > > > > 
> > > > > Instead, introduce a const version of the dev_fwnode() API which takes
> > > > > and returns const pointers and use it where it's applicable.
> > > > > 
> > > > > Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > Fixes: aade55c86033 ("device property: Add const qualifier to device_get_match_data() parameter")
> > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > > Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > ---
> > > > >  drivers/base/property.c  | 11 +++++++++--
> > > > >  include/linux/property.h |  3 ++-
> > > > >  2 files changed, 11 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/base/property.c b/drivers/base/property.c
> > > > > index 4d6278a84868..699f1b115e0a 100644
> > > > > --- a/drivers/base/property.c
> > > > > +++ b/drivers/base/property.c
> > > > > @@ -17,13 +17,20 @@
> > > > >  #include <linux/property.h>
> > > > >  #include <linux/phy.h>
> > > > >  
> > > > > -struct fwnode_handle *dev_fwnode(const struct device *dev)
> > > > > +struct fwnode_handle *dev_fwnode(struct device *dev)
> > > > >  {
> > > > >  	return IS_ENABLED(CONFIG_OF) && dev->of_node ?
> > > > >  		of_fwnode_handle(dev->of_node) : dev->fwnode;
> > > > >  }
> > > > >  EXPORT_SYMBOL_GPL(dev_fwnode);
> > > > >  
> > > > > +const struct fwnode_handle *dev_fwnode_const(const struct device *dev)
> > > > > +{
> > > > > +	return IS_ENABLED(CONFIG_OF) && dev->of_node ?
> > > > > +		of_fwnode_handle(dev->of_node) : dev->fwnode;
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(dev_fwnode_const);
> > > > 
> > > > Ick, no, this is a mess.
> > > > 
> > > > Either always return a const pointer, or don't.  Ideally always return a
> > > > const pointer, so all we really need is:
> > > > 
> > > > const struct fwnode_handle *dev_fwnode(const struct device *dev);
> > > > 
> > > > right?
> > > > 
> > > > Yes, it will take some unwinding backwards to get there, but please do
> > > > that instead of having 2 different functions where the parameter type is
> > > > part of the function name.  This isn't the 1980's...
> > > 
> > > The problem with this approach is that sometimes non-const fwnode_handles
> > > are needed. On OF, for instance, anything that has something to do with
> > > refcounting requires this. Software nodes as well.
> > 
> > If they are writable, then yes, let's keep them writable, and not create
> > two function paths where we have to pick and choose.
> > 
> > > One option which I suggested earlier was to turn dev_fwnode() into a macro
> > > and use C11 _Generic() to check whether the device is const or not.
> > 
> > As much fun as that would be, I don't think it would work well.
> > 
> > Although, maybe it would, have an example of how that would look?
> 
> Similar to what container_of() could be, see below.
> 
> We could also partially revert aade55c86033bee868a93e4bf3843c9c99e84526
> which (also) made dev_fwnode() argument const (which is the source of the
> issue).
> 
> > 
> > I ask as I just went through a large refactoring of the kobject layer to
> > mark many things const * and I find it a bit "sad" that functions like
> > this:
> > 	static inline struct device *kobj_to_dev(const struct kobject *kobj)
> > 	{
> > 		return container_of(kobj, struct device, kobj);
> > 	}
> > have the ability to take a read-only pointer and spit out a writable one
> > thanks to the pointer math in container_of() with no one being the
> > wiser.
> 
> Yeah, container_of() is dangerous, especially in macros. It could of course
> be made safer. Something like this:
> 
> <URL:https://lore.kernel.org/linux-kernel/1495195570-5249-1-git-send-email-sakari.ailus@linux.intel.com/>
> 
> I can respin it, back in 2017 I got no replies.

I don't like how we loose the ability to do this in an inline C function
by being forced to do it in a macro (as it makes build errors harder to
understand), but I do like the intent here.

Let me play around with this a bit on some "smaller" uses of
container_of() and see how that works...

thanks,

greg k-h
Greg Kroah-Hartman Oct. 3, 2022, 4:17 p.m. UTC | #8
On Mon, Oct 03, 2022 at 05:07:27PM +0200, Greg Kroah-Hartman wrote:
> On Mon, Oct 03, 2022 at 11:02:19AM +0000, Sakari Ailus wrote:
> > Hi Greg,
> > 
> > On Fri, Sep 30, 2022 at 04:43:19PM +0200, Greg Kroah-Hartman wrote:
> > > On Fri, Sep 30, 2022 at 02:30:53PM +0000, Sakari Ailus wrote:
> > > > Hi Greg,
> > > > 
> > > > On Wed, Sep 28, 2022 at 01:05:20PM +0200, Greg Kroah-Hartman wrote:
> > > > > On Wed, Sep 28, 2022 at 01:57:42PM +0300, Andy Shevchenko wrote:
> > > > > > It's not fully correct to take a const parameter pointer to a struct
> > > > > > and return a non-const pointer to a member of that struct.
> > > > > > 
> > > > > > Instead, introduce a const version of the dev_fwnode() API which takes
> > > > > > and returns const pointers and use it where it's applicable.
> > > > > > 
> > > > > > Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > Fixes: aade55c86033 ("device property: Add const qualifier to device_get_match_data() parameter")
> > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > > > Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > > > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > ---
> > > > > >  drivers/base/property.c  | 11 +++++++++--
> > > > > >  include/linux/property.h |  3 ++-
> > > > > >  2 files changed, 11 insertions(+), 3 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/base/property.c b/drivers/base/property.c
> > > > > > index 4d6278a84868..699f1b115e0a 100644
> > > > > > --- a/drivers/base/property.c
> > > > > > +++ b/drivers/base/property.c
> > > > > > @@ -17,13 +17,20 @@
> > > > > >  #include <linux/property.h>
> > > > > >  #include <linux/phy.h>
> > > > > >  
> > > > > > -struct fwnode_handle *dev_fwnode(const struct device *dev)
> > > > > > +struct fwnode_handle *dev_fwnode(struct device *dev)
> > > > > >  {
> > > > > >  	return IS_ENABLED(CONFIG_OF) && dev->of_node ?
> > > > > >  		of_fwnode_handle(dev->of_node) : dev->fwnode;
> > > > > >  }
> > > > > >  EXPORT_SYMBOL_GPL(dev_fwnode);
> > > > > >  
> > > > > > +const struct fwnode_handle *dev_fwnode_const(const struct device *dev)
> > > > > > +{
> > > > > > +	return IS_ENABLED(CONFIG_OF) && dev->of_node ?
> > > > > > +		of_fwnode_handle(dev->of_node) : dev->fwnode;
> > > > > > +}
> > > > > > +EXPORT_SYMBOL_GPL(dev_fwnode_const);
> > > > > 
> > > > > Ick, no, this is a mess.
> > > > > 
> > > > > Either always return a const pointer, or don't.  Ideally always return a
> > > > > const pointer, so all we really need is:
> > > > > 
> > > > > const struct fwnode_handle *dev_fwnode(const struct device *dev);
> > > > > 
> > > > > right?
> > > > > 
> > > > > Yes, it will take some unwinding backwards to get there, but please do
> > > > > that instead of having 2 different functions where the parameter type is
> > > > > part of the function name.  This isn't the 1980's...
> > > > 
> > > > The problem with this approach is that sometimes non-const fwnode_handles
> > > > are needed. On OF, for instance, anything that has something to do with
> > > > refcounting requires this. Software nodes as well.
> > > 
> > > If they are writable, then yes, let's keep them writable, and not create
> > > two function paths where we have to pick and choose.
> > > 
> > > > One option which I suggested earlier was to turn dev_fwnode() into a macro
> > > > and use C11 _Generic() to check whether the device is const or not.
> > > 
> > > As much fun as that would be, I don't think it would work well.
> > > 
> > > Although, maybe it would, have an example of how that would look?
> > 
> > Similar to what container_of() could be, see below.
> > 
> > We could also partially revert aade55c86033bee868a93e4bf3843c9c99e84526
> > which (also) made dev_fwnode() argument const (which is the source of the
> > issue).
> > 
> > > 
> > > I ask as I just went through a large refactoring of the kobject layer to
> > > mark many things const * and I find it a bit "sad" that functions like
> > > this:
> > > 	static inline struct device *kobj_to_dev(const struct kobject *kobj)
> > > 	{
> > > 		return container_of(kobj, struct device, kobj);
> > > 	}
> > > have the ability to take a read-only pointer and spit out a writable one
> > > thanks to the pointer math in container_of() with no one being the
> > > wiser.
> > 
> > Yeah, container_of() is dangerous, especially in macros. It could of course
> > be made safer. Something like this:
> > 
> > <URL:https://lore.kernel.org/linux-kernel/1495195570-5249-1-git-send-email-sakari.ailus@linux.intel.com/>
> > 
> > I can respin it, back in 2017 I got no replies.
> 
> I don't like how we loose the ability to do this in an inline C function
> by being forced to do it in a macro (as it makes build errors harder to
> understand), but I do like the intent here.
> 
> Let me play around with this a bit on some "smaller" uses of
> container_of() and see how that works...

Odd, this doesn't work for me at all.

I tried the following change:

diff --git a/include/linux/device.h b/include/linux/device.h
index 424b55df0272..5575c87e6c3b 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -680,11 +680,21 @@ struct device_link {
 	bool supplier_preactivated; /* Owned by consumer probe. */
 };
 
-static inline struct device *kobj_to_dev(struct kobject *kobj)
+static inline struct device *__kobj_to_dev(struct kobject *kobj)
 {
 	return container_of(kobj, struct device, kobj);
 }
 
+static inline const struct device *__kobj_to_dev_const(const struct kobject *kobj)
+{
+	return container_of(kobj, const struct device, kobj);
+}
+
+#define kobj_to_dev(kobj)						\
+	_Generic((kobj),						\
+		 const struct kobject *: __kobj_to_dev_const(kobj),	\
+		 struct kobject *: __kobj_to_dev(kobj))
+
 /**
  * device_iommu_mapped - Returns true when the device DMA is translated
  *			 by an IOMMU


which seems all is fine for normal kobject pointers passed in, but for
the first 'const struct kobject *' the compiler hits, I get the
following error:

  CC      drivers/base/core.o
In file included from ./include/linux/acpi.h:15,
                 from drivers/base/core.c:11:
drivers/base/core.c: In function ‘dev_attr_show’:
drivers/base/core.c:2193:48: error: passing argument 1 of ‘__kobj_to_dev’ discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers]
 2193 |         const struct device *dev = kobj_to_dev(kobj);
      |                                                ^~~~
./include/linux/device.h:696:50: note: in definition of macro ‘kobj_to_dev’
  696 |                  struct kobject *: __kobj_to_dev(kobj))
      |                                                  ^~~~
./include/linux/device.h:683:60: note: expected ‘struct kobject *’ but argument is of type ‘const struct kobject *’
  683 | static inline struct device *__kobj_to_dev(struct kobject *kobj)
      |                                            ~~~~~~~~~~~~~~~~^~~~


(note, I faked up a constant pointer just to trip the compiler)

The selection of _Generic() seems not to be working here, any hints?  I tried
playing around with 'default' and 'typeof' and the like, but all error out the
same way.

thanks,

greg k-h
Sakari Ailus Oct. 3, 2022, 8:08 p.m. UTC | #9
Hi Greg,

On Mon, Oct 03, 2022 at 06:17:17PM +0200, Greg Kroah-Hartman wrote:
> On Mon, Oct 03, 2022 at 05:07:27PM +0200, Greg Kroah-Hartman wrote:
> > On Mon, Oct 03, 2022 at 11:02:19AM +0000, Sakari Ailus wrote:
> > > Hi Greg,
> > > 
> > > On Fri, Sep 30, 2022 at 04:43:19PM +0200, Greg Kroah-Hartman wrote:
> > > > On Fri, Sep 30, 2022 at 02:30:53PM +0000, Sakari Ailus wrote:
> > > > > Hi Greg,
> > > > > 
> > > > > On Wed, Sep 28, 2022 at 01:05:20PM +0200, Greg Kroah-Hartman wrote:
> > > > > > On Wed, Sep 28, 2022 at 01:57:42PM +0300, Andy Shevchenko wrote:
> > > > > > > It's not fully correct to take a const parameter pointer to a struct
> > > > > > > and return a non-const pointer to a member of that struct.
> > > > > > > 
> > > > > > > Instead, introduce a const version of the dev_fwnode() API which takes
> > > > > > > and returns const pointers and use it where it's applicable.
> > > > > > > 
> > > > > > > Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > > Fixes: aade55c86033 ("device property: Add const qualifier to device_get_match_data() parameter")
> > > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > > > > Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > > > > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > > ---
> > > > > > >  drivers/base/property.c  | 11 +++++++++--
> > > > > > >  include/linux/property.h |  3 ++-
> > > > > > >  2 files changed, 11 insertions(+), 3 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/base/property.c b/drivers/base/property.c
> > > > > > > index 4d6278a84868..699f1b115e0a 100644
> > > > > > > --- a/drivers/base/property.c
> > > > > > > +++ b/drivers/base/property.c
> > > > > > > @@ -17,13 +17,20 @@
> > > > > > >  #include <linux/property.h>
> > > > > > >  #include <linux/phy.h>
> > > > > > >  
> > > > > > > -struct fwnode_handle *dev_fwnode(const struct device *dev)
> > > > > > > +struct fwnode_handle *dev_fwnode(struct device *dev)
> > > > > > >  {
> > > > > > >  	return IS_ENABLED(CONFIG_OF) && dev->of_node ?
> > > > > > >  		of_fwnode_handle(dev->of_node) : dev->fwnode;
> > > > > > >  }
> > > > > > >  EXPORT_SYMBOL_GPL(dev_fwnode);
> > > > > > >  
> > > > > > > +const struct fwnode_handle *dev_fwnode_const(const struct device *dev)
> > > > > > > +{
> > > > > > > +	return IS_ENABLED(CONFIG_OF) && dev->of_node ?
> > > > > > > +		of_fwnode_handle(dev->of_node) : dev->fwnode;
> > > > > > > +}
> > > > > > > +EXPORT_SYMBOL_GPL(dev_fwnode_const);
> > > > > > 
> > > > > > Ick, no, this is a mess.
> > > > > > 
> > > > > > Either always return a const pointer, or don't.  Ideally always return a
> > > > > > const pointer, so all we really need is:
> > > > > > 
> > > > > > const struct fwnode_handle *dev_fwnode(const struct device *dev);
> > > > > > 
> > > > > > right?
> > > > > > 
> > > > > > Yes, it will take some unwinding backwards to get there, but please do
> > > > > > that instead of having 2 different functions where the parameter type is
> > > > > > part of the function name.  This isn't the 1980's...
> > > > > 
> > > > > The problem with this approach is that sometimes non-const fwnode_handles
> > > > > are needed. On OF, for instance, anything that has something to do with
> > > > > refcounting requires this. Software nodes as well.
> > > > 
> > > > If they are writable, then yes, let's keep them writable, and not create
> > > > two function paths where we have to pick and choose.
> > > > 
> > > > > One option which I suggested earlier was to turn dev_fwnode() into a macro
> > > > > and use C11 _Generic() to check whether the device is const or not.
> > > > 
> > > > As much fun as that would be, I don't think it would work well.
> > > > 
> > > > Although, maybe it would, have an example of how that would look?
> > > 
> > > Similar to what container_of() could be, see below.
> > > 
> > > We could also partially revert aade55c86033bee868a93e4bf3843c9c99e84526
> > > which (also) made dev_fwnode() argument const (which is the source of the
> > > issue).
> > > 
> > > > 
> > > > I ask as I just went through a large refactoring of the kobject layer to
> > > > mark many things const * and I find it a bit "sad" that functions like
> > > > this:
> > > > 	static inline struct device *kobj_to_dev(const struct kobject *kobj)
> > > > 	{
> > > > 		return container_of(kobj, struct device, kobj);
> > > > 	}
> > > > have the ability to take a read-only pointer and spit out a writable one
> > > > thanks to the pointer math in container_of() with no one being the
> > > > wiser.
> > > 
> > > Yeah, container_of() is dangerous, especially in macros. It could of course
> > > be made safer. Something like this:
> > > 
> > > <URL:https://lore.kernel.org/linux-kernel/1495195570-5249-1-git-send-email-sakari.ailus@linux.intel.com/>
> > > 
> > > I can respin it, back in 2017 I got no replies.
> > 
> > I don't like how we loose the ability to do this in an inline C function
> > by being forced to do it in a macro (as it makes build errors harder to
> > understand), but I do like the intent here.
> > 
> > Let me play around with this a bit on some "smaller" uses of
> > container_of() and see how that works...
> 
> Odd, this doesn't work for me at all.
> 
> I tried the following change:
> 
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 424b55df0272..5575c87e6c3b 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -680,11 +680,21 @@ struct device_link {
>  	bool supplier_preactivated; /* Owned by consumer probe. */
>  };
>  
> -static inline struct device *kobj_to_dev(struct kobject *kobj)
> +static inline struct device *__kobj_to_dev(struct kobject *kobj)
>  {
>  	return container_of(kobj, struct device, kobj);
>  }
>  
> +static inline const struct device *__kobj_to_dev_const(const struct kobject *kobj)
> +{
> +	return container_of(kobj, const struct device, kobj);
> +}
> +
> +#define kobj_to_dev(kobj)						\
> +	_Generic((kobj),						\
> +		 const struct kobject *: __kobj_to_dev_const(kobj),	\
> +		 struct kobject *: __kobj_to_dev(kobj))
> +
>  /**
>   * device_iommu_mapped - Returns true when the device DMA is translated
>   *			 by an IOMMU
> 
> 
> which seems all is fine for normal kobject pointers passed in, but for
> the first 'const struct kobject *' the compiler hits, I get the
> following error:
> 
>   CC      drivers/base/core.o
> In file included from ./include/linux/acpi.h:15,
>                  from drivers/base/core.c:11:
> drivers/base/core.c: In function ‘dev_attr_show’:
> drivers/base/core.c:2193:48: error: passing argument 1 of ‘__kobj_to_dev’ discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers]
>  2193 |         const struct device *dev = kobj_to_dev(kobj);
>       |                                                ^~~~
> ./include/linux/device.h:696:50: note: in definition of macro ‘kobj_to_dev’
>   696 |                  struct kobject *: __kobj_to_dev(kobj))
>       |                                                  ^~~~
> ./include/linux/device.h:683:60: note: expected ‘struct kobject *’ but argument is of type ‘const struct kobject *’
>   683 | static inline struct device *__kobj_to_dev(struct kobject *kobj)
>       |                                            ~~~~~~~~~~~~~~~~^~~~
> 
> 
> (note, I faked up a constant pointer just to trip the compiler)
> 
> The selection of _Generic() seems not to be working here, any hints?  I tried
> playing around with 'default' and 'typeof' and the like, but all error out the
> same way.

Even though only one gets evaluated, it seems the compiler will still
perform type check on it. I think this problem was partially shared by the
original patch.

This should work if written as:

#define kobj_to_dev(kobj)						\
	(_Generic((kobj),						\
		  const struct kobject *: __kobj_to_dev_const,		\
		  struct kobject *: __kobj_to_dev)(kobj))
Greg Kroah-Hartman Oct. 4, 2022, 7:55 a.m. UTC | #10
On Mon, Oct 03, 2022 at 08:08:58PM +0000, Sakari Ailus wrote:
> Hi Greg,
> 
> On Mon, Oct 03, 2022 at 06:17:17PM +0200, Greg Kroah-Hartman wrote:
> > On Mon, Oct 03, 2022 at 05:07:27PM +0200, Greg Kroah-Hartman wrote:
> > > On Mon, Oct 03, 2022 at 11:02:19AM +0000, Sakari Ailus wrote:
> > > > Hi Greg,
> > > > 
> > > > On Fri, Sep 30, 2022 at 04:43:19PM +0200, Greg Kroah-Hartman wrote:
> > > > > On Fri, Sep 30, 2022 at 02:30:53PM +0000, Sakari Ailus wrote:
> > > > > > Hi Greg,
> > > > > > 
> > > > > > On Wed, Sep 28, 2022 at 01:05:20PM +0200, Greg Kroah-Hartman wrote:
> > > > > > > On Wed, Sep 28, 2022 at 01:57:42PM +0300, Andy Shevchenko wrote:
> > > > > > > > It's not fully correct to take a const parameter pointer to a struct
> > > > > > > > and return a non-const pointer to a member of that struct.
> > > > > > > > 
> > > > > > > > Instead, introduce a const version of the dev_fwnode() API which takes
> > > > > > > > and returns const pointers and use it where it's applicable.
> > > > > > > > 
> > > > > > > > Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > > > Fixes: aade55c86033 ("device property: Add const qualifier to device_get_match_data() parameter")
> > > > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > > > > > Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > > > > > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > > > ---
> > > > > > > >  drivers/base/property.c  | 11 +++++++++--
> > > > > > > >  include/linux/property.h |  3 ++-
> > > > > > > >  2 files changed, 11 insertions(+), 3 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/base/property.c b/drivers/base/property.c
> > > > > > > > index 4d6278a84868..699f1b115e0a 100644
> > > > > > > > --- a/drivers/base/property.c
> > > > > > > > +++ b/drivers/base/property.c
> > > > > > > > @@ -17,13 +17,20 @@
> > > > > > > >  #include <linux/property.h>
> > > > > > > >  #include <linux/phy.h>
> > > > > > > >  
> > > > > > > > -struct fwnode_handle *dev_fwnode(const struct device *dev)
> > > > > > > > +struct fwnode_handle *dev_fwnode(struct device *dev)
> > > > > > > >  {
> > > > > > > >  	return IS_ENABLED(CONFIG_OF) && dev->of_node ?
> > > > > > > >  		of_fwnode_handle(dev->of_node) : dev->fwnode;
> > > > > > > >  }
> > > > > > > >  EXPORT_SYMBOL_GPL(dev_fwnode);
> > > > > > > >  
> > > > > > > > +const struct fwnode_handle *dev_fwnode_const(const struct device *dev)
> > > > > > > > +{
> > > > > > > > +	return IS_ENABLED(CONFIG_OF) && dev->of_node ?
> > > > > > > > +		of_fwnode_handle(dev->of_node) : dev->fwnode;
> > > > > > > > +}
> > > > > > > > +EXPORT_SYMBOL_GPL(dev_fwnode_const);
> > > > > > > 
> > > > > > > Ick, no, this is a mess.
> > > > > > > 
> > > > > > > Either always return a const pointer, or don't.  Ideally always return a
> > > > > > > const pointer, so all we really need is:
> > > > > > > 
> > > > > > > const struct fwnode_handle *dev_fwnode(const struct device *dev);
> > > > > > > 
> > > > > > > right?
> > > > > > > 
> > > > > > > Yes, it will take some unwinding backwards to get there, but please do
> > > > > > > that instead of having 2 different functions where the parameter type is
> > > > > > > part of the function name.  This isn't the 1980's...
> > > > > > 
> > > > > > The problem with this approach is that sometimes non-const fwnode_handles
> > > > > > are needed. On OF, for instance, anything that has something to do with
> > > > > > refcounting requires this. Software nodes as well.
> > > > > 
> > > > > If they are writable, then yes, let's keep them writable, and not create
> > > > > two function paths where we have to pick and choose.
> > > > > 
> > > > > > One option which I suggested earlier was to turn dev_fwnode() into a macro
> > > > > > and use C11 _Generic() to check whether the device is const or not.
> > > > > 
> > > > > As much fun as that would be, I don't think it would work well.
> > > > > 
> > > > > Although, maybe it would, have an example of how that would look?
> > > > 
> > > > Similar to what container_of() could be, see below.
> > > > 
> > > > We could also partially revert aade55c86033bee868a93e4bf3843c9c99e84526
> > > > which (also) made dev_fwnode() argument const (which is the source of the
> > > > issue).
> > > > 
> > > > > 
> > > > > I ask as I just went through a large refactoring of the kobject layer to
> > > > > mark many things const * and I find it a bit "sad" that functions like
> > > > > this:
> > > > > 	static inline struct device *kobj_to_dev(const struct kobject *kobj)
> > > > > 	{
> > > > > 		return container_of(kobj, struct device, kobj);
> > > > > 	}
> > > > > have the ability to take a read-only pointer and spit out a writable one
> > > > > thanks to the pointer math in container_of() with no one being the
> > > > > wiser.
> > > > 
> > > > Yeah, container_of() is dangerous, especially in macros. It could of course
> > > > be made safer. Something like this:
> > > > 
> > > > <URL:https://lore.kernel.org/linux-kernel/1495195570-5249-1-git-send-email-sakari.ailus@linux.intel.com/>
> > > > 
> > > > I can respin it, back in 2017 I got no replies.
> > > 
> > > I don't like how we loose the ability to do this in an inline C function
> > > by being forced to do it in a macro (as it makes build errors harder to
> > > understand), but I do like the intent here.
> > > 
> > > Let me play around with this a bit on some "smaller" uses of
> > > container_of() and see how that works...
> > 
> > Odd, this doesn't work for me at all.
> > 
> > I tried the following change:
> > 
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index 424b55df0272..5575c87e6c3b 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -680,11 +680,21 @@ struct device_link {
> >  	bool supplier_preactivated; /* Owned by consumer probe. */
> >  };
> >  
> > -static inline struct device *kobj_to_dev(struct kobject *kobj)
> > +static inline struct device *__kobj_to_dev(struct kobject *kobj)
> >  {
> >  	return container_of(kobj, struct device, kobj);
> >  }
> >  
> > +static inline const struct device *__kobj_to_dev_const(const struct kobject *kobj)
> > +{
> > +	return container_of(kobj, const struct device, kobj);
> > +}
> > +
> > +#define kobj_to_dev(kobj)						\
> > +	_Generic((kobj),						\
> > +		 const struct kobject *: __kobj_to_dev_const(kobj),	\
> > +		 struct kobject *: __kobj_to_dev(kobj))
> > +
> >  /**
> >   * device_iommu_mapped - Returns true when the device DMA is translated
> >   *			 by an IOMMU
> > 
> > 
> > which seems all is fine for normal kobject pointers passed in, but for
> > the first 'const struct kobject *' the compiler hits, I get the
> > following error:
> > 
> >   CC      drivers/base/core.o
> > In file included from ./include/linux/acpi.h:15,
> >                  from drivers/base/core.c:11:
> > drivers/base/core.c: In function ‘dev_attr_show’:
> > drivers/base/core.c:2193:48: error: passing argument 1 of ‘__kobj_to_dev’ discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers]
> >  2193 |         const struct device *dev = kobj_to_dev(kobj);
> >       |                                                ^~~~
> > ./include/linux/device.h:696:50: note: in definition of macro ‘kobj_to_dev’
> >   696 |                  struct kobject *: __kobj_to_dev(kobj))
> >       |                                                  ^~~~
> > ./include/linux/device.h:683:60: note: expected ‘struct kobject *’ but argument is of type ‘const struct kobject *’
> >   683 | static inline struct device *__kobj_to_dev(struct kobject *kobj)
> >       |                                            ~~~~~~~~~~~~~~~~^~~~
> > 
> > 
> > (note, I faked up a constant pointer just to trip the compiler)
> > 
> > The selection of _Generic() seems not to be working here, any hints?  I tried
> > playing around with 'default' and 'typeof' and the like, but all error out the
> > same way.
> 
> Even though only one gets evaluated, it seems the compiler will still
> perform type check on it. I think this problem was partially shared by the
> original patch.
> 
> This should work if written as:
> 
> #define kobj_to_dev(kobj)						\
> 	(_Generic((kobj),						\
> 		  const struct kobject *: __kobj_to_dev_const,		\
> 		  struct kobject *: __kobj_to_dev)(kobj))

Ah, doh!  I had the (kobj) part in the wrong place, thanks for that
fix...

Ok, this looks better, let me see how well the build breaks with some of
these changes

thanks,

greg k-h
Andy Shevchenko Oct. 4, 2022, 8:14 a.m. UTC | #11
On Tue, Oct 04, 2022 at 09:55:21AM +0200, Greg Kroah-Hartman wrote:
> On Mon, Oct 03, 2022 at 08:08:58PM +0000, Sakari Ailus wrote:
> > On Mon, Oct 03, 2022 at 06:17:17PM +0200, Greg Kroah-Hartman wrote:

...

> > #define kobj_to_dev(kobj)						\
> > 	(_Generic((kobj),						\
> > 		  const struct kobject *: __kobj_to_dev_const,		\
> > 		  struct kobject *: __kobj_to_dev)(kobj))
> 
> Ah, doh!  I had the (kobj) part in the wrong place, thanks for that
> fix...
> 
> Ok, this looks better, let me see how well the build breaks with some of
> these changes

I believe I can rewrite my patch like this and then it will be much nicer since
we may constify all the rest without calling __dev_fwnode_const() directly.

Are you agree?
Greg Kroah-Hartman Oct. 4, 2022, 8:24 a.m. UTC | #12
On Tue, Oct 04, 2022 at 11:14:14AM +0300, Andy Shevchenko wrote:
> On Tue, Oct 04, 2022 at 09:55:21AM +0200, Greg Kroah-Hartman wrote:
> > On Mon, Oct 03, 2022 at 08:08:58PM +0000, Sakari Ailus wrote:
> > > On Mon, Oct 03, 2022 at 06:17:17PM +0200, Greg Kroah-Hartman wrote:
> 
> ...
> 
> > > #define kobj_to_dev(kobj)						\
> > > 	(_Generic((kobj),						\
> > > 		  const struct kobject *: __kobj_to_dev_const,		\
> > > 		  struct kobject *: __kobj_to_dev)(kobj))
> > 
> > Ah, doh!  I had the (kobj) part in the wrong place, thanks for that
> > fix...
> > 
> > Ok, this looks better, let me see how well the build breaks with some of
> > these changes
> 
> I believe I can rewrite my patch like this and then it will be much nicer since
> we may constify all the rest without calling __dev_fwnode_const() directly.
> 
> Are you agree?

Yes, I think this is a much better option, try it and see.  Below is the
patch I'm running through my build systems at the moment, feel free to
copy the style for your change as well.

thanks,

greg k-h


diff --git a/include/linux/device.h b/include/linux/device.h
index 424b55df0272..023ea50b1916 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -680,11 +680,27 @@ struct device_link {
 	bool supplier_preactivated; /* Owned by consumer probe. */
 };
 
-static inline struct device *kobj_to_dev(struct kobject *kobj)
+static inline struct device *__kobj_to_dev(struct kobject *kobj)
 {
 	return container_of(kobj, struct device, kobj);
 }
 
+static inline const struct device *__kobj_to_dev_const(const struct kobject *kobj)
+{
+	return container_of(kobj, const struct device, kobj);
+}
+
+/*
+ * container_of() will happily take a const * and spit back a non-const * as it
+ * is just doing pointer math.  But we want to be a bit more careful in the
+ * driver code, so manually force any const * of a kobject to also be a const *
+ * to a device.
+ */
+#define kobj_to_dev(kobj)					\
+	_Generic((kobj),					\
+		 const struct kobject *: __kobj_to_dev_const,	\
+		 struct kobject *: __kobj_to_dev)(kobj)
+
 /**
  * device_iommu_mapped - Returns true when the device DMA is translated
  *			 by an IOMMU
Andy Shevchenko Oct. 4, 2022, 8:25 a.m. UTC | #13
On Tue, Oct 04, 2022 at 10:24:13AM +0200, Greg Kroah-Hartman wrote:
> On Tue, Oct 04, 2022 at 11:14:14AM +0300, Andy Shevchenko wrote:
> > On Tue, Oct 04, 2022 at 09:55:21AM +0200, Greg Kroah-Hartman wrote:
> > > On Mon, Oct 03, 2022 at 08:08:58PM +0000, Sakari Ailus wrote:
> > > > On Mon, Oct 03, 2022 at 06:17:17PM +0200, Greg Kroah-Hartman wrote:

...

> > > > #define kobj_to_dev(kobj)						\
> > > > 	(_Generic((kobj),						\
> > > > 		  const struct kobject *: __kobj_to_dev_const,		\
> > > > 		  struct kobject *: __kobj_to_dev)(kobj))
> > > 
> > > Ah, doh!  I had the (kobj) part in the wrong place, thanks for that
> > > fix...
> > > 
> > > Ok, this looks better, let me see how well the build breaks with some of
> > > these changes
> > 
> > I believe I can rewrite my patch like this and then it will be much nicer since
> > we may constify all the rest without calling __dev_fwnode_const() directly.
> > 
> > Are you agree?
> 
> Yes, I think this is a much better option, try it and see.  Below is the
> patch I'm running through my build systems at the moment, feel free to
> copy the style for your change as well.

Yep, thanks for sharing!
Sakari Ailus Oct. 4, 2022, 9:15 a.m. UTC | #14
Hi Andy, Greg,

On Tue, Oct 04, 2022 at 11:14:14AM +0300, Andy Shevchenko wrote:
> On Tue, Oct 04, 2022 at 09:55:21AM +0200, Greg Kroah-Hartman wrote:
> > On Mon, Oct 03, 2022 at 08:08:58PM +0000, Sakari Ailus wrote:
> > > On Mon, Oct 03, 2022 at 06:17:17PM +0200, Greg Kroah-Hartman wrote:
> 
> ...
> 
> > > #define kobj_to_dev(kobj)						\
> > > 	(_Generic((kobj),						\
> > > 		  const struct kobject *: __kobj_to_dev_const,		\
> > > 		  struct kobject *: __kobj_to_dev)(kobj))
> > 
> > Ah, doh!  I had the (kobj) part in the wrong place, thanks for that
> > fix...
> > 
> > Ok, this looks better, let me see how well the build breaks with some of
> > these changes
> 
> I believe I can rewrite my patch like this and then it will be much nicer since
> we may constify all the rest without calling __dev_fwnode_const() directly.
> 
> Are you agree?

Sounds good to me, thanks!
Andy Shevchenko Oct. 4, 2022, 9:21 a.m. UTC | #15
On Tue, Oct 04, 2022 at 11:25:56AM +0300, Andy Shevchenko wrote:
> On Tue, Oct 04, 2022 at 10:24:13AM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Oct 04, 2022 at 11:14:14AM +0300, Andy Shevchenko wrote:
> > > On Tue, Oct 04, 2022 at 09:55:21AM +0200, Greg Kroah-Hartman wrote:
> > > > On Mon, Oct 03, 2022 at 08:08:58PM +0000, Sakari Ailus wrote:
> > > > > On Mon, Oct 03, 2022 at 06:17:17PM +0200, Greg Kroah-Hartman wrote:

...

> > > > > #define kobj_to_dev(kobj)						\
> > > > > 	(_Generic((kobj),						\
> > > > > 		  const struct kobject *: __kobj_to_dev_const,		\
> > > > > 		  struct kobject *: __kobj_to_dev)(kobj))
> > > > 
> > > > Ah, doh!  I had the (kobj) part in the wrong place, thanks for that
> > > > fix...
> > > > 
> > > > Ok, this looks better, let me see how well the build breaks with some of
> > > > these changes
> > > 
> > > I believe I can rewrite my patch like this and then it will be much nicer since
> > > we may constify all the rest without calling __dev_fwnode_const() directly.
> > > 
> > > Are you agree?
> > 
> > Yes, I think this is a much better option, try it and see.  Below is the
> > patch I'm running through my build systems at the moment, feel free to
> > copy the style for your change as well.
> 
> Yep, thanks for sharing!

v3 has been just sent out.
diff mbox series

Patch

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 4d6278a84868..699f1b115e0a 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -17,13 +17,20 @@ 
 #include <linux/property.h>
 #include <linux/phy.h>
 
-struct fwnode_handle *dev_fwnode(const struct device *dev)
+struct fwnode_handle *dev_fwnode(struct device *dev)
 {
 	return IS_ENABLED(CONFIG_OF) && dev->of_node ?
 		of_fwnode_handle(dev->of_node) : dev->fwnode;
 }
 EXPORT_SYMBOL_GPL(dev_fwnode);
 
+const struct fwnode_handle *dev_fwnode_const(const struct device *dev)
+{
+	return IS_ENABLED(CONFIG_OF) && dev->of_node ?
+		of_fwnode_handle(dev->of_node) : dev->fwnode;
+}
+EXPORT_SYMBOL_GPL(dev_fwnode_const);
+
 /**
  * device_property_present - check if a property of a device is present
  * @dev: Device whose property is being checked
@@ -1202,7 +1209,7 @@  EXPORT_SYMBOL(fwnode_graph_parse_endpoint);
 
 const void *device_get_match_data(const struct device *dev)
 {
-	return fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data, dev);
+	return fwnode_call_ptr_op(dev_fwnode_const(dev), device_get_match_data, dev);
 }
 EXPORT_SYMBOL_GPL(device_get_match_data);
 
diff --git a/include/linux/property.h b/include/linux/property.h
index 117cc200c656..ae5d7f8eccf4 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -32,7 +32,8 @@  enum dev_dma_attr {
 	DEV_DMA_COHERENT,
 };
 
-struct fwnode_handle *dev_fwnode(const struct device *dev);
+struct fwnode_handle *dev_fwnode(struct device *dev);
+const struct fwnode_handle *dev_fwnode_const(const struct device *dev);
 
 bool device_property_present(struct device *dev, const char *propname);
 int device_property_read_u8_array(struct device *dev, const char *propname,