[PATCH v3 0/4] device_schedule_reprobe(): core helper and conversions

[PATCH v3 0/4] device_schedule_reprobe(): core helper and conversions

[Daniel Golle]

Three in-tree drivers (iwlwifi, hci_h5, btintel_pcie) schedule a
deferred re-probe of their own device from a work item whose work
function lives in module text. The hand-rolled copies share two bug
classes: the work function ends with module_put(THIS_MODULE), racing a
concurrent rmmod freeing the module text (the race
module_put_and_kthread_exit() exists to close for kthreads), and
nothing synchronizes the deferred detach against device_shutdown() or
an administrative unbind.

Patch 1 moves the deferred work into the driver core.
device_schedule_reprobe() runs builtin code, so no module reference is
needed; it checks under a single __device_driver_lock() hold that the
device is still bound to the driver that scheduled the re-probe, and
skips the detach once device_shutdown() has reached the device (a new
one-bit shutdown_done flag in struct device_private). It pins both the
device and its parent for the lifetime of the work and re-locks the
parent across the attach on buses that require it. Patches 2 and 3 are
mechanical conversions; patch 4 (btintel_pcie) also retires that
driver's remove()-from-own-work contract.

This is a cross-tree series. Each patch applies cleanly on the tree
that owns it:

  patch 1 (driver core)      -> driver-core
  patch 2 (iwlwifi)          -> wireless-next
  patches 3, 4 (bluetooth)   -> bluetooth-next

It is based on bluetooth-next so patches 3 and 4 apply there; patch 1
applies equally on driver-core and patch 2 on wireless-next (verified
against all three trees). Patches 2-4 depend on the helper from patch
1, so they need it in their tree -- or an immutable branch/tag of it --
before they build.

As Hans put it on the RFC: the tree already has device_reprobe() with
15 callers, so this just adds a way to run one from a worker safely
rather than having each open-coded copy reimplement the locking.

History: this was first floated as an RFC (v1) [1]. In parallel the
helper rode along in the mxl862xx DSA firmware-update series, whose
review [2] caught a parent use-after-free (the work locks the parent,
but only the child was pinned) and a missing parent lock across
device_attach() on need_parent_lock buses -- both fixed here. net-next
is closed and the mxl862xx feature waits for the next window, so the
helper is submitted on its own now; mxl862xx is going to use it once it
lands.

Testing: Hans de Goede reviewed and tested patches 1 and 3 on hci_h5
hardware that hits the resume re-probe path. Patches 1-3 were also
runtime-tested (backported to 7.0.11) on Intel AX101 with PROVE_LOCKING
and DEBUG_OBJECTS_WORK, driving iwlwifi's crash escalation into the
re-probe path: normal detach+rebind, an unbind racing a pending
re-probe (not undone), rmmod with a re-probe pending (now succeeds
instead of EBUSY), and reboot with a re-probe pending; no lockdep or
debugobjects reports. The helper was exercised further by the mxl862xx
v11/v12 hardware testing. btintel_pcie (patch 4) is compile-tested
only.

Changes since v2 [3]:
 - patch 1: use dev_err_probe() for the re-probe error path so a
   re-probe deferred at resume no longer logs a spurious error; pick up
   Hans de Goede's Tested-by/Reviewed-by
 - patch 3: pick up Hans de Goede's Tested-by/Reviewed-by
 - patch 4: rebased onto bluetooth-next, whose btintel_pcie has four
   dump workers rather than the single one the v2 mainline snapshot had
 - base the series on bluetooth-next instead of v7.2-rc7 so the
   bluetooth patches apply for their maintainers

Changes since the RFC (v1) [1]:
 - pin the parent device across the deferred work and re-lock it across
   device_attach() on need_parent_lock buses (found reviewing the
   mxl862xx posting [2])
 - schedule the work on system_dfl_wq instead of the deprecated
   system_unbound_wq
 - thread the series properly (the RFC's patches were unthreaded)
 - drop RFC status

[1] https://lore.kernel.org/all/anpxFdwNxk0XwPjQ@makrotopia.org/
[2] https://lore.kernel.org/all/cover.1786922210.git.daniel@makrotopia.org/
[3] https://lore.kernel.org/all/cover.1787185594.git.daniel@makrotopia.org/

Daniel Golle (4):
  driver core: add device_schedule_reprobe()
  wifi: iwlwifi: use device_schedule_reprobe()
  Bluetooth: hci_h5: use device_schedule_reprobe()
  Bluetooth: btintel_pcie: use device_schedule_reprobe() after reset

 drivers/base/base.h                           |   5 +
 drivers/base/core.c                           |   3 +
 drivers/base/dd.c                             | 102 ++++++++++++++++++
 drivers/bluetooth/btintel_pcie.c              |  49 +++++----
 drivers/bluetooth/hci_h5.c                    |  43 ++------
 .../net/wireless/intel/iwlwifi/iwl-trans.c    |  40 +------
 include/linux/device.h                        |   2 +
 7 files changed, 146 insertions(+), 98 deletions(-)


base-commit: 486f8908aa587ab2a213bbef39311743e4f8f57a
-- 
2.55.0

[PATCH v3 1/4] driver core: add device_schedule_reprobe()

[Daniel Golle]

Three in-tree drivers schedule a deferred re-probe of their own device
from a work item whose work function lives in module text: iwlwifi
(iwl_trans_schedule_reprobe(), firmware crash recovery when a lighter
restart is not sufficient), hci_h5 (h5_btrtl_resume(), RTL devices
lose their firmware state over suspend) and btintel_pcie (synchronous
device_reprobe() from its own reset work, with a hand-rolled locking
contract spanning several comments).

Two bug classes affect the hand-rolled implementations:

1. The work function ends with put_device(); kfree();
   module_put(THIS_MODULE); in module text. After the atomic decrement
   a concurrent rmmod can free the module text before the function
   epilogue has finished executing. This is exactly the race
   module_put_and_kthread_exit() exists to close for kthreads; there
   is no work-item equivalent.

2. There is no synchronization between the deferred device_reprobe()
   and device_shutdown() or a driver unbind. The drivers do not check
   any bound state before calling device_reprobe(), so a stale
   re-probe can undo an administrative unbind, and the detach half can
   run against a device whose ->shutdown() callback has already run.
   The core already blocks the attach half during shutdown
   (device_shutdown() calls device_block_probing() before any
   callback, and really_probe() honors defer_all_probes), but nothing
   blocks the detach half. For drivers which clear their drvdata in
   ->shutdown() so that a subsequent ->remove() becomes a no-op this
   escalates to use-after-free of driver state which other subsystem
   structures still reference.

Both classes disappear when the driver core owns the deferred work.
Add device_schedule_reprobe(), which schedules a detach and re-probe
of a device after a caller-specified delay:

- The work function is builtin text, so callers do not need to hold a
  module reference. If the driver module is unloaded before the work
  runs, driver_unregister() has already unbound the device, the bound
  driver no longer matches the driver recorded at scheduling time and
  the work does nothing.

- The recorded driver pointer is only ever compared, never
  dereferenced, so it may legitimately point to freed memory.

- The bound-state check and __device_release_driver() run under a
  single __device_driver_lock() hold, the same lock dance
  device_release_driver_internal() uses. This closes the
  check-vs-detach TOCTOU that drivers cannot close themselves,
  because device_reprobe() takes the device lock internally.

- Both @dev and its parent are pinned for the lifetime of the work.
  __device_driver_lock() and the attach half lock the parent, and an
  unregister of @dev drops @dev's reference to the parent, so without
  a reference of our own the parent could be freed before the work
  runs.

- A new shutdown_done flag in struct device_private, set under the
  device lock once device_shutdown() reaches a device, suppresses the
  detach half during shutdown. It occupies a spare bit in an existing
  byte, mirroring how kill_device() sets the dead flag.

- The attach half is plain device_attach(), which already honors both
  the dead flag and defer_all_probes: a re-probe landing during
  system suspend detaches immediately and the probe is deferred until
  device_restore_probing() at resume time. The detach half
  deliberately does not check defer_all_probes so that a re-probe
  scheduled before suspend is not silently dropped. The parent is
  re-locked across device_attach() on buses that set
  need_parent_lock, mirroring bus_rescan_devices_helper().

One pre-existing window remains: __device_release_driver()
transiently drops the locks while consumer device links are busy, so
for devices with busy consumers a ->shutdown() can still interleave
in the middle of the release. That window exists identically for
every unbind path in the kernel, sysfs unbind included, and is not
made worse by this helper.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
Tested-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
---
v3: use dev_err_probe() for the re-probe error path, so a re-probe
    deferred at resume no longer logs a spurious error (Hans de Goede)
v2: pin the parent across the deferred work and re-lock it across
    device_attach() on need_parent_lock buses; schedule on system_dfl_wq
    rather than the deprecated system_unbound_wq (found reviewing the
    mxl862xx v12 posting)
v1: initial RFC

 drivers/base/base.h    |   5 ++
 drivers/base/core.c    |   3 ++
 drivers/base/dd.c      | 102 +++++++++++++++++++++++++++++++++++++++++
 include/linux/device.h |   2 +
 4 files changed, 112 insertions(+)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index a5b7abc10ff0..6234e37de7e9 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -106,6 +106,10 @@ struct driver_private {
  * @dead: This device is currently either in the process of or has been
  *	  removed from the system. Any asynchronous events scheduled for this
  *	  device should exit without taking any action.
+ * @shutdown_done: Set once device_shutdown() has reached this device, under
+ *	  the device lock, before any shutdown callback runs. Read under the
+ *	  device lock. A deferred re-probe scheduled with
+ *	  device_schedule_reprobe() must not detach the device anymore.
  *
  * Nothing outside of the driver core should ever touch these fields.
  */
@@ -120,6 +124,7 @@ struct device_private {
 	char *deferred_probe_reason;
 	struct device *device;
 	u8 dead:1;
+	u8 shutdown_done:1;
 };
 #define to_device_private_parent(obj)	\
 	container_of(obj, struct device_private, knode_parent)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 4d026682944f..8a7dbe4e8362 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -4906,6 +4906,9 @@ void device_shutdown(void)
 			device_lock(parent);
 		device_lock(dev);
 
+		if (dev->p)
+			dev->p->shutdown_done = true;
+
 		/* Don't allow any more runtime suspends */
 		pm_runtime_get_noresume(dev);
 		pm_runtime_barrier(dev);
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 60c005223844..d765e1ae5614 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -1436,3 +1436,105 @@ void driver_detach(const struct device_driver *drv)
 		put_device(dev);
 	}
 }
