device property: Consitify a few APIs and correct dev_fwnode()

PATCH Superseded Andy Shevchenko 4 participants 22 messages Sep 28, 2022 – Oct 4, 2022

Subsystems (5)

USB ACPI Driver core USB Type-C class +1 more subsystems

lore

Share & tools

lore

Andy Shevchenko

author reviewer

2022-09-28 10:58 UTC #0 lore raw

v2 cover device property: Consitify a few APIs and correct dev_fwnode()

The property.h has inconsistency in how we annotate the parameters which are not modified anyhow by the certain APIs. Also dev_fwnode() needs to be rectified in sense of the handling const qualifier.

This series improves the above with only a couple of APIs left for now untouched (PHY, which I believe doesn't belong to property.h to begin with).

Changelog v2:
- fixed USB Type-C compilation issues (LKP)
- added tags (Sakari, Heikki)

Andy Shevchenko (5):
device property: Keep dev_fwnode() and dev_fwnode_const() separate device property: Constify fwnode connection match APIs device property: Constify parameter in fwnode_graph_is_endpoint() device property: Constify device child node APIs
device property: Constify parameter in device_dma_supported() and
device_get_dma_attr()

 drivers/base/property.c     | 39 ++++++++++++++++++++++---------------
 drivers/usb/roles/class.c   |  2 +-
 drivers/usb/typec/mux.c     |  8 ++++----
 drivers/usb/typec/retimer.c |  2 +-
 include/linux/property.h    | 32 +++++++++++++++---------------
 5 files changed, 45 insertions(+), 38 deletions(-)

Signature · 1 line

2.35.1

Andy Shevchenko

author reviewer

2022-09-28 10:58 UTC #1 lore raw

PATCH v2 1/5 device property: Keep dev_fwnode() and dev_fwnode_const() separate

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.

Fixes aade55c86033 ("device property: Add const qualifier to device_get_match_data() parameter")

Signed-off-by Andy Shevchenko <[email protected]>

Acked-by Heikki Krogerus <[email protected]>

Reviewed-by Sakari Ailus <[email protected]>

---
 drivers/base/property.c  | 11 +++++++++--
 include/linux/property.h |  3 ++-
 2 files changed, 11 insertions(+), 3 deletions(-)

drivers/base/property.c +9 -2

index 4d6278a84868..699f1b115e0a 100644
@@ -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);

include/linux/property.h +2 -1

index 117cc200c656..ae5d7f8eccf4 100644
@@ -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,

Signature · 1 line

2.35.1

Greg Kroah-Hartman

maintainer

2022-09-28 11:08 UTC #2 lore raw

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

Quoted 28 lines · Andy Shevchenko

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 <[email protected]>
Fixes aade55c86033 ("device property: Add const qualifier to device_get_match_data() parameter")
Signed-off-by Andy Shevchenko <[email protected]>
Acked-by Heikki Krogerus <[email protected]>
Reviewed-by Sakari Ailus <[email protected]>
---
 drivers/base/property.c  | 11 +++++++++--
 include/linux/property.h |  3 ++-
 2 files changed, 11 insertions(+), 3 deletions(-)
drivers/base/property.c +1 -1
index 4d6278a84868..699f1b115e0a 100644
@@ -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 ?

drivers/base/property.c line 32

Quoted 10 lines

drivers/base/property.c +6 -0

 		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...

thanks,

greg k-h

Sakari Ailus

reviewer

2022-09-30 14:31 UTC #3 lore raw

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

Hi Greg,

Quoted 45 lines · Greg Kroah-Hartman

Quoted 28 lines · Andy Shevchenko
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 <[email protected]>
Fixes aade55c86033 ("device property: Add const qualifier to device_get_match_data() parameter")
Signed-off-by Andy Shevchenko <[email protected]>
Acked-by Heikki Krogerus <[email protected]>
Reviewed-by Sakari Ailus <[email protected]>
---
 drivers/base/property.c  | 11 +++++++++--
 include/linux/property.h |  3 ++-
 2 files changed, 11 insertions(+), 3 deletions(-)
