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

[PATCH v2 0/4] 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 3/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 27/30, the other patch series will simply change its
relevant parameter type to const void *.

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 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 (4):
      driver core: Make parameter check consistent for API cluster device_(for_each|find)_child()
      cxl/region: Prevent device_find_child() from modifying caller's match data
      firewire: core: Prevent device_find_child() from modifying caller's match data
      net: qcom/emac: Prevent device_find_child() from modifying caller's match data

 drivers/base/core.c                             |  6 ++--
 drivers/cxl/core/region.c                       | 36 +++++++++++++++++++++++-
 drivers/firewire/core-device.c                  | 37 +++++++++++++++++++++++--
 drivers/net/ethernet/qualcomm/emac/emac-sgmii.c | 36 ++++++++++++++++++++++--
 4 files changed, 107 insertions(+), 8 deletions(-)
---
base-commit: bfa54a793ba77ef696755b66f3ac4ed00c7d1248
change-id: 20240811-const_dfc_prepare-3ff23c6598e5

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

[PATCH v2 1/4] 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) 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 1688e76cb64b..b1dd8c5590dc 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -4004,7 +4004,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);
@@ -4034,7 +4034,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);
@@ -4068,7 +4068,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 v2 2/4] cxl/region: 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 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, fixed by implementing a equivalent
cxl_device_find_child() instead of the old API usage.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/cxl/core/region.c | 36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 21ad5f242875..8d8f0637f7ac 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -134,6 +134,39 @@ static const struct attribute_group *get_cxl_region_access1_group(void)
 	return &cxl_region_access1_coordinate_group;
 }
 