+
+struct device_reprobe {
+	struct delayed_work work;
+	struct device *dev;
+	struct device *parent;
+	const struct device_driver *drv;
+};
+
+static void device_reprobe_work_fn(struct work_struct *work)
+{
+	struct device_reprobe *rp = container_of(work, struct device_reprobe,
+						 work.work);
+	struct device *dev = rp->dev;
+	struct device *parent = rp->parent;
+	bool detached = false;
+	int ret;
+
+	__device_driver_lock(dev, parent);
+	/*
+	 * rp->drv is only ever compared, never dereferenced: the driver it
+	 * points to may have been unregistered and freed by now.
+	 */
+	if (!dev->p->dead && !dev->p->shutdown_done &&
+	    dev->driver && dev->driver == rp->drv) {
+		__device_release_driver(dev, parent);
+		detached = true;
+	}
+	__device_driver_unlock(dev, parent);
+
+	if (detached) {
+		/*
+		 * device_attach() must run with the parent locked on buses
+		 * that require it, mirroring bus_rescan_devices_helper().
+		 */
+		if (parent && dev->bus->need_parent_lock)
+			device_lock(parent);
+		ret = device_attach(dev);
+		if (ret < 0)
+			dev_err_probe(dev, ret,
+				      "re-probe failed, device left unbound\n");
+		if (parent && dev->bus->need_parent_lock)
+			device_unlock(parent);
+	}
+
+	put_device(dev);
+	put_device(parent);
+	kfree(rp);
+}
+
+/**
+ * device_schedule_reprobe - schedule a deferred detach and re-probe
+ * @dev: device to detach and re-probe
+ * @delay_ms: delay in milliseconds before the re-probe runs
+ *
+ * Schedule a detach and re-probe of @dev after @delay_ms milliseconds.
+ * The re-probe is skipped if, by the time the scheduled work runs, the
+ * device has been removed, the system shutdown sequence has reached the
+ * device, or @dev is no longer bound to the driver that was bound at
+ * scheduling time. In particular an administrative unbind is never
+ * undone by a stale re-probe.
+ *
+ * The work function is built-in text, so the bound driver may call this
+ * from its own code without holding a module reference. If the driver
+ * module is unloaded before the work runs, driver unregistration unbinds
+ * @dev first and the scheduled work does nothing.
+ *
+ * Multiple pending re-probes for the same device are individually safe;
+ * a caller that wants at most one pending re-probe must gate scheduling
+ * itself.
+ *
+ * May only be called from process context.
+ *
+ * Returns: 0 on success, -EINVAL if @dev is not a registered device
+ * bound to a driver, -ENOMEM on allocation failure.
+ */
+int device_schedule_reprobe(struct device *dev, unsigned int delay_ms)
+{
+	struct device_reprobe *rp;
+
+	if (!dev->bus || !dev->p || !device_is_registered(dev))
+		return -EINVAL;
+	if (!dev->driver)
+		return -EINVAL;
+
+	rp = kzalloc_obj(*rp);
+	if (!rp)
+		return -ENOMEM;
+
+	rp->dev = get_device(dev);
+	/*
+	 * Pin the parent too: the work locks it, and an unregister of @dev
+	 * would otherwise drop the last reference before the work runs.
+	 */
+	rp->parent = get_device(dev->parent);
+	rp->drv = READ_ONCE(dev->driver);
+	INIT_DELAYED_WORK(&rp->work, device_reprobe_work_fn);
+	queue_delayed_work(system_dfl_wq, &rp->work,
+			   msecs_to_jiffies(delay_ms));
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(device_schedule_reprobe);
diff --git a/include/linux/device.h b/include/linux/device.h
index 7b2baffdd2f5..1cdd40a6a48d 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1312,6 +1312,8 @@ int  __must_check device_attach(struct device *dev);
 int __must_check driver_attach(const struct device_driver *drv);
 void device_initial_probe(struct device *dev);
 int __must_check device_reprobe(struct device *dev);
+int __must_check device_schedule_reprobe(struct device *dev,
+					 unsigned int delay_ms);
 
 bool device_is_bound(struct device *dev);
 
-- 
2.55.0

RE: device_schedule_reprobe(): core helper and conversions

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2609 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=1149424

---Test result---

Test Summary:
CheckPatch                    PASS      3.98 seconds
VerifyFixes                   PASS      0.14 seconds
VerifySignedoff               PASS      0.14 seconds
GitLint                       PASS      1.36 seconds
SubjectPrefix                 FAIL      0.56 seconds
BuildKernel                   PASS      27.88 seconds
CheckAllWarning               PASS      30.35 seconds
CheckSparse                   PASS      29.30 seconds
BuildKernel32                 PASS      26.40 seconds
CheckKernelLLVM               SKIP      0.00 seconds
TestRunnerSetup               PASS      502.81 seconds
TestRunner_l2cap-tester       PASS      65.90 seconds
TestRunner_iso-tester         PASS      83.60 seconds
TestRunner_bnep-tester        PASS      19.21 seconds
TestRunner_mgmt-tester        FAIL      221.72 seconds
TestRunner_rfcomm-tester      PASS      25.46 seconds
TestRunner_sco-tester         PASS      36.60 seconds
TestRunner_ioctl-tester       PASS      26.30 seconds
TestRunner_mesh-tester        FAIL      26.05 seconds
TestRunner_smp-tester         PASS      23.47 seconds
TestRunner_userchan-tester    PASS      20.58 seconds
TestRunner_6lowpan-tester     PASS      23.63 seconds
IncrementalBuild              PASS      30.46 seconds

Details
##############################
Test: SubjectPrefix - FAIL
Desc: Check subject contains "Bluetooth" prefix
Output:
"Bluetooth: " prefix is not specified in the subject
"Bluetooth: " prefix is not specified in the subject
##############################
Test: CheckKernelLLVM - SKIP
Desc: Build kernel with LLVM + context analysis
Output:
Clang not found
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 501, Passed: 496 (99.0%), Failed: 1, Not Run: 4

Failed Test Cases
Read Exp Feature - Success                           Failed       0.255 seconds
##############################
Test: TestRunner_mesh-tester - FAIL
Desc: Run mesh-tester with test-runner
Output:
Total: 10, Passed: 8 (80.0%), Failed: 2, Not Run: 0

Failed Test Cases
Mesh - Send cancel - 1                               Timed out    1.908 seconds
Mesh - Send cancel - 2                               Timed out    1.988 seconds


https://github.com/bluez/bluetooth-next/pull/627

---
Regards,
Linux Bluetooth

RE: device_schedule_reprobe(): core helper and conversions

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2455 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/series/1149424/

---Test result---

Test Summary:
CheckPatch                    PASS      3.48 seconds
VerifyFixes                   PASS      0.10 seconds
VerifySignedoff               PASS      0.10 seconds
GitLint                       PASS      1.24 seconds
SubjectPrefix                 FAIL      0.39 seconds
BuildKernel                   PASS      29.55 seconds
CheckAllWarning               PASS      33.66 seconds
CheckSparse                   PASS      36.63 seconds
BuildKernel32                 PASS      30.00 seconds
CheckKernelLLVM               PASS      43.82 seconds
TestRunnerSetup               PASS      775.59 seconds
TestRunner_l2cap-tester       PASS      44.07 seconds
TestRunner_iso-tester         PASS      93.05 seconds
TestRunner_bnep-tester        PASS      12.56 seconds
TestRunner_mgmt-tester        FAIL      145.83 seconds
TestRunner_rfcomm-tester      PASS      16.05 seconds
TestRunner_sco-tester         PASS      23.41 seconds
TestRunner_ioctl-tester       PASS      17.22 seconds
TestRunner_mesh-tester        FAIL      18.17 seconds
TestRunner_smp-tester         PASS      14.88 seconds
TestRunner_userchan-tester    PASS      12.96 seconds
TestRunner_6lowpan-tester     PASS      16.38 seconds
IncrementalBuild              PASS      39.00 seconds

Details
##############################
Test: SubjectPrefix - FAIL
Desc: Check subject contains "Bluetooth" prefix
Output:
"Bluetooth: " prefix is not specified in the subject
"Bluetooth: " prefix is not specified in the subject
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 501, Passed: 496 (99.0%), Failed: 1, Not Run: 4

Failed Test Cases
Read Exp Feature - Success                           Failed       0.156 seconds
##############################
Test: TestRunner_mesh-tester - FAIL
Desc: Run mesh-tester with test-runner
Output:
Total: 10, Passed: 8 (80.0%), Failed: 2, Not Run: 0

Failed Test Cases
Mesh - Send cancel - 1                               Timed out    2.116 seconds
Mesh - Send cancel - 2                               Timed out    1.996 seconds


https://github.com/bluez/bluetooth-next/pull/740

---
Regards,
Linux Bluetooth

[PATCH v3 2/4] wifi: iwlwifi: use device_schedule_reprobe()

[Daniel Golle]

iwl_trans_schedule_reprobe() open-codes a deferred re-probe: it takes
a module reference, allocates a work item, and the work function calls
device_reprobe() and then ends with put_device(); kfree();
module_put(THIS_MODULE); in module text. That final module_put() is
racy: once the reference count is decremented a concurrent rmmod can
free the module text before the work function's epilogue has finished
executing. The work also does not synchronize against shutdown or
unbind, so a stale re-probe could undo an administrative unbind or
detach a device whose ->shutdown() callback has already run.

Convert to the new device_schedule_reprobe() helper, whose work
function is builtin text and which skips the re-probe when the device
was removed, shutdown reached it, or it is no longer bound to the
driver that scheduled the re-probe. Both call sites keep their delays
(IWL_TRANS_TOP_FOLLOWER_WAIT for the TOP follower case, 0 for the
escalated firmware error case).

Behavioral changes:

- A pending re-probe no longer pins the module: rmmod with a re-probe
  pending now succeeds immediately and the re-probe becomes a no-op,
  instead of rmmod failing with EBUSY. The "Module is being unloaded -
  abort" path disappears together with the try_module_get().

- A re-probe scheduled before a system shutdown or before an
  administrative unbind no longer detaches and rebinds the device
  afterwards.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v3: no changes
v2: no changes
v1: initial RFC

 .../net/wireless/intel/iwlwifi/iwl-trans.c    | 40 +------------------
 1 file changed, 2 insertions(+), 38 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-trans.c b/drivers/net/wireless/intel/iwlwifi/iwl-trans.c
index 73aae1125042..5ae734cb9027 100644
--- a/drivers/net/wireless/intel/iwlwifi/iwl-trans.c
+++ b/drivers/net/wireless/intel/iwlwifi/iwl-trans.c
@@ -78,47 +78,11 @@ void iwl_trans_free_restart_list(void)
 	}
 }
 
