Re: [PATCH net-next 3/3] ice: use per-interface clock_id for E825 generic DPLLs

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 3 · Low: 0

- [Medium] In ice_dpll_init_txclk_pins()
  (drivers/net/ethernet/intel/ice/ice_dpll.c:4279-4282) the board-level…
- [Medium] ice_dpll_pin_notify()
  (drivers/net/ethernet/intel/ice/ice_dpll.c:3928) replaces the board-…
- [Medium] ice_generate_dpll_clock_id()
  (drivers/net/ethernet/intel/ice/ice_dpll.c:3086-3092) feeds…

--

ice: use per-interface clock_id for E825 generic DPLLs

On E825 the TX-CLK and TSPLL generic DPLL devices no longer take their
clock_id from the board-level PCIe DSN, but from the permanent port MAC
(TSPLL adds BIT_ULL(63) as a tag).  The notifier self-suppression check in
ice_dpll_pin_notify() is extended with a new helper that compares the
incoming src_clock_id against this PF's board, TXC and TSPLL clock_ids.

[Severity: Medium]
Are these two values still valid clock identifiers as far as the dpll uAPI
is concerned?  Documentation/driver-api/dpll.rst describes the field as:

  ``DPLL_A_CLOCK_ID``                attr Unique Clock Identifier
                                         (EUI-64), as defined by the
                                         IEEE 1588 standard

and dpll_msg_add_dev_handle() exports the driver value verbatim with
nla_put_64bit(msg, DPLL_A_CLOCK_ID, ...), so whatever is stored here
becomes uAPI.

ether_addr_to_u64() packs the six MAC bytes into the low 48 bits and leaves
bits 48-63 zero, which is not the MAC-48 to EUI-64 mapping IEEE 1588 asks
for (OUI[0..2] || FF FE || NIC[3..5]).  The same driver keeps feeding the
same attribute a genuine EUI-64 for the EEC/PPS and non-E825 objects:

	static u64 ice_generate_clock_id(struct ice_pf *pf)
	{
		return pci_get_dsn(pf->pdev);
	}

Should the MAC be expanded into a proper EUI-64 so both producers in this
driver use one format?

For the TSPLL variant, mac_clock_id | BIT_ULL(63) sets a bit inside what
would be the first octet of the OUI, so it can collide with a real clock
identifier starting 80:00, and the exported value no longer visibly matches
the interface MAC that the commit message wants userspace to key on.

