Re: [PATCH net-next 09/10] ice: dpll: update locking comments

[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 locking note this patch adds to the struct ice_dplls kernel-
  doc (drivers/net/ethernet/intel/ice/ice_dpll.h, lines 168-170) is…

--

ice: dpll: update locking comments

This commit updates the ice DPLL locking comments now that callbacks run
under the DPLL core domain locks instead of dpll_lock. It also adds a note
saying that callbacks of the external DPLL's pins run under that DPLL's
domain, so only pf->dplls.lock serializes them with the rest of the
driver.

> diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.h b/drivers/net/ethernet/intel/ice/ice_dpll.h
> index 4ba76e2e2e0b0..39e041a30090d 100644
> --- a/drivers/net/ethernet/intel/ice/ice_dpll.h
> +++ b/drivers/net/ethernet/intel/ice/ice_dpll.h
> @@ -165,6 +165,9 @@ struct ice_dpll {
>   *     tx_refclks usage map), acquire pf->dplls.lock first, then
>   *     ctrl_pf->dplls.lock. Skip the second acquire when pf == ctrl_pf
>   *     to avoid recursive locking.
> + *   - Callbacks of the SYNCE and TIME_REF pins of an external DPLL run under
> + *     the DPLL core domain lock of that DPLL, not of the TXC and TSPLL DPLLs,
> + *     so only @lock serializes them with the callbacks of these DPLLs.

[Severity: Low]
Could this note be clearer that TXC and TSPLL are not in the same core
domain either?

As written, it presents the external fwnode pins as the special case. A
reader could take that to mean the other ice objects share one core
domain, which would serialize their callbacks against each other.

Looking at ice_generate_dpll_clock_id(), each PF gets three different
clock_ids:

drivers/net/ethernet/intel/ice/ice_dpll.c:ice_generate_dpll_clock_id() {
    ...
		if (d->dpll_idx >= E825_DPLL_TXCLK_BASE_IDX)
			return mac_clock_id;

		if (d->dpll_idx == E825_DPLL_TSPLL_BASE_IDX)
			return mac_clock_id | BIT_ULL(63);
	}

	return pf->dplls.clock_id;
}

dpll_domain_get() keys domains on (clock_id, module). That gives three
separate domains on one PF:

  - TXC, plus the EXT_EREF0 pin, which ice_dpll_init_txclk_pins() creates
    with the TXC clock_id
  - TSPLL
  - the DSN-based objects, such as the rclk pin

Each PF's TXC and TSPLL domains are also separate from those of other PFs.
The struct dpll_domain comment in dpll_core.h says only one domain lock is
held at a time.

So the core does not serialize TXC callbacks against TSPLL callbacks, or
either of them against rclk callbacks. pf->dplls.lock (with
ctrl_pf->dplls.lock and cgu_synce_lock) is what does that, and the fwnode
pins are one more case of the same thing.

The new sentence is accurate per DPLL, and the first bullet still says
@lock serializes all DPLL state mutations. Since this patch exists to
document the domain-lock rules, though, would it make sense to say that
TXC, TSPLL and the DSN-domain objects are each in their own domain?

>   *   - @txclk_notify_rwsem is held for read across
>   *     ice_txclk_update_and_notify(), including the out-of-lock
>   *     dpll_*_change_ntf() calls. ice_dpll_deinit() takes the write side

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