-struct iwl_trans_reprobe {
-	struct device *dev;
-	struct delayed_work work;
-};
-
-static void iwl_trans_reprobe_wk(struct work_struct *wk)
-{
-	struct iwl_trans_reprobe *reprobe;
-
-	reprobe = container_of(wk, typeof(*reprobe), work.work);
-
-	if (device_reprobe(reprobe->dev))
-		dev_err(reprobe->dev, "reprobe failed!\n");
-	put_device(reprobe->dev);
-	kfree(reprobe);
-	module_put(THIS_MODULE);
-}
-
 static void iwl_trans_schedule_reprobe(struct iwl_trans *trans,
 				       unsigned int delay_ms)
 {
-	struct iwl_trans_reprobe *reprobe;
-
-	/*
-	 * get a module reference to avoid doing this while unloading
-	 * anyway and to avoid scheduling a work with code that's
-	 * being removed.
-	 */
-	if (!try_module_get(THIS_MODULE)) {
-		IWL_ERR(trans, "Module is being unloaded - abort\n");
-		return;
-	}
-
-	reprobe = kzalloc_obj(*reprobe);
-	if (!reprobe) {
-		module_put(THIS_MODULE);
-		return;
-	}
-	reprobe->dev = get_device(trans->dev);
-	INIT_DELAYED_WORK(&reprobe->work, iwl_trans_reprobe_wk);
-	schedule_delayed_work(&reprobe->work, msecs_to_jiffies(delay_ms));
+	if (device_schedule_reprobe(trans->dev, delay_ms))
+		IWL_ERR(trans, "Could not schedule reprobe\n");
 }
 
 #define IWL_TRANS_RESET_OK_TIME	7 /* seconds */
-- 
2.55.0

[PATCH v3 3/4] Bluetooth: hci_h5: use device_schedule_reprobe()

[Daniel Golle]

h5_btrtl_resume() open-codes a deferred re-probe for RTL devices that
lose their firmware state over suspend: it takes a module reference,
allocates a work item, and the work function calls device_reprobe()
and then ends with put_device(); kfree(); module_put(THIS_MODULE); in
module text. That final module_put() is racy: once the reference
count is decremented a concurrent rmmod can free the module text
before the work function's epilogue has finished executing. The work
also does not synchronize against shutdown or unbind, so a stale
re-probe could undo an administrative unbind or detach a device whose
->shutdown() callback has already run.

Convert to the new device_schedule_reprobe() helper, whose work
function is builtin text and which skips the re-probe when the device
was removed, shutdown reached it, or it is no longer bound to the
driver that scheduled the re-probe.

The old worker suppressed its error message for -EPROBE_DEFER; the
helper needs no equivalent because its attach half is
device_attach(), which folds probe deferral into the deferred-probe
machinery silently.

Behavioral changes:

- A pending re-probe no longer pins the module: rmmod with a re-probe
  pending now succeeds immediately and the re-probe becomes a no-op,
  instead of rmmod failing with EBUSY.

- A re-probe scheduled before a system shutdown or before an
  administrative unbind no longer detaches and rebinds the device
  afterwards.

- A re-probe racing the next suspend now detaches immediately while
  the probe is deferred until the following resume by the
  defer_all_probes machinery, instead of probing mid-suspend.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
Tested-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
---
v3: picked up Hans de Goede's Tested-by/Reviewed-by
v2: also correct the stale device_reprobe() reference in the
    h5_btrtl_open() comment
v1: initial RFC

 drivers/bluetooth/hci_h5.c | 43 ++++++--------------------------------
 1 file changed, 6 insertions(+), 37 deletions(-)

diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
index b1999e14aade..3fde1d5a5ae9 100644
--- a/drivers/bluetooth/hci_h5.c
+++ b/drivers/bluetooth/hci_h5.c
@@ -990,7 +990,7 @@ static int h5_btrtl_setup(struct h5 *h5)
 static void h5_btrtl_open(struct h5 *h5)
 {
 	/*
-	 * Since h5_btrtl_resume() does a device_reprobe() the suspend handling
+	 * Since h5_btrtl_resume() schedules a device re-probe the suspend handling
 	 * done by the hci_suspend_notifier is not necessary; it actually causes
 	 * delays and a bunch of errors to get logged, so disable it.
 	 */
@@ -1049,46 +1049,15 @@ static int h5_btrtl_suspend(struct h5 *h5)
 	return 0;
 }
 
