Re: [PATCH] driver core: take parent lock on deferred probe

["Danilo Krummrich" <dakr@kernel.org>]

On Thu Oct 15, 2026 at 12:40 AM CEST, Danilo Krummrich wrote:
> On Wed Oct 14, 2026 at 5:14 PM CEST, Johan Hovold wrote:
>> On Wed, Oct 14, 2026 at 04:42:09PM +0200, Danilo Krummrich wrote:
>>> On Wed Oct 14, 2026 at 10:15 AM CEST, Johan Hovold wrote:
>>
>>> > But it's driver core that needs the extra reference. It seems
>>> > reasonable to require that any subsystem that sets a new
>>> > type->need_parent_lock also makes sure that any device of that type
>>> > holds a reference to its parent. And importantly, it's already the case
>>> > for the only user of this feature today (i.e. USB interfaces).
>>> 
>>> Exactly, it is the driver core that needs the extra reference count; for a field
>>> that is owned by the driver core, and for which the driver core manages its
>>> lifetime.  If it needs an additional reference count for async or deferred work
>>> it should obtain the reference count for this scope explicitly.
>>
>> As I've mentioned, it's not just for async or deferred probe. It's also
>> needed when a driver gets added or when probe is triggered through
>> sysfs. The bus iterators only takes a reference to the each device and
>> not their parents before calling __driver_attach() etc.
>>
>>> USB interfaces already holding an additional reference makes the proposed
>>> contract work for today's user, but I'd rather not make that prerequisite of the
>>> API.  It would push a prerequisite for an implementation detail to the user.
>>
>> I understand your concern, but my reasoning is that this feature is only
>> needed when devices are tightly coupled with their parents and where it
>> therefore probably always make sense to pin the parent.
>>
>> Note that the need_parent_lock flag is now set in the device type struct
>> which also holds a pointer to the release function that drops the
>> reference. So it's pretty easy to check any theoretical further users.
>>
>>> Please, let's not add this API contract. Let's have the driver core deal with
>>> this internally.
>>
>> I can take another look at it, but the alternatives seemed either error
>> prone or a bit icky (e.g. a helper that returns a reference to the
>> parent after checking the dead flag under the device lock).
>
> I thought some more about this, and since it is only the USB device type using
> it it's probably fine for now.  Should we ever gain new users of this API (which
> I do not hope), we can still change it and add something like the diff below.
>
> I'd suggest to also add a comment in the USB code, explaining that this
> reference count (also) serves the purpose to fulfill the driver core API
> contract.

I pasted the wrong diff, I meant the one below, but either should work.

diff --git a/drivers/base/base.h b/drivers/base/base.h
index 3c36a2128f88..b3730d030f5f 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -161,8 +161,9 @@ bool bus_is_registered(const struct bus_type *bus);

 int bus_add_driver(struct device_driver *drv);
 void bus_remove_driver(struct device_driver *drv);
+struct device *device_get_parent_for_lock(struct device *dev);
 void device_release_driver_internal(struct device *dev, const struct device_driver *drv,
-				    struct device *parent);
+				    bool lock_parent);

 void driver_detach(const struct device_driver *drv);
 void driver_deferred_probe_del(struct device *dev);
diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index fd2257f73253..9975f250d22e 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -637,11 +637,13 @@ void bus_probe_device(struct device *dev)

 void bus_probe_device_deferred(struct device *dev)
 {
-	if (dev->type && dev->type->need_parent_lock)
-		device_lock(dev->parent);
+	struct device *parent __free(put_device) = device_get_parent_for_lock(dev);
+
+	if (parent)
+		device_lock(parent);
 	__bus_probe_device(dev);
-	if (dev->type && dev->type->need_parent_lock)
-		device_unlock(dev->parent);
+	if (parent)
+		device_unlock(parent);
 }

 /**
@@ -866,11 +868,13 @@ static int __must_check bus_rescan_devices_helper(struct device *dev,
 	int ret = 0;

 	if (!dev->driver) {
-		if (dev->type && dev->type->need_parent_lock)
-			device_lock(dev->parent);
+		struct device *parent __free(put_device) = device_get_parent_for_lock(dev);
+
+		if (parent)
+			device_lock(parent);
 		ret = device_attach(dev);
-		if (dev->type && dev->type->need_parent_lock)
-			device_unlock(dev->parent);
+		if (parent)
+			device_unlock(parent);
 	}
 	return ret < 0 ? ret : 0;
 }
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 5daa88fa724a..3a924c4e194e 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1676,8 +1676,7 @@ void device_links_unbind_consumers(struct device *dev)

 			device_links_write_unlock();

-			device_release_driver_internal(consumer, NULL,
-						       consumer->parent);
+			device_release_driver_internal(consumer, NULL, true);
 			put_device(consumer);
 			goto start;
 		}
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 52123251965d..6ff8d0653769 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -1030,6 +1030,30 @@ static int __device_attach_driver(struct device_driver *drv, void *_data)
 	return ret == 0;
 }

+/*
+ * Pin the parent before taking its lock. A reference to dev alone does not
+ * keep its parent alive after device_del().
+ *
+ * device_del() marks the device dead under its lock before dropping the
+ * registration reference to the parent. A dead device may still need to be
+ * unbound; while dev->driver is set, device_del() cannot have completed the
+ * unbind and dropped that reference either.
+ *
+ * The caller must hold a reference to dev and put the returned parent after
+ * its last access, including any temporary unlock/relock during unbind.
+ */
+struct device *device_get_parent_for_lock(struct device *dev)
+{
+	if (!dev->type || !dev->type->need_parent_lock)
+		return NULL;
+
+	guard(device)(dev);
+	if (dev->p->dead && !dev->driver)
+		return NULL;
+
+	return get_device(dev->parent);
+}
+
 /*
  * __device_driver_lock - acquire locks needed to manipulate dev->drv
  * @dev: Device we will update driver info for
@@ -1065,13 +1089,14 @@ static void __device_driver_unlock(struct device *dev, struct device *parent)
 static void __device_attach_async_helper(void *_dev, async_cookie_t cookie)
 {
 	struct device *dev = _dev;
+	struct device *parent __free(put_device) = device_get_parent_for_lock(dev);
 	struct device_attach_data data = {
 		.dev		= dev,
 		.check_async	= true,
 		.want_async	= true,
 	};

-	__device_driver_lock(dev, dev->parent);
+	__device_driver_lock(dev, parent);

 	/*
 	 * Check if device has already been removed or claimed. This may
@@ -1093,7 +1118,7 @@ static void __device_attach_async_helper(void *_dev, async_cookie_t cookie)
 	if (dev->parent)
 		pm_runtime_put(dev->parent);
 out_unlock:
-	__device_driver_unlock(dev, dev->parent);
+	__device_driver_unlock(dev, parent);

 	put_device(dev);
 }
@@ -1205,11 +1230,12 @@ void device_initial_probe(struct device *dev)
  */
 int device_driver_attach(const struct device_driver *drv, struct device *dev)
 {
+	struct device *parent __free(put_device) = device_get_parent_for_lock(dev);
 	int ret;

-	__device_driver_lock(dev, dev->parent);
+	__device_driver_lock(dev, parent);
 	ret = __driver_probe_device(drv, dev);
-	__device_driver_unlock(dev, dev->parent);
+	__device_driver_unlock(dev, parent);

 	/* also return probe errors as normal negative errnos */
 	if (ret > 0)
@@ -1223,14 +1249,15 @@ EXPORT_SYMBOL_GPL(device_driver_attach);
 static void __driver_attach_async_helper(void *_dev, async_cookie_t cookie)
 {
 	struct device *dev = _dev;
+	struct device *parent __free(put_device) = device_get_parent_for_lock(dev);
 	const struct device_driver *drv;
 	int ret;

-	__device_driver_lock(dev, dev->parent);
+	__device_driver_lock(dev, parent);
 	drv = dev->p->async_driver;
 	dev->p->async_driver = NULL;
 	ret = driver_probe_device(drv, dev);
-	__device_driver_unlock(dev, dev->parent);
+	__device_driver_unlock(dev, parent);

 	dev_dbg(dev, "driver %s async attach completed: %d\n", drv->name, ret);

@@ -1296,9 +1323,11 @@ static int __driver_attach(struct device *dev, void *data)
 		return 0;
 	}

-	__device_driver_lock(dev, dev->parent);
+	struct device *parent __free(put_device) = device_get_parent_for_lock(dev);
+
+	__device_driver_lock(dev, parent);
 	driver_probe_device(drv, dev);
-	__device_driver_unlock(dev, dev->parent);
+	__device_driver_unlock(dev, parent);

 	return 0;
 }
@@ -1372,8 +1401,11 @@ static void __device_release_driver(struct device *dev, struct device *parent)

 void device_release_driver_internal(struct device *dev,
 				    const struct device_driver *drv,
-				    struct device *parent)
+				    bool lock_parent)
 {
+	struct device *parent __free(put_device) = lock_parent ?
+		device_get_parent_for_lock(dev) : NULL;
+
 	__device_driver_lock(dev, parent);

 	if (!drv || drv == dev->driver)
@@ -1400,7 +1432,7 @@ void device_release_driver(struct device *dev)
 	 * within their ->remove callback for the same device, they
 	 * will deadlock right here.
 	 */
-	device_release_driver_internal(dev, NULL, NULL);
+	device_release_driver_internal(dev, NULL, false);
 }
 EXPORT_SYMBOL_GPL(device_release_driver);

@@ -1413,7 +1445,7 @@ EXPORT_SYMBOL_GPL(device_release_driver);
  */
 void device_driver_detach(struct device *dev)
 {
-	device_release_driver_internal(dev, NULL, dev->parent);
+	device_release_driver_internal(dev, NULL, true);
 }

 /**
@@ -1440,7 +1472,7 @@ void driver_detach(const struct device_driver *drv)
 		dev = dev_prv->device;
 		get_device(dev);
 		spin_unlock(&drv->p->klist_devices.k_lock);
-		device_release_driver_internal(dev, drv, dev->parent);
+		device_release_driver_internal(dev, drv, true);
 		put_device(dev);
 	}
 }
diff --git a/include/linux/device.h b/include/linux/device.h
index 7c7682e4dafe..7604060ff99b 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -85,8 +85,7 @@ int subsys_virtual_register(const struct bus_type *subsys,
  * the DEVTYPE variable.
  *
  * @need_parent_lock:	When probing or removing a device of this type,
- *			driver core should lock the device's parent. Note that
- *			the device must hold a reference to its parent.
+ *			driver core should lock the device's parent.
  */
 struct device_type {
 	const char *name;