diff mbox series

[2/5] driver core: Introduce an API constify_device_find_child_helper()

Message ID 20240811-const_dfc_prepare-v1-2-d67cc416b3d3@quicinc.com
State Superseded
Headers show
Series driver core: Prevent device_find_child() from modifying caller's match data | expand

Commit Message

Zijun Hu Aug. 11, 2024, 12:18 a.m. UTC
From: Zijun Hu <quic_zijuhu@quicinc.com>

Introduce constify_device_find_child_helper() to replace existing
device_find_child()'s usages whose match functions will modify
caller's match data.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/base/core.c    | 35 +++++++++++++++++++++++++++++++++++
 include/linux/device.h |  7 +++++++
 2 files changed, 42 insertions(+)

Comments

Greg Kroah-Hartman Aug. 13, 2024, 9:45 a.m. UTC | #1
On Sun, Aug 11, 2024 at 08:18:08AM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> Introduce constify_device_find_child_helper() to replace existing
> device_find_child()'s usages whose match functions will modify
> caller's match data.

Ick, that's not a good name, it should be "noun_verb" with the subsystem being on the prefix always.

But why is this even needed?  Device pointers are NOT const for the
obvious reason that they can be changed by loads of different things.
Trying to force them to be const is going to be hard, if not impossible.

thanks,

greg k-h
quic_zijuhu Aug. 13, 2024, 10:50 a.m. UTC | #2
On 8/13/2024 5:45 PM, Greg Kroah-Hartman wrote:
> On Sun, Aug 11, 2024 at 08:18:08AM +0800, Zijun Hu wrote:
>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>
>> Introduce constify_device_find_child_helper() to replace existing
>> device_find_child()'s usages whose match functions will modify
>> caller's match data.
> 
> Ick, that's not a good name, it should be "noun_verb" with the subsystem being on the prefix always.
> 
okay, got it.

is it okay to use device_find_child_mut() suggested by Przemek Kitszel ?

> But why is this even needed?  Device pointers are NOT const for the
> obvious reason that they can be changed by loads of different things.
> Trying to force them to be const is going to be hard, if not impossible.
> 

[PATCH 3/5] have more discussion about these questions with below link:
https://lore.kernel.org/all/8b8ce122-f16b-4207-b03b-f74b15756ae7@icloud.com/


The ultimate goal is to make device_find_child() have below prototype:

struct device *device_find_child(struct device *dev, const void *data,
		int (*match)(struct device *dev, const void *data));

Why ?

(1) It does not make sense, also does not need to, for such device
finding operation to modify caller's match data which is mainly
used for comparison.

(2) It will make the API's match function parameter have the same
signature as all other APIs (bus|class|driver)_find_device().


My idea is that:
use device_find_child() for READ only accessing caller's match data.

use below API if need to Modify caller's data as
constify_device_find_child_helper() does.
int device_for_each_child(struct device *dev, void *data,
                    int (*fn)(struct device *dev, void *data));

So the The ultimate goal is to protect caller's *match data* @*data  NOT
device @*dev.

> thanks,
> 
> greg k-h
Greg Kroah-Hartman Aug. 13, 2024, 10:57 a.m. UTC | #3
On Tue, Aug 13, 2024 at 06:50:04PM +0800, quic_zijuhu wrote:
> On 8/13/2024 5:45 PM, Greg Kroah-Hartman wrote:
> > On Sun, Aug 11, 2024 at 08:18:08AM +0800, Zijun Hu wrote:
> >> From: Zijun Hu <quic_zijuhu@quicinc.com>
> >>
> >> Introduce constify_device_find_child_helper() to replace existing
> >> device_find_child()'s usages whose match functions will modify
> >> caller's match data.
> > 
> > Ick, that's not a good name, it should be "noun_verb" with the subsystem being on the prefix always.
> > 
> okay, got it.
> 
> is it okay to use device_find_child_mut() suggested by Przemek Kitszel ?

No, just switch all callers over to be const and keep the same name.

> > But why is this even needed?  Device pointers are NOT const for the
> > obvious reason that they can be changed by loads of different things.
> > Trying to force them to be const is going to be hard, if not impossible.
> > 
> 
> [PATCH 3/5] have more discussion about these questions with below link:
> https://lore.kernel.org/all/8b8ce122-f16b-4207-b03b-f74b15756ae7@icloud.com/
> 
> 
> The ultimate goal is to make device_find_child() have below prototype:
> 
> struct device *device_find_child(struct device *dev, const void *data,
> 		int (*match)(struct device *dev, const void *data));
> 
> Why ?
> 
> (1) It does not make sense, also does not need to, for such device
> finding operation to modify caller's match data which is mainly
> used for comparison.
> 
> (2) It will make the API's match function parameter have the same
> signature as all other APIs (bus|class|driver)_find_device().
> 
> 
> My idea is that:
> use device_find_child() for READ only accessing caller's match data.
> 
> use below API if need to Modify caller's data as
> constify_device_find_child_helper() does.
> int device_for_each_child(struct device *dev, void *data,
>                     int (*fn)(struct device *dev, void *data));
> 
> So the The ultimate goal is to protect caller's *match data* @*data  NOT
> device @*dev.

Ok, sorry, I was confused.

thanks,