-struct h5_btrtl_reprobe {
-	struct device *dev;
-	struct work_struct work;
-};
-
-static void h5_btrtl_reprobe_worker(struct work_struct *work)
-{
-	struct h5_btrtl_reprobe *reprobe =
-		container_of(work, struct h5_btrtl_reprobe, work);
-	int ret;
-
-	ret = device_reprobe(reprobe->dev);
-	if (ret && ret != -EPROBE_DEFER)
-		dev_err(reprobe->dev, "Reprobe error %d\n", ret);
-
-	put_device(reprobe->dev);
-	kfree(reprobe);
-	module_put(THIS_MODULE);
-}
-
 static int h5_btrtl_resume(struct h5 *h5)
 {
-	if (test_bit(H5_WAKEUP_DISABLE, &h5->flags)) {
-		struct h5_btrtl_reprobe *reprobe;
-
-		reprobe = kzalloc_obj(*reprobe);
-		if (!reprobe)
-			return -ENOMEM;
-
-		__module_get(THIS_MODULE);
+	if (test_bit(H5_WAKEUP_DISABLE, &h5->flags))
+		return device_schedule_reprobe(&h5->hu->serdev->dev, 0);
 
-		INIT_WORK(&reprobe->work, h5_btrtl_reprobe_worker);
-		reprobe->dev = get_device(&h5->hu->serdev->dev);
-		queue_work(system_long_wq, &reprobe->work);
-	} else {
-		gpiod_set_value_cansleep(h5->device_wake_gpio, 1);
+	gpiod_set_value_cansleep(h5->device_wake_gpio, 1);
 
-		if (test_bit(H5_HW_FLOW_CONTROL, &h5->flags))
-			serdev_device_set_flow_control(h5->hu->serdev, true);
-	}
+	if (test_bit(H5_HW_FLOW_CONTROL, &h5->flags))
+		serdev_device_set_flow_control(h5->hu->serdev, true);
 
 	return 0;
 }
-- 
2.55.0

[PATCH v3 4/4] Bluetooth: btintel_pcie: use device_schedule_reprobe() after reset

[Daniel Golle]

btintel_pcie re-probes its own device synchronously from its own reset
work via device_reprobe(), both after an FLR and after an ACPI PLDR.
The synchronous call runs .remove() from inside the reset work, which
requires a hand-rolled correctness contract spanning several comments:
.remove() must skip draining the very reset work it is running from
(the current_work() check), the reset must use pci_try_reset_function()
to dodge a device_lock ABBA against .remove(), and both reset paths
must not touch 'data' after the reprobe call because .remove() has
freed it.

Convert the two BT self re-probes to device_schedule_reprobe(). The
driver core's builtin work item now triggers .remove(), never the
reset work itself, so the current_work() check in .remove() is dead
and is removed: disable_work_sync(&data->reset_work) now also
guarantees the reset work has fully returned before 'data' is freed.
The Wi-Fi sibling re-probe in the PLDR path is left untouched; the
sibling is a different device re-probed from a bounded context and has
neither of the bug classes the helper addresses.

Safety of the window between the reset work returning and the deferred
detach running, during which 'data' now stays alive:

- hci callbacks: send_frame fails with -ENODEV while
  BTINTEL_PCIE_RECOVERY_IN_PROGRESS is set, and that bit is only
  cleared by a fresh probe. New reset requests coalesce into the
  in-flight one via the same bit. open/close are no-ops.

- interrupts: the reset work masks all interrupt causes and
  synchronizes the IRQs before the reset, the dump workers stay
  disabled (disable count >= 1) until .remove(), rx_work is flushed,
  and the freshly reset device raises no traffic until the next probe
  re-initialises it. The same exposure already exists today in the
  window between pci_try_reset_function() and the synchronous
  re-probe; the conversion only lengthens it.

- work disable counts: the success-path contract is unchanged in
  substance. The reset work's disable_work_sync() calls stay
  unbalanced on success, .remove() disables again, and the fresh
  probe re-INIT_WORKs the dump workers with disable count 0.

pci_lock_rescan_remove() now only covers the reset itself, no longer
the re-probe. Hot-removal between the reset and the deferred re-probe
is handled by the helper's dead-device check.

If scheduling the re-probe fails in the FLR path, the error is
returned so the reset work re-enables the dump workers, matching the
existing FLR failure handling; unlike before, 'data' is still alive
in that case. The PLDR path keeps logging only, as before.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v3: rebased onto bluetooth-next, whose btintel_pcie has four dump
    workers (coredump/hwexp/fwtrigger/mbox) rather than the single one
    the mainline snapshot v2 targeted
v2: reconciled to the current tree and adapted the commit message
v1: initial RFC

 drivers/bluetooth/btintel_pcie.c | 49 +++++++++++++++++---------------
 1 file changed, 26 insertions(+), 23 deletions(-)

diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
index baa621b3fef9..29be05d41429 100644
--- a/drivers/bluetooth/btintel_pcie.c
+++ b/drivers/bluetooth/btintel_pcie.c
@@ -3012,7 +3012,7 @@ static void btintel_pcie_perform_pldr(struct btintel_pcie_data *data)
 	 * BT needs pci_save_state()/pci_restore_state() because the BT driver
 	 * is still partially attached when the _PRR runs (it hasn't been unbound yet).
 	 * The PCI device needs to remain minimally functional so that
-	 * device_reprobe(&pdev->dev) can work afterward
+	 * the deferred re-probe of the BT device can work afterward
 	 */
 	ret = btintel_pcie_acpi_reset_method(data);
 
@@ -3023,14 +3023,16 @@ static void btintel_pcie_perform_pldr(struct btintel_pcie_data *data)
 	}
 
 	if (!ret) {
-		if (device_reprobe(&pdev->dev))
-			BT_ERR("BT reprobe failed for BDF:%s", pci_name(pdev));
+		if (device_schedule_reprobe(&pdev->dev, 0))
+			BT_ERR("BT reprobe scheduling failed for BDF:%s",
+			       pci_name(pdev));
 	}
 }
 
 /*
- * Issue a Function Level Reset and hand teardown/re-init off to the PCI
- * core via device_reprobe(), mirroring the PLDR path's contract.
+ * Issue a Function Level Reset and hand teardown/re-init off to the
+ * driver core via device_schedule_reprobe(), mirroring the PLDR path's
+ * contract.
  *
  * Caller must hold pci_lock_rescan_remove() and must have already
  * disabled interrupts and drained both rx_work and coredump_work.
@@ -3052,14 +3054,12 @@ static int btintel_pcie_perform_flr(struct btintel_pcie_data *data)
 		return err;
 	}
 
-	/* device_reprobe() always detaches the driver first (running
-	 * .remove(), which frees 'data'); any re-probe failure leaves the
-	 * device unbound but 'data' is already gone, so just log it.
-	 */
-	if (device_reprobe(&pdev->dev))
-		BT_ERR("BT reprobe failed for BDF:%s", pci_name(pdev));
+	err = device_schedule_reprobe(&pdev->dev, 0);
+	if (err)
+		BT_ERR("BT reprobe scheduling failed for BDF:%s",
+		       pci_name(pdev));
 
-	return 0;
+	return err;
 }
 
 static void btintel_pcie_reset_work(struct work_struct *wk)
@@ -3090,11 +3090,15 @@ static void btintel_pcie_reset_work(struct work_struct *wk)
 
 	bt_dev_dbg(data->hdev, "Release bluetooth interface");
 
-	/* Both reset paths follow the same contract: on success they
-	 * destroy 'data' via device_reprobe() (a fresh probe re-INIT_WORKs
-	 * the dump workers with disable count 0), so enable_work() must
-	 * NOT be called on the success path. Only the FLR path can fail
-	 * with 'data' still alive, in which case we balance the
+	/* Both reset paths follow the same contract: on success the
+	 * deferred re-probe scheduled with device_schedule_reprobe()
+	 * destroys 'data' by re-running .probe() (which re-INIT_WORKs the
+	 * dump workers with disable count 0), so enable_work() must NOT be
+	 * called on the success path. 'data' stays alive until the deferred
+	 * detach runs; in this window new activity is fenced by
+	 * BTINTEL_PCIE_RECOVERY_IN_PROGRESS, the masked interrupts and the
+	 * disabled dump workers. Only the FLR path can fail with no
+	 * re-probe scheduled, in which case we balance the
 	 * disable_work_sync() calls above so a later successful reset is
 	 * not permanently blocked.
 	 *
@@ -3460,13 +3464,12 @@ static void btintel_pcie_remove(struct pci_dev *pdev)
 	disable_work_sync(&data->fwtrigger_work);
 	disable_work_sync(&data->mbox_work);
 
-	/* Cancel pending reset work. Skip only when remove() is called from
-	 * within the reset work itself (PLDR device_reprobe path) to avoid
-	 * deadlock. current_work() returns the work_struct of the caller if
-	 * we are in a workqueue context.
+	/* The deferred re-probe triggers .remove() from the driver core's
+	 * work item, never from reset_work itself, so this no longer runs
+	 * nested in reset_work; disable_work_sync() also guarantees the
+	 * reset work has fully returned before 'data' is freed.
 	 */
