> -----Original Message----- > From: Simon Horman <[email protected]> > Sent: Thursday, March 19, 2026 6:38 PM > To: Nitka, Grzegorz <[email protected]> > Cc: Simon Horman <[email protected]>; [email protected]; > [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; > Loktionov, Aleksandr <[email protected]>; > [email protected]; [email protected]; Nguyen, Anthony > L <[email protected]>; Vecera, Ivan <[email protected]>; > [email protected]; [email protected]; > [email protected]; Kubalewski, Arkadiusz > <[email protected]>; Kitszel, Przemyslaw > <[email protected]>; Oros, Petr <[email protected]>; linux- > [email protected] > Subject: Re: [PATCH net-next 5/8] ice: add TX clock (TXC) DPLL interface for > E825 devices > > 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; > > +} >
AI review comment is valid. To be fixed in the next iteration. Thanks Grzegorz > [ ... ]