+struct cxl_dfc_data {
+	int (*match)(struct device *dev, void *data);
+	void *data;
+	struct device *target_device;
+};
+
+static int cxl_dfc_match_modify(struct device *dev, void *data)
+{
+	struct cxl_dfc_data *dfc_data = data;
+	int res;
+
+	res = dfc_data->match(dev, dfc_data->data);
+	if (res && get_device(dev)) {
+		dfc_data->target_device = dev;
+		return res;
+	}
+
+	return 0;
+}
+
+/*
+ * I have the same function as device_find_child() but allow to modify
+ * caller's match data @*data.
+ */
+static struct device *cxl_device_find_child(struct device *parent, void *data,
+					    int (*match)(struct device *dev, void *data))
+{
+	struct cxl_dfc_data dfc_data = {match, data, NULL};
+
+	device_for_each_child(parent, &dfc_data, cxl_dfc_match_modify);
+	return dfc_data.target_device;
+}
+
 static ssize_t uuid_show(struct device *dev, struct device_attribute *attr,
 			 char *buf)
 {
@@ -849,7 +882,8 @@ 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 = cxl_device_find_child(&port->dev, &id,
+					    match_free_decoder);
 	if (!dev)
 		return NULL;
 	/*

-- 
2.34.1

[PATCH v2 3/4] firewire: core: 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 lookup_existing_device() as the old
API's match function indeed modifies relevant match data, so it is not
suitable for the new API any more, fixed by implementing a equivalent
fw_device_find_child() instead of the old API usage.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/firewire/core-device.c | 37 +++++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c
index 00e9a13e6c45..7fbccb113d54 100644
--- a/drivers/firewire/core-device.c
+++ b/drivers/firewire/core-device.c
@@ -33,6 +33,39 @@
 
 #define ROOT_DIR_OFFSET	5
 
+struct fw_dfc_data {
+	int (*match)(struct device *dev, void *data);
+	void *data;
+	struct device *target_device;
+};
+
+static int fw_dfc_match_modify(struct device *dev, void *data)
+{
+	struct fw_dfc_data *dfc_data =  data;
+	int res;
+
+	res = dfc_data->match(dev, dfc_data->data);
+	if (res && get_device(dev)) {
+		dfc_data->target_device = dev;
+		return res;
+	}
+
+	return 0;
+}
+
+/*
+ * I have the same function as device_find_child() but allow to modify
+ * caller's match data @*data.
+ */
+static struct device *fw_device_find_child(struct device *parent, void *data,
+					   int (*match)(struct device *dev, void *data))
+{
+	struct fw_dfc_data dfc_data = {match, data, NULL};
+
+	device_for_each_child(parent, &dfc_data, fw_dfc_match_modify);
+	return dfc_data.target_device;
+}
+
 void fw_csr_iterator_init(struct fw_csr_iterator *ci, const u32 *p)
 {
 	ci->p = p + 1;
@@ -1087,8 +1120,8 @@ static void fw_device_init(struct work_struct *work)
 		return;
 	}
 
-	revived_dev = device_find_child(card->device,
-					device, lookup_existing_device);
+	revived_dev = fw_device_find_child(card->device, device,
+					   lookup_existing_device);
 	if (revived_dev) {
 		put_device(revived_dev);
 		fw_device_release(&device->device);

-- 
2.34.1

[PATCH v2 4/4] 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, fixed by implementing a equivalent
emac_device_find_child() instead of the old API usage.

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

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c b/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
index e4bc18009d08..1c799be77d99 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
@@ -47,6 +47,38 @@
 
 #define SERDES_START_WAIT_TIMES			100
 
+struct emac_dfc_data {
+	int (*match)(struct device *dev, void *data);
+	void *data;
+	struct device *target_device;
+};
+
+static int emac_dfc_match_modify(struct device *dev, void *data)
+{
+	struct emac_dfc_data *dfc_data =  data;
+	int res;
+
+	res = dfc_data->match(dev, dfc_data->data);
+	if (res && get_device(dev)) {
+		dfc_data->target_device = dev;
+		return res;
+	}
+
+	return 0;
+}
+
+/* I have the same function as device_find_child() but allow to modify
+ * caller's match data @*data.
+ */
+static struct device *emac_device_find_child(struct device *parent, void *data,
+					     int (*match)(struct device *dev, void *data))
+{
+	struct emac_dfc_data dfc_data = {match, data, NULL};
+
+	device_for_each_child(parent, &dfc_data, emac_dfc_match_modify);
+	return dfc_data.target_device;
+}
+
 int emac_sgmii_init(struct emac_adapter *adpt)
 {
 	if (!(adpt->phy.sgmii_ops && adpt->phy.sgmii_ops->init))
@@ -358,8 +390,8 @@ int emac_sgmii_config(struct platform_device *pdev, struct emac_adapter *adpt)
 	if (has_acpi_companion(&pdev->dev)) {
 		struct device *dev;
 
-		dev = device_find_child(&pdev->dev, &phy->sgmii_ops,
-					emac_sgmii_acpi_match);
+		dev = emac_device_find_child(&pdev->dev, &phy->sgmii_ops,
+					     emac_sgmii_acpi_match);
 
 		if (!dev) {
 			dev_warn(&pdev->dev, "cannot find internal phy node\n");

-- 
2.34.1

Re: [PATCH v2 3/4] firewire: core: Prevent device_find_child() from modifying caller's match data

[Takashi Sakamoto]

Hi,

On Thu, Aug 15, 2024 at 10:58:04PM +0800, 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 lookup_existing_device() as the old
> API's match function indeed modifies relevant match data, so it is not
> suitable for the new API any more, fixed by implementing a equivalent
> fw_device_find_child() instead of the old API usage.
> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/firewire/core-device.c | 37 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 35 insertions(+), 2 deletions(-)

Thanks for the patch.

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

I have found another issue in respect to this subsystem.

The whole procedure in 'lookup_existing_device()' in the call of
'device_find_child()' is a bit superfluous, since it includes not only
finding but also updating. The helper function passed to
'device_find_child()' should do quick finding only.

I think we can change the relevant codes like the following patch. It
would solve your concern, too. If you prefer the change, I'm going to
evaluate it.

======== 8< --------

From ceaa8a986ae07865eb3fec810de330e96b6d56e2 Mon Sep 17 00:00:00 2001
From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
Date: Sat, 17 Aug 2024 17:52:53 +0900
Subject: [PATCH] firewire: core: update fw_device outside of
 device_find_child()

When detecting updates of bus topology, the data of fw_device is newly
allocated and caches the content of configuration ROM from the
corresponding node. Then, the tree of device is sought to find the
previous data of fw_device corresponding to the node, since in IEEE 1394
specification numeric node identifier could be changed dynamically every
generation of bus topology. If it is found, the previous data is updated
and reused, then the newly allocated data is going to be released.

The above procedure is done in the call of device_find_child(), however it
is a bit abusing against the intention of the helper function, since the
call would not only find but also update.

This commit splits the update outside of the call.
---
 drivers/firewire/core-device.c | 109 ++++++++++++++++-----------------
 1 file changed, 54 insertions(+), 55 deletions(-)

diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c
index bc4c9e5a..62e8d839 100644
--- a/drivers/firewire/core-device.c
+++ b/drivers/firewire/core-device.c
@@ -928,56 +928,6 @@ static void fw_device_update(struct work_struct *work)
 	device_for_each_child(&device->device, NULL, update_unit);
 }
 
-/*
- * If a device was pending for deletion because its node went away but its
- * bus info block and root directory header matches that of a newly discovered
- * device, revive the existing fw_device.
- * The newly allocated fw_device becomes obsolete instead.
- */
-static int lookup_existing_device(struct device *dev, void *data)
-{
-	struct fw_device *old = fw_device(dev);
-	struct fw_device *new = data;
-	struct fw_card *card = new->card;
-	int match = 0;
-
-	if (!is_fw_device(dev))
-		return 0;
-
-	guard(rwsem_read)(&fw_device_rwsem); // serialize config_rom access
-	guard(spinlock_irq)(&card->lock); // serialize node access
-
-	if (memcmp(old->config_rom, new->config_rom, 6 * 4) == 0 &&
-	    atomic_cmpxchg(&old->state,
-			   FW_DEVICE_GONE,
-			   FW_DEVICE_RUNNING) == FW_DEVICE_GONE) {
-		struct fw_node *current_node = new->node;
-		struct fw_node *obsolete_node = old->node;
-
-		new->node = obsolete_node;
-		new->node->data = new;
-		old->node = current_node;
-		old->node->data = old;
-
-		old->max_speed = new->max_speed;
-		old->node_id = current_node->node_id;
-		smp_wmb();  /* update node_id before generation */
-		old->generation = card->generation;
-		old->config_rom_retries = 0;
-		fw_notice(card, "rediscovered device %s\n", dev_name(dev));
-
-		old->workfn = fw_device_update;
-		fw_schedule_device_work(old, 0);
-
-		if (current_node == card->root_node)
-			fw_schedule_bm_work(card, 0);
-
-		match = 1;
-	}
-
-	return match;
-}
-
 enum { BC_UNKNOWN = 0, BC_UNIMPLEMENTED, BC_IMPLEMENTED, };
 
 static void set_broadcast_channel(struct fw_device *device, int generation)
@@ -1038,6 +988,17 @@ int fw_device_set_broadcast_channel(struct device *dev, void *gen)
 	return 0;
 }
 
+static int compare_configuration_rom(struct device *dev, void *data)
+{
+	const struct fw_device *old = fw_device(dev);
+	const u32 *config_rom = data;
+
+	if (!is_fw_device(dev))
+		return 0;
+
+	return !!memcmp(old->config_rom, config_rom, 6 * 4);
+}
+
 static void fw_device_init(struct work_struct *work)
 {
 	struct fw_device *device =
@@ -1071,13 +1032,51 @@ static void fw_device_init(struct work_struct *work)
 		return;
 	}
 
-	revived_dev = device_find_child(card->device,
-					device, lookup_existing_device);
+	// If a device was pending for deletion because its node went away but its bus info block
+	// and root directory header matches that of a newly discovered device, revive the
+	// existing fw_device. The newly allocated fw_device becomes obsolete instead.
+	//
+	// serialize config_rom access.
+	scoped_guard(rwsem_read, &fw_device_rwsem) {
+		// TODO: The cast to 'void *' could be removed if Zijun Hu's work goes well.
+		revived_dev = device_find_child(card->device, (void *)device->config_rom,
+						compare_configuration_rom);
+	}
 	if (revived_dev) {
-		put_device(revived_dev);
-		fw_device_release(&device->device);
+		struct fw_device *found = fw_device(revived_dev);
 
-		return;
+		// serialize node access
+		guard(spinlock_irq)(&card->lock);
+
+		if (atomic_cmpxchg(&found->state,
+				   FW_DEVICE_GONE,
+				   FW_DEVICE_RUNNING) == FW_DEVICE_GONE) {
+			struct fw_node *current_node = device->node;
+			struct fw_node *obsolete_node = found->node;
+
+			device->node = obsolete_node;
+			device->node->data = device;
+			found->node = current_node;
+			found->node->data = found;
+
+			found->max_speed = device->max_speed;
+			found->node_id = current_node->node_id;
+			smp_wmb();  /* update node_id before generation */
+			found->generation = card->generation;
+			found->config_rom_retries = 0;
+			fw_notice(card, "rediscovered device %s\n", dev_name(revived_dev));
+
+			found->workfn = fw_device_update;
+			fw_schedule_device_work(found, 0);
+
+			if (current_node == card->root_node)
+				fw_schedule_bm_work(card, 0);
+
+			put_device(revived_dev);
+			fw_device_release(&device->device);
+
+			return;
+		}
 	}
 
 	device_initialize(&device->device);
-- 
2.43.0



Regards

Takashi Sakamoto

Re: [PATCH v2 3/4] firewire: core: Prevent device_find_child() from modifying caller's match data

[Zijun Hu]

On 2024/8/17 17:57, Takashi Sakamoto wrote:
> Hi,
> 
> On Thu, Aug 15, 2024 at 10:58:04PM +0800, 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 lookup_existing_device() as the old
>> API's match function indeed modifies relevant match data, so it is not
>> suitable for the new API any more, fixed by implementing a equivalent
>> fw_device_find_child() instead of the old API usage.
>>
>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>> ---
>>  drivers/firewire/core-device.c | 37 +++++++++++++++++++++++++++++++++++--
>>  1 file changed, 35 insertions(+), 2 deletions(-)
> 
> Thanks for the patch.
> 
>> 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));
> 
> I have found another issue in respect to this subsystem.
> 
> The whole procedure in 'lookup_existing_device()' in the call of
> 'device_find_child()' is a bit superfluous, since it includes not only
> finding but also updating. The helper function passed to
> 'device_find_child()' should do quick finding only.
> 
> I think we can change the relevant codes like the following patch. It
> would solve your concern, too. If you prefer the change, I'm going to
> evaluate it.
> 

thanks for your reply.
of course, i prefer your change.

> ======== 8< --------
> 
> From ceaa8a986ae07865eb3fec810de330e96b6d56e2 Mon Sep 17 00:00:00 2001
> From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> Date: Sat, 17 Aug 2024 17:52:53 +0900
> Subject: [PATCH] firewire: core: update fw_device outside of
>  device_find_child()
> 
> When detecting updates of bus topology, the data of fw_device is newly
> allocated and caches the content of configuration ROM from the
> corresponding node. Then, the tree of device is sought to find the
> previous data of fw_device corresponding to the node, since in IEEE 1394
> specification numeric node identifier could be changed dynamically every
> generation of bus topology. If it is found, the previous data is updated
> and reused, then the newly allocated data is going to be released.
> 
> The above procedure is done in the call of device_find_child(), however it
> is a bit abusing against the intention of the helper function, since the
> call would not only find but also update.
> 
> This commit splits the update outside of the call.
> ---
>  drivers/firewire/core-device.c | 109 ++++++++++++++++-----------------
>  1 file changed, 54 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c
> index bc4c9e5a..62e8d839 100644
> --- a/drivers/firewire/core-device.c
> +++ b/drivers/firewire/core-device.c
> @@ -928,56 +928,6 @@ static void fw_device_update(struct work_struct *work)
>  	device_for_each_child(&device->device, NULL, update_unit);
>  }
>  
> -/*
> - * If a device was pending for deletion because its node went away but its
> - * bus info block and root directory header matches that of a newly discovered
> - * device, revive the existing fw_device.
> - * The newly allocated fw_device becomes obsolete instead.
> - */
> -static int lookup_existing_device(struct device *dev, void *data)
> -{
> -	struct fw_device *old = fw_device(dev);
> -	struct fw_device *new = data;
> -	struct fw_card *card = new->card;
> -	int match = 0;
> -
> -	if (!is_fw_device(dev))
> -		return 0;
> -
> -	guard(rwsem_read)(&fw_device_rwsem); // serialize config_rom access
> -	guard(spinlock_irq)(&card->lock); // serialize node access
> -
> -	if (memcmp(old->config_rom, new->config_rom, 6 * 4) == 0 &&
> -	    atomic_cmpxchg(&old->state,
> -			   FW_DEVICE_GONE,
> -			   FW_DEVICE_RUNNING) == FW_DEVICE_GONE) {
> -		struct fw_node *current_node = new->node;
> -		struct fw_node *obsolete_node = old->node;
> -
> -		new->node = obsolete_node;
> -		new->node->data = new;
> -		old->node = current_node;
> -		old->node->data = old;
> -
> -		old->max_speed = new->max_speed;
> -		old->node_id = current_node->node_id;
> -		smp_wmb();  /* update node_id before generation */
> -		old->generation = card->generation;
> -		old->config_rom_retries = 0;
> -		fw_notice(card, "rediscovered device %s\n", dev_name(dev));
> -
> -		old->workfn = fw_device_update;
> -		fw_schedule_device_work(old, 0);
> -
> -		if (current_node == card->root_node)
> -			fw_schedule_bm_work(card, 0);
> -
> -		match = 1;
> -	}
> -
> -	return match;
> -}
> -
>  enum { BC_UNKNOWN = 0, BC_UNIMPLEMENTED, BC_IMPLEMENTED, };
>  
>  static void set_broadcast_channel(struct fw_device *device, int generation)
> @@ -1038,6 +988,17 @@ int fw_device_set_broadcast_channel(struct device *dev, void *gen)
>  	return 0;
>  }
>  
> +static int compare_configuration_rom(struct device *dev, void *data)
> +{
> +	const struct fw_device *old = fw_device(dev);
> +	const u32 *config_rom = data;
> +
> +	if (!is_fw_device(dev))
> +		return 0;
> +
> +	return !!memcmp(old->config_rom, config_rom, 6 * 4);

!memcmp(old->config_rom, config_rom, 6 * 4) ?

is this extra condition old->state == FW_DEVICE_GONE required ?

namely, is it okay for  below return ?
return  !memcmp(old->config_rom, config_rom, 6 * 4) && old->state ==
FW_DEVICE_GONE

> +}
> +
>  static void fw_device_init(struct work_struct *work)
>  {
>  	struct fw_device *device =
> @@ -1071,13 +1032,51 @@ static void fw_device_init(struct work_struct *work)
>  		return;
>  	}
>  
> -	revived_dev = device_find_child(card->device,
> -					device, lookup_existing_device);
> +	// If a device was pending for deletion because its node went away but its bus info block
> +	// and root directory header matches that of a newly discovered device, revive the
> +	// existing fw_device. The newly allocated fw_device becomes obsolete instead.
> +	//
> +	// serialize config_rom access.
> +	scoped_guard(rwsem_read, &fw_device_rwsem) {
> +		// TODO: The cast to 'void *' could be removed if Zijun Hu's work goes well.

may remove this TODO line since i will simply remove the cast with the
other patch series as shown below:
https://lore.kernel.org/all/20240811-const_dfc_done-v1-0-9d85e3f943cb@quicinc.com/


> +		revived_dev = device_find_child(card->device, (void *)device->config_rom,
> +						compare_configuration_rom);
> +	}
>  	if (revived_dev) {
> -		put_device(revived_dev);
> -		fw_device_release(&device->device);
> +		struct fw_device *found = fw_device(revived_dev);
>  
> -		return;
> +		// serialize node access
> +		guard(spinlock_irq)(&card->lock);
> +
> +		if (atomic_cmpxchg(&found->state,
> +				   FW_DEVICE_GONE,
> +				   FW_DEVICE_RUNNING) == FW_DEVICE_GONE) {
> +			struct fw_node *current_node = device->node;
> +			struct fw_node *obsolete_node = found->node;
> +
> +			device->node = obsolete_node;
> +			device->node->data = device;
> +			found->node = current_node;
> +			found->node->data = found;
> +
> +			found->max_speed = device->max_speed;
> +			found->node_id = current_node->node_id;
> +			smp_wmb();  /* update node_id before generation */
> +			found->generation = card->generation;
> +			found->config_rom_retries = 0;
> +			fw_notice(card, "rediscovered device %s\n", dev_name(revived_dev));
> +
> +			found->workfn = fw_device_update;
> +			fw_schedule_device_work(found, 0);
> +
> +			if (current_node == card->root_node)
> +				fw_schedule_bm_work(card, 0);
> +
> +			put_device(revived_dev);
> +			fw_device_release(&device->device);
> +
> +			return;
> +		}

is it okay to put_device() here as well ?
put_device(revived_dev);

>  	}
>  
>  	device_initialize(&device->device);