-	if (current_work() != &data->reset_work)
-		disable_work_sync(&data->reset_work);
+	disable_work_sync(&data->reset_work);
 
 	btintel_pcie_disable_interrupts(data);
 
-- 
2.55.0

[PATCH v4 1/4] driver core: add device_schedule_reprobe()

[Daniel Golle]

iwlwifi and hci_h5 re-probe their own device from a work item in module
text that ends with module_put(THIS_MODULE). Once the count drops, a
concurrent rmmod can free the module while the work still runs: the
text of its epilogue and, with lockdep, the work's lock class key. Nor
is the re-probe tied to the binding that scheduled it, which a driver
cannot check from outside device_reprobe().

Add device_schedule_reprobe(), a delayed detach and re-probe run from
built-in work. One request per device is kept in struct device_private
and cancelled by device_unbind_cleanup(), so it ends with the binding
that scheduled it, by an unbind, a removal or a failed probe. The
caller is the bound driver, in a context its ->remove() waits for; no
lock is taken there, so the device lock may be held. Buses that take
the parent lock to bind are refused, as the parent can change before
the work runs.

Assisted-by: LLM
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v4: reworked as patch 3/6 of the mxl862xx devlink flash series, v13 to
    v19 [1][2], mostly after Sashiko AI review there: one request per
    device, cancelled with the binding, replaces the driver pointer
    check and shutdown_done; parent-lock buses are refused, the parent
    is not pinned; no lock in the caller's context; freezable work; no
    error message of its own; Hans de Goede's tags dropped
    [1] https://lore.kernel.org/all/cover.1788783126.git.daniel@makrotopia.org/
    [2] https://lore.kernel.org/all/cover.1791035885.git.daniel@makrotopia.org/
v3: use dev_err_probe() for the re-probe error path, so a re-probe
    deferred at resume no longer logs a spurious error (Hans de Goede)
v2: pin the parent across the deferred work and re-lock it across
    device_attach() on need_parent_lock buses; schedule on system_dfl_wq
    rather than the deprecated system_unbound_wq (found reviewing the
    mxl862xx v12 posting)
v1: initial RFC

---
 drivers/base/base.h    |   4 +
 drivers/base/dd.c      | 171 +++++++++++++++++++++++++++++++++++++++++
 include/linux/device.h |   2 +
 3 files changed, 177 insertions(+)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index a5b7abc10ff02..3a6883de7cef3 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -106,6 +106,9 @@ struct driver_private {
  * @dead: This device is currently either in the process of or has been
  *	  removed from the system. Any asynchronous events scheduled for this
  *	  device should exit without taking any action.
+ * @reprobe: request scheduled with device_schedule_reprobe(), held until
+ *	     its work has released the driver; cancelled when the binding
+ *	     that scheduled it ends
  *
  * Nothing outside of the driver core should ever touch these fields.
  */
@@ -119,6 +122,7 @@ struct device_private {
 	const struct device_driver *async_driver;
 	char *deferred_probe_reason;
 	struct device *device;
+	struct device_reprobe *reprobe;
 	u8 dead:1;
 };
 #define to_device_private_parent(obj)	\
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index f6525a7ee8c54..1ec740f0e4816 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -590,6 +590,40 @@ static ssize_t state_synced_show(struct device *dev,
 }
 static DEVICE_ATTR_RW(state_synced);
 
+struct device_reprobe {
+	struct delayed_work work;
+	struct device *dev;
+};
+
+/* Ties the slot in struct device_private to the queueing of its work. */
+static DEFINE_SPINLOCK(device_reprobe_lock);
+
+static void device_reprobe_free(struct device_reprobe *rp)
+{
+	put_device(rp->dev);
+	kfree(rp);
+}
+
+/*
+ * The slot holds a request from its scheduling until its work has released
+ * the driver. A pending work is freed here; a running one finds the slot no
+ * longer its own and frees itself.
+ */
+static void device_reprobe_cancel(struct device *dev)
+{
+	struct device_reprobe *rp;
+	bool pending = false;
+
+	spin_lock(&device_reprobe_lock);
+	rp = dev->p->reprobe;
+	WRITE_ONCE(dev->p->reprobe, NULL);
+	if (rp)
+		pending = cancel_delayed_work(&rp->work);
+	spin_unlock(&device_reprobe_lock);
+	if (pending)
+		device_reprobe_free(rp);
+}
+
 static void device_unbind_cleanup(struct device *dev)
 {
 	if (dev->driver->p_cb.post_unbind_rust)
@@ -599,6 +633,7 @@ static void device_unbind_cleanup(struct device *dev)
 	kfree(dev->dma_range_map);
 	dev->dma_range_map = NULL;
 	device_set_driver(dev, NULL);
+	device_reprobe_cancel(dev);
 	dev_set_drvdata(dev, NULL);
 	dev_pm_domain_detach(dev, dev->power.detach_power_off);
 	if (dev->pm_domain && dev->pm_domain->dismiss)
@@ -1436,3 +1471,139 @@ void driver_detach(const struct device_driver *drv)
 		put_device(dev);
 	}
 }