Is the tag bit needed at all?  dpll_device_get() matches on (clock_id,
device_idx, module):

	if (dpll->clock_id == clock_id &&
	    dpll->device_idx == device_idx &&
	    dpll->module == module) {

and TSPLL uses E825_DPLL_TSPLL_BASE_IDX while TX-CLK uses
E825_DPLL_TXCLK_BASE_IDX and above, so the device_idx already separates the
two objects.

+
+/**
+ * ice_dpll_is_own_dpll_clock_id - check if clock_id belongs to this pf's DPLLs
+ * @pf: board private structure
+ * @clock_id: clock_id from a DPLL notification
+ *
+ * Match info->src_clock_id from a DPLL pin notification against any DPLL
+ * device this PF has registered. Used to suppress self-notifications
+ * generated as a side effect of our own dpll_pin_register() and
+ * dpll_pin_unregister() calls on the fwnode-backed SYNCE and TIME_REF pins,
+ * whose DPLLs (TXC and TSPLL) use MAC-derived clock_ids on E825.
+ *
+ * Return: true if clock_id matches one of this PF's registered DPLL devices.
+ */
+static bool ice_dpll_is_own_dpll_clock_id(struct ice_pf *pf, u64 clock_id)
+{
+	if (clock_id == pf->dplls.clock_id)
+		return true;
+	if (pf->hw.mac_type != ICE_MAC_GENERIC_3K_E825)
+		return false;
+	if (clock_id == ice_generate_dpll_clock_id(pf, &pf->dplls.txc,
+						   DPLL_TYPE_GENERIC))
+		return true;
+	return clock_id == ice_generate_dpll_clock_id(pf, &pf->dplls.tspll,
+						      DPLL_TYPE_GENERIC);
+}

[Severity: Medium]
Does this helper still cover notifications generated by a sibling PF of the
same NAC?

The old test compared src_clock_id against pf->dplls.clock_id, which is
pci_get_dsn() and, as the commit message says, "identical for all interfaces
sharing the same NAC/quad".  It therefore matched register/unregister events
raised by any PF on the board.  The new helper only matches this PF's three
values: board DSN, MAC(self) and MAC(self) | BIT(63).

The notification carries the target DPLL device's clock_id.  In
__dpll_pin_unregister():

	dpll_pin_delete_ntf(pin, dpll->clock_id);

so for the shared fwnode SYNCE pin the value is now MAC(sibling PF), which
ice_dpll_is_own_dpll_clock_id() rejects.

If PF-A is unbound:

  ice_dpll_deinit() -> ice_dpll_deinit_txclk_pins() ->
  ice_dpll_unregister_pins() -> dpll_pin_unregister(PF-A txc dpll, synce pin)
  -> __dpll_pin_unregister() -> dpll_pin_delete_ntf(pin, MAC(PF-A))

PF-B's ice_dpll_pin_notify() passes the fwnode test (it is the same shared
pin), the new clock_id test returns false, work gets queued, and
ice_dpll_pin_notify_work() takes the DPLL_PIN_DELETED branch:

	} else if (is_tx_synce_parent) {
		/* Unregister TX-CLK SYNCE pin from TXC DPLL */
		pin = &pf->dplls.txclks[E825_EXT_SYNCE_PIN_IDX];
		dpll_pin_unregister(pf->dplls.txc.dpll, pin->pin,
				    &ice_dpll_txclk_ops, pin);

Can this tear down PF-B's own still-valid SYNCE registration, so that after
unbinding one PF the surviving ports of the same NAC can no longer select
SyncE as TX reference?

This depends on both PFs' clk_ref_synce reference resolving to the same
fwnode object, which is what ice_dpll_pin_node_get() looks up and what
ice_txclk_enable_peer() suggests when it notes that SyncE must be enabled on
both PHY complexes.  If that is how the platform description is expected to
look, should the helper also accept clock_ids belonging to other PFs of the
same board, or should the suppression be keyed on something other than the
clock_id?

[Severity: Medium]
Does this change how many pin objects the single physical EXT_EREF0
connector produces?

dpll_pin_get() keys pin identity on (clock_id, pin_idx, module):

	if (pos->clock_id == clock_id &&
	    pos->pin_idx == pin_idx &&
	    pos->module == module) {

Every PF reaches here with the same start_idx, so with the board DSN all
PFs of a NAC obtained the same dpll_pin object for EREF0 and each registered
it on its own TX-CLK DPLL device, i.e. one pin with several parent DPLLs.
With the MAC-derived clock_id the lookup can no longer match a sibling PF's
pin, so each PF allocates a private pin object.

EREF0 is described as a board-level signal in
ice_dpll_init_info_txclk_pins_e825c():

	tx_pin->prop.board_label = ice_dpll_ext_eref_pin;
	tx_pin->tx_ref_src = ICE_REF_CLK_EREF0;

Does userspace then see N pins all labelled "EXT_EREF0" with no indication
that they are the same input?  The sibling input of the same TX-CLK DPLL
(EXT_SYNCE, fwnode-backed) stays a single shared pin with several parent
DPLLs, so the two external TX reference inputs of one DPLL device end up
modelled differently.

The commit message only talks about DPLL devices:

  "Since these DPLLs are per-interface, use the permanent port MAC as
   the clock_id basis for E825 generic DPLLs"

Could it also mention that DPLL_A_PIN_CLOCK_ID and the number of exported
EREF0 pin objects change, since that is userspace-visible as well?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908222428.872254-1-anthony.l.nguyen%40intel.com