Re: [PATCH v2 3/4] firewire: core: Prevent device_find_child() from modifying caller's match data

[Takashi Sakamoto]

Hi,

On 2024/8/18 22:34, Zijun Hu wrote:
>On 2024/8/17 17:57, Takashi Sakamoto wrote:
>> ======== 8< --------
>> 
>> From ceaa8a986ae07865eb3fec810de330e96b6d56e2 Mon Sep 17 00:00:00 2001
>> From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
>> Date: Sat, 17 Aug 2024 17:52:53 +0900
>> Subject: [PATCH] firewire: core: update fw_device outside of
>>  device_find_child()
>> 
>> When detecting updates of bus topology, the data of fw_device is newly
>> allocated and caches the content of configuration ROM from the
>> corresponding node. Then, the tree of device is sought to find the
>> previous data of fw_device corresponding to the node, since in IEEE 1394
>> specification numeric node identifier could be changed dynamically every
>> generation of bus topology. If it is found, the previous data is updated
>> and reused, then the newly allocated data is going to be released.
>> 
>> The above procedure is done in the call of device_find_child(), however it
>> is a bit abusing against the intention of the helper function, since the
>> call would not only find but also update.
>> 
>> This commit splits the update outside of the call.
>> ---
>>  drivers/firewire/core-device.c | 109 ++++++++++++++++-----------------
>>  1 file changed, 54 insertions(+), 55 deletions(-)
>> 
>> diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c
>> index bc4c9e5a..62e8d839 100644
>> --- a/drivers/firewire/core-device.c
>> +++ b/drivers/firewire/core-device.c
>> ...
>> @@ -1038,6 +988,17 @@ int fw_device_set_broadcast_channel(struct device *dev, void *gen)
>>  	return 0;
>>  }
>>  
>> +static int compare_configuration_rom(struct device *dev, void *data)
>> +{
>> +	const struct fw_device *old = fw_device(dev);
>> +	const u32 *config_rom = data;
>> +
>> +	if (!is_fw_device(dev))
>> +		return 0;
>> +
>> +	return !!memcmp(old->config_rom, config_rom, 6 * 4);
>
>!memcmp(old->config_rom, config_rom, 6 * 4) ?

Indeed.

>is this extra condition old->state == FW_DEVICE_GONE required ?
>
>namely, is it okay for  below return ?
>return  !memcmp(old->config_rom, config_rom, 6 * 4) && old->state ==
>FW_DEVICE_GONE

If so, atomic_read() should be used, however I avoid it since the access
to state member happens twice in in the path to reuse the instance.

>> +}
>> +
>>  static void fw_device_init(struct work_struct *work)
>>  {
>>  	struct fw_device *device =
>> @@ -1071,13 +1032,51 @@ static void fw_device_init(struct work_struct *work)
>>  		return;
>>  	}
>>  
>> -	revived_dev = device_find_child(card->device,
>> -					device, lookup_existing_device);
>> +	// If a device was pending for deletion because its node went away but its bus info block
>> +	// and root directory header matches that of a newly discovered device, revive the
>> +	// existing fw_device. The newly allocated fw_device becomes obsolete instead.
>> +	//
>> +	// serialize config_rom access.
>> +	scoped_guard(rwsem_read, &fw_device_rwsem) {
>> +		// TODO: The cast to 'void *' could be removed if Zijun Hu's work goes well.
>
>may remove this TODO line since i will simply remove the cast with the
>other patch series as shown below:
>https://lore.kernel.org/all/20240811-const_dfc_done-v1-0-9d85e3f943cb@quicinc.com/

Of course, I won't apply this patch as is. It is just a mark to hold
your attention.

>> +		revived_dev = device_find_child(card->device, (void *)device->config_rom,
>> +						compare_configuration_rom);
>> +	}
>>  	if (revived_dev) {
>> -		put_device(revived_dev);
>> -		fw_device_release(&device->device);
>> +		struct fw_device *found = fw_device(revived_dev);
>>  
>> -		return;
>> +		// serialize node access
>> +		guard(spinlock_irq)(&card->lock);
>> +
>> +		if (atomic_cmpxchg(&found->state,
>> +				   FW_DEVICE_GONE,
>> +				   FW_DEVICE_RUNNING) == FW_DEVICE_GONE) {
>> +			struct fw_node *current_node = device->node;
>> +			struct fw_node *obsolete_node = found->node;
>> +
>> +			device->node = obsolete_node;
>> +			device->node->data = device;
>> +			found->node = current_node;
>> +			found->node->data = found;
>> +
>> +			found->max_speed = device->max_speed;
>> +			found->node_id = current_node->node_id;
>> +			smp_wmb();  /* update node_id before generation */
>> +			found->generation = card->generation;
>> +			found->config_rom_retries = 0;
>> +			fw_notice(card, "rediscovered device %s\n", dev_name(revived_dev));
>> +
>> +			found->workfn = fw_device_update;
>> +			fw_schedule_device_work(found, 0);
>> +
>> +			if (current_node == card->root_node)
>> +				fw_schedule_bm_work(card, 0);
>> +
>> +			put_device(revived_dev);
>> +			fw_device_release(&device->device);
>> +
>> +			return;
>> +		}
>
>is it okay to put_device() here as well ?
>put_device(revived_dev);

Exactly. The call of put_device() should be done when the call of
device_find_child() returns non-NULL value.

Additionally, I realize that the call of fw_device_release() under
acquiring card->lock causes dead lock.

>>  	}
>>  
>>  	device_initialize(&device->device);

