dpll: add a simulated DPLL device driver

tools/testing/selftests/drivers/dpll/dpll_basic.sh line 62

[Severity: Low]
Is DPLLSIM_CREATED_NAME always empty here when this run never created a device? It is only assigned inside dpllsim_create_device(). Neither dpll_lib.sh nor the scripts initialize it at the top level, unlike DPLLSIM_MOUNTED_CONFIGFS=0 and DPLL_LOCK_FD="" here, and DPLL_CREATED_DEVICE="" and DPLLSIM_PROBE_DEVICE="" later in the series.

Bash imports environment variables as shell variables. If DPLLSIM_CREATED_NAME is set in the inherited environment, this EXIT trap undeploys and removes an existing $DPLLSIM_CFG/<name> device even on paths that never create anything. Examples are a run against hardware, the dpllsim_check_module() skip, or an early signal.

cleanup() in dpll_stress.sh has the same check, and the variable is still uninitialized at the end of the series. Would a top-level DPLLSIM_CREATED_NAME="" in dpll_lib.sh be enough?

tools/testing/selftests/drivers/dpll/dpll_basic.sh line 84

[Severity: Medium]
Should every failure of dpllsim_setup_standard_device() be reported as a skip? That function returns 1 in three cases: the configfs mkdir fails, the write of 1 to <dev>/live fails, or dpll_device_wait_by_clock_id() times out.

The live write goes to dpllsim_dev_live_store(), which passes back the DPLL core registration error:

drivers/dpll/dpllsim/core.c:dpllsim_dev_live_store() {
    ...
	err = dpllsim_dev_deploy(sim);
	if (err)
		return err;
    ...
}

The timeout means the registered device never showed up in the netlink dump. Both are failures of the core and netlink paths this test covers. dpll_stress.sh treats the same condition as a failure in test_deploy_cycle():

check_err 1 "device not on netlink (iteration $i)"

Could only environment problems be skipped here, with deploy and netlink visibility failures reported as failures? This check is unchanged at the end of the series.

tools/testing/selftests/drivers/dpll/dpll_lib.sh line 123

[Severity: Medium]
Can a failed dump be told apart from a dump with no matching device here? Both the DPLL_DRIVER branch and the default branch use a pipeline, and nothing sets pipefail, so the command substitution only returns the status of jq.

If dpll device show -j fails, jq reads empty input, prints nothing and exits 0, so DPLL_DEV_ID ends up empty.

With DPLL_DRIVER set to a hardware driver, the run then skips with "no DPLL device of driver". With neither variable set, dpll_basic.sh decides the host has no hardware and tests a dpllsim device instead:

dpll_basic.sh
dpll_lib_init()
dpll device show -j fails, jq prints nothing
DPLL_DEV_ID is empty
if [ -z "$DPLL_DEV_ID" ]; then
dpllsim_check_module
dpllsim_setup_standard_device

The commit message says:

Every query runs the tool first and jq on what it printed, so that a failed query is told apart from a missing attribute.

The other helpers follow that with out=$(dpll ...) || return 1, but the device selection path does not. The DPLL_MODULE_NAME lookup at the end of dpll_lib_init() also ignores a failure of dpll_device_get_module().

This is unchanged at the end of the series. dpll_features.sh, added later in the series, calls the same dpll_lib_init().

[ ... ]

tools/testing/selftests/drivers/dpll/dpll_lib.sh line 232

[Severity: Low]
Is the "one side effect" comment above accurate? dpll_lib_run_dir() and dpll_lib_lock() also create /run/dpll-selftest/ and its lock file, and nothing removes them.

This modprobe is not recorded either, and dpll_lib_cleanup() never unloads the module. A run on a system where dpllsim was not loaded therefore leaves it loaded.

A few other comments also don't match the code. The file header says:

  # devices. Sourcing this file has no side effects; call
  # dpll_lib_check_prereqs and dpll_lib_init explicitly.

dpll_lib_lock is also required, though, and dpll_stress.sh never calls dpll_lib_init.

dpllsim_setup_standard_device() has:

  # shellcheck disable=SC2120 # the argument is optional

but the function uses no positional parameter.

The comment on dpllsim_alloc_clock_id() about per-worker id ranges refers to workers that don't exist in this patch. That part is resolved later in the series by "selftests: dpll: add concurrency stress tests".

At the end of the series, the header, the SC2120 directive and the side effect comment are still there. The side effect comment now reads "which are the only side effects the library has outside the dpllsim directory", and it still leaves out the /run files and the module load.

tools/testing/selftests/drivers/dpll/dpll_lib.sh line 235

[Severity: Low]
Can a signal leave configfs mounted while DPLLSIM_MOUNTED_CONFIGFS is still 0? Both scripts install trap cleanup EXIT and trap 'exit 1' INT TERM before dpllsim_check_module().

A SIGTERM or SIGINT that arrives while mount runs is handled after mount returns, before the right-hand side of the && list runs:

  mount -t configfs ... succeeds
  pending trap: exit 1
    cleanup()
      dpll_lib_cleanup()
        [ "$DPLLSIM_MOUNTED_CONFIGFS" -eq 1 ] || return 0

dpllsim_create_device() records its name before mkdir for this reason. The mount flag doesn't get the same ordering, so "a timeout imposed by the harness still cleans up" doesn't hold for the mount. The same pattern is used for debugfs (DPLL_MOUNTED_DEBUGFS) later in the series.

tools/testing/selftests/drivers/dpll/dpll_lib.sh line 257