+
+/* Retry interval while probing is blocked; the caller's delay may be 0. */
+#define DEVICE_REPROBE_BLOCKED_RETRY	HZ
+
+static void device_reprobe_work_fn(struct work_struct *work)
+{
+	struct device_reprobe *rp = container_of(work, struct device_reprobe,
+						 work.work);
+	struct device *dev = rp->dev;
+	bool rearm;
+	int ret;
+
+	device_lock(dev);
+	/* The slot still holding rp means the binding that scheduled it does. */
+	if (dev->p->dead || READ_ONCE(dev->p->reprobe) != rp) {
+		device_unlock(dev);
+		goto out;
+	}
+	/* A stale read means one more retry or a probe that defers itself. */
+	if (data_race(defer_all_probes)) {
+		device_unlock(dev);
+		spin_lock(&device_reprobe_lock);
+		rearm = dev->p->reprobe == rp &&
+			(system_state < SYSTEM_HALT ||
+			 system_state == SYSTEM_SUSPEND) &&
+			queue_delayed_work(system_freezable_wq, &rp->work,
+					   DEVICE_REPROBE_BLOCKED_RETRY);
+		spin_unlock(&device_reprobe_lock);
+		if (rearm)
+			return;
+		goto out;
+	}
+	/* Releasing the driver clears the slot, which was this request. */
+	__device_release_driver(dev, NULL);
+	device_unlock(dev);
+
+	/* A failed probe is logged by the probe path, as for a first probe. */
+	ret = device_attach(dev);
+	dev_dbg(dev, "re-probe: device_attach() returned %d\n", ret);
+out:
+	spin_lock(&device_reprobe_lock);
+	if (dev->p->reprobe == rp)
+		WRITE_ONCE(dev->p->reprobe, NULL);
+	spin_unlock(&device_reprobe_lock);
+	device_reprobe_free(rp);
+}
+
+/**
+ * device_schedule_reprobe - schedule a deferred detach and re-probe
+ * @dev: device to detach and re-probe
+ * @delay_ms: delay in milliseconds before the re-probe runs
+ *
+ * Schedule a detach and re-probe of @dev after @delay_ms milliseconds,
+ * from built-in driver-core work rather than a driver-owned work item,
+ * so the bound driver may call it without pinning its own module.
+ *
+ * At most one request exists per device, from its scheduling until its
+ * work has released the driver, and the request identifies the binding
+ * that scheduled it. It is cancelled when that binding ends, whether by
+ * an unbind, by the deletion of @dev or by a failed probe, so neither an
+ * unbind followed by a rebind nor a module reload within the delay gets a
+ * re-probe it did not ask for; its reference on @dev is dropped with it,
+ * or once a work already running has finished. A request that fires
+ * while probing is blocked for a system suspend or a hibernation restore
+ * re-arms itself every second and runs once the system has resumed; one
+ * that fires while probing is blocked for a halt, power-off or restart is
+ * dropped. A failed re-probe leaves @dev unbound, as a failed initial
+ * probe would, and is logged by the probe path like one.
+ *
+ * This is device_reprobe() deferred, and shares its limitations; the
+ * release path of __device_release_driver() is untouched. The detach and
+ * the re-attach are not one locked operation, so an administrative unbind
+ * arriving between them may be undone. If @dev has managed consumers,
+ * detaching it unbinds them as any driver release does, so a re-probe a
+ * concurrent device_shutdown() overtakes may run ->remove() after
+ * ->shutdown(). None of this is specific to this helper.
+ *
+ * Buses that take the parent lock to bind (only usb_bus_type) are refused
+ * with -EINVAL: the parent would have to be recorded before either lock
+ * is held, where device_move() can replace it.
+ *
+ * Context: May sleep (allocates with %GFP_KERNEL). Must be called by the
+ * driver bound to @dev, from a process context its ->remove() waits for,
+ * so that the binding outlives the call; @dev's own device lock may be
+ * held. A request from ->probe() runs once the probe has returned and is
+ * cancelled if the probe fails.
+ *
+ * Returns: 0 on success, -EINVAL if @dev is not a registered device
+ * bound to a driver or sits on a bus which takes the parent lock to
+ * bind, -EBUSY if a re-probe of @dev is pending or has not yet released
+ * the driver, -ENOMEM on allocation failure.
+ */
+int device_schedule_reprobe(struct device *dev, unsigned int delay_ms)
+{
+	const struct device_driver *drv;
+	struct device_reprobe *rp;
+	int ret = 0;
+
+	drv = READ_ONCE(dev->driver);
+	/*
+	 * A bus taking the parent lock would need @dev's parent pinned until
+	 * the work runs, which device_move() can invalidate.
+	 */
+	if (!drv || !dev->bus || dev->bus->need_parent_lock || !dev->p ||
+	    !device_is_registered(dev))
+		return -EINVAL;
+
+	rp = kzalloc_obj(*rp);
+	if (!rp)
+		return -ENOMEM;
+
+	rp->dev = get_device(dev);
+	INIT_DELAYED_WORK(&rp->work, device_reprobe_work_fn);
+
+	/*
+	 * A release that got here first has cleared the driver before
+	 * cancelling, so a request published after its cancel is refused
+	 * here; one published before is cancelled by it.
+	 */
+	spin_lock(&device_reprobe_lock);
+	if (dev->p->reprobe)
+		ret = -EBUSY;
+	else if (READ_ONCE(dev->driver) != drv)
+		ret = -EINVAL;
+	else
+		WRITE_ONCE(dev->p->reprobe, rp);
+	if (!ret)
+		queue_delayed_work(system_freezable_wq, &rp->work,
+				   msecs_to_jiffies(delay_ms));
+	spin_unlock(&device_reprobe_lock);
+	if (ret)
+		device_reprobe_free(rp);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(device_schedule_reprobe);
diff --git a/include/linux/device.h b/include/linux/device.h
index aee79fd6b32b4..7a99169505772 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1314,6 +1314,8 @@ int  __must_check device_attach(struct device *dev);
 int __must_check driver_attach(const struct device_driver *drv);
 void device_initial_probe(struct device *dev);
 int __must_check device_reprobe(struct device *dev);
+int __must_check device_schedule_reprobe(struct device *dev,
+					 unsigned int delay_ms);
 
 bool device_is_bound(struct device *dev);
 
-- 
2.56.0

RE: device_schedule_reprobe(): core helper and conversions

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1833 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/series/1187879/

---Test result---

Test Summary:
CheckPatch                    PASS      3.81 seconds
VerifyFixes                   PASS      0.12 seconds
VerifySignedoff               PASS      0.12 seconds
GitLint                       PASS      1.29 seconds
SubjectPrefix                 FAIL      2.34 seconds
BuildKernel                   PASS      28.99 seconds
CheckAllWarning               PASS      33.40 seconds
CheckSparse                   PASS      36.18 seconds
BuildKernel32                 PASS      29.23 seconds
CheckKernelLLVM               PASS      34.71 seconds
TestRunnerSetup               PASS      700.37 seconds
TestRunner_l2cap-tester       PASS      12.88 seconds
TestRunner_iso-tester         PASS      10.82 seconds
TestRunner_bnep-tester        PASS      3.12 seconds
TestRunner_mgmt-tester        PASS      87.78 seconds
TestRunner_rfcomm-tester      PASS      4.49 seconds
TestRunner_sco-tester         PASS      5.64 seconds
TestRunner_ioctl-tester       PASS      4.68 seconds
TestRunner_mesh-tester        PASS      5.54 seconds
TestRunner_smp-tester         PASS      4.32 seconds
TestRunner_userchan-tester    PASS      3.21 seconds
TestRunner_6lowpan-tester     PASS      4.39 seconds
IncrementalBuild              PASS      32.84 seconds

Details
##############################
Test: SubjectPrefix - FAIL
Desc: Check subject contains "Bluetooth" prefix
Output:
"Bluetooth: " prefix is not specified in the subject
"Bluetooth: " prefix is not specified in the subject


https://github.com/bluez/bluetooth-next/pull/960

---
Regards,
Linux Bluetooth

[PATCH v2 1/4] driver core: add device_schedule_reprobe()

[Daniel Golle]

Three in-tree drivers schedule a deferred re-probe of their own device
from a work item whose work function lives in module text: iwlwifi
(iwl_trans_schedule_reprobe(), firmware crash recovery when a lighter
restart is not sufficient), hci_h5 (h5_btrtl_resume(), RTL devices
lose their firmware state over suspend) and btintel_pcie (synchronous
device_reprobe() from its own reset work, with a hand-rolled locking
contract spanning several comments).

Two bug classes affect the hand-rolled implementations:

1. The work function ends with put_device(); kfree();
   module_put(THIS_MODULE); in module text. After the atomic decrement
   a concurrent rmmod can free the module text before the function
   epilogue has finished executing. This is exactly the race
   module_put_and_kthread_exit() exists to close for kthreads; there
   is no work-item equivalent.

2. There is no synchronization between the deferred device_reprobe()
   and device_shutdown() or a driver unbind. The drivers do not check
   any bound state before calling device_reprobe(), so a stale
   re-probe can undo an administrative unbind, and the detach half can
   run against a device whose ->shutdown() callback has already run.
   The core already blocks the attach half during shutdown
   (device_shutdown() calls device_block_probing() before any
   callback, and really_probe() honors defer_all_probes), but nothing
   blocks the detach half. For drivers which clear their drvdata in
   ->shutdown() so that a subsequent ->remove() becomes a no-op this
   escalates to use-after-free of driver state which other subsystem
   structures still reference.

Both classes disappear when the driver core owns the deferred work.
Add device_schedule_reprobe(), which schedules a detach and re-probe
of a device after a caller-specified delay:

- The work function is builtin text, so callers do not need to hold a
  module reference. If the driver module is unloaded before the work
  runs, driver_unregister() has already unbound the device, the bound
  driver no longer matches the driver recorded at scheduling time and
  the work does nothing.

- The recorded driver pointer is only ever compared, never
  dereferenced, so it may legitimately point to freed memory.

- The bound-state check and __device_release_driver() run under a
  single __device_driver_lock() hold, the same lock dance
  device_release_driver_internal() uses. This closes the
  check-vs-detach TOCTOU that drivers cannot close themselves,
  because device_reprobe() takes the device lock internally.

- Both @dev and its parent are pinned for the lifetime of the work.
  __device_driver_lock() and the attach half lock the parent, and an
  unregister of @dev drops @dev's reference to the parent, so without
  a reference of our own the parent could be freed before the work
  runs.

- A new shutdown_done flag in struct device_private, set under the
  device lock once device_shutdown() reaches a device, suppresses the
  detach half during shutdown. It occupies a spare bit in an existing
  byte, mirroring how kill_device() sets the dead flag.

- The attach half is plain device_attach(), which already honors both
  the dead flag and defer_all_probes: a re-probe landing during
  system suspend detaches immediately and the probe is deferred until
  device_restore_probing() at resume time. The detach half
  deliberately does not check defer_all_probes so that a re-probe
  scheduled before suspend is not silently dropped. The parent is
  re-locked across device_attach() on buses that set
  need_parent_lock, mirroring bus_rescan_devices_helper().

One pre-existing window remains: __device_release_driver()
transiently drops the locks while consumer device links are busy, so
for devices with busy consumers a ->shutdown() can still interleave
in the middle of the release. That window exists identically for
every unbind path in the kernel, sysfs unbind included, and is not
made worse by this helper.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/base/base.h    |  5 +++
 drivers/base/core.c    |  3 ++
 drivers/base/dd.c      | 99 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/device.h |  2 +
 4 files changed, 109 insertions(+)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index a5b7abc10ff0..6234e37de7e9 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -106,6 +106,10 @@ struct driver_private {
  * @dead: This device is currently either in the process of or has been
  *	  removed from the system. Any asynchronous events scheduled for this
  *	  device should exit without taking any action.
+ * @shutdown_done: Set once device_shutdown() has reached this device, under
+ *	  the device lock, before any shutdown callback runs. Read under the
+ *	  device lock. A deferred re-probe scheduled with
+ *	  device_schedule_reprobe() must not detach the device anymore.
  *
  * Nothing outside of the driver core should ever touch these fields.
  */
@@ -120,6 +124,7 @@ struct device_private {
 	char *deferred_probe_reason;
 	struct device *device;
 	u8 dead:1;
+	u8 shutdown_done:1;
 };
 #define to_device_private_parent(obj)	\
 	container_of(obj, struct device_private, knode_parent)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 4d026682944f..8a7dbe4e8362 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -4906,6 +4906,9 @@ void device_shutdown(void)
 			device_lock(parent);
 		device_lock(dev);
 
+		if (dev->p)
+			dev->p->shutdown_done = true;
+
 		/* Don't allow any more runtime suspends */
 		pm_runtime_get_noresume(dev);
 		pm_runtime_barrier(dev);
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 60c005223844..3394b1c7ed18 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -1436,3 +1436,102 @@ void driver_detach(const struct device_driver *drv)
 		put_device(dev);
 	}
 }