drivers/base/property.c +1 -1
index 4d6278a84868..699f1b115e0a 100644
@@ -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 ?
drivers/base/property.c line 32
Quoted 10 lines
drivers/base/property.c +6 -0
 		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:

Quoted 7 lines

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.

Signature · 3 lines

Regards,

Sakari Ailus

Greg Kroah-Hartman

maintainer

2022-09-30 14:43 UTC #4 lore raw

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

Quoted 52 lines · Sakari Ailus

Hi Greg,
Quoted 49 lines · Greg Kroah-Hartman
Quoted 28 lines · Andy Shevchenko
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 <[email protected]>
Fixes aade55c86033 ("device property: Add const qualifier to device_get_match_data() parameter")
Signed-off-by Andy Shevchenko <[email protected]>
Acked-by Heikki Krogerus <[email protected]>
Reviewed-by Sakari Ailus <[email protected]>
---
 drivers/base/property.c  | 11 +++++++++--
 include/linux/property.h |  3 ++-
 2 files changed, 11 insertions(+), 3 deletions(-)
drivers/base/property.c +1 -1
index 4d6278a84868..699f1b115e0a 100644
@@ -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 ?
drivers/base/property.c line 32
Quoted 10 lines
drivers/base/property.c +6 -0
 		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?

Quoted 7 lines

Quoted 3 lines
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.

Quoted 2 lines

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.

Quoted 2 lines

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

reviewer

2022-10-03 11:02 UTC #5 lore raw

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

Hi Greg,

Quoted 64 lines · Greg Kroah-Hartman

Quoted 52 lines · Sakari Ailus
Hi Greg,
Quoted 49 lines · Greg Kroah-Hartman
Quoted 28 lines · Andy Shevchenko
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 <[email protected]>
Fixes aade55c86033 ("device property: Add const qualifier to device_get_match_data() parameter")
Signed-off-by Andy Shevchenko <[email protected]>
Acked-by Heikki Krogerus <[email protected]>
Reviewed-by Sakari Ailus <[email protected]>
---
 drivers/base/property.c  | 11 +++++++++--
 include/linux/property.h |  3 ++-
 2 files changed, 11 insertions(+), 3 deletions(-)
drivers/base/property.c +1 -1
index 4d6278a84868..699f1b115e0a 100644
@@ -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 ?
drivers/base/property.c line 32
Quoted 10 lines
drivers/base/property.c +6 -0
 		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?
Quoted 7 lines
Quoted 3 lines
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.

Quoted 6 lines

Quoted 2 lines
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).

Quoted 11 lines

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/[email protected]/>

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

Quoted 5 lines

Quoted 3 lines
Being able to turn struct device pointers const is certainly not worth violating constness properties.
Agreed, but we can do better...

Signature · 3 lines

Regards,

Sakari Ailus

Greg Kroah-Hartman

maintainer

2022-10-03 15:07 UTC #6 lore raw

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

Quoted 92 lines · Sakari Ailus

Hi Greg,
Quoted 64 lines · Greg Kroah-Hartman
Quoted 52 lines · Sakari Ailus
Hi Greg,
Quoted 49 lines · Greg Kroah-Hartman
Quoted 28 lines · Andy Shevchenko
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 <[email protected]>
Fixes aade55c86033 ("device property: Add const qualifier to device_get_match_data() parameter")
Signed-off-by Andy Shevchenko <[email protected]>
Acked-by Heikki Krogerus <[email protected]>
Reviewed-by Sakari Ailus <[email protected]>
---
 drivers/base/property.c  | 11 +++++++++--
 include/linux/property.h |  3 ++-
 2 files changed, 11 insertions(+), 3 deletions(-)
drivers/base/property.c +1 -1
index 4d6278a84868..699f1b115e0a 100644
@@ -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 ?
drivers/base/property.c line 32
Quoted 10 lines
drivers/base/property.c +6 -0
 		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?
Quoted 7 lines
Quoted 3 lines
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.
Quoted 6 lines
Quoted 2 lines
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).
Quoted 11 lines
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.

Quoted 6 lines

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/[email protected]/>
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

maintainer

2022-10-03 16:17 UTC #7 lore raw

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

