Re: [net-next,v2,12/12] ice: dpll: Support E825-C SyncE and dynamic pin discovery
From: Jakub Kicinski <kuba@kernel.org>
Date: 2026-01-21 00:18:57
Also in:
intel-wired-lan, linux-devicetree, linux-rdma, lkml
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 hunk ↗ jump to hunk
diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c index 4eca62688d83..9d82748810a3 100644
[ ... ]
+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.
+ if (ret) {
+ dev_err(ice_pf_to_dev(pf),
+ "Failed to register pin: %pe\n", ERR_PTR(ret));
+ dpll_pin_put(parent->pin, &parent->tracker);
+ parent->pin = NULL;
+ goto out;
+ }
+ break;[ ... ]
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c 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) 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.
+ 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);
+ }