+
+struct device_reprobe {
+	struct delayed_work work;
+	struct device *dev;
+	struct device *parent;
+	const struct device_driver *drv;
+};
+
+static void device_reprobe_work_fn(struct work_struct *work)
+{
+	struct device_reprobe *rp = container_of(work, struct device_reprobe,
+						 work.work);
+	struct device *dev = rp->dev;
+	struct device *parent = rp->parent;
+	bool detached = false;
+
+	__device_driver_lock(dev, parent);
+	/*
+	 * rp->drv is only ever compared, never dereferenced: the driver it
+	 * points to may have been unregistered and freed by now.
+	 */
+	if (!dev->p->dead && !dev->p->shutdown_done &&
+	    dev->driver && dev->driver == rp->drv) {
+		__device_release_driver(dev, parent);
+		detached = true;
+	}
+	__device_driver_unlock(dev, parent);
+
+	if (detached) {
+		/*
+		 * device_attach() must run with the parent locked on buses
+		 * that require it, mirroring bus_rescan_devices_helper().
+		 */
+		if (parent && dev->bus->need_parent_lock)
+			device_lock(parent);
+		if (device_attach(dev) < 0)
+			dev_err(dev, "re-probe failed, device left unbound\n");
+		if (parent && dev->bus->need_parent_lock)
+			device_unlock(parent);
+	}
+
+	put_device(dev);
+	put_device(parent);
+	kfree(rp);
+}
+
+/**
+ * device_schedule_reprobe - schedule a deferred detach and re-probe
+ * @dev: device to detach and re-probe
+ * @delay_ms: delay in milliseconds before the re-probe runs
+ *
+ * Schedule a detach and re-probe of @dev after @delay_ms milliseconds.
+ * The re-probe is skipped if, by the time the scheduled work runs, the
+ * device has been removed, the system shutdown sequence has reached the
+ * device, or @dev is no longer bound to the driver that was bound at
+ * scheduling time. In particular an administrative unbind is never
+ * undone by a stale re-probe.
+ *
+ * The work function is built-in text, so the bound driver may call this
+ * from its own code without holding a module reference. If the driver
+ * module is unloaded before the work runs, driver unregistration unbinds
+ * @dev first and the scheduled work does nothing.
+ *
+ * Multiple pending re-probes for the same device are individually safe;
+ * a caller that wants at most one pending re-probe must gate scheduling
+ * itself.
+ *
+ * May only be called from process context.
+ *
+ * Returns: 0 on success, -EINVAL if @dev is not a registered device
+ * bound to a driver, -ENOMEM on allocation failure.
+ */
+int device_schedule_reprobe(struct device *dev, unsigned int delay_ms)
+{
+	struct device_reprobe *rp;
+
+	if (!dev->bus || !dev->p || !device_is_registered(dev))
+		return -EINVAL;
+	if (!dev->driver)
+		return -EINVAL;
+
+	rp = kzalloc_obj(*rp);
+	if (!rp)
+		return -ENOMEM;
+
+	rp->dev = get_device(dev);
+	/*
+	 * Pin the parent too: the work locks it, and an unregister of @dev
+	 * would otherwise drop the last reference before the work runs.
+	 */
+	rp->parent = get_device(dev->parent);
+	rp->drv = READ_ONCE(dev->driver);
+	INIT_DELAYED_WORK(&rp->work, device_reprobe_work_fn);
+	queue_delayed_work(system_dfl_wq, &rp->work,
+			   msecs_to_jiffies(delay_ms));
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(device_schedule_reprobe);
diff --git a/include/linux/device.h b/include/linux/device.h
index aee79fd6b32b..7a9916950577 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1314,6 +1314,8 @@ int  __must_check device_attach(struct device *dev);
 int __must_check driver_attach(const struct device_driver *drv);
 void device_initial_probe(struct device *dev);
 int __must_check device_reprobe(struct device *dev);
+int __must_check device_schedule_reprobe(struct device *dev,
+					 unsigned int delay_ms);
 
 bool device_is_bound(struct device *dev);
 
-- 
2.55.0

RE: device_schedule_reprobe(): core helper and conversions

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 567 bytes --]

This is an automated email and please do not reply to this email.

Dear Submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
While preparing the CI tests, the patches you submitted couldn't be applied to the current HEAD of the repository.

----- Output -----

error: patch failed: drivers/bluetooth/btintel_pcie.c:2657
error: drivers/bluetooth/btintel_pcie.c: patch does not apply
hint: Use 'git am --show-current-patch' to see the failed patch

Please resolve the issue and submit the patches again.


---
Regards,
Linux Bluetooth

[RFC PATCH 1/4] driver core: add device_schedule_reprobe()

[Daniel Golle]

Three in-tree drivers schedule a deferred re-probe of their own device
from a work item whose work function lives in module text: iwlwifi
(iwl_trans_schedule_reprobe(), firmware crash recovery when a lighter
restart is not sufficient), hci_h5 (h5_btrtl_resume(), RTL devices
lose their firmware state over suspend) and btintel_pcie (synchronous
device_reprobe() from its own reset work, with a hand-rolled locking
contract spanning several comments).

Two bug classes affect the hand-rolled implementations:

1. The work function ends with put_device(); kfree();
   module_put(THIS_MODULE); in module text. After the atomic decrement
   a concurrent rmmod can free the module text before the function
   epilogue has finished executing. This is exactly the race
   module_put_and_kthread_exit() exists to close for kthreads; there
   is no work-item equivalent.

2. There is no synchronization between the deferred device_reprobe()
   and device_shutdown() or a driver unbind. The drivers do not check
   any bound state before calling device_reprobe(), so a stale
   re-probe can undo an administrative unbind, and the detach half can
   run against a device whose ->shutdown() callback has already run.
   The core already blocks the attach half during shutdown
   (device_shutdown() calls device_block_probing() before any
   callback, and really_probe() honors defer_all_probes), but nothing
   blocks the detach half. For drivers which clear their drvdata in
   ->shutdown() so that a subsequent ->remove() becomes a no-op this
   escalates to use-after-free of driver state which other subsystem
   structures still reference.

Both classes disappear when the driver core owns the deferred work.
Add device_schedule_reprobe(), which schedules a detach and re-probe
of a device after a caller-specified delay:

- The work function is builtin text, so callers do not need to hold a
  module reference. If the driver module is unloaded before the work
  runs, driver_unregister() has already unbound the device, the bound
  driver no longer matches the driver recorded at scheduling time and
  the work does nothing.

- The recorded driver pointer is only ever compared, never
  dereferenced, so it may legitimately point to freed memory.