Quoted 98 lines · Greg Kroah-Hartman

Quoted 97 lines · Sakari Ailus
Hi Greg,
Quoted 64 lines · Greg Kroah-Hartman
Quoted 52 lines · Sakari Ailus
Hi Greg,
Quoted 49 lines · Greg Kroah-Hartman
Quoted 28 lines · Andy Shevchenko
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 <[email protected]>
Fixes aade55c86033 ("device property: Add const qualifier to device_get_match_data() parameter")
Signed-off-by Andy Shevchenko <[email protected]>
Acked-by Heikki Krogerus <[email protected]>
Reviewed-by Sakari Ailus <[email protected]>
---
 drivers/base/property.c  | 11 +++++++++--
 include/linux/property.h |  3 ++-
 2 files changed, 11 insertions(+), 3 deletions(-)
drivers/base/property.c +1 -1
index 4d6278a84868..699f1b115e0a 100644
@@ -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 ?
drivers/base/property.c line 32
Quoted 10 lines
drivers/base/property.c +6 -0
 		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?
Quoted 7 lines
Quoted 3 lines
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.
Quoted 6 lines
Quoted 2 lines
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).
Quoted 11 lines
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/[email protected]/>

Quoted 8 lines

Quoted 1 line
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:

include/linux/device.h +11 -1

index 424b55df0272..5575c87e6c3b 100644
@@ -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

reviewer

2022-10-03 20:09 UTC #8 lore raw

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

Hi Greg,

Quoted 157 lines · Greg Kroah-Hartman

Quoted 98 lines · Greg Kroah-Hartman
Quoted 97 lines · Sakari Ailus
Hi Greg,
Quoted 64 lines · Greg Kroah-Hartman
Quoted 52 lines · Sakari Ailus
Hi Greg,
Quoted 49 lines · Greg Kroah-Hartman
Quoted 28 lines · Andy Shevchenko
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 <[email protected]>
Fixes aade55c86033 ("device property: Add const qualifier to device_get_match_data() parameter")
Signed-off-by Andy Shevchenko <[email protected]>
Acked-by Heikki Krogerus <[email protected]>
Reviewed-by Sakari Ailus <[email protected]>
---
 drivers/base/property.c  | 11 +++++++++--
 include/linux/property.h |  3 ++-
 2 files changed, 11 insertions(+), 3 deletions(-)
drivers/base/property.c +1 -1
index 4d6278a84868..699f1b115e0a 100644
@@ -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 ?
drivers/base/property.c line 32
Quoted 10 lines
drivers/base/property.c +6 -0
 		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?
Quoted 7 lines
Quoted 3 lines
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.
Quoted 6 lines
Quoted 2 lines
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).
Quoted 11 lines
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/[email protected]/>
Quoted 8 lines
Quoted 1 line
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:
include/linux/device.h +11 -1
index 424b55df0272..5575c87e6c3b 100644
@@ -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)

Quoted 8 lines

      |                                            ~~~~~~~~~~~~~~~~^~~~
(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))

Signature · 3 lines

Kind regards,

Sakari Ailus

Greg Kroah-Hartman

maintainer

2022-10-04 07:55 UTC #9 lore raw

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

Quoted 173 lines · Sakari Ailus

Hi Greg,
Quoted 157 lines · Greg Kroah-Hartman
Quoted 98 lines · Greg Kroah-Hartman
Quoted 97 lines · Sakari Ailus
Hi Greg,
Quoted 64 lines · Greg Kroah-Hartman
Quoted 52 lines · Sakari Ailus
Hi Greg,
Quoted 49 lines · Greg Kroah-Hartman
Quoted 28 lines · Andy Shevchenko
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 <[email protected]>
Fixes aade55c86033 ("device property: Add const qualifier to device_get_match_data() parameter")
Signed-off-by Andy Shevchenko <[email protected]>
Acked-by Heikki Krogerus <[email protected]>
Reviewed-by Sakari Ailus <[email protected]>
---
 drivers/base/property.c  | 11 +++++++++--
 include/linux/property.h |  3 ++-
 2 files changed, 11 insertions(+), 3 deletions(-)
