-----Original Message-----
From: Jakub Kicinski <kuba@kernel.org>
Sent: Wednesday, January 21, 2026 1:19 AM
To: Vecera, Ivan <ivecera@redhat.com>
Cc: Jakub Kicinski <kuba@kernel.org>; conor+dt@kernel.org; Oros, Petr
[off-list ref]; Nguyen, Anthony L [off-list ref];
linux-rdma@vger.kernel.org; tariqt@nvidia.com; robh@kernel.org;
netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Lobakin, Aleksander
[off-list ref]; mbloch@nvidia.com; jiri@resnulli.us;
Prathosh.Satish@microchip.com; krzk+dt@kernel.org; saeedm@nvidia.com;
devicetree@vger.kernel.org; davem@davemloft.net; pabeni@redhat.com;
Kitszel, Przemyslaw [off-list ref]; Kubalewski, Arkadiusz
[off-list ref]; jonathan.lemon@gmail.com;
saravanak@kernel.org; Loktionov, Aleksandr
[off-list ref]; Schmidt, Michal
[off-list ref]; edumazet@google.com; leon@kernel.org;
vadim.fedorenko@linux.dev; Nitka, Grzegorz [off-list ref];
intel-wired-lan@lists.osuosl.org; richardcochran@gmail.com;
andrew+netdev@lunn.ch
Subject: Re: [net-next,v2,12/12] ice: dpll: Support E825-C SyncE and dynamic
pin discovery

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-
local.html
---
ice: dpll: Support E825-C SyncE and dynamic pin discovery

This patch adds DPLL support for the Intel E825-C Ethernet controller.
It implements dynamic pin discovery via firmware nodes, asynchronous
registration using a DPLL notifier chain, and hardware configuration
for CGU registers.

quoted

diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c

b/drivers/net/ethernet/intel/ice/ice_dpll.c

quoted

index 4eca62688d83..9d82748810a3 100644

[ ... ]

quoted

+static void ice_dpll_pin_notify_work(struct work_struct *work)
+{
+	struct ice_dpll_pin_work *w = container_of(work,
+						   struct ice_dpll_pin_work,
+						   work);
+	struct ice_dpll_pin *pin, *parent = w->pin;
+	struct ice_pf *pf = parent->pf;
+	int ret;
+
+	switch (w->action) {
+	case DPLL_PIN_CREATED:
+		if (!IS_ERR_OR_NULL(parent->pin)) {
+			/* We have already our pin registered */
+			goto out;
+		}
+
+		/* Grab reference on fwnode pin */
+		parent->pin = fwnode_dpll_pin_find(parent->fwnode,
+						   &parent->tracker);
+		if (IS_ERR_OR_NULL(parent->pin)) {
+			dev_err(ice_pf_to_dev(pf),
+				"Cannot get fwnode pin reference\n");
+			goto out;
+		}
+
+		/* Register rclk pin */
+		pin = &pf->dplls.rclk;
+		ret = dpll_pin_on_pin_register(parent->pin, pin->pin,
+					       &ice_dpll_rclk_ops, pin);

Can pin->pin (pf->dplls.rclk.pin) be NULL here? Looking at the init
order in ice_dpll_init_pins_e825():

    ret = ice_dpll_init_fwnode_pins(pf, pf->dplls.inputs, 0);
    ...
    ret = ice_dpll_init_rclk_pin(pf, DPLL_PIN_IDX_UNSPEC, ...);

The fwnode pins are initialized first, which registers notifiers. If a
DPLL_PIN_CREATED notification fires before ice_dpll_init_rclk_pin()
completes, this work handler will run while pf->dplls.rclk.pin is still
NULL (the structure is zeroed by kcalloc). Passing NULL to
dpll_pin_on_pin_register() causes a NULL dereference in
dpll_xa_ref_pin_add() when it accesses pin->parent_refs.

This is a good catch! Yes, it's possible to go with the flow described above.
And this can lead to NULL pointer dereference.
To be fixed in the next iteration.
I believe, NULL checker and/or acquiring dpll mutex in notifier callback is needed 
to let the driver complete DPLL initialization.

quoted

+		if (ret) {
+			dev_err(ice_pf_to_dev(pf),
+				"Failed to register pin: %pe\n",

ERR_PTR(ret));

quoted

+			dpll_pin_put(parent->pin, &parent->tracker);
+			parent->pin = NULL;
+			goto out;
+		}
+		break;

[ ... ]

quoted

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c

b/drivers/net/ethernet/intel/ice/ice_ptp.c

quoted

index 4c8d20f2d2c0..0eb8a9e29a52 100644

--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c

@@ -1341,6 +1341,35 @@ void ice_ptp_link_change(struct ice_pf *pf, bool

linkup)

quoted

 	if (pf->hw.reset_ongoing)
 		return;

+	if (hw->mac_type == ICE_MAC_GENERIC_3K_E825) {
+		int pin, err;
+
+		mutex_lock(&pf->dplls.lock);

Should this check ICE_FLAG_DPLL before using pf->dplls.lock? Looking at
ice_dpll_init_e825(), if initialization fails the mutex is destroyed:

    err = ice_dpll_init_info_e825c(pf);
    if (err)
        goto err_exit;
    ...
    err_exit:
        mutex_destroy(&d->lock);

But ICE_FLAG_DPLL is only set on success. If DPLL init fails and a link
change event occurs, this code will use a destroyed mutex.

This is also a valid comment. Yes, using dpll mutex should be conditioned by
mentioned flag.

quoted

+		for (pin = 0; pin < ICE_SYNCE_CLK_NUM; pin++) {
+			enum ice_synce_clk clk_pin;
+			bool active;
+			u8 port_num;
+
+			port_num = ptp_port->port_num;
+			clk_pin = (enum ice_synce_clk)pin;
+			err = ice_tspll_bypass_mux_active_e825c(hw,
+								port_num,
+								&active,
+								clk_pin);
+			if (WARN_ON_ONCE(err)) {
+				mutex_unlock(&pf->dplls.lock);
+				return;
+			}
+
+			err = ice_tspll_cfg_synce_ethdiv_e825c(hw, clk_pin);
+			if (active && WARN_ON_ONCE(err)) {
+				mutex_unlock(&pf->dplls.lock);
+				return;
+			}
+		}
+		mutex_unlock(&pf->dplls.lock);
+	}