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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] device_schedule_reprobe() (drivers/base/dd.c:1578-1579 and 1597)
  reads the bitfield dev->p->dead with plain accesses: first with no…

--

driver core: add device_schedule_reprobe()

This commit adds device_schedule_reprobe(). It detaches and re-probes a
device after a delay chosen by the caller, from work that lives in
built-in driver core code, so callers do not need a module reference.
Each device has at most one request, tracked in struct device_private,
and device_del() and device_unbind_cleanup() cancel it.

> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index f6525a7ee8c54..21973cf0f1a41 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c

[ ... ]

> +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 ||
> +	    dev->p->dead || !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 or deletion that got here first has cleared the driver or
> +	 * set dead 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 (dev->p->dead || READ_ONCE(dev->driver) != drv)

[Severity: Low]
This isn't a functional bug, but will KCSAN report these reads of
dev->p->dead as a data race?

kill_device() writes the bitfield while holding only the device lock:

drivers/base/core.c:kill_device() {
	device_lock_assert(dev);
	...
	dev->p->dead = true;
}

device_schedule_reprobe() reads it with plain accesses in two places. The
first read takes no lock. The second runs under device_reprobe_lock, but
kill_device() never takes that lock. The kernel-doc says callers do not
need to hold the device lock. So device_del() can run at the same time,
for example when a device removal races the mxl862xx devlink
flash_update path. The first check also has an unlocked read of
kobj.state_in_sysfs through device_is_registered().

The ordering looks correct. kill_device() is followed by device_unlock()
and then spin_lock() in device_reprobe_cancel(). A schedule that runs
after the cancel sees dead set, and a request published before the
cancel gets cancelled.

Would it help to wrap these lockless reads in data_race(), or add a
comment saying they are intentional snapshots? READ_ONCE() cannot be used
on the bitfield.

The plain read of defer_all_probes in device_reprobe_work_fn() is
similar, although it matches existing unannotated reads elsewhere in
dd.c.

> +		ret = -EINVAL;
> +	else
> +		WRITE_ONCE(dev->p->reprobe, rp);

[ ... ]

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