Anyway, I'll post take 2 and work for its evaluation.


Thanks

Takashi Sakamoto

Re: [PATCH v2 3/4] firewire: core: Prevent device_find_child() from modifying caller's match data

[Zijun Hu]

On 2024/8/19 16:58, Takashi Sakamoto wrote:
> 
> Hi,
> 
> On 2024/8/18 22:34, Zijun Hu wrote:
>> On 2024/8/17 17:57, Takashi Sakamoto wrote:
>>> ======== 8< --------
>>>
>>> From ceaa8a986ae07865eb3fec810de330e96b6d56e2 Mon Sep 17 00:00:00 2001
>>> From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
>>> Date: Sat, 17 Aug 2024 17:52:53 +0900
>>> Subject: [PATCH] firewire: core: update fw_device outside of
>>>  device_find_child()
>>>
>>> When detecting updates of bus topology, the data of fw_device is newly
>>> allocated and caches the content of configuration ROM from the
>>> corresponding node. Then, the tree of device is sought to find the
>>> previous data of fw_device corresponding to the node, since in IEEE 1394
>>> specification numeric node identifier could be changed dynamically every
>>> generation of bus topology. If it is found, the previous data is updated
>>> and reused, then the newly allocated data is going to be released.
>>>
>>> The above procedure is done in the call of device_find_child(), however it
>>> is a bit abusing against the intention of the helper function, since the
>>> call would not only find but also update.
>>>
>>> This commit splits the update outside of the call.
>>> ---
>>>  drivers/firewire/core-device.c | 109 ++++++++++++++++-----------------
>>>  1 file changed, 54 insertions(+), 55 deletions(-)
>>>
>>> diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c
>>> index bc4c9e5a..62e8d839 100644
>>> --- a/drivers/firewire/core-device.c
>>> +++ b/drivers/firewire/core-device.c
>>> ...
>>> @@ -1038,6 +988,17 @@ int fw_device_set_broadcast_channel(struct device *dev, void *gen)
>>>  	return 0;
>>>  }
>>>  
>>> +static int compare_configuration_rom(struct device *dev, void *data)
>>> +{
>>> +	const struct fw_device *old = fw_device(dev);
>>> +	const u32 *config_rom = data;
>>> +
>>> +	if (!is_fw_device(dev))
>>> +		return 0;
>>> +
>>> +	return !!memcmp(old->config_rom, config_rom, 6 * 4);
>>
>> !memcmp(old->config_rom, config_rom, 6 * 4) ?
> 
> Indeed.
> 
>> is this extra condition old->state == FW_DEVICE_GONE required ?
>>
>> namely, is it okay for  below return ?
>> return  !memcmp(old->config_rom, config_rom, 6 * 4) && old->state ==
>> FW_DEVICE_GONE
> 
> If so, atomic_read() should be used, however I avoid it since the access
> to state member happens twice in in the path to reuse the instance.
> 

it sounds good to not append the extra condition.

>>> +}
>>> +
>>>  static void fw_device_init(struct work_struct *work)
>>>  {
>>>  	struct fw_device *device =
>>> @@ -1071,13 +1032,51 @@ static void fw_device_init(struct work_struct *work)
>>>  		return;
>>>  	}
>>>  
>>> -	revived_dev = device_find_child(card->device,
>>> -					device, lookup_existing_device);
>>> +	// If a device was pending for deletion because its node went away but its bus info block
>>> +	// and root directory header matches that of a newly discovered device, revive the
>>> +	// existing fw_device. The newly allocated fw_device becomes obsolete instead.
>>> +	//
>>> +	// serialize config_rom access.
>>> +	scoped_guard(rwsem_read, &fw_device_rwsem) {
>>> +		// TODO: The cast to 'void *' could be removed if Zijun Hu's work goes well.
>>
>> may remove this TODO line since i will simply remove the cast with the
>> other patch series as shown below:
>> https://lore.kernel.org/all/20240811-const_dfc_done-v1-0-9d85e3f943cb@quicinc.com/
> 
> Of course, I won't apply this patch as is. It is just a mark to hold
> your attention.
> 
>>> +		revived_dev = device_find_child(card->device, (void *)device->config_rom,
>>> +						compare_configuration_rom);
>>> +	}
>>>  	if (revived_dev) {
>>> -		put_device(revived_dev);
>>> -		fw_device_release(&device->device);
>>> +		struct fw_device *found = fw_device(revived_dev);
>>>  
>>> -		return;
>>> +		// serialize node access
>>> +		guard(spinlock_irq)(&card->lock);
>>> +
>>> +		if (atomic_cmpxchg(&found->state,
>>> +				   FW_DEVICE_GONE,
>>> +				   FW_DEVICE_RUNNING) == FW_DEVICE_GONE) {
>>> +			struct fw_node *current_node = device->node;
>>> +			struct fw_node *obsolete_node = found->node;
>>> +
>>> +			device->node = obsolete_node;
>>> +			device->node->data = device;
>>> +			found->node = current_node;
>>> +			found->node->data = found;
>>> +
>>> +			found->max_speed = device->max_speed;
>>> +			found->node_id = current_node->node_id;
>>> +			smp_wmb();  /* update node_id before generation */
>>> +			found->generation = card->generation;
>>> +			found->config_rom_retries = 0;
>>> +			fw_notice(card, "rediscovered device %s\n", dev_name(revived_dev));
>>> +
>>> +			found->workfn = fw_device_update;
>>> +			fw_schedule_device_work(found, 0);
>>> +
>>> +			if (current_node == card->root_node)
>>> +				fw_schedule_bm_work(card, 0);
>>> +
>>> +			put_device(revived_dev);
>>> +			fw_device_release(&device->device);
>>> +
>>> +			return;
>>> +		}
>>
>> is it okay to put_device() here as well ?
>> put_device(revived_dev);
> 
> Exactly. The call of put_device() should be done when the call of
> device_find_child() returns non-NULL value.
> 
> Additionally, I realize that the call of fw_device_release() under
> acquiring card->lock causes dead lock.
> 
>>>  	}
>>>  
>>>  	device_initialize(&device->device);
> 
> Anyway, I'll post take 2 and work for its evaluation.
> 
great
> 
> Thanks
> 
> Takashi Sakamoto

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

[Ira Weiny]

Zijun Hu wrote:
> 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)
> 

Seems reasonable.

What about device_find_child_by_name()?

> Fixed by using consistent check (!parent || !parent->p) 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 1688e76cb64b..b1dd8c5590dc 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -4004,7 +4004,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);
> @@ -4034,7 +4034,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);
> @@ -4068,7 +4068,7 @@ struct device *device_find_child(struct device *parent, void *data,
>  	struct klist_iter i;
>  	struct device *child;
>  
> -	if (!parent)
> +	if (!parent || !parent->p)

Perhaps this was just a typo which should have been.

	if (!parent->p)
?

I think there is an expectation that none of these are called with a NULL
parent.

Ira

>  		return NULL;
>  
>  	klist_iter_init(&parent->p->klist_children, &i);
> 
> -- 
> 2.34.1
> 

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

[Zijun Hu]

On 2024/8/20 20:53, Ira Weiny wrote:
> Zijun Hu wrote:
>> 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)
>>
> 
> Seems reasonable.
> 
> What about device_find_child_by_name()?
> 

Plan to simplify this API implementation by * atomic * API
device_find_child() as following:

https://lore.kernel.org/all/20240811-simply_api_dfcbn-v2-1-d0398acdc366@quicinc.com
struct device *device_find_child_by_name(struct device *parent,
 					 const char *name)
{
	return device_find_child(parent, name, device_match_name);
}

