Re: [PATCH net-next v3 03/14] dpll: dpllsim: Add configurable 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] A pin mkdir or rmdir that runs alongside `live=1` on the same
  instance makes the live transition fail with -ENOENT.
- [Low] Two pin-creation limits are visible to users but not documented.
- [Low] dpllsim.rst describes the pin frequency attribute only as 'must
  be supported by the pin type (default 0, which is rejected)' and never…
- [Low] Concern: dpllsim_pin_types[] maps only 5 of the 6 values of enum
  dpll_pin_type and leaves out DPLL_PIN_TYPE_INT_NCO, so a simulated pin…

--

dpll: dpllsim: Add configurable pins

This patch lets a dpllsim device instance carry configurable pins. Each
pin is a configfs directory under pins/ with type, frequency and direction
attributes. At live=1 the pins are validated, ordered, obtained and
registered with the DPLL subsystem after the device, and while the
instance is live the pin items are held with
configfs_depend_item_unlocked().

> diff --git a/drivers/dpll/dpllsim/core.c b/drivers/dpll/dpllsim/core.c
> index 0772f409d013b..97b3cf48772e7 100644
> --- a/drivers/dpll/dpllsim/core.c
> +++ b/drivers/dpll/dpllsim/core.c

[ ... ]

> +static struct config_item *dpllsim_pins_make_item(struct config_group *group,
> +						  const char *name)
> +{
> +	struct dpllsim_dev *sim = container_of(group, struct dpllsim_dev,
> +					       pins_group);
> +	struct dpllsim_pin *pin;
> +
> +	lockdep_assert_held(&dpllsim_subsys.su_mutex);
> +
> +	if (dpllsim_dev_frozen(sim))
> +		return ERR_PTR(-EBUSY);
> +	if (list_count_nodes(&sim->pin_list) >= DPLLSIM_MAX_PINS)
> +		return ERR_PTR(-ENOSPC);

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

> +
> +	pin = kzalloc_obj(*pin);

[ ... ]

> +/* Pin mkdir and rmdir run under the subsystem mutex like live transitions */
> +static void dpllsim_pins_drop_item(struct config_group *group,
> +				   struct config_item *item)
> +{
> +	list_del(&to_dpllsim_pin(item)->list);
> +	config_item_put(item);
> +}

[ ... ]

> +static int dpllsim_dev_depend_pins(struct dpllsim_dev *sim)
> +{
> +	struct dpllsim_pin *pin;
> +	int err;
> +
> +	list_for_each_entry(pin, &sim->pin_list, list) {
> +		err = configfs_depend_item_unlocked(&dpllsim_subsys,
> +						    &pin->item);

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

> +		if (err) {
> +			dpllsim_dev_undepend_pins(sim, pin);
> +			return err;
> +		}
> +	}
> +
> +	return 0;
> +}

[ ... ]

> diff --git a/drivers/dpll/dpllsim/dpll.c b/drivers/dpll/dpllsim/dpll.c
> index ae2c8fcf238c6..a1d5921fb8d01 100644
> --- a/drivers/dpll/dpllsim/dpll.c
> +++ b/drivers/dpll/dpllsim/dpll.c

[ ... ]

> +static void dpllsim_pin_props(struct dpllsim_pin *pin,
> +			      struct dpll_pin_properties *prop)
> +{
> +	prop->type = pin->type;
> +	prop->board_label = config_item_name(&pin->item);
> +
> +	switch (pin->type) {
> +	case DPLL_PIN_TYPE_GNSS:
> +		prop->freq_supported = dpllsim_freq_1pps;
> +		prop->freq_supported_num = ARRAY_SIZE(dpllsim_freq_1pps);
> +		break;
> +	case DPLL_PIN_TYPE_INT_OSCILLATOR:
> +		prop->freq_supported = dpllsim_freq_int_osc;
> +		prop->freq_supported_num = ARRAY_SIZE(dpllsim_freq_int_osc);
> +		break;
> +	case DPLL_PIN_TYPE_MUX:
> +	case DPLL_PIN_TYPE_SYNCE_ETH_PORT:
> +		prop->freq_supported = dpllsim_freq_synce;
> +		prop->freq_supported_num = ARRAY_SIZE(dpllsim_freq_synce);
> +		break;
> +	default:
> +		prop->freq_supported = dpllsim_freq_ext;
> +		prop->freq_supported_num = ARRAY_SIZE(dpllsim_freq_ext);
> +		break;
> +	}
> +}

[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