Re: [PATCH net-next 10/10] dpll: document locking rules

[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] The new Locking section in Documentation/driver-api/dpll.rst
  covers three cases where a pin callback reaches private data owned by…
- [Low] The new driver rule in Documentation/driver-api/dpll.rst (lines
  716-718) says notifications for objects outside the callback's domain…

--

dpll: document locking rules

This commit adds a Locking section to the dpll driver API documentation.
The section describes the dpll_lock and per-domain lock levels, the domain
each driver callback runs under, and the rules drivers have to follow. It
also adds matching notes to the kernel-doc of dpll_pin_register() and
dpll_pin_on_pin_register().

> diff --git a/Documentation/driver-api/dpll.rst b/Documentation/driver-api/dpll.rst
> index 7c117ae37cc1b..40d5037ff3bb0 100644
> --- a/Documentation/driver-api/dpll.rst
> +++ b/Documentation/driver-api/dpll.rst
> @@ -675,6 +675,50 @@ In the error path we have to rewind every allocation in the reverse order:

[ ... ]

> +Callbacks of a dpll device run under the domain of the device. Callbacks of a
> +pin run under the domain of the pin, whichever driver registered them:
> +
> +- A driver registering a pin of another driver with its dpll device has these
> +  callbacks serialized with the driver that created the pin, not with the other
> +  callbacks of its device, so it has to protect the state they share by itself.
> +  A callback stuck in such a registration stops the domain of the pin.
> +- The parent pin of a pin registered with dpll_pin_on_pin_register() and its
> +  dpll devices can belong to another driver, so the callback must not
> +  dereference their private data.
> +- The frequency monitor belongs to the dpll device, so only callbacks of a pin
> +  of the same domain registered with dpll_pin_register() can include
> +  ``.measured_freq_get``.

[Severity: Low]
Should reference-sync pairs be in this list too?

dpll_pin_ref_sync_pair_add() only does an xa_insert() into
pin->ref_sync_pins. It never checks that pin->domain matches
ref_sync_pin->domain, so a pair can span two domains.

When the ref-sync callbacks run, only the domain lock of the pin is held,
but they get the private data of the paired pin:

drivers/dpll/dpll_netlink.c:dpll_msg_add_pin_ref_sync() {
    ...
	ref_sync_pin_priv = dpll_pin_on_dpll_priv(dpll, ref_sync_pin);
    ...
	ret = ops->ref_sync_get(pin, pin_priv, ref_sync_pin,
				ref_sync_pin_priv, &state, extack);
    ...
}

dpll_pin_ref_sync_state_set() does the same for ref_sync_get() and
ref_sync_set(). It then calls __dpll_pin_change_ntf() only for the pin, not
for ref_sync_pin.

Under the rules above, the callbacks from ref_sync_pin's own registration
run under ref_sync_pin's domain. If the two domains differ, nothing
serializes these two sets of callbacks.

The in-tree implementers zl3073x_dpll_input_pin_ref_sync_get() and
zl3073x_dpll_input_pin_ref_sync_set() do dereference this data:

	struct zl3073x_dpll_pin *sync_pin = ref_sync_pin_priv;

No in-tree driver creates a cross-domain pair today, since zl3073x
registers only its own chip's pins. The first bullet partly covers the case
where one driver registered both pins. It does not cover pins that
different drivers registered on one dpll.

Could the documentation say whether a ref-sync pair may cross domains, and
whether ref_sync_pin_priv may be dereferenced? The parent pin case in the
dpll_pin_on_pin_register() bullet already has an explicit rule like this.

> +
> +Device drivers have to follow these rules:
> +
> +- A callback must not call dpll_device_change_ntf(), dpll_pin_change_ntf() or
> +  any function changing the topology.
> +- A callback notifies about objects of the domain it runs under with
> +  __dpll_device_change_ntf() or __dpll_pin_change_ntf(). Other objects have to
> +  be notified from a context that holds no domain lock, for example a work item.
> +- A driver must not call dpll_device_change_ntf(), dpll_pin_change_ntf() or any
> +  function changing the topology while holding a lock that its callbacks take.

[Severity: Low]
Can a driver follow the work item suggestion safely without two more rules?

The first rule is about waiting on the work. The deferred work ends up in
dpll_pin_change_ntf() or dpll_device_change_ntf(). Both take dpll_lock for
read and then the domain mutex:

drivers/dpll/dpll_netlink.c:dpll_pin_change_ntf() {
    ...
	down_read(&dpll_lock);
	mutex_lock(&pin->domain->lock);
	ret = __dpll_pin_change_ntf(pin);
    ...
}

Callbacks already run with dpll_lock held for read, for example from
dpll_pin_pre_doit() or dpll_pre_doit(). A callback, or code holding a lock
the callbacks take, might wait for the work with flush_work() or
cancel_work_sync() while a writer is queued on dpll_lock. Could that
deadlock?

callback                  writer                  work
down_read(&dpll_lock)
                          dpll_pin_register()
                            down_write(&dpll_lock)
                                                  dpll_pin_change_ntf()
                                                    down_read(&dpll_lock)
flush_work()

Here the writer waits for the callback's read hold. The work's down_read()
blocks behind the queued writer, and the callback waits for the work.

The last rule above only bans direct calls to dpll_device_change_ntf() and
dpll_pin_change_ntf() while holding such a lock. It does not cover waiting
for a work item that makes those calls.

The second rule is about lifetime. The text does not say that the work has
to be cancelled or flushed before the target pin or device is unregistered
and released with dpll_pin_put() or dpll_device_put(). __dpll_device_put()
ends in kfree(dpll). A work item that keeps a struct dpll_device or struct
dpll_pin pointer and runs after the final put would touch freed memory.

Would it make sense to add both rules next to the work item suggestion?

> +
>  SyncE enablement
>  ================
>  For SyncE enablement it is required to allow control over dpll device

[ ... ]

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