RE: [PATCH v7 net-next 5/8] ice: introduce TXC DPLL device and TX ref clock pin framework for E825

[Nitka, Grzegorz]

> -----Original Message-----
> From: Jakub Kicinski <kuba@xxxxxxxxxx>
> Sent: Saturday, May 2, 2026 7:33 PM
> To: Nitka, Grzegorz <grzegorz.nitka@xxxxxxxxx>
> Cc: Jakub Kicinski <kuba@xxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx; Vecera, Ivan
> <ivecera@xxxxxxxxxx>; vadim.fedorenko@xxxxxxxxx; jiri@xxxxxxxxxxx;
> edumazet@xxxxxxxxxx; Kitszel, Przemyslaw <przemyslaw.kitszel@xxxxxxxxx>;
> richardcochran@xxxxxxxxx; donald.hunter@xxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Kubalewski, Arkadiusz
> <arkadiusz.kubalewski@xxxxxxxxx>; andrew+netdev@xxxxxxx; intel-wired-
> lan@xxxxxxxxxxxxxxxx; horms@xxxxxxxxxx; Prathosh.Satish@xxxxxxxxxxxxx;
> Nguyen, Anthony L <anthony.l.nguyen@xxxxxxxxx>; pabeni@xxxxxxxxxx;
> davem@xxxxxxxxxxxxx
> Subject: Re: [PATCH v7 net-next 5/8] ice: introduce TXC DPLL device and TX ref
> clock pin framework for E825
> 
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> ice: introduce TXC DPLL device and TX ref clock pin framework for E825
> 
> This patch adds a new TXC DPLL device of DPLL_TYPE_GENERIC for E825
> devices, together with a TXCLK pin type and two TX reference pins
> (EXT_EREF0 and a fwnode-described SYNCE pin). Pin state callbacks and
> TXC lock status are placeholders at this stage.
> 
> > diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c
> b/drivers/net/ethernet/intel/ice/ice_dpll.c
> > index 62f75701d652..d839b50187ba 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_dpll.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_dpll.c
> 
> [ ... ]
> 
> > @@ -3199,19 +3276,40 @@ static bool ice_dpll_is_fwnode_pin(struct
> ice_dpll_pin *pin)
> >  	return !IS_ERR_OR_NULL(pin->fwnode);
> >  }
> >
> > +static bool ice_dpll_fwnode_eq(const struct fwnode_handle *a,
> > +			       const struct fwnode_handle *b)
> > +{
> > +	return a && b && a == b;
> > +}
> > +
> >  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;
> > +	bool is_tx_synce_parent = false;
> >  	struct ice_pf *pf = parent->pf;
> > +	bool is_rclk_parent = false;
> >  	int ret;
> >
> >  	wait_for_completion(&pf->dplls.dpll_init);
> >  	if (!test_bit(ICE_FLAG_DPLL, pf->flags))
> >  		goto out; /* DPLL initialization failed */
> >
> > +	/* Decide which parent we are handling, defensively checking FWNs */
> 
> [ ... ]
> 
> > +		} else if (is_tx_synce_parent) {
> > +			/* Register TX-CLK SYNCE pin directly to TXC DPLL */
> > +			pin = &pf->dplls.txclks[E825_EXT_SYNCE_PIN_IDX];
> > +			ret = dpll_pin_register(pf->dplls.txc.dpll, pin->pin,
> > +						&ice_dpll_txclk_ops, pin);
> > +			if (ret) {
> > +				dev_err(ice_pf_to_dev(pf),
> > +					"TX SYNCE pin register failed: %pe\n",
> > +					ERR_PTR(ret));
> > +				goto drop_parent_ref;
> > +			}
> >  		}
> >  		break;
> 
> [ ... ]
> 
> > @@ -3401,6 +3524,19 @@ ice_dpll_deinit_fwnode_pins(struct ice_pf *pf,
> struct ice_dpll_pin *pins,
> >  	destroy_workqueue(pf->dplls.wq);
> >  }
> >
> > +static int ice_dpll_deinit_txclk_pins(struct ice_pf *pf)
> > +{
> > +	struct ice_dpll_pin *synce_pin = &pf-
> >dplls.txclks[E825_EXT_SYNCE_PIN_IDX];
> > +	struct ice_dpll *dt = &pf->dplls.txc;
> > +
> > +	ice_dpll_unregister_pins(dt->dpll, pf->dplls.txclks,
> > +				 &ice_dpll_txclk_ops,
> > +				 ARRAY_SIZE(pf->dplls.txclks));
> > +	ice_dpll_release_pins(&pf->dplls.txclks[E825_EXT_EREF_PIN_IDX], 1);
> > +	ice_dpll_deinit_fwnode_pin(synce_pin);
> > +	return 0;
> > +}
> 
> Is there a teardown race between this function and a concurrent
> ice_dpll_pin_notify_work() that is about to register the SYNCE pin on
> the TXC DPLL?
> 
> The work's registration path runs:
> 
> 	parent->pin = fwnode_dpll_pin_find(parent->fwnode,
> 					   &parent->tracker);
> 	...
> 	} else if (is_tx_synce_parent) {
> 		pin = &pf->dplls.txclks[E825_EXT_SYNCE_PIN_IDX];
> 		ret = dpll_pin_register(pf->dplls.txc.dpll, pin->pin,
> 					&ice_dpll_txclk_ops, pin);
> 
> while ice_dpll_deinit_txclk_pins() first scans pf->dplls.txclks[] with
> ice_dpll_unregister_pins(), which skips any slot where pins[i].pin is
> still NULL:
> 
> 	for (i = 0; i < count; i++) {
> 		if (pins[i].hidden)
> 			continue;
> 		if (IS_ERR_OR_NULL(pins[i].pin))
> 			continue;
> 		dpll_pin_unregister(dpll, pins[i].pin, ops, &pins[i]);
> 	}
> 
> Only afterward does it call ice_dpll_deinit_fwnode_pin(synce_pin), which
> unregisters the notifier and flushes pf->dplls.wq.
> 
> If an external fwnode DPLL provider emits a DPLL_PIN_CREATED for
> clk_ref_synce between the two steps, can this sequence occur?
> 
>   deinit thread                    notifier work
>   -------------                    -------------
>   ice_dpll_unregister_pins(txclks)
>      sees txclks[1].pin == NULL
>      skips SYNCE slot
>                                    parent->pin = fwnode_dpll_pin_find()
>                                    dpll_pin_register(txc->dpll,
>                                        pin->pin, &ops, pin)
>                                      -> adds to txc->dpll->pin_refs
>                                         and pin->dpll_refs
>   ice_dpll_release_pins(EREF0, 1)
>   ice_dpll_deinit_fwnode_pin(synce_pin)
>      unregister_dpll_notifier()
>      flush_workqueue()   /* no work pending */
>      dpll_pin_put(synce_pin->pin, ...)
> 
> After this, dpll_pin_put() only drops the pin refcount; it does not call
> dpll_pin_unregister(). So the TXC dpll's pin_refs still contains the
> SYNCE entry when ice_dpll_deinit_dpll(pf, &pf->dplls.txc, false) later
> runs dpll_device_unregister() and dpll_device_put(). Does this trip
> WARN_ON_ONCE(!xa_empty(&dpll->pin_refs)) in __dpll_device_put() and
> then
> free the dpll_device while pin->dpll_refs still references it, leaving a
> use-after-free on any subsequent walk of pin->dpll_refs?
> 
> Would it be safer to call ice_dpll_deinit_fwnode_pin(synce_pin) first so
> the notifier is torn down and pf->dplls.wq drained before txclks[] is
> scanned, or to hold pf->dplls.lock across both the deinit scan and the
> work's parent->pin = ...; dpll_pin_register(...) sequence?
> 
> A similar structural race exists for the rclk path via
> dpll_pin_on_pin_register(), but this patch introduces a new instance on
> the direct-on-TXC SYNCE registration. The ordering in
> ice_dpll_deinit_txclk_pins() appears unchanged by later patches in the
> series, so the same race seems to persist beyond this commit.

This is a valid concern and to be fixed in the next iteration.
I'd go with stopping new notifier-driven work and then tear down DPLL
registration and references.

Thanks

Grzegorz