[Severity: Low]
Could this unmount configfs while another test is using it? The loop above only looks at $DPLLSIM_CFG/*/. The flock in dpll_lib_lock() only orders dpll selftests against each other.

Other configfs users, such as the gpio-sim, netconsole or USB gadget tests, use the same check-then-mount pattern without a lock. Say one of them starts while a dpll script holds the mount:

- It sees configfs already mounted and skips its own mount.
- It starts using /sys/kernel/config/<subsys>.
- This umount succeeds as long as that test has no open file or cwd in the mount, and its paths disappear in the middle of the test.

dpll_lib.sh seems to be the only selftest that unmounts configfs. Later in the series, "selftests: dpll: add reference leak tests" applies the same mount and unmount handling to debugfs.

tools/testing/selftests/drivers/dpll/dpll_lib.sh line 284

[Severity: Low]
What happens when mkdir fails because a directory of that name already exists? Leftover devices can exist: dpll_lib_cleanup() deliberately keeps devices it could not remove, and the probe added later in the series retries on exactly that case.

A SIGTERM or SIGINT that arrives while mkdir runs is handled after mkdir returns, before DPLLSIM_CREATED_NAME is cleared:

DPLLSIM_CREATED_NAME=$name
mkdir "$dev_path" fails with EEXIST pending trap: exit 1
cleanup()
dpllsim_delete_device "$DPLLSIM_CREATED_NAME"

In that case, cleanup() undeploys and removes a device this run did not create.

drivers/dpll/dpllsim/core.c static struct dpllsim_dev *to_dpllsim_dev(struct config_item *item) · line 171

[Severity: Low]
Should these two mkdir limits be described in
Documentation/driver-api/dpllsim.rst?

Once the instance is frozen, mkdir of a new pin fails with EBUSY. Once the instance has DPLLSIM_MAX_PINS (128) pins, it fails with ENOSPC. The documentation only says:

Structural attributes return EBUSY while the instance is live and pins of a live instance cannot be removed.

It doesn't say that pins can't be added to a live instance, and it doesn't mention the 128 pin cap. The commit message also says an instance can carry "an arbitrary set of them".

Separately, a pin with an invalid frequency or direction makes live=1 fail with a bare -EINVAL from dpllsim_order_pins():

	list_for_each_entry(pin, &sim->pin_list, list) {
		if (!dpllsim_pin_frequency_valid(pin) ||
		    !dpllsim_pin_direction_valid(pin))
			return -EINVAL;

Nothing tells the user which pin was rejected. Could that be reported?

drivers/dpll/dpllsim/core.c static bool dpllsim_clock_id_in_use(struct dpllsim_dev *sim) · line 283

[Severity: Low]
Can a pin mkdir or rmdir that runs at the same time as "echo 1 > live" on the same instance make this return -ENOENT?

The comments above dpllsim_pins_drop_item() and dpllsim_dev_live_store() say the subsystem mutex excludes pin mkdir and rmdir. However, configfs only holds su_mutex around the make_item() and drop_item() callbacks.

For mkdir, dpllsim_pins_make_item() adds the pin to sim->pin_list under su_mutex. The dirent is only attached and readied after the mutex has been dropped:

fs/configfs/dir.c:configfs_mkdir() {
    ...
	mutex_unlock(&subsys->su_mutex);
    ...
		ret = configfs_attach_item(item, dentry, frag);
    ...
		configfs_dir_set_ready(dentry->d_fsdata);
    ...
}

Suppose dpllsim_dev_live_store() takes su_mutex inside that window. dpllsim_dev_depend_pins() then finds the half-created pin on pin_list. configfs_depend_prep() doesn't find it, either because there is no dirent yet or because it skips CONFIGFS_USET_CREATING entries. So live=1 fails with -ENOENT.

rmdir has the same window. configfs_rmdir() marks the item as dropping in configfs_detach_prep() and detaches the dirent without su_mutex. Only after that does it take su_mutex for client_drop_item()->dpllsim_pins_drop_item(), which does the list_del():

fs/configfs/dir.c:configfs_rmdir() {
    ...
			ret = configfs_detach_prep(sd, &wait);
    ...
		configfs_detach_item(dentry);

		mutex_lock(&subsys->su_mutex);
    ...
	client_drop_item(parent_item, item);
	mutex_unlock(&subsys->su_mutex);
    ...
}

A live=1 in between still sees the pin on pin_list, and the depend fails with -ENOENT.

Nothing is corrupted and a retry succeeds. Should the comments be corrected, or should a pin that is still being created or removed be handled here?

drivers/dpll/dpllsim/dpll.c static const struct dpll_device_ops dpllsim_dpll_ops = { · line 152

[Severity: Low]
Should dpllsim.rst list the frequencies each pin type accepts? The frequency row only says:

Frequency in Hz, must be supported by the pin type (default 0, which is rejected)

dpllsim_pin_frequency_valid() accepts only the fixed tables chosen here:

gnss: 1 Hz
int-oscillator: 25 MHz
synce-eth-port and mux: 1.544, 2.048, 10 and 25 MHz
ext: 1 Hz and 10, 19.44, 25, 77.76, 125, 156.25 MHz

The documentation also doesn't say that mux uses the SyncE table.

freq-supported can only be seen over netlink after live=1 has succeeded. An unsupported frequency makes live=1 fail with a bare -EINVAL that names neither the pin nor the value.

[ ... ]

Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109093523.931201-1-poros%40redhat.com

tools/testing/selftests/drivers/dpll/dpll_basic.sh test_device_properties() · line 79

[Severity: Medium]
Can a failed pin dump get reported as a skip here?

dpll_pin_get_ids() returns 1 when the tool fails:

	out=$(dpll pin show -j 2>/dev/null) || return 1

The exit status of a process substitution is lost, though, and mapfile itself succeeds. So a failed query leaves pin_ids empty, which looks the same as a device with no pins.

test_pin_properties() uses the same pattern:

	mapfile -t pin_ids < <(dpll_pin_get_ids "$DPLL_DEV_ID")

test_pin_state() has the same problem. pipefail is not enabled there, so the pipeline returns the status of head:

	pin_id=$(dpll_pin_find_by_direction "$DPLL_DEV_ID" input | head -1)

All three then call dpll_log_test_skip_or_fail(), which calls log_test_skip whenever DPLL_CREATED_DEVICE is empty. That happens for the hardware device dpll_lib_init() picks by default, for a device chosen through DPLL_DRIVER and for one set with DPLL_DEV_ID.

On such a device, would a pin dump that fails because of a driver or netlink regression be logged as a skip with "no pins found" or "no input pin", and not as a failure? On the created dpllsim device the test still fails, but it gives "no pins found" as the reason and does not say that the query failed.

The commit message says:

every pin query runs the tool first and jq on what it printed, so that a query which fails returns the status of the tool instead of the empty output a missing attribute produces too

None of these callers seem to check that status.

tools/testing/selftests/drivers/dpll/dpll_basic.sh test_device_properties() · line 109

[Severity: Low]
This isn't a bug, but is the first sentence of this comment accurate? In the DPLL core, whether the parent-device nest is present depends on how the pin was registered, not on its type:

drivers/dpll/dpll_netlink.c:dpll_cmd_pin_get_one() {
    ...
	if (xa_empty(&pin->parent_refs))
		ret = dpll_msg_add_pin_dplls(msg, pin, extack);
	else
		ret = dpll_msg_add_pin_parents(msg, pin, ref, extack);
    ...
}

A pin registered directly on a device always gets parent-device nests, MUX pins included. A pin registered on a parent pin gets parent-pin nests instead, whatever its type.

The comment above dpll_pin_get_ids() in dpll_lib.sh gets this right ("pins registered on those pins (MUX children)"). Could this comment say the same thing, that the nest is missing for pins registered on a parent pin? The check itself is not affected.

[ ... ]

Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109093523.931201-1-poros%40redhat.com

tools/testing/selftests/drivers/dpll/dpll_features.sh line 9

[Severity: Low]
Is this description accurate? Nothing in the script seems to skip based on whether the device is a dpllsim one.

The only simulator-specific check in test_pin_state_transitions() depends on DPLL_CREATED_DEVICE, not on the module name:

	if [ "$mode" = automatic ] && [ -n "$DPLL_CREATED_DEVICE" ]; then

So a dpllsim device passed through DPLL_DEV_ID silently loses that check, and no skip line is logged.

Also, dpll_lib_init() clears DPLL_DEV_ID when DPLL_DRIVER=dpllsim:

	elif [ "${DPLL_DRIVER:-}" = "dpllsim" ]; then
		DPLL_DEV_ID=""

So that setting always creates a new simulator device instead of running against an existing one. The header is still the same at the end of the series.

[ ... ]

tools/testing/selftests/drivers/dpll/dpll_features.sh line 105

[Severity: Medium]
Do these two deferred commands run in the order the comment describes?

defer_scope_pop() in net/lib/sh/defer.sh runs deferred jobs in reverse order, last in first out:

	for ((defer_ix=$(__defer__ndefers d); defer_ix-->0; )); do
		__defer__run d $defer_ix
	done

tests_run() wraps every test in in_defer_scope(), so both commands run after every test, including one that passed. In manual mode the sequence would be:

test body restores PIN_TUNE_ID to $before and reconnects $ref_pin test body finds $ref_pin connected, log_test reports a pass defer_scope_pop() -> dpll_pin_set_state ... "$ref_pin" connected defer_scope_pop() -> dpll_pin_set_state ... "$PIN_TUNE_ID" disconnected

On zl3073x, ZL_DPLL_MODE_REFSEL_MODE_REFLOCK is reported as manual mode. In that mode, zl3073x_dpll_input_pin_state_on_dpll_set() handles a disconnect request on any input pin by leaving the reference:

	case ZL_DPLL_MODE_REFSEL_MODE_REFLOCK:
		...
		} else if (state == DPLL_PIN_STATE_DISCONNECTED) {
			/* Choose new mode based on lock status */
			switch (zldpll->lock_status) {
			...
				mode = ZL_DPLL_MODE_REFSEL_MODE_FREERUN;

The dpll core does not skip a set that matches the current state.

Could this last deferred disconnect of the test pin leave the device in holdover or freerun after the test has already reported a pass?

That would also contradict the commit message:

"connects it back at the end and checks that it is the reference again"

An interrupted run has the same ordering problem. The reconnect of $ref_pin runs while PIN_TUNE_ID may still be the selected reference, and PIN_TUNE_ID is disconnected afterwards.

Would registering the $ref_pin defer before the PIN_TUNE_ID one give the intended order? This code is still the same at the end of the series.

[ ... ]

tools/testing/selftests/drivers/dpll/dpll_features.sh line 124

[Severity: Medium]
Can this skip hide a failure that an earlier iteration recorded?

Suppose the first state is set but reads back differently, so check_err records a failure. If the second set is then refused on a user-provided device, dpll_log_test_skip_or_fail() calls log_test_skip(), which does:

RET=$ksft_skip retmsg= log_test "$@"

The test then reports a skip and the earlier failure message is lost.

The restore path further down guards against this with
[ "$RET" -eq 0 ], and the comment above the loop says "a skip would hide the earlier one". The loop path has no such check.

tools/testing/selftests/drivers/dpll/dpll_features.sh line 129

[Severity: Medium]
In automatic mode, can the selectable step change the reference the device is locked to?

When PIN_TUNE_ID starts out disconnected, setting it selectable puts it back into hardware selection with its configured priority. ice enables the input in ice_dpll_input_state_set():

	bool enable = state == DPLL_PIN_STATE_SELECTABLE;

zl3073x restores pin->prio in the hardware priority table.

If the pin outranks the current reference and has a valid signal, the DPLL would switch to it. The restore to disconnected would then switch it back.

The commit message says the pin is picked "so that no test touches the reference the device is locked to". However, pick_tune_pin() only looks at the state, not at priority or operstate.

On ice, ice_dpll_pin_state_update() reports the active input as connected:

	pin->state[pf->dplls.eec.dpll_idx] =
		pin->pin == pf->dplls.eec.active_input ?
		DPLL_PIN_STATE_CONNECTED :
		DPLL_PIN_STATE_SELECTABLE;

Would this readback then fail with "pin not selectable, state 'connected'", depending on how the board's priorities are set up?

[ ... ]

tools/testing/selftests/drivers/dpll/dpll_features.sh line 245

[Severity: Medium]
On zl3073x, could this pick the reference the device is locked to?

In automatic mode, zl3073x_dpll_input_pin_state_on_dpll_get() never reports connected:

	case ZL_DPLL_MODE_REFSEL_MODE_AUTO:
		if (zl3073x_chan_ref_is_selectable(chan, ref))
			*state = DPLL_PIN_STATE_SELECTABLE;
		else
			*state = DPLL_PIN_STATE_DISCONNECTED;

So the active reference reads as selectable and passes this filter. pick_tune_pin() prefers a qualifying pin other than input_pins[0], so it would pick the locked reference whenever that reference is not the first dumped pin.

test_pin_state_transitions() then sets the pin disconnected. The ZL_DPLL_MODE_REFSEL_MODE_AUTO branch of zl3073x_dpll_input_pin_state_on_dpll_set() handles that like this:

			/* Set pin priority to none in HW */
			zl3073x_chan_ref_prio_set(&chan, ref,
						  ZL_DPLL_REF_PRIO_NONE);

That drops the active reference.

Devices that do report the active input as connected have a related gap. The state is checked only once here, before any test runs. test_pin_state_transitions() reads $before again but does not reject connected in automatic mode. A hardware reselection between pick_tune_pin() and the test would therefore go unnoticed.

Would checking the pin operstate (active or standby), and checking again right before the first set, avoid touching the locked reference? This code is still the same at the end of the series.

tools/testing/selftests/drivers/dpll/dpll_features.sh line 249

[Severity: Low]
This isn't a bug, but the commit message and the comment above pick_tune_pin() don't match this code.

The commit message says the script picks:

"the first input pin that is not connected on the device"

This early return, however, prefers any qualifying pin other than input_pins[0].

The comment explains that choice with "the one a simulated device locks to once it is selectable". dpllsim_pin_state_set() only stores the state, though:

	sim_pin->state = state;
	return 0;

Nothing in dpllsim selects a reference or changes the lock status from its initial unlocked value.

Could the comment and the commit message be updated?

[ ... ]

tools/testing/selftests/drivers/dpll/dpll_stress.sh test_rapid_create_delete() · line 181

[Severity: Low]
Can this readback catch a state setter that does nothing? With i running from 1 to 50, the last request is states[50 % 2], which is disconnected.

dpllsim_pin_reset() initialises every pin to that same state:

	pin->state = DPLL_PIN_STATE_DISCONNECTED;

On a freshly created device, a state_on_dpll_set that returned 0 without storing anything would still pass. The fails counter only catches non-zero returns. The test is still the same at the end of the series.

Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109093523.931201-1-poros%40redhat.com

Documentation/driver-api/dpllsim.rst Pin directory ``/sys/kernel/config/dpllsim/<dev>/pins/<pin>/``: · line 41

[Severity: Low]
Is "used for automatic reference selection" accurate for dpllsim at this point?

As far as I can tell, only dpllsim_pin_prio_get(), dpllsim_pin_prio_valid() and dpllsim_pin_prio_show() read pin->prio. The device ops have no selection logic:

static const struct dpll_device_ops dpllsim_dpll_ops = {
.mode_get = dpllsim_mode_get, .lock_status_get = dpllsim_lock_status_get,
};

sim->lock_status stays at the DPLL_LOCK_STATUS_UNLOCKED value set in dpllsim_make_group(). Nothing changes a pin's state based on its priority.

The example further down sets gnss0 prio 0 and ext0 prio 1 as if that sets up a reference order. The selftest library comment in dpllsim_setup_standard_device() ("gnss0: ... prio 0 (active reference)") gives the same impression.

The commit message says priorities "will drive the automatic reference selection once the simulation loop exists". I couldn't find a later patch in the series that adds that loop.

Could the documentation say that the priority is currently only stored and reported back?

[ ... ]

drivers/dpll/dpllsim/dpll.c static int dpllsim_pin_state_set(const struct dpll_pin *pin, void *pin_priv, · line 168

[Severity: Low]
Should the netlink setter write to the same field that holds the priority configured through configfs?

dpllsim_pins_make_item() and dpllsim_pin_prio_store() set pin->prio, and this function writes it at runtime. On each live=1, dpllsim_order_pins() calls dpllsim_pin_reset(), which resets ops and state but not prio:

static void dpllsim_pin_reset(struct dpllsim_pin *pin)

{
	pin->ops = dpllsim_pin_select_ops(pin);
	pin->state = DPLL_PIN_STATE_DISCONNECTED;
}

dpllsim_dev_undeploy() doesn't restore the configured value either.

Consider this sequence:

echo 1 > pins/ext0/prio
echo 1 > live
dpll pin set ... prio 200
echo 0 > live
echo 1 > live

The second deployment starts with prio 200. Reading pins/ext0/prio through dpllsim_pin_prio_show() also returns 200, which was never written through configfs.

There is a second case. Take an input pin whose prio was changed over netlink to something other than 128. Then write 0 to live, write output to direction, and write 1 to live. The live write fails with EINVAL from dpllsim_order_pins(), because of a priority that was only set at runtime:

static bool dpllsim_pin_prio_valid(const struct dpllsim_pin *pin)

{
	return pin->direction == DPLL_PIN_DIRECTION_INPUT ||
	       pin->prio == DPLLSIM_DEFAULT_PIN_PRIO;
}

The commit message describes the attribute as setting the initial priority "so that a test can lay out the reference order before the device goes live". Neither the commit message nor dpllsim.rst says that netlink changes carry into later deployments and replace that layout.

The later commit "dpll: dpllsim: Add support to set frequency on pins" takes the same shared-field approach for frequency. No later patch in the series separates the configured and runtime prio or resets it.

Would it make sense to keep the configured priority separate and copy it into the runtime value in dpllsim_pin_reset()?

[ ... ]

Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109093523.931201-1-poros%40redhat.com

tools/testing/selftests/drivers/dpll/dpll_features.sh find_connected_pin() · line 73

[Severity: Low]
Is "the simulator accepts every priority" accurate? The commit message makes the same claim. However, dpllsim_pin_prio_set() rejects anything above DPLLSIM_PIN_PRIO_MAX, which is 255 in dpllsim.h:

drivers/dpll/dpllsim/dpll.c:dpllsim_pin_prio_set() {
    ...
	if (prio > DPLLSIM_PIN_PRIO_MAX) {
		NL_SET_ERR_MSG_FMT(extack, "Pin priority must be in range 0-%u",
				   DPLLSIM_PIN_PRIO_MAX);
		return -EINVAL;
	}
    ...
}

The configfs store dpllsim_pin_prio_store() applies the same limit.

The test still works today. Since before is capped at 255, the candidates (before - 1, 1 and 7) are always in range.

This comment is the reason a refusal on the created device is a failure and not a skip. Could it say "every priority in 0-255" or "every value tried here"? The comment is still the same at the end of the series.

tools/testing/selftests/drivers/dpll/dpll_features.sh find_connected_pin() · line 79

[Severity: Medium]
Can this make a user-provided device switch its reference?

The comment above pick_tune_pin() says PIN_TUNE_ID "must not be the reference the device is locked to". It also says tests skip "rather than reconfiguring the reference". But pick_tune_pin() accepts selectable pins:

	case "$state" in
	disconnected|selectable)
		;;

In automatic mode, a selectable input takes part in reference selection by priority. This loop gives that pin a higher priority (a smaller value). It never compares the new value with the priority of the current reference.

On a device set through DPLL_DEV_ID or DPLL_DRIVER, the new priority reaches hardware straight away. zl3073x_dpll_input_pin_prio_set() writes REF_PRIO through zl3073x_chan_state_set() when zl3073x_chan_ref_is_selectable() holds. ice_dpll_hw_input_prio_set() sends it to the CGU firmware through ice_aq_set_cgu_ref_prio().

Take a device in automatic, revertive mode with a valid signal on the spare input. If before - 1 ties the locked reference's priority, or if 1 or 7 beats it, could the DPLL switch to the spare pin? Restoring the priority could then switch it back. Each switch can disturb outputs feeding SyncE or PTP.

Would it work to pick a target that does not outrank the connected reference? Other options are to change the priority only on disconnected pins, or to skip in automatic mode on user-provided devices.

tools/testing/selftests/drivers/dpll/dpll_features.sh find_connected_pin() · line 96

[Severity: Medium]
What happens here in the case the candidate loop is built for? The commit message says:

since a large current value may mark an unused pin outside the settable range, as the ice driver does with 255.

In that case before itself cannot be set. Suppose before - 1 (254) is refused and then 1 or 7 is accepted. The pin's hardware priority has changed, and this restore to before would be refused for the same reason.

The test would then report a failure, not a skip, on a user-provided device. The deferred restore sends the same value, so it also fails, and without any message. The unused input stays at priority 1 or 7 on the user's hardware, and in automatic mode it could then be picked as a reference.

ice_dpll_input_prio_set() and ice_dpll_hw_input_prio_set() do no range check of their own. They pass (u8)prio to firmware, so only firmware decides whether 255 can be written back.

test_pin_state_transitions() treats a refused restore as a skip when RET is 0, but this test does not. Should it first check that before can be set, for example by writing it back once, before trying a different value? The code is still the same at the end of the series.

tools/testing/selftests/drivers/dpll/dpll_lib.sh dpllsim_alloc_clock_id() · line 582

[Severity: Low]
Is "(active reference)" correct for the simulator? It does not seem to ever select or lock to a reference.

dpllsim_pin_reset() starts every pin disconnected:

	pin->ops = dpllsim_pin_select_ops(pin);
	pin->state = DPLL_PIN_STATE_DISCONNECTED;

dpllsim_pin_prio_set() only stores sim_pin->prio, and nothing else reads it. sim->lock_status is set once to DPLL_LOCK_STATUS_UNLOCKED in core.c and never changes.

The device is always in automatic mode. dpllsim_pin_state_set() accepts only disconnected and selectable there, so no pin is ever connected.

So gnss0 never becomes the active reference, and its prio 0 does not affect which pin is used. That includes the tie test_pin_priority_get_set() can create by setting ext0 to 0. The comment is still the same at the end of the series, and no later patch adds selection by priority.

drivers/dpll/dpllsim/dpll.c static int dpllsim_pin_frequency_get(const struct dpll_pin *pin, · line 93

[Severity: Low]
This isn't a bug, but should Documentation/driver-api/dpllsim.rst be updated along with this?

dpllsim_pin_frequency_set() writes sim_pin->frequency. That is the same field the configfs frequency attribute stores and shows through dpllsim_pin_frequency_store() and dpllsim_pin_frequency_show() in core.c. dpllsim_pin_frequency_valid() checks it again on the next live=1.

Nothing on the undeploy path resets it, so a frequency set over netlink:

- shows up when the configfs frequency attribute is read,
- is still there after live=0,
- is used on the next live=1 instead of the value written to configfs.

The rst describes pin attributes only as configuration written before going live:

Attributes are written while the instance is not live, then ``live`` is set to 1 to register the device and its pins with the DPLL subsystem.

``frequency`` Frequency in Hz, must be supported by the pin type
(default 0, which is rejected)

The struct dpllsim_pin comment in dpllsim.h was updated to say "The netlink setters change prio and frequency at runtime under dev->lock", but the rst still doesn't mention runtime changes or that they carry over to the next deploy. Prio, added by the earlier "dpll: dpllsim: Add support to get/set priority on input pins" commit, has the same gap.

Could a sentence be added to dpllsim.rst covering both prio and frequency?

[ ... ]

Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109093523.931201-1-poros%40redhat.com

tools/testing/selftests/drivers/dpll/dpll_features.sh test_pin_state_transitions() · line 264

[Severity: Medium]
Can the original frequency always be written back here? The only check on before is that it is not empty. The DPLL core refuses any value outside the pin's frequency-supported ranges, and it does this before it compares the value with the current frequency:

drivers/dpll/dpll_netlink.c:dpll_pin_freq_set() {
	...
	if (!dpll_pin_is_freq_supported(pin, freq)) {
		NL_SET_ERR_MSG_ATTR(extack, a, "frequency is not supported by the device");
		return -EINVAL;
	}
	...
}

zl3073x hits this case on purpose. It reads a sub-Hz reference back as 0 Hz and leaves 0 out of freq_supported:

drivers/dpll/zl3073x/prop.c:zl3073x_pin_props_get() {
	...
	/* Start with current frequency at index 0. A sub-Hz frequency is
	 * read back as 0 Hz and cannot be set, so it is not offered.
	 */
	j = 0;
	if (curr_freq) {
	...
}

Here is what happens on such a pin on a device the user provided:

- before=0 passes the -z check.
- The jq filter picks the first advertised frequency as target.
- Setting target succeeds and reprograms the hardware reference.
- The explicit restore and the deferred restore are both refused with EINVAL.

Wouldn't this leave the user's device on a frequency the test picked? The commit message says the test "restores the original value".

The phase adjust test later in the series bails out before it schedules the restore when the current value could not be written back. Would a similar check fit here, one that confirms before lies inside an advertised frequency-supported range before the pin is touched?

tools/testing/selftests/drivers/dpll/dpll_features.sh test_pin_state_transitions() · line 283

[Severity: Medium]
Is it safe to reprogram this pin's frequency based only on its state on DPLL_DEV_ID? pick_tune_pin() counts a pin as spare when it is disconnected or selectable on that one device:

tools/testing/selftests/drivers/dpll/dpll_features.sh:pick_tune_pin() {
	...
		state=$(dpll_pin_get_state "$DPLL_DEV_ID" "$pin_id")
		case "$state" in
		disconnected|selectable)
	...
}

State and priority are per parent device, but frequency belongs to the pin itself. It is shared by every DPLL device the pin is registered with.

On zl3073x, every channel gets the same dpll_pin for an input index through dpll_pin_get(clock_id, index). The setter then writes the chip-wide reference state:

drivers/dpll/zl3073x/dpll.c:zl3073x_dpll_input_pin_frequency_set() {
	...
	ref_id = zl3073x_input_pin_ref_get(pin->id);
	ref = *zl3073x_ref_state_get(zldev, ref_id);

	/* Update frequency */
	zl3073x_ref_freq_set(&ref, frequency);

	/* Commit reference state */
	return zl3073x_ref_state_set(zldev, ref_id, &ref);
}

Say the chosen input is selectable on the EEC channel but is the locked reference of the PPS channel. Could the PPS channel then lose lock, go into holdover or switch references until the restore runs? ice also registers its inputs with both its EEC and PPS DPLLs. The test never checks that the other channel locks again.

Should the pin be checked against all of its parent devices before this test changes its frequency?

tools/testing/selftests/drivers/dpll/dpll_features.sh test_pin_state_transitions() · line 286

[Severity: Medium]
Can the restore be refused when before lies outside [min, max]?

The granularity guard is the only check before the defer. The core also checks the range, and it does that first, in dpll_pin_phase_adj_set():

drivers/dpll/dpll_netlink.c:dpll_pin_phase_adj_set() {
    ...
	phase_adj = nla_get_s32(phase_adj_attr);
	if (phase_adj > pin->prop.phase_range.max ||
	    phase_adj < pin->prop.phase_range.min) {
		NL_SET_ERR_MSG_ATTR(extack, phase_adj_attr,
				    "phase adjust value of out range");
		return -EINVAL;
	}
	if (pin->prop.phase_gran && phase_adj % (s32)pin->prop.phase_gran) {
    ...
}

So an out-of-range current value cannot be written back either, the same as a value that is not a granularity multiple.

On such a pin, target is built inside [min, max] and the set succeeds. After that, the explicit restore and the deferred one both fail with -EINVAL. The pin on a user-supplied device then stays at the top (or bottom) of the range, and the test reports a FAIL instead of a skip.

ice may hit this. ice_dpll_phase_range_set() derives the range from the CGU abilities. The current value is read separately from firmware via ice_aq_get_input_pin_cfg() in ice_dpll_pin_state_update(), and nothing checks that the two agree. If the abilities report 0, the range is [0, 0], and any nonzero firmware value is out of range.

Should the guard also bail out when before is less than min or greater than max, before the restore is scheduled?

tools/testing/selftests/drivers/dpll/dpll_stress.sh untrack_device() · line 132

[Severity: Medium]
Does bash really leave the pid unreaped until the wait below? Bash reaps exited background children itself. It does this from its SIGCHLD handler and from waitpid(-1) whenever it waits for a foreground command. It keeps their status in its bgpids table for a later wait.

pids_gone() runs seq, $(sed ...) and sleep 0.1 on every pass. A worker that exits after the SIGTERM can therefore be reaped, and its pid freed, before this loop sends SIGKILL to every entry of pids[]. The loop does not check pid_running() first.

test_concurrent_workers() makes the window wider:

	for pid in "${BACKGROUND_PIDS[@]}"; do
		wait "$pid" || failed=1
	done
	BACKGROUND_PIDS=()

Workers that finish early are reaped right away, but they stay in BACKGROUND_PIDS until the last wait returns. Suppose a harness timeout or Ctrl-C arrives while the parent is still waiting on a slower worker. cleanup() then sends SIGTERM and later SIGKILL, as root, to those stale pids.

If one of those pids was handed out again after a pid-space wrap, could this kill an unrelated process?

pid_running() relies on kill -0 and /proc/<pid>/stat, so a reused pid also looks like a live worker. In that case pids_gone() would spin for the full five seconds and log a false "worker N did not exit".

tools/testing/selftests/drivers/dpll/dpll_stress.sh untrack_device() · line 149

[Severity: Low]
Does the forced kill above reach the external helpers that a worker runs? In dpll_lib.sh, dpllsim_create_device() and dpllsim_add_pin() create the configfs entries with an external mkdir. dpllsim_delete_device() removes them with an external rmdir.

The commit message describes a worker "stuck in the kernel". Take the case where a worker's mkdir blocks in configfs. Bash holds back the worker's TERM trap until the foreground child returns. kill -KILL then kills only the subshell, and the mkdir child keeps running as an orphan.

The second pids_gone() and pid_running() check only the subshell pid. The worker therefore counts as gone and is reaped, and "worker $pid did not exit, giving up" is never logged.

cleanup() then runs delete_worker_devices() and dpll_lib_cleanup() while the orphan is still busy. If its mkdir completes afterwards, can a dpllsim device or pin directory appear after the last removal pass and be leaked?

The in-flight mkdir would also keep the configfs mount busy, so the unmount in dpll_lib_cleanup() may fail.

This seems to contradict this part of the commit message:

"whatever a killed worker left behind can be removed by name, at the end of the test and from the cleanup handler"

Would it help to start each worker in its own process group and signal the whole group, or to track the helper processes?

[ ... ]

tools/testing/selftests/drivers/dpll/dpll_stress.sh test_pin_state_flooding() · line 412

[Severity: Low]
This isn't a bug, but is "maximum devices" the right name for this test? dpllsim has no limit on the number of devices. Its only per-device limit is DPLLSIM_MAX_PINS:

drivers/dpll/dpllsim/core.c:dpllsim_pins_make_item() {
    ...
	if (list_count_nodes(&sim->pin_list) >= DPLLSIM_MAX_PINS)
		return ERR_PTR(-ENOSPC);
    ...
}

By default the test creates 5 devices with 10 pins each. It never reaches that limit, and it never checks that a pin beyond the limit is rejected. The comment above STRESS_PINS_PER_DEVICE mentions the 128 pin limit, but the default is well below it.

The visibility check also only looks up the devices with dpll_device_wait_by_clock_id(). It does not check their pins, so the test would still pass if the pins of these many-pin devices did not appear on netlink.

Could the test be renamed, or extended to cover the pin limit and to check that the pins are visible?

Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109093523.931201-1-poros%40redhat.com

tools/testing/selftests/drivers/dpll/dpll_lib.sh dpllsim_setup_standard_device() · line 719

[Severity: Low]
The only sign that this script owns the mount is that debugfs was not mounted when it checked. If no dpllsim device is left behind, dpll_lib_cleanup() unmounts it at exit:

	[ "$DPLL_MOUNTED_DEBUGFS" -eq 1 ] && mounted+=("$DEBUGFS_DIR")
	...
	for dir in "${mounted[@]}"; do
		__dpll_lib_umount "$dir"
	done

During a long dpll_stress.sh run, another process (another selftest, a tracing tool, a user) can find debugfs already mounted and start using it without mounting it itself.

Would the umount at exit pull debugfs out from under that process? A plain umount succeeds unless a file or cwd is held open at that moment, and the other process would not remount it.

The configfs handling already works this way. This patch applies the same pattern to debugfs, which is shared far more widely.

[ ... ]

tools/testing/selftests/drivers/dpll/dpll_lib.sh dpllsim_setup_standard_device() · line 761

[Severity: Low]
The only attribution here is snapshot membership plus non-empty content. What happens when another DPLL driver (ice, zl3073x, ptp_ocp, ...) registers a new dpll_device or dpll_pin after the snapshot? This could come from a deferred or async probe, a driver bind, a module reload, or a driver re-allocating its pins during the long dpll_stress.sh run.

A live object of that driver always holds tracked references, so its entry is non-empty and not in the snapshot.

Would test_basic_reftrack(), test_final_leak_check() and test_ref_leaks() then wait BUSYWAIT_TIMEOUT and fail, blaming dpllsim for references it does not own?

dpll_lib_lock() is an flock on /run/dpll-selftest/lock, so it only excludes other dpll selftest scripts. The library comment only covers objects of other drivers that already exist when the snapshot is taken.

[ ... ]

tools/testing/selftests/drivers/dpll/dpll_lib.sh dpllsim_setup_standard_device() · line 858

[Severity: Low]
Can this tell a kernel without CONFIG_DPLL_REFCNT_TRACKER apart from one where the tracker is enabled but does not record anything?

dpll_device_alloc() and dpll_pin_alloc() call ref_tracker_dir_init() unconditionally:

	ref_tracker_dir_init(&dpll->refcnt_tracker, 128, "dpll_device");

So the dpll_device@ and dpll_pin@ files exist whenever CONFIG_REF_TRACKER and CONFIG_DEBUG_FS are set. Only dpll_device_tracker_alloc/free and dpll_pin_tracker_alloc/free are under #ifdef CONFIG_DPLL_REFCNT_TRACKER, and an empty dir->list prints nothing.

Suppose a regression makes the core stop calling ref_tracker_alloc(), or pass a NULL tracker so that only no_tracker is bumped. The probe would then return 1, and both scripts log:

	log_test_skip "DPLL ref_tracker probe" \
		      "CONFIG_DPLL_REFCNT_TRACKER not enabled"

This happens even though the selftest config sets CONFIG_DPLL_REFCNT_TRACKER=y. Would such a regression show up as a SKIP with a misleading reason instead of a FAIL?

tools/testing/selftests/drivers/dpll/dpll_lib.sh dpllsim_setup_standard_device() · line 872

[Severity: Medium]
Can the snapshot taken right after the probe record the probe device's own dead ref_tracker entries?

The commit message says:

The teardown of a device releases its references before the write to live returns, so the entries are expected to be gone at the first look

In lib/ref_tracker.c only the references go away synchronously. ref_tracker_dir_exit() calls ref_tracker_debugfs_mark(), which only marks the dir dead and schedules the reaper:

	__xa_set_mark(&debugfs_dentries, (unsigned long)dir, REF_TRACKER_DIR_DEAD);
	...
	schedule_work(&debugfs_reap_worker);

The dentry is removed later, in debugfs_reap_work(). Until then the file still exists, and reading it returns -ENODATA from ref_tracker_debugfs_show().

That allows this sequence:

dpllsim_delete_device(probe)
dpll_device_put() -> ref_tracker_dir_exit(), kfree(dpll)
busywait __dpll_ref_tracker_settled
cat of the dead entry fails, content is empty, entry is skipped
test_ref_tracker_probe()
dpll_ref_tracker_snapshot "$REF_SNAPSHOT"
[ -f "$entry" ] is still true, so the probe's names are recorded

The next test (test_basic_reftrack() or test_rapid_create_delete()) then allocates a dpll_device right away. It can land on the probe's freed slab slot, and the %p hash then gives the same entry name.

If the reaper has run by then, the new object's entry is in the snapshot, and __dpll_ref_tracker_new_entries() skips it:

	grep -q "^$name " "$file" && continue

If the reaper still has not run, ref_tracker_dir_debugfs() returns early:

	/* No-op if already created */
	dentry = xa_load(&debugfs_dentries, (unsigned long)dir);
	if (dentry && !xa_is_err(dentry))
		return;

The reaper later removes the old dentry, so the new object never gets a debugfs file at all.

In either case, would a leak in the first device created after the probe go unreported?

Would it help to skip unreadable entries in dpll_ref_tracker_snapshot(), or to wait after deleting the probe until its entries are gone from the ref_tracker dir before taking the snapshot again?

[ ... ]

tools/testing/selftests/drivers/dpll/dpll_stress.sh test_deploy_cycle() · line 539

[Severity: Low]
Should this skip when CREATED_DEVICES is not empty, the way test_final_leak_check() in dpll_reftrack.sh does?

# A device an earlier test could not remove still holds its # references, the failure to report is that one.
if [ -n "$CREATED_DEVICE" ]; then
log_test_skip "DPLL final ref_tracker check" \
"device from an earlier test still present"

dpllsim_delete_device() can fail in test_pin_state_flooding(), test_max_devices() or test_deploy_cycle() because the undeploy failed. The device then stays live and stays in CREATED_DEVICES.

It still holds its dpll_device_get(..., &sim->tracker) and dpll_pin_get(..., &pin->tracker) references, so its entries are non-empty and not in the snapshot.

cleanup() only removes those devices at exit. Wouldn't test_ref_leaks() spin for BUSYWAIT_TIMEOUT and then report "leaked DPLL references after stress" for references the device legitimately holds?