>> Fixed by using consistent check (!parent || !parent->p) 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 1688e76cb64b..b1dd8c5590dc 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -4004,7 +4004,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);
>> @@ -4034,7 +4034,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);
>> @@ -4068,7 +4068,7 @@ struct device *device_find_child(struct device *parent, void *data,
>>  	struct klist_iter i;
>>  	struct device *child;
>>  
>> -	if (!parent)
>> +	if (!parent || !parent->p)
> 
> Perhaps this was just a typo which should have been.
> 
> 	if (!parent->p)
> ?
> 
maybe, but the following device_find_child_by_name() also use (!parent).

> I think there is an expectation that none of these are called with a NULL
> parent.
>

this patch aim is to make these atomic APIs have consistent checks as
far as possible, that will make other patches within this series more
acceptable.

i combine two checks to (!parent || !parent->p) since i did not know
which is better.

> Ira
> 
>>  		return NULL;
>>  
>>  	klist_iter_init(&parent->p->klist_children, &i);
>>
>> -- 
>> 2.34.1
>>
> 
> 

Re: [PATCH v2 2/4] cxl/region: Prevent device_find_child() from modifying caller's match data

[Ira Weiny]

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, fixed by implementing a equivalent
> cxl_device_find_child() instead of the old API usage.

Generally it seems ok but I think some name changes will make this more
clear.  See below.

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?

> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/cxl/core/region.c | 36 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 21ad5f242875..8d8f0637f7ac 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -134,6 +134,39 @@ static const struct attribute_group *get_cxl_region_access1_group(void)
>  	return &cxl_region_access1_coordinate_group;
>  }
>  
> +struct cxl_dfc_data {

struct cxld_match_data

'cxld' == cxl decoder in our world.

> +	int (*match)(struct device *dev, void *data);
> +	void *data;
> +	struct device *target_device;
> +};
> +
> +static int cxl_dfc_match_modify(struct device *dev, void *data)

Why not just put this logic into match_free_decoder?

> +{
> +	struct cxl_dfc_data *dfc_data = data;
> +	int res;
> +
> +	res = dfc_data->match(dev, dfc_data->data);
> +	if (res && get_device(dev)) {
> +		dfc_data->target_device = dev;
> +		return res;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * I have the same function as device_find_child() but allow to modify
> + * caller's match data @*data.
> + */

No need for this comment after the new API is established.

> +static struct device *cxl_device_find_child(struct device *parent, void *data,
> +					    int (*match)(struct device *dev, void *data))
> +{
> +	struct cxl_dfc_data dfc_data = {match, data, NULL};
> +
> +	device_for_each_child(parent, &dfc_data, cxl_dfc_match_modify);
> +	return dfc_data.target_device;
> +}
> +
>  static ssize_t uuid_show(struct device *dev, struct device_attribute *attr,
>  			 char *buf)
>  {
> @@ -849,7 +882,8 @@ 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 = cxl_device_find_child(&port->dev, &id,
> +					    match_free_decoder);

This is too literal.  How about the following (passes basic cxl-tests).

Ira

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c              
index 21ad5f242875..c1e46254efb8 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,30 @@ 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 && get_device(dev)) {                                 
+               match_data->target_device = dev;                                
                return 1;                                                       
+       }                                                                       
                                                                                
-       (*id)++;                                                                
+       match_data->id++;                                                       
                                                                                
        return 0;                                                               
 }                                                                              
                                                                                
+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 +858,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 +866,8 @@ 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 v2 1/4] driver core: Make parameter check consistent for API cluster device_(for_each|find)_child()

[Ira Weiny]

Zijun Hu wrote:
> On 2024/8/20 20:53, Ira Weiny wrote:
> > Zijun Hu wrote:
> >> 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)
> >>
> > 
> > Seems reasonable.
> > 
> > What about device_find_child_by_name()?
> > 
> 
> Plan to simplify this API implementation by * atomic * API
> device_find_child() as following:
> 
> https://lore.kernel.org/all/20240811-simply_api_dfcbn-v2-1-d0398acdc366@quicinc.com
> struct device *device_find_child_by_name(struct device *parent,
>  					 const char *name)
> {
> 	return device_find_child(parent, name, device_match_name);
> }

Ok.  Thanks.

> 
> >> Fixed by using consistent check (!parent || !parent->p) 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 1688e76cb64b..b1dd8c5590dc 100644
> >> --- a/drivers/base/core.c
> >> +++ b/drivers/base/core.c
> >> @@ -4004,7 +4004,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);
> >> @@ -4034,7 +4034,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);
> >> @@ -4068,7 +4068,7 @@ struct device *device_find_child(struct device *parent, void *data,
> >>  	struct klist_iter i;
> >>  	struct device *child;
> >>  
> >> -	if (!parent)
> >> +	if (!parent || !parent->p)
> > 
> > Perhaps this was just a typo which should have been.
> > 
> > 	if (!parent->p)
> > ?
> > 
> maybe, but the following device_find_child_by_name() also use (!parent).
> 
> > I think there is an expectation that none of these are called with a NULL
> > parent.
> >
> 
> this patch aim is to make these atomic APIs have consistent checks as
> far as possible, that will make other patches within this series more
> acceptable.
> 
> i combine two checks to (!parent || !parent->p) since i did not know
> which is better.

I'm not entirely clear either.  But checking the member p makes more sense
to me than the parent parameter.  I would expect that iterating the
children of a device must be done only when the parent device is not NULL.

parent->p is more subtle.  I'm unclear why the API would need to allow
that to run without error.

Ira

Re: [PATCH v2 2/4] cxl/region: Prevent device_find_child() from modifying caller's match data

[Zijun Hu]

On 2024/8/20 21:59, Ira Weiny wrote:
> 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, fixed by implementing a equivalent
>> cxl_device_find_child() instead of the old API usage.
> 
> Generally it seems ok but I think some name changes will make this more
> clear.  See below.
> 

okay.

> 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?
> 
>>
>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>> ---
>>  drivers/cxl/core/region.c | 36 +++++++++++++++++++++++++++++++++++-
>>  1 file changed, 35 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index 21ad5f242875..8d8f0637f7ac 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> @@ -134,6 +134,39 @@ static const struct attribute_group *get_cxl_region_access1_group(void)
>>  	return &cxl_region_access1_coordinate_group;
>>  }
>>  
>> +struct cxl_dfc_data {
> 
> struct cxld_match_data
> 
> 'cxld' == cxl decoder in our world.
> 

make sense.

>> +	int (*match)(struct device *dev, void *data);
>> +	void *data;
>> +	struct device *target_device;
>> +};
>> +
>> +static int cxl_dfc_match_modify(struct device *dev, void *data)
> 
> Why not just put this logic into match_free_decoder?
> 

Actually, i ever considered solution B as you suggested in the end.

For this change, namely, solution A:
1) this change is clearer and easier to understand.
2) this change does not touch any existing cxld logic

For solution B:
it is more reasonable

i finally select A since it can express my concern and relevant solution
clearly.

>> +{
>> +	struct cxl_dfc_data *dfc_data = data;
>> +	int res;
>> +
>> +	res = dfc_data->match(dev, dfc_data->data);
>> +	if (res && get_device(dev)) {
>> +		dfc_data->target_device = dev;
>> +		return res;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * I have the same function as device_find_child() but allow to modify
>> + * caller's match data @*data.
>> + */
> 
> No need for this comment after the new API is established.
> 

i have given up the idea within v1 to introduce a new API which *should
ONLY* be used by this patch series, so it is not worthy of a new API
even if it can bring convenient for this patch series.

>> +static struct device *cxl_device_find_child(struct device *parent, void *data,
>> +					    int (*match)(struct device *dev, void *data))
>> +{
>> +	struct cxl_dfc_data dfc_data = {match, data, NULL};
>> +
>> +	device_for_each_child(parent, &dfc_data, cxl_dfc_match_modify);
>> +	return dfc_data.target_device;
>> +}
>> +
>>  static ssize_t uuid_show(struct device *dev, struct device_attribute *attr,
>>  			 char *buf)
>>  {
>> @@ -849,7 +882,8 @@ 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 = cxl_device_find_child(&port->dev, &id,
>> +					    match_free_decoder);
> 
> This is too literal.  How about the following (passes basic cxl-tests).
> 

it is reasonable.

do you need me to submit that you suggest in the end and add you as
co-developer ?

OR

you submit it by yourself ?

either is okay for me.

> Ira
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c              
> index 21ad5f242875..c1e46254efb8 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,30 @@ 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 && get_device(dev)) {                                 

get_device(dev) failure may cause different logic against existing
but i think it should be impossible to happen normally.

> +               match_data->target_device = dev;                                
>                 return 1;                                                       
> +       }                                                                       
>                                                                                 
> -       (*id)++;                                                                
> +       match_data->id++;                                                       
>                                                                                 
>         return 0;                                                               
>  }                                                                              
>                                                                                 
> +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 +858,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 +866,8 @@ 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 v2 1/4] driver core: Make parameter check consistent for API cluster device_(for_each|find)_child()

[Zijun Hu]

