Re: [PATCH net-next v16 3/6] driver core: add device_schedule_reprobe()

Thank you for your contribution! Sashiko AI review found 8 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 3 · Low: 4

- [High] The new `abort_if_blocked` protection stops at @dev.
- [Medium] `__device_release_driver()` (drivers/base/dd.c:1341-1345) can
  now abandon a release *inside* the `while (device_links_busy(dev))`…
- [Medium] Only the detach half of the re-probe is gated on
  `defer_all_probes`; the attach half is not.
- [Medium] The detach and the re-attach are not one serialized operation,
  so an administrative unbind can be acknowledged and then undone.
- [Low] `device_schedule_reprobe()` stores only a raw `const struct
  device_driver *` (`drv = READ_ONCE(dev->driver)`, no reference, no…
- [Low] The kernel-doc of `device_schedule_reprobe()` says "Nothing is
  locked in the caller's context, so this may be called from any process…
- [Low] Reported for completeness; my baseline decision is that this is
  NOT a defect to act on.
- [Low] Reported for completeness; my baseline decision is FALSE
  POSITIVE.

--

driver core: add device_schedule_reprobe()

This adds a driver-core helper that schedules a delayed detach and
re-probe of a device from built-in work, replacing the open-coded work
items drivers use today. __device_release_driver() gains a return value
and an abort_if_blocked flag so a re-probe can abandon the unbind when
probing has been blocked meanwhile.

A few questions about the new helper and the new flag.

[Severity: Medium]
When the new abort_if_blocked && defer_all_probes arm fires, is the
device-links state left half torn down?

By the time this check runs, device_links_busy() has already done:

drivers/base/core.c:device_links_busy() {
	...
		WRITE_ONCE(link->status, DL_STATE_SUPPLIER_UNBIND);
	}

	dev->links.status = DL_DEV_UNBINDING;
	...
}

and device_links_unbind_consumers() has already force-released the drivers
of the active consumers.

Returning false here leaves @dev bound, so device_links_driver_cleanup() -
the only code that moves those links back to DL_STATE_DORMANT and clears
DL_DEV_UNBINDING - never runs, and device_links_driver_bound() cannot run
either because the supplier never rebinds.

Can the already-detached consumers then re-probe?
device_links_check_suppliers() looks like it would defer them forever:

