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: add TX clock (TXC) DPLL interface for E825 devices This commit introduces a TX clock DPLL instance and pins for E825-based devices. The TXC domain exposes two reference pins: EXT_EREF0 (board external electrical reference) and SYNCE (port-derived reference, described via fwnode). A new pin type and pin ops are added to support TX clock references. > diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c > b/drivers/net/ethernet/intel/ice/ice_dpll.c > index 62f75701d652..a1258f2e03a9 100644 > --- a/drivers/net/ethernet/intel/ice/ice_dpll.c > +++ b/drivers/net/ethernet/intel/ice/ice_dpll.c [ ... ] > @@ -3552,6 +3686,39 @@ ice_dpll_init_fwnode_pins(struct ice_pf *pf, struct > ice_dpll_pin *pins, > return ret; > } > > +static int ice_dpll_init_txclk_pins(struct ice_pf *pf, int start_idx) > +{ > + struct ice_dpll_pin *ref_pin = pf->dplls.txclks; > + struct ice_dpll *txc = &pf->dplls.txc; > + int ret = 0; > + > + /* configure EXT_EREF0 pin */ > + ret = ice_dpll_get_pins(pf, ref_pin, start_idx, 1, pf->dplls.clock_id); > + if (ret) > + return ret; > + ret = dpll_pin_register(txc->dpll, ref_pin->pin, &ice_dpll_txclk_ops, > + ref_pin); > + if (ret) > + return ret; > + > + /* configure EXT_SYNCE pin which is based on fwnode pin */ > + ref_pin++; > + ret = ice_dpll_init_fwnode_pin(ref_pin, ice_dpll_fwnode_ext_synce); > + if (ret) > + return ret; > + > + ret = dpll_pin_register(txc->dpll, ref_pin->pin, &ice_dpll_txclk_ops, > + ref_pin); > + if (ret) > + return ret; Does ice_dpll_init_txclk_pins leak the EXT_EREF0 pin on error? If dpll_pin_register for the EXT_SYNCE pin fails, the function returns the error without cleaning up the EXT_EREF0 pin that was already successfully registered. Looking at the call chain: ice_dpll_init_pins_e825() calls: ice_dpll_init_txclk_pins() ice_dpll_get_pins() -> acquires pin reference for EXT_EREF0 dpll_pin_register() -> registers EXT_EREF0 with TXC DPLL ice_dpll_init_fwnode_pin() -> may fail here dpll_pin_register() -> or may fail here for EXT_SYNCE If either of the last two steps fail, the function returns the error directly without calling dpll_pin_unregister or ice_dpll_release_pins for the EXT_EREF0 pin. The caller ice_dpll_init_pins_e825() also doesn't compensate - on txclk init failure it only calls ice_dpll_deinit_rclk_pin() and ice_dpll_deinit_fwnode_pins(), neither of which clean up txclk resources. Compare with ice_dpll_init_fwnode_pins() and ice_dpll_get_pins() which both have proper error-path rollback using goto labels. > + > + if (IS_ERR_OR_NULL(ref_pin->pin)) > + dev_dbg(ice_pf_to_dev(pf), > + "Tx-clk mux pin not registered yet\n"); Can this cause a NULL pointer dereference? The IS_ERR_OR_NULL check appears after calling dpll_pin_register with ref_pin->pin. Looking at the code flow: ice_dpll_init_fwnode_pin() returns 0 (success) even when fwnode_dpll_pin_find() returns NULL and sets ref_pin->pin = NULL. This is the expected case - the commit message states "The SYNCE pin is registered when its fwnode becomes available" and the code prints "DPLL pin for %pfwp not registered yet". However, the code unconditionally calls: dpll_pin_register(txc->dpll, ref_pin->pin, ...) where ref_pin->pin is NULL. Looking at dpll_pin_register in dpll_core.c: int dpll_pin_register(struct dpll_device *dpll, struct dpll_pin *pin, ...) { ... ret = dpll_pin_fwnode_register(dpll, pin, &ice_dpll_txclk_ops, pin_priv); ... } which then calls code that dereferences pin->fwnode without a NULL check, causing a kernel oops. The IS_ERR_OR_NULL guard at line 3716 was clearly intended to protect the dpll_pin_register call but was placed after it instead of before it. > + > + return 0; > +} [ ... ]