On 2024/8/20 22:14, Ira Weiny wrote:
> Zijun Hu wrote:
>> On 2024/8/20 20:53, Ira Weiny wrote:
>>> Zijun Hu wrote:
>>>> 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)
>>>>
>>>
>>> Seems reasonable.
>>>
>>> What about device_find_child_by_name()?
>>>
>>
>> Plan to simplify this API implementation by * atomic * API
>> device_find_child() as following:
>>
>> https://lore.kernel.org/all/20240811-simply_api_dfcbn-v2-1-d0398acdc366@quicinc.com
>> struct device *device_find_child_by_name(struct device *parent,
>>  					 const char *name)
>> {
>> 	return device_find_child(parent, name, device_match_name);
>> }
> 
> Ok.  Thanks.
> 
>>
>>>> Fixed by using consistent check (!parent || !parent->p) 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 1688e76cb64b..b1dd8c5590dc 100644
>>>> --- a/drivers/base/core.c
>>>> +++ b/drivers/base/core.c
>>>> @@ -4004,7 +4004,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);
>>>> @@ -4034,7 +4034,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);
>>>> @@ -4068,7 +4068,7 @@ struct device *device_find_child(struct device *parent, void *data,
>>>>  	struct klist_iter i;
>>>>  	struct device *child;
>>>>  
>>>> -	if (!parent)
>>>> +	if (!parent || !parent->p)
>>>
>>> Perhaps this was just a typo which should have been.
>>>
>>> 	if (!parent->p)
>>> ?
>>>
>> maybe, but the following device_find_child_by_name() also use (!parent).
>>
>>> I think there is an expectation that none of these are called with a NULL
>>> parent.
>>>
>>
>> this patch aim is to make these atomic APIs have consistent checks as
>> far as possible, that will make other patches within this series more
>> acceptable.
>>
>> i combine two checks to (!parent || !parent->p) since i did not know
>> which is better.
> 
> I'm not entirely clear either.  But checking the member p makes more sense
> to me than the parent parameter.  I would expect that iterating the
> children of a device must be done only when the parent device is not NULL.
> 
> parent->p is more subtle.  I'm unclear why the API would need to allow
> that to run without error.
> 
i prefer (!parent || !parent->p) with below reasons:

1)
original API authors have such concern that either (!parent) or
(!parent->p) maybe happen since they are checked, all their concerns
can be covered by (!parent || !parent->p).

2)
It is the more robust than either (!parent) or (!parent->p)

3)
it also does not have any negative effect.

> Ira

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

[Ira Weiny]

Zijun Hu wrote:
> On 2024/8/20 22:14, Ira Weiny wrote:
> > Zijun Hu wrote:
> >> On 2024/8/20 20:53, Ira Weiny wrote:
> >>> Zijun Hu wrote:
> >>>> 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)
> >>>>
> >>>
> >>> Seems reasonable.
> >>>
> >>> What about device_find_child_by_name()?
> >>>
> >>
> >> Plan to simplify this API implementation by * atomic * API
> >> device_find_child() as following:
> >>
> >> https://lore.kernel.org/all/20240811-simply_api_dfcbn-v2-1-d0398acdc366@quicinc.com
> >> struct device *device_find_child_by_name(struct device *parent,
> >>  					 const char *name)
> >> {
> >> 	return device_find_child(parent, name, device_match_name);
> >> }
> > 
> > Ok.  Thanks.
> > 
> >>
> >>>> Fixed by using consistent check (!parent || !parent->p) 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 1688e76cb64b..b1dd8c5590dc 100644
> >>>> --- a/drivers/base/core.c
> >>>> +++ b/drivers/base/core.c
> >>>> @@ -4004,7 +4004,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);
> >>>> @@ -4034,7 +4034,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);
> >>>> @@ -4068,7 +4068,7 @@ struct device *device_find_child(struct device *parent, void *data,
> >>>>  	struct klist_iter i;
> >>>>  	struct device *child;
> >>>>  
> >>>> -	if (!parent)
> >>>> +	if (!parent || !parent->p)
> >>>
> >>> Perhaps this was just a typo which should have been.
> >>>
> >>> 	if (!parent->p)
> >>> ?
> >>>
> >> maybe, but the following device_find_child_by_name() also use (!parent).
> >>
> >>> I think there is an expectation that none of these are called with a NULL
> >>> parent.
> >>>
> >>
> >> this patch aim is to make these atomic APIs have consistent checks as
> >> far as possible, that will make other patches within this series more
> >> acceptable.
> >>
> >> i combine two checks to (!parent || !parent->p) since i did not know
> >> which is better.
> > 
> > I'm not entirely clear either.  But checking the member p makes more sense
> > to me than the parent parameter.  I would expect that iterating the
> > children of a device must be done only when the parent device is not NULL.
> > 
> > parent->p is more subtle.  I'm unclear why the API would need to allow
> > that to run without error.
> > 
> i prefer (!parent || !parent->p) with below reasons:
> 
> 1)
> original API authors have such concern that either (!parent) or
> (!parent->p) maybe happen since they are checked, all their concerns
> can be covered by (!parent || !parent->p).
> 
> 2)
> It is the more robust than either (!parent) or (!parent->p)
> 
> 3)
> it also does not have any negative effect.

It adds code and instructions to all paths calling these functions.

What is the reason to allow?

void foo() {
...
	device_for_each_child(NULL, ...);
...
}

What are we finding the child of in that case?

Ira

> 
> > Ira
> 

Re: [PATCH v2 2/4] cxl/region: Prevent device_find_child() from modifying caller's match data

[Ira Weiny]

Zijun Hu wrote:
> On 2024/8/20 21:59, Ira Weiny wrote:
> > 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, fixed by implementing a equivalent
> >> cxl_device_find_child() instead of the old API usage.
> > 
> > Generally it seems ok but I think some name changes will make this more
> > clear.  See below.
> > 
> 
> okay.
> 
> > 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?
> > 
> >>
> >> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> >> ---
> >>  drivers/cxl/core/region.c | 36 +++++++++++++++++++++++++++++++++++-
> >>  1 file changed, 35 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> >> index 21ad5f242875..8d8f0637f7ac 100644
> >> --- a/drivers/cxl/core/region.c
> >> +++ b/drivers/cxl/core/region.c
> >> @@ -134,6 +134,39 @@ static const struct attribute_group *get_cxl_region_access1_group(void)
> >>  	return &cxl_region_access1_coordinate_group;
> >>  }
> >>  
> >> +struct cxl_dfc_data {
> > 
> > struct cxld_match_data
> > 
> > 'cxld' == cxl decoder in our world.
> > 
> 
> make sense.
> 
> >> +	int (*match)(struct device *dev, void *data);
> >> +	void *data;
> >> +	struct device *target_device;
> >> +};
> >> +
> >> +static int cxl_dfc_match_modify(struct device *dev, void *data)
> > 
> > Why not just put this logic into match_free_decoder?
> > 
> 
> Actually, i ever considered solution B as you suggested in the end.
> 
> For this change, namely, solution A:
> 1) this change is clearer and easier to understand.
> 2) this change does not touch any existing cxld logic
> 
> For solution B:
> it is more reasonable
> 
> i finally select A since it can express my concern and relevant solution
> clearly.

Understood.

> 
> >> +{
> >> +	struct cxl_dfc_data *dfc_data = data;
> >> +	int res;
> >> +
> >> +	res = dfc_data->match(dev, dfc_data->data);
> >> +	if (res && get_device(dev)) {
> >> +		dfc_data->target_device = dev;
> >> +		return res;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +/*
> >> + * I have the same function as device_find_child() but allow to modify
> >> + * caller's match data @*data.
> >> + */
> > 
> > No need for this comment after the new API is established.
> > 
> 
> i have given up the idea within v1 to introduce a new API which *should
> ONLY* be used by this patch series, so it is not worthy of a new API
> even if it can bring convenient for this patch series.

I'm not clear on this.  Are you still proposing to change the parameter to
const?

> 
> >> +static struct device *cxl_device_find_child(struct device *parent, void *data,
> >> +					    int (*match)(struct device *dev, void *data))
> >> +{
> >> +	struct cxl_dfc_data dfc_data = {match, data, NULL};
> >> +
> >> +	device_for_each_child(parent, &dfc_data, cxl_dfc_match_modify);
> >> +	return dfc_data.target_device;
> >> +}
> >> +
> >>  static ssize_t uuid_show(struct device *dev, struct device_attribute *attr,
> >>  			 char *buf)
> >>  {
> >> @@ -849,7 +882,8 @@ 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 = cxl_device_find_child(&port->dev, &id,
> >> +					    match_free_decoder);
> > 
> > This is too literal.  How about the following (passes basic cxl-tests).
> > 
> 
> it is reasonable.
> 
> do you need me to submit that you suggest in the end and add you as
> co-developer ?

You can submit it with Suggested-by:

> 
> OR
> 
> you submit it by yourself ?
> 
> either is okay for me.
> 
> > Ira
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c              
> > index 21ad5f242875..c1e46254efb8 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,30 @@ 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 && get_device(dev)) {                                 
> 
> get_device(dev) failure may cause different logic against existing
> but i think it should be impossible to happen normally.

Indeed this is slightly different.  :-/

Move the get_device() to find_free_decoder()?

Ira

> 
> > +               match_data->target_device = dev;                                
> >                 return 1;                                                       
> > +       }                                                                       
> >                                                                                 
> > -       (*id)++;                                                                
> > +       match_data->id++;                                                       
> >                                                                                 
> >         return 0;                                                               
> >  }                                                                              
> >                                                                                 
> > +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;                                        
> > +}                                                                              
> > +                                                                               