drivers/base/property.c +1 -1
index 4d6278a84868..699f1b115e0a 100644
@@ -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 ?
drivers/base/property.c line 32
Quoted 10 lines
drivers/base/property.c +6 -0
 		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?
Quoted 7 lines
Quoted 3 lines
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.
Quoted 6 lines
Quoted 2 lines
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).
Quoted 11 lines
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/[email protected]/>
Quoted 8 lines
Quoted 1 line
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:
include/linux/device.h +11 -1
index 424b55df0272..5575c87e6c3b 100644
@@ -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)
Quoted 8 lines
      |                                            ~~~~~~~~~~~~~~~~^~~~
(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.

Quoted 6 lines

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

author reviewer

2022-10-04 08:17 UTC #10 lore raw

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

Quoted 2 lines · Greg Kroah-Hartman

Quoted 1 line · Sakari Ailus
On Mon, Oct 03, 2022 at 06:17:17PM +0200, Greg Kroah-Hartman wrote:

...

Quoted 10 lines

Quoted 4 lines
#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?

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Greg Kroah-Hartman

maintainer

2022-10-04 08:24 UTC #11 lore raw

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

Quoted 14 lines · Andy Shevchenko

Quoted 2 lines · Greg Kroah-Hartman
Quoted 1 line · Sakari Ailus
On Mon, Oct 03, 2022 at 06:17:17PM +0200, Greg Kroah-Hartman wrote:
...
Quoted 8 lines
Quoted 4 lines
#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...

Quoted 7 lines

Quoted 2 lines
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

include/linux/device.h +17 -1

index 424b55df0272..023ea50b1916 100644
@@ -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

author reviewer

2022-10-04 08:26 UTC #12 lore raw

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

Quoted 4 lines · Greg Kroah-Hartman

Quoted 3 lines · Andy Shevchenko
Quoted 2 lines · Greg Kroah-Hartman
Quoted 1 line · Sakari Ailus
On Mon, Oct 03, 2022 at 06:17:17PM +0200, Greg Kroah-Hartman wrote:

...

Quoted 11 lines

Quoted 11 lines
Quoted 10 lines
Quoted 4 lines
#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

Quoted 8 lines

Quoted 4 lines
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!

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

author reviewer

2022-10-04 09:22 UTC #13 lore raw

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

Quoted 5 lines · Andy Shevchenko

Quoted 4 lines · Greg Kroah-Hartman
Quoted 3 lines · Andy Shevchenko
Quoted 2 lines · Greg Kroah-Hartman
Quoted 1 line · Sakari Ailus
On Mon, Oct 03, 2022 at 06:17:17PM +0200, Greg Kroah-Hartman wrote:

...

Quoted 14 lines

Quoted 14 lines
Quoted 14 lines
Quoted 10 lines
Quoted 4 lines
#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.

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
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.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Sakari Ailus

reviewer

2022-10-04 09:28 UTC #14 lore raw

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

In reply to Andy Shevchenko

Hi Andy, Greg,

Quoted 14 lines · Andy Shevchenko

Quoted 2 lines · Greg Kroah-Hartman
Quoted 1 line · Sakari Ailus
On Mon, Oct 03, 2022 at 06:17:17PM +0200, Greg Kroah-Hartman wrote:
...
Quoted 8 lines
Quoted 4 lines
#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...

Quoted 7 lines

Quoted 2 lines
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!

Signature · 1 line

Sakari Ailus

Rafael J. Wysocki

maintainer

2022-10-03 11:54 UTC #15 lore raw

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

In reply to Greg Kroah-Hartman

Quoted 75 lines · Greg Kroah-Hartman

Quoted 52 lines · Sakari Ailus
Hi Greg,
Quoted 49 lines · Greg Kroah-Hartman
Quoted 28 lines · Andy Shevchenko
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 <[email protected]>
Fixes aade55c86033 ("device property: Add const qualifier to device_get_match_data() parameter")
Signed-off-by Andy Shevchenko <[email protected]>
Acked-by Heikki Krogerus <[email protected]>
Reviewed-by Sakari Ailus <[email protected]>
---
 drivers/base/property.c  | 11 +++++++++--
 include/linux/property.h |  3 ++-
 2 files changed, 11 insertions(+), 3 deletions(-)
drivers/base/property.c +1 -1
index 4d6278a84868..699f1b115e0a 100644
@@ -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 ?
drivers/base/property.c line 32
Quoted 10 lines
drivers/base/property.c +6 -0
          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?
Quoted 7 lines
Quoted 3 lines
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.
Quoted 2 lines
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:

Quoted 7 lines

        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.

Quoted 4 lines

Quoted 2 lines
Being able to turn struct device pointers const is certainly not worth violating constness properties.
Agreed, but we can do better...

Sakari Ailus

reviewer

2022-10-03 12:01 UTC #16 lore raw

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

Hi Rafael,

Quoted 11 lines · Rafael J. Wysocki

Quoted 10 lines
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.

Quoted 6 lines

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.

Signature · 3 lines

Regards,

Sakari Ailus

Greg Kroah-Hartman

maintainer

2022-10-03 15:05 UTC #17 lore raw

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

In reply to Rafael J. Wysocki

Quoted 85 lines · Rafael J. Wysocki

Quoted 75 lines · Greg Kroah-Hartman
Quoted 52 lines · Sakari Ailus
Hi Greg,
Quoted 49 lines · Greg Kroah-Hartman
Quoted 28 lines · Andy Shevchenko
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 <[email protected]>
Fixes aade55c86033 ("device property: Add const qualifier to device_get_match_data() parameter")
Signed-off-by Andy Shevchenko <[email protected]>
Acked-by Heikki Krogerus <[email protected]>
Reviewed-by Sakari Ailus <[email protected]>
---
 drivers/base/property.c  | 11 +++++++++--
 include/linux/property.h |  3 ++-
 2 files changed, 11 insertions(+), 3 deletions(-)
drivers/base/property.c +1 -1
index 4d6278a84868..699f1b115e0a 100644
@@ -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 ?
drivers/base/property.c line 32
Quoted 10 lines
drivers/base/property.c +6 -0
          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?
Quoted 7 lines
Quoted 3 lines
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.
Quoted 2 lines
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:
Quoted 7 lines
        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.

Quoted 6 lines

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

Andy Shevchenko

author reviewer

2022-09-28 10:58 UTC #18 lore raw

PATCH v2 2/5 device property: Constify fwnode connection match APIs

In reply to Andy Shevchenko

The fwnode and device parameters are not altered in the fwnode connection match APIs, constify them.

Signed-off-by Andy Shevchenko <[email protected]>

Acked-by Heikki Krogerus <[email protected]>

Reviewed-by Sakari Ailus <[email protected]>

---
 drivers/base/property.c     |  8 ++++----
 drivers/usb/roles/class.c   |  2 +-
 drivers/usb/typec/mux.c     |  8 ++++----
 drivers/usb/typec/retimer.c |  2 +-
 include/linux/property.h    | 10 +++++-----
 5 files changed, 15 insertions(+), 15 deletions(-)

drivers/base/property.c +4 -4

index 699f1b115e0a..1a1616c9b599 100644
@@ -1213,7 +1213,7 @@ const void *device_get_match_data(const struct device *dev)
 }
 EXPORT_SYMBOL_GPL(device_get_match_data);
 
