[PATCH v3 0/3] driver core: Prevent device_find_child() from modifying caller's match data

[PATCH v3 0/3] driver core: Prevent device_find_child() from modifying caller's match data

[Zijun Hu]

This patch series is to prepare for constifying the following driver API:

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

How to constify the API ?
There are total 30 usages of the API in kernel tree:

For 2/30 usages, the API's match function (*match)() will modify
caller's match data @*data, and this patch series will clean up them.

For remaining 28/30, the following patch series will simply change its
relevant parameter type to const void *.
https://lore.kernel.org/all/20240811-const_dfc_done-v1-1-9d85e3f943cb@quicinc.com/

Why to constify the API ?

(1) It normally 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 and match data parameter
have the same type as all other APIs (bus|class|driver)_find_device().

(3) It will give driver author hints about choice between this API and
the following one:
int device_for_each_child(struct device *dev, void *data,
		int (*fn)(struct device *dev, void *data));
 

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
Changes in v3:
- Git rebase
- Correct commit message for the driver core patch
- Use changes suggested by Ira Weiny cxl/region
- Drop firewire core patch
- Make qcom/emac follow cxl/region solution suggested by Greg
- Link to v2: https://lore.kernel.org/r/20240815-const_dfc_prepare-v2-0-8316b87b8ff9@quicinc.com

Changes in v2:
- Give up introducing the API constify_device_find_child_helper()
- Correct commit message and inline comments
- Implement a driver specific and equivalent one instead of device_find_child()
- Link to v1: https://lore.kernel.org/r/20240811-const_dfc_prepare-v1-0-d67cc416b3d3@quicinc.com

---
Zijun Hu (3):
      driver core: Make parameter check consistent for API cluster device_(for_each|find)_child()
      cxl/region: Find free cxl decoder by device_for_each_child()
      net: qcom/emac: Prevent device_find_child() from modifying caller's match data

 drivers/base/core.c                             |  6 ++---
 drivers/cxl/core/region.c                       | 30 ++++++++++++++++++++-----
 drivers/net/ethernet/qualcomm/emac/emac-sgmii.c | 22 +++++++++++++-----
 3 files changed, 44 insertions(+), 14 deletions(-)
---
base-commit: 888f67e621dda5c2804a696524e28d0ca4cf0a80
change-id: 20240811-const_dfc_prepare-3ff23c6598e5

Best regards,
-- 
Zijun Hu <quic_zijuhu@quicinc.com>

[PATCH v3 1/3] driver core: Make parameter check consistent for API cluster device_(for_each|find)_child()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

The following API cluster takes the same type parameter list, but do not
have consistent parameter check as shown below.

device_for_each_child(struct device *parent, ...)  // check (!parent->p)
device_for_each_child_reverse(struct device *parent, ...) // same as above
device_find_child(struct device *parent, ...)      // check (!parent)