[snip]

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

[Zijun Hu]

On 2024/8/24 01:19, Ira Weiny wrote:
> Zijun Hu wrote:
>> On 2024/8/20 22:14, Ira Weiny wrote:
>>> Zijun Hu wrote:
>>>> On 2024/8/20 20:53, Ira Weiny wrote:
>>>>> Zijun Hu wrote:
>>>>>> 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)
>>>>>>
>>>>>
>>>>> Seems reasonable.
>>>>>
>>>>> What about device_find_child_by_name()?
>>>>>
>>>>
>>>> Plan to simplify this API implementation by * atomic * API
>>>> device_find_child() as following:
>>>>
>>>> https://lore.kernel.org/all/20240811-simply_api_dfcbn-v2-1-d0398acdc366@quicinc.com
>>>> struct device *device_find_child_by_name(struct device *parent,
>>>>  					 const char *name)
>>>> {
>>>> 	return device_find_child(parent, name, device_match_name);
>>>> }
>>>
>>> Ok.  Thanks.
>>>
>>>>
>>>>>> Fixed by using consistent check (!parent || !parent->p) 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 1688e76cb64b..b1dd8c5590dc 100644
>>>>>> --- a/drivers/base/core.c
>>>>>> +++ b/drivers/base/core.c
>>>>>> @@ -4004,7 +4004,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);
>>>>>> @@ -4034,7 +4034,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);
>>>>>> @@ -4068,7 +4068,7 @@ struct device *device_find_child(struct device *parent, void *data,
>>>>>>  	struct klist_iter i;
>>>>>>  	struct device *child;
>>>>>>  
>>>>>> -	if (!parent)
>>>>>> +	if (!parent || !parent->p)
>>>>>
>>>>> Perhaps this was just a typo which should have been.
>>>>>
>>>>> 	if (!parent->p)
>>>>> ?
>>>>>
>>>> maybe, but the following device_find_child_by_name() also use (!parent).
>>>>
>>>>> I think there is an expectation that none of these are called with a NULL
>>>>> parent.
>>>>>
>>>>
>>>> this patch aim is to make these atomic APIs have consistent checks as
>>>> far as possible, that will make other patches within this series more
>>>> acceptable.
>>>>
>>>> i combine two checks to (!parent || !parent->p) since i did not know
>>>> which is better.
>>>
>>> I'm not entirely clear either.  But checking the member p makes more sense
>>> to me than the parent parameter.  I would expect that iterating the
>>> children of a device must be done only when the parent device is not NULL.
>>>
>>> parent->p is more subtle.  I'm unclear why the API would need to allow
>>> that to run without error.
>>>
>> i prefer (!parent || !parent->p) with below reasons:
>>
>> 1)
>> original API authors have such concern that either (!parent) or
>> (!parent->p) maybe happen since they are checked, all their concerns
>> can be covered by (!parent || !parent->p).
>>
>> 2)
>> It is the more robust than either (!parent) or (!parent->p)
>>
>> 3)
>> it also does not have any negative effect.
> 
> It adds code and instructions to all paths calling these functions.
> 
such slight impacts can be ignored if a machine run linux OS.

right?

> What is the reason to allow?
> 
1)
it allow to use device_for_each_child() without misgiving.

2)
there are many many existing APIs which have similar checks such as
get_device(), kfree()...

> void foo() {
> ...
> 	device_for_each_child(NULL, ...);
> ...
> }
> 
> What are we finding the child of in that case?
>
similar usage as device_find_child(NULL, ...) which have check (!parent).

both device_for_each_child() and device_find_child() iterates over its
child.

original author's concern (!parent->p) for device_for_each_child() is
applicable for the other.

original author's concern (!parent) for device_find_child() is
applicable for the other as well.

so i use (!parent || !parent->p).

> Ira
> 
>>
>>> Ira
>>
> 
> 

Re: [PATCH v2 2/4] cxl/region: Prevent device_find_child() from modifying caller's match data

[Zijun Hu]

On 2024/8/24 02:10, Ira Weiny wrote:
> Zijun Hu wrote:
>> On 2024/8/20 21:59, Ira Weiny wrote:
>>> 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, fixed by implementing a equivalent
>>>> cxl_device_find_child() instead of the old API usage.
>>>
>>> Generally it seems ok but I think some name changes will make this more
>>> clear.  See below.
>>>
>>
>> okay.
>>
>>> 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?
>>>
>>>>
>>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>>>> ---
>>>>  drivers/cxl/core/region.c | 36 +++++++++++++++++++++++++++++++++++-
>>>>  1 file changed, 35 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>>>> index 21ad5f242875..8d8f0637f7ac 100644
>>>> --- a/drivers/cxl/core/region.c
>>>> +++ b/drivers/cxl/core/region.c
>>>> @@ -134,6 +134,39 @@ static const struct attribute_group *get_cxl_region_access1_group(void)
>>>>  	return &cxl_region_access1_coordinate_group;
>>>>  }
>>>>  
>>>> +struct cxl_dfc_data {
>>>
>>> struct cxld_match_data
>>>
>>> 'cxld' == cxl decoder in our world.
>>>
>>
>> make sense.
>>
>>>> +	int (*match)(struct device *dev, void *data);
>>>> +	void *data;
>>>> +	struct device *target_device;
>>>> +};
>>>> +
>>>> +static int cxl_dfc_match_modify(struct device *dev, void *data)
>>>
>>> Why not just put this logic into match_free_decoder?
>>>
>>
>> Actually, i ever considered solution B as you suggested in the end.
>>
>> For this change, namely, solution A:
>> 1) this change is clearer and easier to understand.
>> 2) this change does not touch any existing cxld logic
>>
>> For solution B:
>> it is more reasonable
>>
>> i finally select A since it can express my concern and relevant solution
>> clearly.
> 
> Understood.
> 
>>
>>>> +{
>>>> +	struct cxl_dfc_data *dfc_data = data;
>>>> +	int res;
>>>> +
>>>> +	res = dfc_data->match(dev, dfc_data->data);
>>>> +	if (res && get_device(dev)) {
>>>> +		dfc_data->target_device = dev;
>>>> +		return res;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +/*
>>>> + * I have the same function as device_find_child() but allow to modify
>>>> + * caller's match data @*data.
>>>> + */
>>>
>>> No need for this comment after the new API is established.
>>>
>>
>> i have given up the idea within v1 to introduce a new API which *should
>> ONLY* be used by this patch series, so it is not worthy of a new API
>> even if it can bring convenient for this patch series.
> 
> I'm not clear on this.  Are you still proposing to change the parameter to
> const?
> 
yes.
>>
>>>> +static struct device *cxl_device_find_child(struct device *parent, void *data,
>>>> +					    int (*match)(struct device *dev, void *data))
>>>> +{
>>>> +	struct cxl_dfc_data dfc_data = {match, data, NULL};
>>>> +
>>>> +	device_for_each_child(parent, &dfc_data, cxl_dfc_match_modify);
>>>> +	return dfc_data.target_device;
>>>> +}
>>>> +
>>>>  static ssize_t uuid_show(struct device *dev, struct device_attribute *attr,
>>>>  			 char *buf)
>>>>  {
>>>> @@ -849,7 +882,8 @@ 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 = cxl_device_find_child(&port->dev, &id,
>>>> +					    match_free_decoder);
>>>
>>> This is too literal.  How about the following (passes basic cxl-tests).
>>>
>>
>> it is reasonable.
>>
>> do you need me to submit that you suggest in the end and add you as
>> co-developer ?
> 
> You can submit it with Suggested-by:
> 
okay.
>>
>> OR
>>
>> you submit it by yourself ?
>>
>> either is okay for me.
>>
>>> Ira
>>>
>>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c              
>>> index 21ad5f242875..c1e46254efb8 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,30 @@ 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 && get_device(dev)) {                                 
>>
>> get_device(dev) failure may cause different logic against existing
>> but i think it should be impossible to happen normally.
> 
> Indeed this is slightly different.  :-/
> 
> Move the get_device() to find_free_decoder()?
> 
i think we can keep your change. so ignore this slight difference.
i also notice that you have done some verification for this change.
> Ira
> 
>>
>>> +               match_data->target_device = dev;                                
>>>                 return 1;                                                       
>>> +       }                                                                       
>>>                                                                                 
>>> -       (*id)++;                                                                
>>> +       match_data->id++;                                                       
>>>                                                                                 
>>>         return 0;                                                               
>>>  }                                                                              
>>>                                                                                 
>>> +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;                                        
>>> +}                                                                              
>>> +                                                                               
> 
> [snip]

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