-static unsigned int fwnode_graph_devcon_matches(struct fwnode_handle *fwnode,
+static unsigned int fwnode_graph_devcon_matches(const struct fwnode_handle *fwnode,
 						const char *con_id, void *data,
 						devcon_match_fn_t match,
 						void **matches,

@@ -1247,7 +1247,7 @@ static unsigned int fwnode_graph_devcon_matches(struct fwnode_handle *fwnode,
 	return count;
 }
 
-static unsigned int fwnode_devcon_matches(struct fwnode_handle *fwnode,
+static unsigned int fwnode_devcon_matches(const struct fwnode_handle *fwnode,
 					  const char *con_id, void *data,
 					  devcon_match_fn_t match,
 					  void **matches,

@@ -1289,7 +1289,7 @@ static unsigned int fwnode_devcon_matches(struct fwnode_handle *fwnode,
  * device node. @match will be used to convert the connection description to
  * data the caller is expecting to be returned.
  */
-void *fwnode_connection_find_match(struct fwnode_handle *fwnode,
+void *fwnode_connection_find_match(const struct fwnode_handle *fwnode,
 				   const char *con_id, void *data,
 				   devcon_match_fn_t match)
 {

@@ -1326,7 +1326,7 @@ EXPORT_SYMBOL_GPL(fwnode_connection_find_match);
  *
  * Return: Number of matches resolved, or negative errno.
  */
-int fwnode_connection_find_matches(struct fwnode_handle *fwnode,
+int fwnode_connection_find_matches(const struct fwnode_handle *fwnode,
 				   const char *con_id, void *data,
 				   devcon_match_fn_t match,
 				   void **matches, unsigned int matches_len)

drivers/usb/roles/class.c +1 -1

index dfaed7eee94f..a3575a5a18ce 100644
@@ -87,7 +87,7 @@ enum usb_role usb_role_switch_get_role(struct usb_role_switch *sw)
 }
 EXPORT_SYMBOL_GPL(usb_role_switch_get_role);
 
-static void *usb_role_switch_match(struct fwnode_handle *fwnode, const char *id,
+static void *usb_role_switch_match(const struct fwnode_handle *fwnode, const char *id,
 				   void *data)
 {
 	struct device *dev;

drivers/usb/typec/mux.c +4 -4

index 464330776cd6..f81ea26ab389 100644
@@ -32,8 +32,8 @@ static int switch_fwnode_match(struct device *dev, const void *fwnode)
 	return dev_fwnode(dev) == fwnode;
 }
 
-static void *typec_switch_match(struct fwnode_handle *fwnode, const char *id,
-				void *data)
+static void *typec_switch_match(const struct fwnode_handle *fwnode,
+				const char *id, void *data)
 {
 	struct device *dev;

@@ -262,8 +262,8 @@ static int mux_fwnode_match(struct device *dev, const void *fwnode)
 	return dev_fwnode(dev) == fwnode;
 }
 
-static void *typec_mux_match(struct fwnode_handle *fwnode, const char *id,
-			     void *data)
+static void *typec_mux_match(const struct fwnode_handle *fwnode,
+			     const char *id, void *data)
 {
 	const struct typec_altmode_desc *desc = data;
 	struct device *dev;

drivers/usb/typec/retimer.c +1 -1

index 2003731f1bee..8edfdc709a28 100644
@@ -34,7 +34,7 @@ static int retimer_fwnode_match(struct device *dev, const void *fwnode)
 	return dev_fwnode(dev) == fwnode && dev_name_ends_with(dev, "-retimer");
 }
 
-static void *typec_retimer_match(struct fwnode_handle *fwnode, const char *id, void *data)
+static void *typec_retimer_match(const struct fwnode_handle *fwnode, const char *id, void *data)
 {
 	struct device *dev;

include/linux/property.h +5 -5

index ae5d7f8eccf4..6f9d6604edc3 100644
@@ -438,21 +438,21 @@ unsigned int fwnode_graph_get_endpoint_count(struct fwnode_handle *fwnode,
 int fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
 				struct fwnode_endpoint *endpoint);
 
-typedef void *(*devcon_match_fn_t)(struct fwnode_handle *fwnode, const char *id,
+typedef void *(*devcon_match_fn_t)(const struct fwnode_handle *fwnode, const char *id,
 				   void *data);
 
-void *fwnode_connection_find_match(struct fwnode_handle *fwnode,
+void *fwnode_connection_find_match(const struct fwnode_handle *fwnode,
 				   const char *con_id, void *data,
 				   devcon_match_fn_t match);
 
-static inline void *device_connection_find_match(struct device *dev,
+static inline void *device_connection_find_match(const struct device *dev,
 						 const char *con_id, void *data,
 						 devcon_match_fn_t match)
 {
-	return fwnode_connection_find_match(dev_fwnode(dev), con_id, data, match);
+	return fwnode_connection_find_match(dev_fwnode_const(dev), con_id, data, match);
 }
 
-int fwnode_connection_find_matches(struct fwnode_handle *fwnode,
+int fwnode_connection_find_matches(const struct fwnode_handle *fwnode,
 				   const char *con_id, void *data,
 				   devcon_match_fn_t match,
 				   void **matches, unsigned int matches_len);

Signature · 1 line

2.35.1

Andy Shevchenko

author reviewer

2022-09-28 10:58 UTC #19 lore raw

PATCH v2 3/5 device property: Constify parameter in fwnode_graph_is_endpoint()

In reply to Andy Shevchenko

Constify parameter in fwnode_graph_is_endpoint() since it doesn't alter anything related to it.

Signed-off-by Andy Shevchenko <[email protected]>

Acked-by Heikki Krogerus <[email protected]>

Reviewed-by Sakari Ailus <[email protected]>

---
 include/linux/property.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

include/linux/property.h +1 -1

index 6f9d6604edc3..fe440211e529 100644
@@ -406,7 +406,7 @@ struct fwnode_handle *fwnode_graph_get_remote_port(
 struct fwnode_handle *fwnode_graph_get_remote_endpoint(
 	const struct fwnode_handle *fwnode);
 
-static inline bool fwnode_graph_is_endpoint(struct fwnode_handle *fwnode)
+static inline bool fwnode_graph_is_endpoint(const struct fwnode_handle *fwnode)
 {
 	return fwnode_property_present(fwnode, "remote-endpoint");
 }

Signature · 1 line

2.35.1

Andy Shevchenko

author reviewer

2022-09-28 10:58 UTC #20 lore raw

PATCH v2 4/5 device property: Constify device child node APIs

In reply to Andy Shevchenko

The device parameter is not altered in the device child node APIs, constify them.

Signed-off-by Andy Shevchenko <[email protected]>

Acked-by Heikki Krogerus <[email protected]>

Reviewed-by Sakari Ailus <[email protected]>

---
 drivers/base/property.c  | 10 +++++-----
 include/linux/property.h | 12 ++++++------
 2 files changed, 11 insertions(+), 11 deletions(-)

drivers/base/property.c +5 -5

index 1a1616c9b599..04eb5e0ab407 100644
@@ -763,10 +763,10 @@ EXPORT_SYMBOL_GPL(fwnode_get_next_available_child_node);
  * @dev: Device to find the next child node for.
  * @child: Handle to one of the device's child nodes or a null handle.
  */
-struct fwnode_handle *device_get_next_child_node(struct device *dev,
+struct fwnode_handle *device_get_next_child_node(const struct device *dev,
 						 struct fwnode_handle *child)
 {
-	const struct fwnode_handle *fwnode = dev_fwnode(dev);
+	const struct fwnode_handle *fwnode = dev_fwnode_const(dev);
 	struct fwnode_handle *next;
 
 	if (IS_ERR_OR_NULL(fwnode))

@@ -800,10 +800,10 @@ EXPORT_SYMBOL_GPL(fwnode_get_named_child_node);
  * @dev: Device to find the named child node for.
  * @childname: String to match child node name against.
  */
-struct fwnode_handle *device_get_named_child_node(struct device *dev,
+struct fwnode_handle *device_get_named_child_node(const struct device *dev,
 						  const char *childname)
 {
-	return fwnode_get_named_child_node(dev_fwnode(dev), childname);
+	return fwnode_get_named_child_node(dev_fwnode_const(dev), childname);
 }
 EXPORT_SYMBOL_GPL(device_get_named_child_node);

@@ -859,7 +859,7 @@ EXPORT_SYMBOL_GPL(fwnode_device_is_available);
  * device_get_child_node_count - return the number of child nodes for device
  * @dev: Device to cound the child nodes for
  */
-unsigned int device_get_child_node_count(struct device *dev)
+unsigned int device_get_child_node_count(const struct device *dev)
 {
 	struct fwnode_handle *child;
 	unsigned int count = 0;

include/linux/property.h +6 -6

index fe440211e529..31dd6cbea9b0 100644
@@ -110,16 +110,16 @@ struct fwnode_handle *fwnode_get_next_available_child_node(
 	for (child = fwnode_get_next_available_child_node(fwnode, NULL); child;\
 	     child = fwnode_get_next_available_child_node(fwnode, child))
 
-struct fwnode_handle *device_get_next_child_node(
-	struct device *dev, struct fwnode_handle *child);
+struct fwnode_handle *device_get_next_child_node(const struct device *dev,
+						 struct fwnode_handle *child);
 
 #define device_for_each_child_node(dev, child)				\
 	for (child = device_get_next_child_node(dev, NULL); child;	\
 	     child = device_get_next_child_node(dev, child))
 
-struct fwnode_handle *fwnode_get_named_child_node(
-	const struct fwnode_handle *fwnode, const char *childname);
-struct fwnode_handle *device_get_named_child_node(struct device *dev,
+struct fwnode_handle *fwnode_get_named_child_node(const struct fwnode_handle *fwnode,
+						  const char *childname);
+struct fwnode_handle *device_get_named_child_node(const struct device *dev,
 						  const char *childname);
 
 struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode);

@@ -128,7 +128,7 @@ void fwnode_handle_put(struct fwnode_handle *fwnode);
 int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index);
 int fwnode_irq_get_byname(const struct fwnode_handle *fwnode, const char *name);
 
-unsigned int device_get_child_node_count(struct device *dev);
+unsigned int device_get_child_node_count(const struct device *dev);
 
 static inline bool device_property_read_bool(struct device *dev,
 					     const char *propname)

Signature · 1 line

2.35.1

Andy Shevchenko

author reviewer

2022-09-28 10:58 UTC #21 lore raw

PATCH v2 5/5 device property: Constify parameter in device_dma_supported() and device_get_dma_attr()

In reply to Andy Shevchenko

Constify parameter in device_dma_supported() and device_get_dma_attr() since they don't alter anything related to it.

Signed-off-by Andy Shevchenko <[email protected]>

Acked-by Heikki Krogerus <[email protected]>

Reviewed-by Sakari Ailus <[email protected]>

---
 drivers/base/property.c  | 10 +++++-----
 include/linux/property.h |  5 ++---
 2 files changed, 7 insertions(+), 8 deletions(-)

drivers/base/property.c +5 -5

index 04eb5e0ab407..5b0417f7e561 100644
@@ -871,18 +871,18 @@ unsigned int device_get_child_node_count(const struct device *dev)
 }
 EXPORT_SYMBOL_GPL(device_get_child_node_count);
 
-bool device_dma_supported(struct device *dev)
+bool device_dma_supported(const struct device *dev)
 {
-	return fwnode_call_bool_op(dev_fwnode(dev), device_dma_supported);
+	return fwnode_call_bool_op(dev_fwnode_const(dev), device_dma_supported);
 }
 EXPORT_SYMBOL_GPL(device_dma_supported);
 
-enum dev_dma_attr device_get_dma_attr(struct device *dev)
+enum dev_dma_attr device_get_dma_attr(const struct device *dev)
 {
-	if (!fwnode_has_op(dev_fwnode(dev), device_get_dma_attr))
+	if (!fwnode_has_op(dev_fwnode_const(dev), device_get_dma_attr))
 		return DEV_DMA_NOT_SUPPORTED;
 
-	return fwnode_call_int_op(dev_fwnode(dev), device_get_dma_attr);
+	return fwnode_call_int_op(dev_fwnode_const(dev), device_get_dma_attr);
 }
 EXPORT_SYMBOL_GPL(device_get_dma_attr);

include/linux/property.h +2 -3

index 31dd6cbea9b0..a9498757155d 100644
@@ -384,9 +384,8 @@ property_entries_dup(const struct property_entry *properties);
 
 void property_entries_free(const struct property_entry *properties);
 
-bool device_dma_supported(struct device *dev);
-
-enum dev_dma_attr device_get_dma_attr(struct device *dev);
+bool device_dma_supported(const struct device *dev);
+enum dev_dma_attr device_get_dma_attr(const struct device *dev);
 
 const void *device_get_match_data(const struct device *dev);

Signature · 1 line

2.35.1