- The bound-state check and __device_release_driver() run under a
  single __device_driver_lock() hold, the same lock dance
  device_release_driver_internal() uses. This closes the
  check-vs-detach TOCTOU that drivers cannot close themselves,
  because device_reprobe() takes the device lock internally.

- A new shutdown_done flag in struct device_private, set under the
  device lock once device_shutdown() reaches a device, suppresses the
  detach half during shutdown. It occupies a spare bit in an existing
  byte, mirroring how kill_device() sets the dead flag.

- The attach half is plain device_attach(), which already honors both
  the dead flag and defer_all_probes: a re-probe landing during
  system suspend detaches immediately and the probe is deferred until
  device_restore_probing() at resume time. The detach half
  deliberately does not check defer_all_probes so that a re-probe
  scheduled before suspend is not silently dropped.

One pre-existing window remains: __device_release_driver()
transiently drops the locks while consumer device links are busy, so
for devices with busy consumers a ->shutdown() can still interleave
in the middle of the release. That window exists identically for
every unbind path in the kernel, sysfs unbind included, and is not
made worse by this helper.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/base/base.h    |  5 +++
 drivers/base/core.c    |  3 ++
 drivers/base/dd.c      | 82 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/device.h |  2 ++
 4 files changed, 92 insertions(+)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index a5b7abc10ff0..6234e37de7e9 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -106,6 +106,10 @@ struct driver_private {
  * @dead: This device is currently either in the process of or has been
  *	  removed from the system. Any asynchronous events scheduled for this
  *	  device should exit without taking any action.
+ * @shutdown_done: Set once device_shutdown() has reached this device, under
+ *	  the device lock, before any shutdown callback runs. Read under the
+ *	  device lock. A deferred re-probe scheduled with
+ *	  device_schedule_reprobe() must not detach the device anymore.
  *
  * Nothing outside of the driver core should ever touch these fields.
  */
@@ -120,6 +124,7 @@ struct device_private {
 	char *deferred_probe_reason;
 	struct device *device;
 	u8 dead:1;
+	u8 shutdown_done:1;
 };
 #define to_device_private_parent(obj)	\
 	container_of(obj, struct device_private, knode_parent)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 4c0c373998a1..3dcd8a3c3aa5 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -4910,6 +4910,9 @@ void device_shutdown(void)
 			device_lock(parent);
 		device_lock(dev);
 
+		if (dev->p)
+			dev->p->shutdown_done = true;
+
 		/* Don't allow any more runtime suspends */
 		pm_runtime_get_noresume(dev);
 		pm_runtime_barrier(dev);
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index f6525a7ee8c5..fc23f5bebd20 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -1436,3 +1436,85 @@ void driver_detach(const struct device_driver *drv)
 		put_device(dev);
 	}
 }
+
+struct device_reprobe {
+	struct delayed_work work;
+	struct device *dev;
+	const struct device_driver *drv;
+};
+
+static void device_reprobe_work_fn(struct work_struct *work)
+{
+	struct device_reprobe *rp = container_of(work, struct device_reprobe,
+						 work.work);
+	struct device *dev = rp->dev;
+	struct device *parent = dev->parent;
+	bool detached = false;
+
+	__device_driver_lock(dev, parent);
+	/*
+	 * rp->drv is only ever compared, never dereferenced: the driver it
+	 * points to may have been unregistered and freed by now.
+	 */
+	if (!dev->p->dead && !dev->p->shutdown_done &&
+	    dev->driver && dev->driver == rp->drv) {
+		__device_release_driver(dev, parent);
+		detached = true;
+	}
+	__device_driver_unlock(dev, parent);
+
+	if (detached && device_attach(dev) < 0)
+		dev_err(dev, "re-probe failed, device left unbound\n");
+
+	put_device(dev);
+	kfree(rp);
+}
+
+/**
+ * device_schedule_reprobe - schedule a deferred detach and re-probe
+ * @dev: device to detach and re-probe
+ * @delay_ms: delay in milliseconds before the re-probe runs
+ *
+ * Schedule a detach and re-probe of @dev after @delay_ms milliseconds.
+ * The re-probe is skipped if, by the time the scheduled work runs, the
+ * device has been removed, the system shutdown sequence has reached the
+ * device, or @dev is no longer bound to the driver that was bound at
+ * scheduling time. In particular an administrative unbind is never
+ * undone by a stale re-probe.
+ *
+ * The work function is built-in text, so the bound driver may call this
+ * from its own code without holding a module reference. If the driver
+ * module is unloaded before the work runs, driver unregistration unbinds
+ * @dev first and the scheduled work does nothing.
+ *
+ * Multiple pending re-probes for the same device are individually safe;
+ * a caller that wants at most one pending re-probe must gate scheduling
+ * itself.
+ *
+ * May only be called from process context.
+ *
+ * Returns: 0 on success, -EINVAL if @dev is not a registered device
+ * bound to a driver, -ENOMEM on allocation failure.
+ */
+int device_schedule_reprobe(struct device *dev, unsigned int delay_ms)
+{
+	struct device_reprobe *rp;
+
+	if (!dev->bus || !dev->p || !device_is_registered(dev))
+		return -EINVAL;
+	if (!dev->driver)
+		return -EINVAL;
+
+	rp = kzalloc_obj(*rp);
+	if (!rp)
+		return -ENOMEM;
+
+	rp->dev = get_device(dev);
+	rp->drv = READ_ONCE(dev->driver);
+	INIT_DELAYED_WORK(&rp->work, device_reprobe_work_fn);
+	queue_delayed_work(system_unbound_wq, &rp->work,
+			   msecs_to_jiffies(delay_ms));
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(device_schedule_reprobe);
diff --git a/include/linux/device.h b/include/linux/device.h
index aee79fd6b32b..7a9916950577 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1314,6 +1314,8 @@ int  __must_check device_attach(struct device *dev);
 int __must_check driver_attach(const struct device_driver *drv);
 void device_initial_probe(struct device *dev);
 int __must_check device_reprobe(struct device *dev);
+int __must_check device_schedule_reprobe(struct device *dev,
+					 unsigned int delay_ms);
 
 bool device_is_bound(struct device *dev);
 
-- 
2.55.0

RE: device_schedule_reprobe(): core helper and conversions

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2609 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=1143724

---Test result---

Test Summary:
CheckPatch                    PASS      2.79 seconds
VerifyFixes                   PASS      0.07 seconds
VerifySignedoff               PASS      0.06 seconds
GitLint                       PASS      0.77 seconds
SubjectPrefix                 FAIL      0.25 seconds
BuildKernel                   PASS      27.53 seconds
CheckAllWarning               PASS      30.70 seconds
CheckSparse                   PASS      30.35 seconds
BuildKernel32                 PASS      27.17 seconds
CheckKernelLLVM               SKIP      0.00 seconds
TestRunnerSetup               PASS      502.65 seconds
TestRunner_l2cap-tester       PASS      63.14 seconds
TestRunner_iso-tester         PASS      85.31 seconds
TestRunner_bnep-tester        PASS      19.27 seconds
TestRunner_mgmt-tester        FAIL      219.15 seconds
TestRunner_rfcomm-tester      PASS      25.44 seconds
TestRunner_sco-tester         PASS      31.90 seconds
TestRunner_ioctl-tester       PASS      27.55 seconds
TestRunner_mesh-tester        FAIL      26.88 seconds
TestRunner_smp-tester         PASS      23.87 seconds
TestRunner_userchan-tester    PASS      20.01 seconds
TestRunner_6lowpan-tester     PASS      23.47 seconds
IncrementalBuild              PASS      30.37 seconds

Details
##############################
Test: SubjectPrefix - FAIL
Desc: Check subject contains "Bluetooth" prefix
Output:
"Bluetooth: " prefix is not specified in the subject
"Bluetooth: " prefix is not specified in the subject
##############################
Test: CheckKernelLLVM - SKIP
Desc: Build kernel with LLVM + context analysis
Output:
Clang not found
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 501, Passed: 496 (99.0%), Failed: 1, Not Run: 4

Failed Test Cases
Read Exp Feature - Success                           Failed       0.252 seconds
##############################
Test: TestRunner_mesh-tester - FAIL
Desc: Run mesh-tester with test-runner
Output:
Total: 10, Passed: 8 (80.0%), Failed: 2, Not Run: 0

Failed Test Cases
Mesh - Send cancel - 1                               Timed out    2.786 seconds
Mesh - Send cancel - 2                               Timed out    1.986 seconds


https://github.com/bluez/bluetooth-next/pull/567

---
Regards,
Linux Bluetooth