greg k-h
quic_zijuhu Aug. 13, 2024, 11:15 a.m. UTC | #4
On 8/13/2024 6:57 PM, Greg Kroah-Hartman wrote:
> On Tue, Aug 13, 2024 at 06:50:04PM +0800, quic_zijuhu wrote:
>> On 8/13/2024 5:45 PM, Greg Kroah-Hartman wrote:
>>> On Sun, Aug 11, 2024 at 08:18:08AM +0800, Zijun Hu wrote:
>>>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>>>
>>>> Introduce constify_device_find_child_helper() to replace existing
>>>> device_find_child()'s usages whose match functions will modify
>>>> caller's match data.
>>>
>>> Ick, that's not a good name, it should be "noun_verb" with the subsystem being on the prefix always.
>>>
>> okay, got it.
>>
>> is it okay to use device_find_child_mut() suggested by Przemek Kitszel ?
> 
> No, just switch all callers over to be const and keep the same name.
> 
>>> But why is this even needed?  Device pointers are NOT const for the
>>> obvious reason that they can be changed by loads of different things.
>>> Trying to force them to be const is going to be hard, if not impossible.
>>>
>>
>> [PATCH 3/5] have more discussion about these questions with below link:
>> https://lore.kernel.org/all/8b8ce122-f16b-4207-b03b-f74b15756ae7@icloud.com/
>>
>>
>> The ultimate goal is to make device_find_child() have below prototype:
>>
>> struct device *device_find_child(struct device *dev, const void *data,
>> 		int (*match)(struct device *dev, const void *data));
>>
>> Why ?
>>
>> (1) It does not make sense, also does not need to, for such device
>> finding operation to modify caller's match data which is mainly
>> used for comparison.
>>
>> (2) It will make the API's match function parameter have the same
>> signature as all other APIs (bus|class|driver)_find_device().
>>
>>
>> My idea is that:
>> use device_find_child() for READ only accessing caller's match data.
>>
>> use below API if need to Modify caller's data as
>> constify_device_find_child_helper() does.
>> int device_for_each_child(struct device *dev, void *data,
>>                     int (*fn)(struct device *dev, void *data));
>>
>> So the The ultimate goal is to protect caller's *match data* @*data  NOT
>> device @*dev.
> 
> Ok, sorry, I was confused.
> 

Current prototype of the API:
struct device *device_find_child(struct device *dev, void *data,
                                 int (*match)(struct device *dev, void
*data));								

prototype we want:

struct device *device_find_child(struct device *dev, const void *data,
                                 int (*match)(struct device *dev, const
void *data));

The only differences are shown below:
void *data -> const void *data  // as argument of paramter @data of
(*match)().
int (*match)(struct device *dev, void *data) -> int (*match)(struct
device *dev, const void *data).

We don't change type of @dev. we just make above two parameters have the
same types as below existing finding APIs.

struct device *bus_find_device(const struct bus_type *bus, struct device
*start,
                               const void *data,
                               int (*match)(struct device *dev, const
void *data));
struct device *driver_find_device(const struct device_driver *drv,
                                  struct device *start, const void *data,
                                  int (*match)(struct device *dev, const
void *data));
struct device *class_find_device(const struct class *class, const struct
device *start,
                                 const void *data, int (*match)(struct
device *, const void *));
> thanks,
> 
> greg k-h
diff mbox series

Patch

diff --git a/drivers/base/core.c b/drivers/base/core.c
index b1dd8c5590dc..3f3ebdb5aa0b 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -4128,6 +4128,41 @@  struct device *device_find_any_child(struct device *parent)
 }
 EXPORT_SYMBOL_GPL(device_find_any_child);
 
+struct fn_data_struct {
+	int (*match)(struct device *dev, void *data);
+	void *data;
+	struct device *target_device;
+};
+
+static int constify_device_match_fn(struct device *dev, void *data)
+{
+	struct fn_data_struct *fn_data =  data;
+	int res;
+
+	res = fn_data->match(dev, fn_data->data);
+	if (res && get_device(dev)) {
+		fn_data->target_device = dev;
+		return res;
+	}
+
+	return 0;
+}
+
+/*
+ * My mission is to clean up existing match functions which will modify
+ * caller's match data for device_find_child(), so i do not introduce
+ * myself here to prevent that i am used for any other purpose.
+ */
+struct device *constify_device_find_child_helper(struct device *parent, void *data,
+						 int (*match)(struct device *dev, void *data))
+{
+	struct fn_data_struct fn_data = {match, data, NULL};
+
+	device_for_each_child(parent, &fn_data, constify_device_match_fn);
+	return fn_data.target_device;
+}
+EXPORT_SYMBOL_GPL(constify_device_find_child_helper);
+
 int __init devices_init(void)
 {
 	devices_kset = kset_create_and_add("devices", &device_uevent_ops, NULL);
diff --git a/include/linux/device.h b/include/linux/device.h
index 34eb20f5966f..b2423fca3d45 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1078,6 +1078,13 @@  struct device *device_find_child(struct device *dev, void *data,
 struct device *device_find_child_by_name(struct device *parent,
 					 const char *name);
 struct device *device_find_any_child(struct device *parent);
+/*
+ * My mission is to clean up existing match functions which will modify
+ * caller's match data for device_find_child(), so please DO NOT use me
+ * for any other purpose.
+ */
+struct device *constify_device_find_child_helper(struct device *parent, void *data,
+						 int (*match)(struct device *dev, void *data));
 
 int device_rename(struct device *dev, const char *new_name);
 int device_move(struct device *dev, struct device *new_parent,