* Re: [PATCH net-next v14 3/6] driver core: add device_schedule_reprobe()
@ 2026-09-12 0:28 netdev-bot+sashiko
0 siblings, 0 replies; 2+ messages in thread
From: netdev-bot+sashiko @ 2026-09-12 0:28 UTC (permalink / raw)
To: daniel
Cc: jiri, davem, edumazet, kuba, pabeni, horms, corbet, skhan,
rdunlap, gregkh, rafael, dakr, andrew, olteanv, netdev,
linux-doc, linux-kernel, driver-core
Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 3 · Low: 3
- [Medium] The new shutdown guard is per-device and set lazily:
device_shutdown() (drivers/base/core.c:4913-4914) sets…
- [Medium] device_schedule_reprobe() validates the device (dev->bus,
dev->p, device_is_registered(), dev->driver) at dd.c:1531-1534 and…
- [Medium] The shutdown_done test that is supposed to suppress the detach
half is evaluated once (dd.c:1472) before __device_release_driver()…
- [Low] Commit message framing vs. what the patch does: the message
devotes most of its length to two concrete bug classes in three named…
- [Low] device_reprobe_work_fn() reports every negative device_attach()
return with dev_err_probe(dev, ret, "re-probe failed, device left…
- [Low] device_reprobe_work_fn() decides whether to forcibly detach a
device solely by comparing dev->driver against the raw pointer value…
--
driver core: add device_schedule_reprobe()
This adds a driver-core helper that schedules a delayed detach and
re-probe of a device on system_freezable_wq, pinning the device and its
parent and recording the bound driver pointer so a stale re-probe does
nothing. It also adds a shutdown_done flag to struct device_private,
set by device_shutdown() under the device lock, and makes
__device_release_driver() return whether it was the caller that released
the driver.
A few questions below.
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:
[Severity: Low]
This isn't a bug, but could the message say more plainly that the three
drivers it describes are not converted here or later in this series?
At the end of the series, iwl_trans_reprobe_wk() in
drivers/net/wireless/intel/iwlwifi/iwl-trans.c and
h5_btrtl_reprobe_worker() in drivers/bluetooth/hci_h5.c still end with:
if (device_reprobe(reprobe->dev))
dev_err(reprobe->dev, "reprobe failed!\n");
put_device(reprobe->dev);
kfree(reprobe);
module_put(THIS_MODULE);
with no bound-state check, and btintel_pcie is unchanged, so both bug
classes are still present in-tree. The helper does gain in-tree callers
in this series (two call sites in drivers/net/dsa/mxl862xx/mxl862xx-fw.c),
just not in the three motivating drivers.
> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index a5b7abc10ff02..6234e37de7e99 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.
> */
[ ... ]
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 4c0c373998a19..3dcd8a3c3aa55 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;
> +
[Severity: Medium]
This flag is set only when the reverse-order walk reaches each device, so
it means "this device has been reached", not "shutdown is in progress".
What happens when a scheduled re-probe fires after device_shutdown() has
started but before the walk reaches @dev?
The guard in device_reprobe_work_fn() sees dead == false and
shutdown_done == false, so the detach half runs:
__device_release_driver() -> device_remove() -> drv->remove()
at a point where every device registered after @dev (children, later
probing suppliers) already had bus->shutdown()/driver->shutdown() run.
The re-attach that follows cannot succeed either, because
device_shutdown() calls device_block_probing() before the walk and
really_probe() then returns -EPROBE_DEFER:
if (defer_all_probes) {
...
return -EPROBE_DEFER;
}
device_unblock_probing() is only reached from dpm_complete(), never on the
shutdown path, so the device stays unbound.
And when the walk finally reaches @dev, this test in device_shutdown():
} else if (dev->driver && dev->driver->shutdown) {
is false, so the driver's ->shutdown() is skipped for that device. Would
a global condition (probing blocked, or system_state) be a better guard
here than a per-device flag?
> /* 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 f6525a7ee8c54..81a3433f726e8 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
[ ... ]
> @@ -1336,7 +1336,7 @@ static void __device_release_driver(struct device *dev, struct device *parent)
> */
> if (dev->driver != drv) {
> pm_runtime_put(dev);
> - return;
> + return false;
> }
> }
[Severity: Medium]
The commit message mentions this window ("One pre-existing window
remains: __device_release_driver() transiently drops the locks while
consumer device links are busy"). Since the new caller tests
shutdown_done only once, before the call, this loop revalidates only
dev->driver after retaking the locks:
while (device_links_busy(dev)) {
__device_driver_unlock(dev, parent);
device_links_unbind_consumers(dev);
__device_driver_lock(dev, parent);
if (dev->driver != drv) {
pm_runtime_put(dev);
return false;
}
}
In that window device_shutdown() can take both locks, set shutdown_done
and run ->shutdown(), after which the work continues into device_remove()
and ->remove() runs after ->shutdown(). Would re-testing
dev->p->shutdown_done right after __device_driver_lock() close this for
the new helper?
[ ... ]
> @@ -1436,3 +1439,116 @@ 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;
> + bool parent_lock;
> +};
> +
> +static void device_reprobe_work_fn(struct work_struct *work)
> +{
> + struct device_reprobe *rp = container_of(work, struct device_reprobe,
> + work.work);
> + struct device *parent = rp->parent;
> + struct device *dev = rp->dev;
> + bool detached = false;
> + int ret;
> +
> + /*
> + * A device unregistered before the work runs may have outlived the
> + * module providing its bus type, so dev->bus is not read until the
> + * device is known to be live.
> + */
> + if (rp->parent_lock)
> + device_lock(parent);
> + device_lock(dev);
> + /*
> + * 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 == rp->drv)
> + detached = __device_release_driver(dev, parent);
> + device_unlock(dev);
> + if (rp->parent_lock)
> + device_unlock(parent);
> +
> + if (detached) {
> + /*
> + * device_attach() must run with the parent locked on buses
> + * that require it, mirroring bus_rescan_devices_helper().
> + */
> + if (rp->parent_lock)
> + device_lock(parent);
> + ret = device_attach(dev);
> + if (ret < 0)
> + dev_err_probe(dev, ret,
> + "re-probe failed, device left unbound\n");
[Severity: Low]
For ret == -EPROBE_DEFER, dev_err_probe() does more than pick a log
level. __dev_probe_failed() in drivers/base/core.c does:
case -EPROBE_DEFER:
device_set_deferred_probe_reason(dev, &vaf);
dev_dbg(dev, "error %pe: %pV", ERR_PTR(err), &vaf);
so this string is stored in dev->p->deferred_probe_reason, which
drivers/base/base.h documents as the message retrieved via debugfs, and
any reason recorded earlier by the driver's own probe is overwritten.
-EPROBE_DEFER is reachable here: really_probe() returns it while
defer_all_probes is set, and __device_attach_driver()/__device_attach()
pass it back unchanged. In that case /sys/kernel/debug/devices_deferred
reports "re-probe failed, device left unbound" for a device that is
queued for a retry. Should the -EPROBE_DEFER case be handled separately
from a real failure?
> + if (rp->parent_lock)
> + device_unlock(parent);
> + }
> +
> + put_device(dev);
> + put_device(parent);
> + kfree(rp);
> +}
> +
[ ... ]
> + * scheduling time. In particular an administrative unbind is never
> + * undone by a stale re-probe.
[ ... ]
> +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->parent_lock = dev->parent && dev->bus->need_parent_lock;
[Severity: Medium]
These reads happen without the device lock, and after a kzalloc_obj()
that can sleep, so the checks above them may already be stale.
device_del() takes its own copy of dev->parent at entry, unbinds the
driver, calls kobject_del() (which clears the only thing
device_is_registered() tests) and ends with:
kobject_del(&dev->kobj);
cleanup_glue_dir(dev, glue_dir);
memalloc_noio_restore(noio_flag);
put_device(parent);
dev->parent itself is never cleared. Can a caller that passed the checks
and then slept in the allocation reach get_device(dev->parent) after that
final put_device(parent) dropped the last reference, i.e. kobject_get() on
a freed struct device, followed later by device_lock(parent) and
put_device(parent) in the work?
Separately, rp->parent and rp->parent_lock come from two independent
reads of dev->parent, and device_move() assigns it without the device
lock:
old_parent = dev->parent;
dev->parent = new_parent;
so can rp->parent end up NULL while rp->parent_lock is true, leaving
device_reprobe_work_fn() calling device_lock(NULL)?
Would re-validating and snapshotting parent/parent_lock/driver under
device_lock(dev), checking dev->p->dead (which kill_device() sets under
that lock at the start of device_del(), before its put_device(parent)),
avoid both cases?
> + rp->drv = READ_ONCE(dev->driver);
[Severity: Low]
Is a raw pointer value enough to identify the binding that asked for the
re-probe?
Within delay_ms, an unbind followed by a bind of the same driver:
echo dev > /sys/bus/.../unbind
echo dev > /sys/bus/.../bind
restores dev->driver == rp->drv, so the check in
device_reprobe_work_fn() matches and the stale work runs a detach and
re-attach on a binding that never requested one. That also narrows the
kernel-doc claim above, "an administrative unbind is never undone by a
stale re-probe": a plain unbind is honoured, an unbind plus rebind is
not.
And once the original module is unloaded, can another module's static
struct device_driver end up at the same address and make the comparison
succeed for an unrelated driver? Would a generation counter, or a
recorded driver name, be a more stable token here?
> + 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);
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1788976064.git.daniel%40makrotopia.org
^ permalink raw reply [flat|nested] 2+ messages in thread* [PATCH net-next v14 0/6] net: dsa: mxl862xx: devlink flash and rescue
@ 2026-09-09 18:46 Daniel Golle
2026-09-09 18:46 ` [PATCH net-next v14 3/6] driver core: add device_schedule_reprobe() Daniel Golle
0 siblings, 1 reply; 2+ messages in thread
From: Daniel Golle @ 2026-09-09 18:46 UTC (permalink / raw)
To: Jiri Pirko, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, Jonathan Corbet, Shuah Khan,
Randy Dunlap, Daniel Golle, Greg Kroah-Hartman,
Rafael J. Wysocki, Danilo Krummrich, Andrew Lunn,
Vladimir Oltean, netdev, linux-doc, linux-kernel, driver-core
This series adds "devlink dev flash" and "devlink dev info" support to
the MaxLinear MxL862xx DSA driver, and makes a switch stuck in its
MCUboot loader recoverable through the same path.
The switch is flashed over the loader's clause-22 SMDIO download
interface after the firmware API has rebooted it into MCUboot, and the
driver reinitialises through a deferred detach and re-probe once the
new image runs. A switch found in MCUboot at probe registers in a
reduced rescue mode with firmware version 0.0.0, so the same flash flow
recovers it; an interrupted download is drained in the background
first. The deferred re-probe comes from a new driver-core helper,
device_schedule_reprobe(), since a driver-owned work item can neither
survive a racing rmmod nor synchronise its detach with
device_shutdown(). fwupd's devlink plugin carries the matching quirks
[17].
Tested on an MxL86252C switch of the BananaPi R4 Pro 8X: repeated flash
cycles, recovery of a download interrupted by a crash, and a flash after
a hard reset into the loader. The helper was runtime-tested on Intel
AX101 with PROVE_LOCKING and DEBUG_OBJECTS_WORK [11].
Changes since v13 [18]:
- patch 5: initialise the SerDes state once mxl862xx_wait_ready() has
cached the firmware version, still before the rescue-mode early
return, so PCS setup can depend on the running firmware
- picked up Andrew's Reviewed-by on patches 1, 4 and 5
Changes since v12 [13]:
- v12 went out just as net-next closed for the 7.3 merge window. The
helper of patch 3 was then posted on its own, with conversions of the
existing open-coded users in iwlwifi, hci_h5 and btintel_pcie, most
recently as v3 [14], which Greg's patch bot deferred past the merge
window; Hans de Goede reviewed and tested the helper and the hci_h5
conversion there on RTL8723BS hardware [15][16]. The Sashiko review
of that posting found the same issues in the helper as the review of
v12 did, so patch 3 here supersedes the helper patch of that series;
once this series is merged, the conversions follow as patches for
bluetooth-next and wireless-next. On the userspace side, fwupd's
devlink plugin has meanwhile gained the quirks for these switches
[17]
- patch 3: queue the re-probe on system_freezable_wq, so one pending
across system suspend runs after resume instead of detaching a
suspended device or racing its late suspend callbacks; record at
scheduling time whether the parent needs locking instead of reading
dev->bus, which may be gone with its module once the device was
unregistered; let __device_release_driver() report whether it
released the driver, so an administrative unbind that wins the race
inside the device links loop is not undone by the re-attach (all
found by Sashiko AI review); use dev_err_probe() for the re-probe
error path (Hans de Goede)
- patch 4: stop the stats poll with disable_delayed_work_sync() in the
flash path, so a racing get_stats64() re-arm is a no-op, and drop the
early return in the work function; the v12 reordering of remove()
is gone as well, since the get_stats64() race it addressed predates
this series and needs a fix of its own (found by Sashiko AI review)
- two further findings of the same review are not acted on: the
recorded driver pointer could in theory match a different driver
loaded at the same address within the delay, which would cost that
driver one spurious detach and re-probe; and the final put_device()
from the work could call a release() whose module was unloaded in
the meantime, which is the same hazard every asynchronous device
reference in the core carries, the async probe helper included
- the changes to patches 3 and 4 were written with an LLM coding
assistant working from the Sashiko findings and reviewed by hand; the
Assisted-by tags on those two patches record this
Changes since v11 [12]:
- patch 3: pin the parent device across the deferred re-probe and take
the parent lock across device_attach() on buses that need it; a
reference on the child alone left a freed dev->parent dereferenced
under __device_driver_lock() (found by Sashiko AI review)
- patch 4: cancel the stats poll after dsa_unregister_switch() so a
racing get_stats64() cannot re-arm it against freed priv; and keep
the host blocked for writes across the post-flash readiness poll,
letting only the flash path's own reads reach the new firmware
(found by Sashiko AI review)
Changes since v10 [10]:
- new patch 3: driver core: add device_schedule_reprobe(), as posted
in the RFC [11], used to schedule the post-flash and post-drain re-probe with
device_schedule_reprobe() instead of a driver-owned work item.
- the dsa_switch allocation returns to devres. Keeping it out of
devres only defused the remaining check-vs-detach window, which the
core helper closes outright
- scheduling the re-probe is now the one step that can fail after the
switch was flashed, since the helper allocates its work item
internally and v10's allocate-up-front dance is no longer possible.
An -ENOMEM there is a system-wide condition no driver-level message
or recovery improves, so flash_update just returns it (unbind and
rebind reinitialises the driver), and a drain whose hand-off fails
still marks recovery failed so devlink does not keep promising a
retry
Changes since v9 [9]:
- Harmonised the SB PDI timeouts. The verify wait is now one constant
shared by the flash and drain paths (15 s), the 1-byte mailbox step
is another (2 s) used by the drain and by both detection waits, and
the per-slice write budget drops from 120 s to 60 s. The last-slice
flush, which cannot see the boundary between programming and
verifying, gets the sum of the two.
- The post-flash and post-drain reprobe no longer detaches a device
that has been shut down or unbound, and the dsa_switch is allocated
outside devres so a lost race cannot leave dsa_switch_find() reading
freed memory. This was a live bug in v9's patch 3 as well, reachable
by rebooting within 500 ms of a flash.
- A failed reprobe hand-off after a successful drain now logs and
fails flashing outright, instead of leaving devlink answering "retry
shortly" for good. Dropped heal_lock and mxl862xx_stop_work() with
it: the lock only made the flag test and the queueing atomic, which
is not the guarantee the comment claimed, and the reprobe's own
check is what actually decides now.
- A loader that publishes READY but never services the register-read
challenge now reports -ENXIO rather than propagating -ETIMEDOUT, and
the documented return sets of mxl862xx_rescue_mode_detect() and
mxl862xx_rescue_drain_finish() match what the code returns.
- Commit message for patch 4 no longer claims a running firmware is
"left untouched" (the presence probe writes two mailbox scratch
registers, inert to a firmware that does not read them), says that
an SB PDI window away from the OTP reset offsets also yields
-ENODEV, and describes the -ENXIO outcome above.
- Commented why STAT == 0 during a drain is unambiguous: by the
r_remain == 0 rule the loader cannot be both inside the receive loop
asking for a chunk and publishing a verdict.
- Documented that a switch power cycled on its own needs the driver
unbound and rebound before a failed recovery is re-examined.
Changes since v8 [8]:
- install the flash_update devlink op unconditionally and return
-EOPNOTSUPP from the trampoline for drivers without the callback,
instead of a second devlink_ops permutation (Andrew Lunn)
- picked up Andrew's Reviewed-by on patches 2 and 5, given on v5
Changes since v7 [7]:
- most of the changes below address findings of the Sashiko AI reviews
of v7
- the MCUboot loader's transfer completion was reverse-engineered to
settle two of them: it publishes an image verification verdict in its
status register, finalises without the END magic once a 2 s timeout
expires, and keeps the host's byte count visible while it programs a
chunk. The interrupted-download drain therefore no longer sends END,
which a loader still in its receive loop consumes as a byte count and
underflows its receive counter on, and the flash path now reports a
rejected image instead of a write timeout
- refuse a second devlink dev flash while the previous one's reprobe is
still pending, and stop publishing a zeroed firmware version after a
failed transfer
- initialise the SerDes state before the rescue-mode early return, so a
successful rescue-mode flash cannot hand phylink an unconfigured PCS
- do not fail probe from the wedged-download branch of the rescue
detection, which a download interrupted with exactly one byte
outstanding would trigger, and report a failed recovery through
devlink rather than refusing every flash for good
- reset the SB PDI mailbox before probing it, log the drain's progress,
and correct the protocol and register comments throughout
Changes since v6 [6]:
- reprobe from a single delayed work item instead of a kthread spawned
by a workqueue kickoff; the kthread existed only to drop the module
reference from core code, but its creation-failure path did the racy
module_put() from module text anyway and could strand the driver
bound with skip_teardown set. The collapsed form matches
iwl_trans_reprobe_wk(), and a failed reprobe now leaves the device
unbound like a failed probe
- only signal END on a successful transfer; a failure leaves the loader
mid-payload, where END is read as a byte count and can underflow the
receive counter, so return the error and let the reprobe recover
- add cond_resched() to the payload loop so a long transfer over a
bit-banged MDIO bus under CONFIG_PREEMPT_NONE does not trip the
soft-lockup detector
- drop the cached firmware version and chip id on a failed flash so
devlink dev info stops reporting the pre-flash version until the
reprobe
- report the firmware version under DEVLINK_INFO_VERSION_GENERIC_FW
instead of a bare "fw" string
- correct the SB PDI header comment's SMDIO register map and expand the
note on why closing the shared conduit is safe
Changes since v5 [5]:
- run the post-flash reprobe from a kthread that drops the module
reference with module_put_and_kthread_exit() from core code, fixing
a use-after-free where a work item's trailing module_put() could
return into module text a racing rmmod had freed; a workqueue kickoff
spawns the kthread off the devlink caller where kthread_create() can
return -EINTR
- send END on every flash failure from the ready handshake onward so an
aborted transfer lets MCUboot reboot instead of leaving it waiting
- after the background drain finalises an interrupted download, reprobe
and let the probe-time detection re-classify the switch, so a valid
image a last-moment interruption left bootable comes up as running
firmware; rescue_drain() no longer inspects or reports the outcome
- re-read the SB PDI status register once more after a poll timeout
expires, so a preempted poll cannot report a spurious -ETIMEDOUT
- bail out of the periodic stats poll when the flash teardown has set
WORK_STOPPED, closing a get_stats64() re-arm race
- allocate the reprobe kickoff before disturbing the switch, so an
-ENOMEM cannot leave it flashed but never reprobed
- omit asic.id/asic.rev when the CHIP ID read returned 0, instead of
publishing a bogus "0000" for fwupd to match firmware against
- treat the flashless-download loop (STAT 0xc33c) as an unsupported
configuration and fail probe with -ENODEV instead of advertising it
as flashable
Changes since v4 [4]:
- report the numeric chip part number and version read from the
static CHIP ID registers as the "asic.id" and "asic.rev" fixed
versions instead of a model-name string, which does not belong in
a devlink version identifier (Jakub Kicinski)
- report the running firmware version as the "stored" version too,
since the switch boots it from its own flash, so userspace can
distinguish a flash-backed part from a flashless one by the
presence of "stored" without a future API change
- run the post-flash reprobe from a self-contained work item again
instead of the v4 kernel thread, which tripped the hung-task
watchdog while parked across the flash and returned -EINTR from
kthread_create() when the devlink command was interrupted
- re-read the new firmware version through the reprobe's fresh probe
and drop the SYS_MISC_FW_VERSION exemption from the host block
- raise the firmware command poll timeout so the FW_UPDATE command
that reboots into MCUboot is not cut short
- detect the switch state from the value MCUboot publishes in the SB
PDI STAT register (loader ready, wedged download, or running
firmware), confirming a live console loader with a register-read
challenge, instead of trusting a bare SMDIO scratch write
- fail probe with -ENODEV over SB PDI when the switch does not respond
at all (absent, unpowered, or misdescribed in the device tree)
instead of letting the clause-45 API flood the log with CRC errors
- drain a wedged interrupted download back to a clean ready state
from a background work item so the multi-minute recovery never
holds the devlink instance lock, reporting no firmware version and
refusing flash with -EBUSY until it completes
- report the rescue-mode null firmware version "0.0.0" as both the
running and stored version, matching the running/stored reporting
above
- split the devlink documentation into its own patch and add
Documentation/networking/devlink/mxl862xx.rst describing the info
versions and the flash update behaviour (Jakub Kicinski)
- include example "devlink dev info" outputs in the commit messages
of patches 3 and 4 (Jakub Kicinski)
Changes since v3 [3]:
- only install the flash_update devlink op for switches whose
driver implements it, so the devlink core rejects unsupported
requests before fetching the firmware file from userspace
- run the deferred reprobe from a kernel thread which ends in
module_put_and_kthread_exit() instead of a work item that
dropped its module reference while still executing module code
- fail firmware API read commands with -ENODEV after the update
has finished instead of faking success with an unfilled buffer,
which could send port_fdb_dump() into an endless loop
- keep the host block in place across the post-update version
query by exempting SYS_MISC_FW_VERSION from block_host instead
of briefly lifting the block, and write all blocking flags under
the MDIO bus lock
- check the return value of all SB PDI control writes; a failed
address write during the half-bank switch could otherwise place
the second half of the payload at the wrong flash offset
- initialise the progress notification deadline from jiffies so
notifications are not suppressed on 32-bit systems shortly
after boot
- log a distinct diagnostic when rescue mode detection fails on an
SMDIO bus error instead of silently treating it as not being in
rescue mode
- flush the switchdev deferred queue after closing the ports so
the bridge's deferred STP DISABLED transitions reach the
firmware while it is still running instead of failing against
the host block with "failed to set STP state" errors
- treat -ENODEV as successful deletion in port_mdb_del() so the
post-update teardown no longer leaves host MDB entries behind
for the DSA core to report when the tree is torn down
Changes since v2 [2]:
- validate the firmware image, including both CRCs, before taking
down any ports, so that a malformed file is rejected without
disturbing the running switch and without the needless flash and
reprobe cycle it previously triggered
- reject images whose declared payload sizes overflow when summed
(check_add_overflow) or sum up to zero; the latter previously
erased the flash without writing anything back
- allocate the reprobe work item and take the module and device
references before starting the update, so scheduling the reprobe
can no longer fail after the switch has been pushed into MCUboot
- prevent the stats poll work from being re-armed and cancel the
CRC error work before starting the transfer
- check the host-blocking flags in mxl862xx_api_wrap() under the
MDIO bus lock to close the race window where an API command
which had already passed the check could reach the bus after the
switch rebooted into MCUboot
- check the return value of SB PDI data word writes so a failed
MDIO transaction aborts the transfer instead of being noticed
only through a corrupted image
- report a per-model chip name (e.g. "MaxLinear MxL86252") as the
devlink "asic.id" fixed version instead of the devicetree
compatible string, whose comma is awkward for userspace
consumers such as fwupd (see discussion on v2 patch 3)
- report the canonical null version "0.0.0" instead of
"mcuboot-rescue" as the running firmware version in rescue mode,
so that version-comparing update tools like fwupd treat every
available release as an upgrade and offer it for recovery
Changes since RFC [1]:
- detect a switch stuck in MCUboot rescue mode at probe, register
the switch without any ports and report "mcuboot-rescue" as the
running firmware version, so devlink flash can recover from a
failed or interrupted update (Andrew Lunn)
- clarify in the commit message of patch 2 that the per-transaction
MDIO bus locking is about other, non-switch devices on the same
MDIO bus (Andrew Lunn)
- mention in the commit message of patch 3 that closing the ports
also stops phylib from polling the switch-internal PHYs during
the transfer (Andrew Lunn)
- split up run-on sentence and explain the dynamically allocated
reprobe work item instead of just pointing at iwlwifi in the
commit message of patch 3 (Manuel Ebner)
- use kzalloc_obj() (Manuel Ebner)
- state the actual duration of a complete flash and reprobe cycle
(just under a minute) in comments and the commit message, and
clarify that the timeout values are generous upper bounds
(Manuel Ebner)
[1] https://lore.kernel.org/all/ak0J-HgzMRea53om@makrotopia.org/
[2] https://lore.kernel.org/all/cover.1783988826.git.daniel@makrotopia.org/
[3] https://lore.kernel.org/all/cover.1784513694.git.daniel@makrotopia.org/
[4] https://lore.kernel.org/all/cover.1784665017.git.daniel@makrotopia.org/
[5] https://lore.kernel.org/all/cover.1784945329.git.daniel@makrotopia.org/
[6] https://lore.kernel.org/all/cover.1785119999.git.daniel@makrotopia.org/
[7] https://lore.kernel.org/all/cover.1785274610.git.daniel@makrotopia.org/
[8] https://lore.kernel.org/all/cover.1785389905.git.daniel@makrotopia.org/
[9] https://lore.kernel.org/all/cover.1785728574.git.daniel@makrotopia.org/
[10] https://lore.kernel.org/all/cover.1786294649.git.daniel@makrotopia.org/
[11] https://lore.kernel.org/all/anpxFdwNxk0XwPjQ@makrotopia.org/
[12] https://lore.kernel.org/all/cover.1786773971.git.daniel@makrotopia.org/
[13] https://lore.kernel.org/all/cover.1786922210.git.daniel@makrotopia.org/
[14] https://lore.kernel.org/all/cover.1787281239.git.daniel@makrotopia.org/
[15] https://lore.kernel.org/all/c461462f-de0b-43e8-ac9e-541013f5f8da@oss.qualcomm.com/
[16] https://lore.kernel.org/all/7ffe0c2e-0742-488a-ab6c-1dc2fabc049c@oss.qualcomm.com/
[17] https://github.com/fwupd/fwupd/commit/e50c9e5ab39d31242e664efbbf441fd46d15a0cd
[18] https://lore.kernel.org/all/cover.1788783126.git.daniel@makrotopia.org/
Daniel Golle (6):
net: dsa: add devlink flash_update callback to dsa_switch_ops
net: dsa: mxl862xx: add SMDIO clause-22 register access
driver core: add device_schedule_reprobe()
net: dsa: mxl862xx: add devlink flash_update and info_get
net: dsa: mxl862xx: recover switch stuck in MCUboot rescue mode
net: dsa: mxl862xx: document devlink flash and info support
Documentation/networking/devlink/index.rst | 1 +
Documentation/networking/devlink/mxl862xx.rst | 74 ++
MAINTAINERS | 1 +
drivers/base/base.h | 5 +
drivers/base/core.c | 3 +
drivers/base/dd.c | 120 +-
drivers/net/dsa/mxl862xx/Makefile | 2 +-
drivers/net/dsa/mxl862xx/mxl862xx-api.h | 10 +
drivers/net/dsa/mxl862xx/mxl862xx-cmd.h | 2 +
drivers/net/dsa/mxl862xx/mxl862xx-fw.c | 1062 +++++++++++++++++
drivers/net/dsa/mxl862xx/mxl862xx-fw.h | 20 +
drivers/net/dsa/mxl862xx/mxl862xx-host.c | 64 +
drivers/net/dsa/mxl862xx/mxl862xx-host.h | 2 +
drivers/net/dsa/mxl862xx/mxl862xx-phylink.c | 2 +
drivers/net/dsa/mxl862xx/mxl862xx.c | 149 ++-
drivers/net/dsa/mxl862xx/mxl862xx.h | 37 +
include/linux/device.h | 2 +
include/net/dsa.h | 3 +
net/dsa/devlink.c | 13 +
19 files changed, 1556 insertions(+), 16 deletions(-)
create mode 100644 Documentation/networking/devlink/mxl862xx.rst
create mode 100644 drivers/net/dsa/mxl862xx/mxl862xx-fw.c
create mode 100644 drivers/net/dsa/mxl862xx/mxl862xx-fw.h
base-commit: 31f961de2f90fbf52eb2d4e15b3eeaa09f9b4fc2
prerequisite-patch-id: 0000000000000000000000000000000000000000
--
2.55.0
^ permalink raw reply [flat|nested] 2+ messages in thread
* [PATCH net-next v14 3/6] driver core: add device_schedule_reprobe()
2026-09-09 18:46 [PATCH net-next v14 0/6] net: dsa: mxl862xx: devlink flash and rescue Daniel Golle
@ 2026-09-09 18:46 ` Daniel Golle
0 siblings, 0 replies; 2+ messages in thread
From: Daniel Golle @ 2026-09-09 18:46 UTC (permalink / raw)
To: Jiri Pirko, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, Jonathan Corbet, Shuah Khan,
Randy Dunlap, Daniel Golle, Greg Kroah-Hartman,
Rafael J. Wysocki, Danilo Krummrich, Andrew Lunn,
Vladimir Oltean, netdev, linux-doc, linux-kernel, driver-core
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 hold of the device lock and, where the bus needs it, the
parent lock, 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. __device_release_driver() reports whether this call was
the one to release the driver, so an unbind that wins the race while
busy consumer links are being unbound is not followed by a re-attach.
- Both @dev and its parent are pinned for the lifetime of the work, and
whether the parent needs locking is recorded at scheduling time. An
unregister of @dev drops @dev's reference to the parent, and a device
that was unregistered before the work runs may have outlived the
module providing its bus type, so neither dev->parent nor dev->bus is
read before the device is found live under the lock.
- 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 work is queued on system_freezable_wq. A re-probe pending across
system suspend therefore runs once the devices have resumed and
probing is unblocked again, instead of detaching a device the PM core
has already suspended or running concurrently with its late suspend
callbacks, which the PM core invokes without the device lock. The
attach half is plain device_attach(), which checks the dead flag, and
it takes the parent lock 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.
Assisted-by: LLM
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v14: no changes
v13:
- queue the work on system_freezable_wq, so a re-probe pending across
system suspend can neither detach a device the PM core has suspended
nor race its late suspend callbacks; it runs after resume instead
(found by Sashiko AI review)
- record at scheduling time whether the parent needs locking, instead
of reading dev->bus in the work, which may be gone with its module
once the device has been unregistered (found by Sashiko AI review)
- let __device_release_driver() report whether it released the driver,
so an administrative unbind that wins the race inside the device
links loop is not undone by the re-attach (found by Sashiko AI
review)
- 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,
on the standalone posting of this helper)
- describe the parent pinning and locking in the commit message, as in
the standalone posting
v12:
- pin the parent device across the deferred work; a reference on the
child alone left device_reprobe_work_fn() dereferencing a freed
dev->parent under __device_driver_lock() when the device was
unregistered before the work ran (found by Sashiko AI review)
- take the parent lock across device_attach() on buses that require
it, matching bus_rescan_devices_helper() (found by Sashiko AI review)
v11: new patch: add device_schedule_reprobe() to the driver core (posted
earlier as an RFC) so mxl862xx can schedule its post-flash and
post-drain re-probe through the core instead of open-coding a work
item
drivers/base/base.h | 5 ++
drivers/base/core.c | 3 ++
drivers/base/dd.c | 120 ++++++++++++++++++++++++++++++++++++++++-
include/linux/device.h | 2 +
4 files changed, 128 insertions(+), 2 deletions(-)
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..81a3433f726e 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -1315,7 +1315,7 @@ EXPORT_SYMBOL_GPL(driver_attach);
* __device_release_driver() must be called with @dev lock held.
* When called for a USB interface, @dev->parent lock must be held as well.
*/
-static void __device_release_driver(struct device *dev, struct device *parent)
+static bool __device_release_driver(struct device *dev, struct device *parent)
{
struct device_driver *drv;
@@ -1336,7 +1336,7 @@ static void __device_release_driver(struct device *dev, struct device *parent)
*/
if (dev->driver != drv) {
pm_runtime_put(dev);
- return;
+ return false;
}
}
@@ -1359,7 +1359,10 @@ static void __device_release_driver(struct device *dev, struct device *parent)
bus_notify(dev, BUS_NOTIFY_UNBOUND_DRIVER);
kobject_uevent(&dev->kobj, KOBJ_UNBIND);
+ return true;
}
+
+ return false;
}
void device_release_driver_internal(struct device *dev,
@@ -1436,3 +1439,116 @@ 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;
+ bool parent_lock;
+};
+
+static void device_reprobe_work_fn(struct work_struct *work)
+{
+ struct device_reprobe *rp = container_of(work, struct device_reprobe,
+ work.work);
+ struct device *parent = rp->parent;
+ struct device *dev = rp->dev;
+ bool detached = false;
+ int ret;
+
+ /*
+ * A device unregistered before the work runs may have outlived the
+ * module providing its bus type, so dev->bus is not read until the
+ * device is known to be live.
+ */
+ if (rp->parent_lock)
+ device_lock(parent);
+ device_lock(dev);
+ /*
+ * 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 == rp->drv)
+ detached = __device_release_driver(dev, parent);
+ device_unlock(dev);
+ if (rp->parent_lock)
+ device_unlock(parent);
+
+ if (detached) {
+ /*
+ * device_attach() must run with the parent locked on buses
+ * that require it, mirroring bus_rescan_devices_helper().
+ */
+ if (rp->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 (rp->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.
+ *
+ * The work is freezable: a re-probe pending across system suspend runs
+ * once the system has resumed.
+ *
+ * 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->parent_lock = dev->parent && dev->bus->need_parent_lock;
+ rp->drv = READ_ONCE(dev->driver);
+ 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);
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2026-09-12 0:28 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-09-12 0:28 [PATCH net-next v14 3/6] driver core: add device_schedule_reprobe() netdev-bot+sashiko
-- strict thread matches above, loose matches on Subject: below --
2026-09-09 18:46 [PATCH net-next v14 0/6] net: dsa: mxl862xx: devlink flash and rescue Daniel Golle
2026-09-09 18:46 ` [PATCH net-next v14 3/6] driver core: add device_schedule_reprobe() Daniel Golle
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®