[Greg Kroah-Hartman]

On Wed, Aug 21, 2024 at 10:44:27PM +0800, Zijun Hu wrote:
> On 2024/8/20 22:14, Ira Weiny wrote:
> > Zijun Hu wrote:
> >> On 2024/8/20 20:53, Ira Weiny wrote:
> >>> Zijun Hu wrote:
> >>>> 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)
> >>>>
> >>>
> >>> Seems reasonable.
> >>>
> >>> What about device_find_child_by_name()?
> >>>
> >>
> >> Plan to simplify this API implementation by * atomic * API
> >> device_find_child() as following:
> >>
> >> https://lore.kernel.org/all/20240811-simply_api_dfcbn-v2-1-d0398acdc366@quicinc.com
> >> struct device *device_find_child_by_name(struct device *parent,
> >>  					 const char *name)
> >> {
> >> 	return device_find_child(parent, name, device_match_name);
> >> }
> > 
> > Ok.  Thanks.
> > 
> >>
> >>>> Fixed by using consistent check (!parent || !parent->p) 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 1688e76cb64b..b1dd8c5590dc 100644
> >>>> --- a/drivers/base/core.c
> >>>> +++ b/drivers/base/core.c
> >>>> @@ -4004,7 +4004,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);
> >>>> @@ -4034,7 +4034,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);
> >>>> @@ -4068,7 +4068,7 @@ struct device *device_find_child(struct device *parent, void *data,
> >>>>  	struct klist_iter i;
> >>>>  	struct device *child;
> >>>>  
> >>>> -	if (!parent)
> >>>> +	if (!parent || !parent->p)
> >>>
> >>> Perhaps this was just a typo which should have been.
> >>>
> >>> 	if (!parent->p)
> >>> ?
> >>>
> >> maybe, but the following device_find_child_by_name() also use (!parent).
> >>
> >>> I think there is an expectation that none of these are called with a NULL
> >>> parent.
> >>>
> >>
> >> this patch aim is to make these atomic APIs have consistent checks as
> >> far as possible, that will make other patches within this series more
> >> acceptable.
> >>
> >> i combine two checks to (!parent || !parent->p) since i did not know
> >> which is better.
> > 
> > I'm not entirely clear either.  But checking the member p makes more sense
> > to me than the parent parameter.  I would expect that iterating the
> > children of a device must be done only when the parent device is not NULL.
> > 
> > parent->p is more subtle.  I'm unclear why the API would need to allow
> > that to run without error.
> > 
> i prefer (!parent || !parent->p) with below reasons:
> 
> 1)
> original API authors have such concern that either (!parent) or
> (!parent->p) maybe happen since they are checked, all their concerns
> can be covered by (!parent || !parent->p).

Wait, a device's parent can NOT be NULL except for some special cases
when it is being created.

And the ->p check is for internal stuff, meaning it has been initialized
and registered properly, again, these functions should never be called
for a device that this has not happened on.  So if they are, crashing is
fine as this should never have gotten through a development cycle.

The ->p checks were added way after the initial driver core was created,
as evolution of moving things out of struct device to prevent drivers
from touching those fields.  They were added add-hoc where needed and
probably not everywhere as you are finding out.

By adding these "robust" checks, we are making it harder for new code to
be written at the expense of doing checks that we "know" are never
going to happen in normal operation.  It's a trade off.  Only add them
when you KNOW they will be needed please.

thanks,

greg k-h

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

[Greg Kroah-Hartman]

On Thu, Aug 15, 2024 at 10:58:02PM +0800, Zijun Hu wrote:
> 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) 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 1688e76cb64b..b1dd8c5590dc 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -4004,7 +4004,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);
> @@ -4034,7 +4034,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);
> @@ -4068,7 +4068,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;

My review comments that I just made previously were more "generic",
sorry.  This change looks fine, there's no additional runtime overhead
for doing this check as we already have to dereference the pointer, so
might as well be consistant.  I'll go queue this up next week when I get
back from conference travel.

thanks,

greg k-h

Re: [PATCH v2 2/4] cxl/region: Prevent device_find_child() from modifying caller's match data

[Greg Kroah-Hartman]

On Tue, Aug 20, 2024 at 08:59:18AM -0500, Ira Weiny wrote:
> 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, fixed by implementing a equivalent
> > cxl_device_find_child() instead of the old API usage.
> 
> Generally it seems ok but I think some name changes will make this more
> clear.  See below.
> 
> 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?
> 
> > 
> > Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> > ---
> >  drivers/cxl/core/region.c | 36 +++++++++++++++++++++++++++++++++++-
> >  1 file changed, 35 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 21ad5f242875..8d8f0637f7ac 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -134,6 +134,39 @@ static const struct attribute_group *get_cxl_region_access1_group(void)
> >  	return &cxl_region_access1_coordinate_group;
> >  }
> >  
> > +struct cxl_dfc_data {
> 
> struct cxld_match_data
> 
> 'cxld' == cxl decoder in our world.
> 
> > +	int (*match)(struct device *dev, void *data);
> > +	void *data;
> > +	struct device *target_device;
> > +};
> > +
> > +static int cxl_dfc_match_modify(struct device *dev, void *data)
> 
> Why not just put this logic into match_free_decoder?
> 
> > +{
> > +	struct cxl_dfc_data *dfc_data = data;
> > +	int res;
> > +
> > +	res = dfc_data->match(dev, dfc_data->data);
> > +	if (res && get_device(dev)) {
> > +		dfc_data->target_device = dev;
> > +		return res;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * I have the same function as device_find_child() but allow to modify
> > + * caller's match data @*data.
> > + */
> 
> No need for this comment after the new API is established.
> 
> > +static struct device *cxl_device_find_child(struct device *parent, void *data,
> > +					    int (*match)(struct device *dev, void *data))
> > +{
> > +	struct cxl_dfc_data dfc_data = {match, data, NULL};
> > +
> > +	device_for_each_child(parent, &dfc_data, cxl_dfc_match_modify);
> > +	return dfc_data.target_device;
> > +}
> > +
> >  static ssize_t uuid_show(struct device *dev, struct device_attribute *attr,
> >  			 char *buf)
> >  {
> > @@ -849,7 +882,8 @@ 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 = cxl_device_find_child(&port->dev, &id,
> > +					    match_free_decoder);
> 
> This is too literal.  How about the following (passes basic cxl-tests).
> 
> Ira
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c              
> index 21ad5f242875..c1e46254efb8 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,30 @@ 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 && get_device(dev)) {                                 
> +               match_data->target_device = dev;                                
>                 return 1;                                                       
> +       }                                                                       
>                                                                                 
> -       (*id)++;                                                                
> +       match_data->id++;                                                       
>                                                                                 
>         return 0;                                                               
>  }                                                                              
>                                                                                 
> +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;                                        
> +}                                                                              

I like this type of re-write much better, thanks!

greg k-h

Re: [PATCH v2 4/4] net: qcom/emac: Prevent device_find_child() from modifying caller's match data

[Greg Kroah-Hartman]

On Thu, Aug 15, 2024 at 10:58:05PM +0800, 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 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, fixed by implementing a equivalent
> emac_device_find_child() instead of the old API usage.
> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/net/ethernet/qualcomm/emac/emac-sgmii.c | 36 +++++++++++++++++++++++--
>  1 file changed, 34 insertions(+), 2 deletions(-)

Can you rewrite this based on the cxl change to make it a bit more less
of a "wrap the logic in yet another layer" type of change like this one
is?

thanks,

greg k-h

Re: [PATCH v2 4/4] net: qcom/emac: Prevent device_find_child() from modifying caller's match data

[Zijun Hu]

On 2024/8/24 11:29, Greg Kroah-Hartman wrote:
> On Thu, Aug 15, 2024 at 10:58:05PM +0800, 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 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, fixed by implementing a equivalent
>> emac_device_find_child() instead of the old API usage.
>>
>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>> ---
>>  drivers/net/ethernet/qualcomm/emac/emac-sgmii.c | 36 +++++++++++++++++++++++--
>>  1 file changed, 34 insertions(+), 2 deletions(-)
> 
> Can you rewrite this based on the cxl change to make it a bit more less
> of a "wrap the logic in yet another layer" type of change like this one
> is?
> 
sure. will do it today.
> thanks,
> 
> greg k-h