Fixed by using consistent check (!parent || !parent->p) which covers
both existing checks for the cluster.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/base/core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 4bc8b88d697e..c066b581ce34 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3993,7 +3993,7 @@ int device_for_each_child(struct device *parent, void *data,
 	struct device *child;
 	int error = 0;
 
-	if (!parent->p)
+	if (!parent || !parent->p)
 		return 0;
 
 	klist_iter_init(&parent->p->klist_children, &i);
@@ -4023,7 +4023,7 @@ int device_for_each_child_reverse(struct device *parent, void *data,
 	struct device *child;
 	int error = 0;
 
-	if (!parent->p)
+	if (!parent || !parent->p)
 		return 0;
 
 	klist_iter_init(&parent->p->klist_children, &i);
@@ -4057,7 +4057,7 @@ struct device *device_find_child(struct device *parent, void *data,
 	struct klist_iter i;
 	struct device *child;
 
-	if (!parent)
+	if (!parent || !parent->p)
 		return NULL;
 
 	klist_iter_init(&parent->p->klist_children, &i);

-- 
2.34.1

[PATCH v3 2/3] cxl/region: Find free cxl decoder by device_for_each_child()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

To prepare for constifying the following old driver core API:

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

The new API does not allow its match function (*match)() to modify
caller's match data @*data, but match_free_decoder() as the old API's
match function indeed modifies relevant match data, so it is not suitable
for the new API any more, solved by using device_for_each_child() to
implement relevant finding free cxl decoder function.

Suggested-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/cxl/core/region.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 21ad5f242875..c2068e90bf2f 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -794,10 +794,15 @@ static size_t show_targetN(struct cxl_region *cxlr, char *buf, int pos)
 	return rc;
 }
 
+struct cxld_match_data {
+	int id;
+	struct device *target_device;
+};
+
 static int match_free_decoder(struct device *dev, void *data)
 {
+	struct cxld_match_data *match_data = data;
 	struct cxl_decoder *cxld;
-	int *id = data;
 
 	if (!is_switch_decoder(dev))
 		return 0;
@@ -805,17 +810,31 @@ static int match_free_decoder(struct device *dev, void *data)
 	cxld = to_cxl_decoder(dev);
 
 	/* enforce ordered allocation */
-	if (cxld->id != *id)
+	if (cxld->id != match_data->id)
 		return 0;
 
-	if (!cxld->region)
+	if (!cxld->region) {
+		match_data->target_device = get_device(dev);
 		return 1;
+	}
 
-	(*id)++;
+	match_data->id++;
 
 	return 0;
 }
 
+/* NOTE: need to drop the reference with put_device() after use. */
+static struct device *find_free_decoder(struct device *parent)
+{
+	struct cxld_match_data match_data = {
+		.id = 0,
+		.target_device = NULL,
+	};
+
+	device_for_each_child(parent, &match_data, match_free_decoder);
+	return match_data.target_device;
+}
+
 static int match_auto_decoder(struct device *dev, void *data)
 {
 	struct cxl_region_params *p = data;
@@ -840,7 +859,6 @@ cxl_region_find_decoder(struct cxl_port *port,
 			struct cxl_region *cxlr)
 {
 	struct device *dev;
-	int id = 0;
 
 	if (port == cxled_to_port(cxled))
 		return &cxled->cxld;
@@ -849,7 +867,7 @@ cxl_region_find_decoder(struct cxl_port *port,
 		dev = device_find_child(&port->dev, &cxlr->params,
 					match_auto_decoder);
 	else
-		dev = device_find_child(&port->dev, &id, match_free_decoder);
+		dev = find_free_decoder(&port->dev);
 	if (!dev)
 		return NULL;
 	/*

-- 
2.34.1

[PATCH v3 3/3] net: qcom/emac: Prevent device_find_child() from modifying caller's match data

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

To prepare for constifying the following old driver core API:

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

The new API does not allow its match function (*match)() to modify
caller's match data @*data, but emac_sgmii_acpi_match() as the old
API's match function indeed modifies relevant match data, so it is not
suitable for the new API any more, solved by using device_for_each_child()
to implement relevant functions.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/net/ethernet/qualcomm/emac/emac-sgmii.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c b/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
index e4bc18009d08..29392c63d115 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
@@ -293,6 +293,11 @@ static struct sgmii_ops qdf2400_ops = {
 };
 #endif
 
+struct emac_match_data {
+	struct sgmii_ops **sgmii_ops;
+	struct device *target_device;
+};
+
 static int emac_sgmii_acpi_match(struct device *dev, void *data)
 {
 #ifdef CONFIG_ACPI
@@ -303,7 +308,7 @@ static int emac_sgmii_acpi_match(struct device *dev, void *data)
 		{}
 	};
 	const struct acpi_device_id *id = acpi_match_device(match_table, dev);
-	struct sgmii_ops **ops = data;
+	struct emac_match_data *match_data = data;
 
 	if (id) {
 		acpi_handle handle = ACPI_HANDLE(dev);
@@ -324,10 +329,12 @@ static int emac_sgmii_acpi_match(struct device *dev, void *data)
 
 		switch (hrv) {
 		case 1:
-			*ops = &qdf2432_ops;
+			*match_data->sgmii_ops = &qdf2432_ops;
+			match_data->target_device = get_device(dev);
 			return 1;
 		case 2:
-			*ops = &qdf2400_ops;
+			*match_data->sgmii_ops = &qdf2400_ops;
+			match_data->target_device = get_device(dev);
 			return 1;
 		}
 	}
@@ -356,10 +363,15 @@ int emac_sgmii_config(struct platform_device *pdev, struct emac_adapter *adpt)
 	int ret;
 
 	if (has_acpi_companion(&pdev->dev)) {
+		struct emac_match_data match_data = {
+			.sgmii_ops = &phy->sgmii_ops,
+			.target_device = NULL,
+		};
 		struct device *dev;
 
-		dev = device_find_child(&pdev->dev, &phy->sgmii_ops,
-					emac_sgmii_acpi_match);
+		device_for_each_child(&pdev->dev, &match_data, emac_sgmii_acpi_match);
+		/* Need to put_device(@dev) after use */
+		dev = match_data.target_device;
 
 		if (!dev) {
 			dev_warn(&pdev->dev, "cannot find internal phy node\n");

-- 
2.34.1

Re: [PATCH v3 2/3] cxl/region: Find free cxl decoder by device_for_each_child()

[Zijun Hu]

On 2024/8/24 17:07, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> To prepare for constifying the following old driver core API:
> 
> struct device *device_find_child(struct device *dev, void *data,
> 		int (*match)(struct device *dev, void *data));
> to new:
> struct device *device_find_child(struct device *dev, const void *data,
> 		int (*match)(struct device *dev, const void *data));
> 
> The new API does not allow its match function (*match)() to modify
> caller's match data @*data, but match_free_decoder() as the old API's
> match function indeed modifies relevant match data, so it is not suitable
> for the new API any more, solved by using device_for_each_child() to
> implement relevant finding free cxl decoder function.
> 
> Suggested-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/cxl/core/region.c | 30 ++++++++++++++++++++++++------
>  1 file changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 21ad5f242875..c2068e90bf2f 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -794,10 +794,15 @@ static size_t show_targetN(struct cxl_region *cxlr, char *buf, int pos)
>  	return rc;
>  }
>  
> +struct cxld_match_data {
> +	int id;
> +	struct device *target_device;
> +};
> +
>  static int match_free_decoder(struct device *dev, void *data)
>  {
> +	struct cxld_match_data *match_data = data;
>  	struct cxl_decoder *cxld;
> -	int *id = data;
>  
>  	if (!is_switch_decoder(dev))
>  		return 0;
> @@ -805,17 +810,31 @@ static int match_free_decoder(struct device *dev, void *data)
>  	cxld = to_cxl_decoder(dev);
>  
>  	/* enforce ordered allocation */
> -	if (cxld->id != *id)
> +	if (cxld->id != match_data->id)
>  		return 0;
>  
> -	if (!cxld->region)
> +	if (!cxld->region) {
> +		match_data->target_device = get_device(dev);

get_device() must != NULL since @dev != NULL for the function parameter
of both device_for_each_child() and device_find_child().

so this change's logic is same as previous existing logic.

>  		return 1;
> +	}
>  
> -	(*id)++;
> +	match_data->id++;
>  
>  	return 0;
>  }
>  
> +/* NOTE: need to drop the reference with put_device() after use. */
> +static struct device *find_free_decoder(struct device *parent)
> +{
> +	struct cxld_match_data match_data = {
> +		.id = 0,
> +		.target_device = NULL,
> +	};
> +
> +	device_for_each_child(parent, &match_data, match_free_decoder);
> +	return match_data.target_device;
> +}
> +
>  static int match_auto_decoder(struct device *dev, void *data)
>  {
>  	struct cxl_region_params *p = data;
> @@ -840,7 +859,6 @@ cxl_region_find_decoder(struct cxl_port *port,
>  			struct cxl_region *cxlr)
>  {
>  	struct device *dev;
> -	int id = 0;
>  
>  	if (port == cxled_to_port(cxled))
>  		return &cxled->cxld;
> @@ -849,7 +867,7 @@ cxl_region_find_decoder(struct cxl_port *port,
>  		dev = device_find_child(&port->dev, &cxlr->params,
>  					match_auto_decoder);
>  	else
> -		dev = device_find_child(&port->dev, &id, match_free_decoder);
> +		dev = find_free_decoder(&port->dev);
>  	if (!dev)
>  		return NULL;
>  	/*
> 

Re: [PATCH v3 2/3] cxl/region: Find free cxl decoder by device_for_each_child()

[Jonathan Cameron]

On Sat, 24 Aug 2024 17:07:44 +0800
Zijun Hu <zijun_hu@icloud.com> wrote:

> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> To prepare for constifying the following old driver core API:
> 
> struct device *device_find_child(struct device *dev, void *data,
> 		int (*match)(struct device *dev, void *data));
> to new:
> struct device *device_find_child(struct device *dev, const void *data,
> 		int (*match)(struct device *dev, const void *data));
> 
> The new API does not allow its match function (*match)() to modify
> caller's match data @*data, but match_free_decoder() as the old API's
> match function indeed modifies relevant match data, so it is not suitable
> for the new API any more, solved by using device_for_each_child() to
> implement relevant finding free cxl decoder function.
> 
> Suggested-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
This seems to functionally do the same as before.

I'm not sure I like the original code though so a comment inline.

> ---
>  drivers/cxl/core/region.c | 30 ++++++++++++++++++++++++------
>  1 file changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 21ad5f242875..c2068e90bf2f 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -794,10 +794,15 @@ static size_t show_targetN(struct cxl_region *cxlr, char *buf, int pos)
>  	return rc;
>  }
>  
> +struct cxld_match_data {
> +	int id;
> +	struct device *target_device;
> +};
> +
>  static int match_free_decoder(struct device *dev, void *data)
>  {
> +	struct cxld_match_data *match_data = data;
>  	struct cxl_decoder *cxld;
> -	int *id = data;
>  
>  	if (!is_switch_decoder(dev))
>  		return 0;
> @@ -805,17 +810,31 @@ static int match_free_decoder(struct device *dev, void *data)
>  	cxld = to_cxl_decoder(dev);
>  
>  	/* enforce ordered allocation */
> -	if (cxld->id != *id)
> +	if (cxld->id != match_data->id)

Why do we carry on in this case?
Conditions are:
1. Start match_data->id == 0
2. First pass cxld->id == 0 (all good) or
   cxld->id == 1 say (and we skip until we match
   on cxld->id == 0 (perhaps on the second child if they are
   ordered (1, 0, 2) etc. 

If we skipped and then matched on second child but it was
already in use (so region set), we will increment match_data->id to 1
but never find that as it was the one we skipped.

So this can only work if the children are ordered.
So if that's the case and the line above is just a sanity check
on that, it should be noisier (so an error print) and might
as well fail as if it doesn't match all bets are off.

Jonathan
 
>  		return 0;
>  
> -	if (!cxld->region)
> +	if (!cxld->region) {
> +		match_data->target_device = get_device(dev);
>  		return 1;
> +	}
>  
> -	(*id)++;
> +	match_data->id++;
>  
>  	return 0;
>  }
>  
> +/* NOTE: need to drop the reference with put_device() after use. */
> +static struct device *find_free_decoder(struct device *parent)
> +{
> +	struct cxld_match_data match_data = {
> +		.id = 0,
> +		.target_device = NULL,
> +	};
> +
> +	device_for_each_child(parent, &match_data, match_free_decoder);
> +	return match_data.target_device;
> +}
> +
>  static int match_auto_decoder(struct device *dev, void *data)
>  {
>  	struct cxl_region_params *p = data;
> @@ -840,7 +859,6 @@ cxl_region_find_decoder(struct cxl_port *port,
>  			struct cxl_region *cxlr)
>  {
>  	struct device *dev;
> -	int id = 0;
>  
>  	if (port == cxled_to_port(cxled))
>  		return &cxled->cxld;
> @@ -849,7 +867,7 @@ cxl_region_find_decoder(struct cxl_port *port,
>  		dev = device_find_child(&port->dev, &cxlr->params,
>  					match_auto_decoder);
>  	else
> -		dev = device_find_child(&port->dev, &id, match_free_decoder);
> +		dev = find_free_decoder(&port->dev);
>  	if (!dev)
>  		return NULL;
>  	/*
> 

Re: [PATCH v3 2/3] cxl/region: Find free cxl decoder by device_for_each_child()

[Zijun Hu]

On 2024/8/27 19:30, Jonathan Cameron wrote:
> On Sat, 24 Aug 2024 17:07:44 +0800
> Zijun Hu <zijun_hu@icloud.com> wrote:
> 
>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>
>> To prepare for constifying the following old driver core API:
>>
>> struct device *device_find_child(struct device *dev, void *data,
>> 		int (*match)(struct device *dev, void *data));
>> to new:
>> struct device *device_find_child(struct device *dev, const void *data,
>> 		int (*match)(struct device *dev, const void *data));
>>
>> The new API does not allow its match function (*match)() to modify
>> caller's match data @*data, but match_free_decoder() as the old API's
>> match function indeed modifies relevant match data, so it is not suitable
>> for the new API any more, solved by using device_for_each_child() to
>> implement relevant finding free cxl decoder function.
>>
>> Suggested-by: Ira Weiny <ira.weiny@intel.com>
>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> This seems to functionally do the same as before.
> 

yes, this change have the same logic as previous existing logic.

> I'm not sure I like the original code though so a comment inline.
> 
>> ---
>>  drivers/cxl/core/region.c | 30 ++++++++++++++++++++++++------
>>  1 file changed, 24 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index 21ad5f242875..c2068e90bf2f 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> @@ -794,10 +794,15 @@ static size_t show_targetN(struct cxl_region *cxlr, char *buf, int pos)
>>  	return rc;
>>  }
>>  
>> +struct cxld_match_data {
>> +	int id;
>> +	struct device *target_device;
>> +};
>> +
>>  static int match_free_decoder(struct device *dev, void *data)
>>  {
>> +	struct cxld_match_data *match_data = data;
>>  	struct cxl_decoder *cxld;
>> -	int *id = data;
>>  
>>  	if (!is_switch_decoder(dev))
>>  		return 0;
>> @@ -805,17 +810,31 @@ static int match_free_decoder(struct device *dev, void *data)
>>  	cxld = to_cxl_decoder(dev);
>>  
>>  	/* enforce ordered allocation */
>> -	if (cxld->id != *id)
>> +	if (cxld->id != match_data->id)
> 
> Why do we carry on in this case?
> Conditions are:
> 1. Start match_data->id == 0
> 2. First pass cxld->id == 0 (all good) or
>    cxld->id == 1 say (and we skip until we match
>    on cxld->id == 0 (perhaps on the second child if they are
>    ordered (1, 0, 2) etc. 
> 
> If we skipped and then matched on second child but it was
> already in use (so region set), we will increment match_data->id to 1
> but never find that as it was the one we skipped.
> 
> So this can only work if the children are ordered.
> So if that's the case and the line above is just a sanity check
> on that, it should be noisier (so an error print) and might
> as well fail as if it doesn't match all bets are off.
> 

it seems Ira Weiny also has some concerns related to previous existing
logic as following:

https://lore.kernel.org/all/66c4a136d9764_2ddc2429435@iweiny-mobl.notmuch/
"Also for those working on CXL I'm questioning the use of ID here and
the dependence on the id's being added to the parent in order.  Is that
a guarantee?"

perhaps, create a new dedicated thread to discuss original design.

> Jonathan
>  
>>  		return 0;
>>  
>> -	if (!cxld->region)
>> +	if (!cxld->region) {
>> +		match_data->target_device = get_device(dev);
>>  		return 1;
>> +	}
>>  
>> -	(*id)++;
>> +	match_data->id++;
>>  
>>  	return 0;
>>  }
>>  
>> +/* NOTE: need to drop the reference with put_device() after use. */
>> +static struct device *find_free_decoder(struct device *parent)
>> +{
>> +	struct cxld_match_data match_data = {
>> +		.id = 0,
>> +		.target_device = NULL,
>> +	};
>> +
>> +	device_for_each_child(parent, &match_data, match_free_decoder);
>> +	return match_data.target_device;
>> +}
>> +
>>  static int match_auto_decoder(struct device *dev, void *data)
>>  {
>>  	struct cxl_region_params *p = data;
>> @@ -840,7 +859,6 @@ cxl_region_find_decoder(struct cxl_port *port,
>>  			struct cxl_region *cxlr)
>>  {
>>  	struct device *dev;
>> -	int id = 0;
>>  
>>  	if (port == cxled_to_port(cxled))
>>  		return &cxled->cxld;
>> @@ -849,7 +867,7 @@ cxl_region_find_decoder(struct cxl_port *port,
>>  		dev = device_find_child(&port->dev, &cxlr->params,
>>  					match_auto_decoder);
>>  	else
>> -		dev = device_find_child(&port->dev, &id, match_free_decoder);
>> +		dev = find_free_decoder(&port->dev);
>>  	if (!dev)
>>  		return NULL;
>>  	/*
>>
> 

Re: [PATCH v3 2/3] cxl/region: Find free cxl decoder by device_for_each_child()

[Zijun Hu]

On 2024/8/27 19:53, Zijun Hu wrote:
> On 2024/8/27 19:30, Jonathan Cameron wrote:
>> On Sat, 24 Aug 2024 17:07:44 +0800
>> Zijun Hu <zijun_hu@icloud.com> wrote:
>>
>>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>>
>>> To prepare for constifying the following old driver core API:
>>>
>>> struct device *device_find_child(struct device *dev, void *data,
>>> 		int (*match)(struct device *dev, void *data));
>>> to new:
>>> struct device *device_find_child(struct device *dev, const void *data,
>>> 		int (*match)(struct device *dev, const void *data));
>>>
>>> The new API does not allow its match function (*match)() to modify
>>> caller's match data @*data, but match_free_decoder() as the old API's
>>> match function indeed modifies relevant match data, so it is not suitable
>>> for the new API any more, solved by using device_for_each_child() to
>>> implement relevant finding free cxl decoder function.
>>>
>>> Suggested-by: Ira Weiny <ira.weiny@intel.com>
>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>> This seems to functionally do the same as before.
>>
> 
> yes, this change have the same logic as previous existing logic.
> 
>> I'm not sure I like the original code though so a comment inline.
>>
>>> ---
>>>  drivers/cxl/core/region.c | 30 ++++++++++++++++++++++++------
>>>  1 file changed, 24 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>>> index 21ad5f242875..c2068e90bf2f 100644
>>> --- a/drivers/cxl/core/region.c
>>> +++ b/drivers/cxl/core/region.c
>>> @@ -794,10 +794,15 @@ static size_t show_targetN(struct cxl_region *cxlr, char *buf, int pos)
>>>  	return rc;
>>>  }
>>>  
>>> +struct cxld_match_data {
>>> +	int id;
>>> +	struct device *target_device;
>>> +};
>>> +
>>>  static int match_free_decoder(struct device *dev, void *data)
>>>  {
>>> +	struct cxld_match_data *match_data = data;
>>>  	struct cxl_decoder *cxld;
>>> -	int *id = data;
>>>  
>>>  	if (!is_switch_decoder(dev))
>>>  		return 0;
>>> @@ -805,17 +810,31 @@ static int match_free_decoder(struct device *dev, void *data)
>>>  	cxld = to_cxl_decoder(dev);
>>>  
>>>  	/* enforce ordered allocation */
>>> -	if (cxld->id != *id)
>>> +	if (cxld->id != match_data->id)
>>
>> Why do we carry on in this case?
>> Conditions are:
>> 1. Start match_data->id == 0
>> 2. First pass cxld->id == 0 (all good) or
>>    cxld->id == 1 say (and we skip until we match
>>    on cxld->id == 0 (perhaps on the second child if they are
>>    ordered (1, 0, 2) etc. 
>>
>> If we skipped and then matched on second child but it was
>> already in use (so region set), we will increment match_data->id to 1
>> but never find that as it was the one we skipped.
>>
>> So this can only work if the children are ordered.
>> So if that's the case and the line above is just a sanity check
>> on that, it should be noisier (so an error print) and might
>> as well fail as if it doesn't match all bets are off.
>>
> 
> it seems Ira Weiny also has some concerns related to previous existing
> logic as following:
> 
> https://lore.kernel.org/all/66c4a136d9764_2ddc2429435@iweiny-mobl.notmuch/
> "Also for those working on CXL I'm questioning the use of ID here and
> the dependence on the id's being added to the parent in order.  Is that
> a guarantee?"
> 
Hi Jonathan, ira

i don't know CXL decoder at all. For your concerns about original logic:

is it okay to find a child with the minimal index as shown below ?

i would like to submit it as a separate patch if you like it.


diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 21ad5f242875..d10cca02eba8 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -797,21 +797,27 @@ static size_t show_targetN(struct cxl_region
*cxlr, char *buf, int pos)
 static int match_free_decoder(struct device *dev, void *data)
 {
        struct cxl_decoder *cxld;
-       int *id = data;
+       struct cxl_decoder *target_cxld;
+       struct device **target_device = data;

        if (!is_switch_decoder(dev))
                return 0;

        cxld = to_cxl_decoder(dev);

-       /* enforce ordered allocation */
-       if (cxld->id != *id)
+       if (cxld->region)
                return 0;

-       if (!cxld->region)
-               return 1;
+       if (!*target_device) {
+               *target_device = get_device(dev);
+               return 0;
+       }

-       (*id)++;
+       target_cxld = to_cxl_decoder(*target_device);
+       if (cxld->id < target_cxld->id) {
+               put_device(*target_device);
+               *target_device = get_device(dev);
+       }

        return 0;
 }
@@ -839,8 +845,7 @@ cxl_region_find_decoder(struct cxl_port *port,
                        struct cxl_endpoint_decoder *cxled,
                        struct cxl_region *cxlr)
 {
-       struct device *dev;
-       int id = 0;
+       struct device *dev = NULL;

        if (port == cxled_to_port(cxled))
                return &cxled->cxld;
@@ -849,7 +854,7 @@ cxl_region_find_decoder(struct cxl_port *port,
                dev = device_find_child(&port->dev, &cxlr->params,
                                        match_auto_decoder);
        else
-               dev = device_find_child(&port->dev, &id,
match_free_decoder);
+               device_for_each_child(&port->dev, &dev, match_free_decoder);
        if (!dev)
                return NULL;
        /*


> perhaps, create a new dedicated thread to discuss original design.
> 
>> Jonathan
>>  
>>>  		return 0;
>>>  
>>> -	if (!cxld->region)
>>> +	if (!cxld->region) {
>>> +		match_data->target_device = get_device(dev);
>>>  		return 1;
>>> +	}
>>>  
>>> -	(*id)++;
>>> +	match_data->id++;
>>>  
>>>  	return 0;
>>>  }
>>>  
>>> +/* NOTE: need to drop the reference with put_device() after use. */
>>> +static struct device *find_free_decoder(struct device *parent)
>>> +{
>>> +	struct cxld_match_data match_data = {
>>> +		.id = 0,
>>> +		.target_device = NULL,
>>> +	};
>>> +
>>> +	device_for_each_child(parent, &match_data, match_free_decoder);
>>> +	return match_data.target_device;
>>> +}
>>> +
>>>  static int match_auto_decoder(struct device *dev, void *data)
>>>  {
>>>  	struct cxl_region_params *p = data;
>>> @@ -840,7 +859,6 @@ cxl_region_find_decoder(struct cxl_port *port,
>>>  			struct cxl_region *cxlr)
>>>  {
>>>  	struct device *dev;
>>> -	int id = 0;
>>>  
>>>  	if (port == cxled_to_port(cxled))
>>>  		return &cxled->cxld;
>>> @@ -849,7 +867,7 @@ cxl_region_find_decoder(struct cxl_port *port,
>>>  		dev = device_find_child(&port->dev, &cxlr->params,
>>>  					match_auto_decoder);
>>>  	else
>>> -		dev = device_find_child(&port->dev, &id, match_free_decoder);
>>> +		dev = find_free_decoder(&port->dev);
>>>  	if (!dev)
>>>  		return NULL;
>>>  	/*
>>>
>>
> 

Re: [PATCH v3 2/3] cxl/region: Find free cxl decoder by device_for_each_child()

[Ira Weiny]

Jonathan Cameron wrote:
> On Sat, 24 Aug 2024 17:07:44 +0800
> Zijun Hu <zijun_hu@icloud.com> wrote:
> 

[snip]

> >  
> > +struct cxld_match_data {
> > +	int id;
> > +	struct device *target_device;
> > +};
> > +
> >  static int match_free_decoder(struct device *dev, void *data)
> >  {
> > +	struct cxld_match_data *match_data = data;
> >  	struct cxl_decoder *cxld;
> > -	int *id = data;
> >  
> >  	if (!is_switch_decoder(dev))
> >  		return 0;
> > @@ -805,17 +810,31 @@ static int match_free_decoder(struct device *dev, void *data)
> >  	cxld = to_cxl_decoder(dev);
> >  
> >  	/* enforce ordered allocation */
> > -	if (cxld->id != *id)
> > +	if (cxld->id != match_data->id)
> 
> Why do we carry on in this case?
> Conditions are:
> 1. Start match_data->id == 0
> 2. First pass cxld->id == 0 (all good) or
>    cxld->id == 1 say (and we skip until we match
>    on cxld->id == 0 (perhaps on the second child if they are
>    ordered (1, 0, 2) etc. 
> 
> If we skipped and then matched on second child but it was
> already in use (so region set), we will increment match_data->id to 1
> but never find that as it was the one we skipped.
> 
> So this can only work if the children are ordered.
> So if that's the case and the line above is just a sanity check
> on that, it should be noisier (so an error print) and might
> as well fail as if it doesn't match all bets are off.
> 

I've worked through this with Dan and the devices are added in order in
devm_cxl_enumerate_decoders().

So I don't think there is an issue with converting the code directly.

Sorry for the noise Jijun,
Ira