Re: [PATCH net-next v3 07/14] dpll: dpllsim: Add support to get/set priority on input pins

[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] One field, `pin->prio`, holds both the configfs setting and the
  live value.
- [Low] Documentation/driver-api/dpllsim.rst (line 39) describes the new
  `prio` attribute as 'Priority 0-255 used for automatic reference…

--

dpll: dpllsim: Add support to get/set priority on input pins

This commit adds prio_get and prio_set callbacks for dpllsim input pins
and advertises the priority-can-change capability. It also adds a prio
configfs attribute that sets the initial priority before the device goes
live. Going live is rejected when an output pin has a non-default
priority.

> diff --git a/Documentation/driver-api/dpllsim.rst b/Documentation/driver-api/dpllsim.rst
> index e7c5ee93392c2..5109d67325f14 100644
> --- a/Documentation/driver-api/dpllsim.rst
> +++ b/Documentation/driver-api/dpllsim.rst
> @@ -36,6 +36,9 @@ Pin directory ``/sys/kernel/config/dpllsim/<dev>/pins/<pin>/``:
>                 (default 0, which is rejected)
>  ``direction``  ``input`` or ``output`` (default ``input``), a ``mux`` pin
>                 must be an input
> +``prio``       Priority 0-255 used for automatic reference selection,
> +               input pins only, output pins must keep the default
> +               (default 128)

[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?

[ ... ]

> diff --git a/drivers/dpll/dpllsim/dpll.c b/drivers/dpll/dpllsim/dpll.c
> index 794020d0c112e..5e36059609761 100644
> --- a/drivers/dpll/dpllsim/dpll.c
> +++ b/drivers/dpll/dpllsim/dpll.c

[ ... ]

> +static int dpllsim_pin_prio_set(const struct dpll_pin *pin, void *pin_priv,
> +				const struct dpll_device *dpll,
> +				void *dpll_priv, u32 prio,
> +				struct netlink_ext_ack *extack)
> +{
> +	struct dpllsim_pin *sim_pin = pin_priv;
> +
> +	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;
> +	}
> +
> +	guard(mutex)(&sim_pin->dev->lock);
> +	sim_pin->prio = prio;

[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