device_schedule_reprobe(): core helper and conversions

iwlwifi and hci_h5 re-probe their own device from a work item in module text ending in module_put(THIS_MODULE), which a concurrent rmmod can race, and nothing ties the re-probe to the binding that asked for it. btintel_pcie re-probes synchronously from its own reset work. This series adds device_schedule_reprobe() to the driver core and converts all three.

Tested on an Intel AX101 with patches 1 to 3, PROVE_LOCKING and
DEBUG_OBJECTS_WORK: a re-probe, and an unbind, rmmod, PCI removal, suspend and reboot with one pending. The unbind and the removals cancelled it, and it ran after resume. The stock iwlwifi code undid the unbind, and rmmod with its work held just past module_put() oopsed in lockdep. Patches 3 and 4 are untested for lack of hardware.

Changes since v3 [4]:
- patch 1 was reworked as patch 3/6 of the mxl862xx devlink flash series, v13 to v19 [5][6], which netdev has deferred until this lands; its history summarises the changes
- patch 1: Hans de Goede's tags dropped [7]; patch 3 keeps his, its code being unchanged, but a re-test would be welcome
- patches 2 and 3: Fixes: tags
- patches 2 to 4: commit messages updated; rebased onto bluetooth-next

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/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/
[3] https://lore.kernel.org/all/[email protected]/
[4] https://lore.kernel.org/all/[email protected]/
[5] https://lore.kernel.org/all/[email protected]/
[6] https://lore.kernel.org/all/[email protected]/
[7] https://lore.kernel.org/all/[email protected]/

---
On LLM use: device_schedule_reprobe() was designed and first implemented by hand. Its later revisions answer AI review (Sashiko on the mxl862xx postings, and local runs of the same prompts); an LLM helped reproduce those findings and draft the changes and changelogs. Every decision and the final review were mine; the Assisted-by tag on patch 1 records this.

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                           |   4 +
 drivers/base/dd.c                             | 171 ++++++++++++++++++
 drivers/bluetooth/btintel_pcie.c              |  47 +++--
 drivers/bluetooth/hci_h5.c                    |  43 +----
 .../net/wireless/intel/iwlwifi/iwl-trans.c    |  40 +---
 include/linux/device.h                        |   2 +
 6 files changed, 208 insertions(+), 99 deletions(-)

base-commit: 185c11232d5f586489bb3b74e6bcc7a6560ddd40

2.56.0

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.

---
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/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/
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(+)

index a5b7abc10ff02..3a6883de7cef3 100644
@@ -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)	\

index aee79fd6b32b4..7a99169505772 100644
@@ -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

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

Tested on a Dell XPS 13 9310 with 7.2.11, called from the .restore callback of hci_qca (QCA6390 on serdev) to download the firmware again after hibernation: the reprobe ran after restore and Bluetooth came back, and s2idle suspend did not trigger it.

iwl_trans_schedule_reprobe() runs device_reprobe() from a work item in module text that ends with module_put(THIS_MODULE), so a concurrent rmmod can free the module while the work still runs, and a stale re-probe can undo an unbind. With the work held just past module_put() by a test-only delay, rmmod ends in a lockdep oops on the work's lock class, freed with the module. Use device_schedule_reprobe(); both callers run in iwl_trans_restart_wk(), which ->remove() cancels synchronously.

A pending re-probe no longer pins the module: rmmod succeeds and cancels it.

---
v4: commit message updated for the reworked helper; add Fixes:
v3: no changes
v2: no changes
v1: initial RFC

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

index 73aae11250421..5ae734cb90272 100644
@@ -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.56.0

h5_btrtl_resume() runs device_reprobe() from a work item in module text that ends with module_put(THIS_MODULE), so a concurrent rmmod can free the module while the work still runs, and a stale re-probe can undo an unbind. Use device_schedule_reprobe(), which may be called from the resume callback with the device lock held.

The re-probe now runs once tasks are thawed, after the resume has unblocked probing.

---
v4: commit message updated for the reworked helper; add Fixes:; code
unchanged, so Hans de Goede's tags, given with the v3 helper, stay
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(-)

index 37f4d9327e724..fb819d769dd52 100644
@@ -989,7 +989,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.
 	 */

@@ -1048,46 +1048,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.56.0

The reset work re-probes its own device with device_reprobe() after an FLR or a PLDR, so .remove() runs inside it. That needs the current_work() check in .remove(), and 'data' must not be touched after the call.

Schedule the re-probe with device_schedule_reprobe(). .remove() then runs from the driver core's work, the current_work() check goes, and disable_work_sync(&data->reset_work) waits for the reset work before 'data' is freed. Until the deferred detach 'data' stays alive but idle:
RECOVERY_IN_PROGRESS fails send_frame and coalesces further resets, interrupts are masked, and the dump and rfkill workers stay disabled. If scheduling fails in the FLR path, the reset work re-enables them as on any FLR failure. The PLDR re-probe of the Wi-Fi sibling, a different device, is unchanged.

---
v4: rebased onto bluetooth-next for its hardware rfkill work; commit
message updated for the reworked helper
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 | 47 ++++++++++++++++----------------
 1 file changed, 23 insertions(+), 24 deletions(-)

index df95d16a163c8..9f65eb0ceb0b2 100644
@@ -4093,7 +4093,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);
 

@@ -4104,14 +4104,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.

@@ -4133,14 +4135,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)

@@ -4173,11 +4173,15 @@ static void btintel_pcie_reset_work(struct work_struct *wk)
 	disable_work_sync(&data->mbox_work);
 	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.
 	 *

@@ -4598,13 +4602,8 @@ 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.
-	 */
-	if (current_work() != &data->reset_work)
-		disable_work_sync(&data->reset_work);
+	/* reset_work may still be running after scheduling the re-probe. */
+	disable_work_sync(&data->reset_work);
 
 	btintel_pcie_mask_hw_rfkill(data);
 	btintel_pcie_synchronize_irqs(data);

2.56.0

Hi Greg,

Now that the most hardcore merge-window ever is over and v7.4-rc1 is out, can you take a look at this?

The series has received positive feedback and was tested by some more of the bluetooth folks by now, and is immediately useful to address currently buggy behavior of existing popular driver, and prevent the same wrong pattern from being duplicated into new drivers.

Cheers

Daniel