Message-ID: <20240817095713.GA182612@workstation.local>
Date: Sat, 17 Aug 2024 18:57:13 +0900
From: Takashi Sakamoto <o-takashi@...amocchi.jp>
To: Zijun Hu <zijun_hu@...oud.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Davidlohr Bueso <dave@...olabs.net>,
	Jonathan Cameron <jonathan.cameron@...wei.com>,
	Dave Jiang <dave.jiang@...el.com>,
	Alison Schofield <alison.schofield@...el.com>,
	Vishal Verma <vishal.l.verma@...el.com>,
	Ira Weiny <ira.weiny@...el.com>,
	Dan Williams <dan.j.williams@...el.com>,
	Timur Tabi <timur@...nel.org>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	linux-kernel@...r.kernel.org, linux-cxl@...r.kernel.org,
	linux1394-devel@...ts.sourceforge.net, netdev@...r.kernel.org,
	Zijun Hu <quic_zijuhu@...cinc.com>
Subject: Re: [PATCH v2 3/4] firewire: core: Prevent device_find_child() from
 modifying caller's match data

Hi,

On Thu, Aug 15, 2024 at 10:58:04PM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@...cinc.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@...cinc.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@...amocchi.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

Powered by blists - more mailing lists