drivers/base/core.c:device_links_check_suppliers() {
	...
		if (link->status != DL_STATE_AVAILABLE &&
		    !device_link_test(link, DL_FLAG_SYNC_STATE_ONLY)) {
			...
			ret = dev_err_probe(dev, -EPROBE_DEFER, ...);
	...
}

and any new device_link_add() against this still-bound supplier starts at
DL_STATE_SUPPLIER_UNBIND because supplier->links.status is DL_DEV_UNBINDING.

The pre-existing dev->driver != drv arm is different in that a concurrent
releaser completes the state machine. Here the only caller passing true,
device_reprobe_work_fn(), treats false as "nothing happened" and does no
compensating cleanup. Should the kernel-doc and commit message mention this,
or should the abort undo the links state it inherits?

[ ... ]

[Severity: High]
Does the new flag actually close the ->shutdown()/->remove() interleaving
for the consumers, or only for @dev?

The commit message says:

  "__device_release_driver() drops the device lock while busy consumer
   links are unbound, which would let a ->shutdown() interleave in the
   middle of a release already under way, so it now re-checks on
   re-acquisition and abandons the release."

But the consumer unbinds happen inside that dropped-lock window, before the
new check is evaluated:

drivers/base/dd.c:__device_release_driver() {
	...
		while (device_links_busy(dev)) {
			__device_driver_unlock(dev, parent);

			device_links_unbind_consumers(dev);

			__device_driver_lock(dev, parent);
	...
}

and each consumer goes through this wrapper, which hardcodes the flag off:

drivers/base/core.c:device_links_unbind_consumers() {
	...
			device_release_driver_internal(consumer, NULL,
						       consumer->parent);
	...
}

So the sequence looks reachable:

device_reprobe_work_fn()
    device_lock(dev)                       /* defer_all_probes == false */
    __device_release_driver(dev, NULL, true)
        device_links_busy(dev) == true
        device_unlock(dev)
        device_links_unbind_consumers(dev)

meanwhile:

device_shutdown()
    device_block_probing()                 /* defer_all_probes = true */
    device_lock(consumer)
    consumer->driver->shutdown(consumer)
    device_unlock(consumer)

back in device_links_unbind_consumers():

    device_release_driver_internal(consumer, ...)
        __device_release_driver(consumer, ..., false)
            device_remove(consumer)        /* consumer ->remove() */

Is the consumer's ->remove() then run after its ->shutdown() has already
quiesced it? Nothing else serialises the two beyond the consumer's own
device lock, and wait_for_device_probe() cannot see the new delayed work:

drivers/base/dd.c:wait_for_device_probe() {
	flush_work(&deferred_probe_work);
	wait_event(probe_waitqueue, atomic_read(&probe_count) == 0);
	async_synchronize_full();
}

Should the abort decision be propagated into the consumer releases before
their removal callbacks run, or should shutdown be synchronised against the
whole in-flight re-probe?

[Severity: Medium]
Only the detach half is gated on defer_all_probes here; is the attach half
also meant to be?

device_attach() runs after device_unlock(dev) with no re-check, and
__device_attach() resumes the parent before anything tests defer_all_probes:

drivers/base/dd.c:__device_attach() {
	...
		if (dev->parent)
			pm_runtime_get_sync(dev->parent);

		ret = bus_for_each_drv(dev->bus, NULL, &data,
					__device_attach_driver);
	...
}

really_probe() only rejects the probe later. device_shutdown() itself only
does pm_runtime_get_noresume() plus a barrier, so a parent that is
RPM_SUSPENDED stays suspended - can this work then runtime-resume a parent
whose ->shutdown() has already run, and add @dev to the deferred-probe list
while probing is blocked?

Nothing cancels or flushes this work either, and wait_for_device_probe()
(which device_block_probing() relies on) only flushes deferred_probe_work,
waits for probe_count and calls async_synchronize_full(), none of which
observe a system_freezable_wq delayed work.

[Severity: Medium]
Can an administrative unbind be acknowledged and then undone here, since the
detach and the re-attach are not one serialised operation?

drivers/base/bus.c:unbind_store() {
	...
	if (dev && dev->driver == drv) {
		device_driver_detach(dev);
		err = count;
	...
}

That dev->driver == drv test is unlocked, and device_driver_detach() ends up
in device_release_driver_internal(), which blocks on the device lock this
work holds. When it gets the lock, dev->driver is NULL, so it does nothing,
and unbind_store() still returns count. Then this work calls
device_attach(dev) and the device is bound again. The same applies to an
unbind arriving in the window between device_unlock(dev) and
device_attach(dev).

The commit message lists "a stale re-probe undoes an administrative unbind"
among the things the new helper addresses, while the new return value only
covers an unbind that wins inside the busy-consumer-links loop. Could that
wording be narrowed, or the gap between detach and re-attach closed?

+
+	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, probing has been blocked for a system
+ * shutdown, or @dev is no longer bound to the driver that was bound at
+ * scheduling time.
+ *
+ * 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.

[ ... ]

+ *
+ * Nothing is locked in the caller's context, so this may be called from
+ * any process context, @dev's own device lock held included, but not
+ * from @dev's ->probe(), which the scheduled work would detach.

[ ... ]

+ */
+int device_schedule_reprobe(struct device *dev, unsigned int delay_ms)
+{
+	const struct device_driver *drv;
+	struct device_reprobe *rp;
+
+	drv = READ_ONCE(dev->driver);

[Severity: Low]
The recorded identity of the binding is a bare struct device_driver pointer,
with no reference, no module pin and no generation counter, and
device_reprobe_work_fn() gates the destructive detach only on

	dev->driver == rp->drv

Is pointer identity enough here? An unbind followed by a rebind of the same
driver within @delay_ms makes the test pass for a binding the caller never
scheduled against. After a module unload, can the same address be reused by
another module's struct device_driver, in which case the kernel-doc promise

  "If the driver module is unloaded before the work runs, driver
   unregistration unbinds @dev first and the scheduled work does nothing."

no longer holds, and the work would drive __device_release_driver() -
including device_links_unbind_consumers() and an unrelated driver's
->remove() - followed by device_attach()?

There is also no cancel or flush counterpart, so a caller cannot gate this
itself. Would a bind generation counter, a per-binding token, or at least
comparing the driver name as well, match the documented semantics?

+	/*
+	 * 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 ||
+	    dev->p->dead || !device_is_registered(dev))
+		return -EINVAL;
+
+	rp = kzalloc_obj(*rp);
+	if (!rp)
+		return -ENOMEM;

[Severity: Low]
The kernel-doc says this "may be called from any process context", but
kzalloc_obj() with no gfp argument resolves to GFP_KERNEL and may sleep:

include/linux/gfp.h:
	#define __default_gfp(a,b,...) b
	#define default_gfp(...) __default_gfp(,##__VA_ARGS__,GFP_KERNEL)

The commit message invites callers from the PM callbacks, whose _late and
_noirq phases run with interrupts disabled. Could a Context: line stating
that this helper may sleep be added, or the gfp flags be passed explicitly?

+
+	rp->dev = get_device(dev);
+	rp->drv = drv;
+
+	INIT_DELAYED_WORK(&rp->work, device_reprobe_work_fn);
+	queue_delayed_work(system_freezable_wq, &rp->work,
+			   msecs_to_jiffies(delay_ms));
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(device_schedule_reprobe);

[Severity: Low]
This isn't a bug, just a wording note. The commit message opens by
describing live defects in iwlwifi's iwl_trans_reprobe_wk() and hci_h5's
h5_btrtl_reprobe_worker():

  "both end that work function with put_device(); kfree();
   module_put(THIS_MODULE);, where a concurrent rmmod can free the module
   text the epilogue is still executing."

Neither driver is converted here and there is no Fixes: tag, while the newly
exported symbol has no in-tree caller until the mxl862xx patches later in
the series. Splitting the core helper from its first user is normal, so
nothing needs changing in the code; would leading with the motivation rather
than with those two drivers read more clearly?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1789477568.git.daniel%40makrotopia.org