RE: [PATCH v14 net-next 03/13] dpll: fix stale iteration in dpll_pin_on_pin_unregister()

[Nitka, Grzegorz]

> -----Original Message-----
> From: Kubalewski, Arkadiusz <arkadiusz.kubalewski@xxxxxxxxx>
> Sent: Monday, June 8, 2026 6:51 PM
> To: Nitka, Grzegorz <grzegorz.nitka@xxxxxxxxx>; netdev@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx; intel-wired-lan@xxxxxxxxxxxxxxxx; Oros, Petr
> <poros@xxxxxxxxxx>; richardcochran@xxxxxxxxx; andrew+netdev@xxxxxxx;
> Kitszel, Przemyslaw <przemyslaw.kitszel@xxxxxxxxx>; Nguyen, Anthony L
> <anthony.l.nguyen@xxxxxxxxx>; Prathosh.Satish@xxxxxxxxxxxxx; Vecera,
> Ivan <ivecera@xxxxxxxxxx>; jiri@xxxxxxxxxxx; vadim.fedorenko@xxxxxxxxx;
> donald.hunter@xxxxxxxxx; horms@xxxxxxxxxx; pabeni@xxxxxxxxxx;
> kuba@xxxxxxxxxx; davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx
> Subject: RE: [PATCH v14 net-next 03/13] dpll: fix stale iteration in
> dpll_pin_on_pin_unregister()
> 
> >From: Nitka, Grzegorz <grzegorz.nitka@xxxxxxxxx>
> >Sent: Sunday, June 7, 2026 8:31 PM
> >
> >Neither parent->dpll_refs nor pin->dpll_refs on its own is a correct
> >iteration target at unregister time:
> >
> >  - pin->dpll_refs includes DPLLs the child was registered against
> >    via a different parent or directly; blind unregister WARNs on
> >    the cookie miss in dpll_xa_ref_pin_del().
> >  - parent->dpll_refs reflects the parent's current attachments, not
> >    those at child-register time. Another driver may have (un)reg'd
> >    the parent against additional DPLLs in the meantime, so we miss
> >    registrations that exist and visit DPLLs that have none.
> >
> >Walk pin->dpll_refs and use dpll_pin_registration_find() to filter
> >to entries whose cookie is this parent. Symmetric with
> >dpll_pin_on_pin_register(), correct under any subsequent change to
> >parent->dpll_refs.
> >
> >Fixes: 9431063ad323 ("dpll: core: Add DPLL framework base functions")
> >Signed-off-by: Grzegorz Nitka <grzegorz.nitka@xxxxxxxxx>
> >---
> > drivers/dpll/dpll_core.c | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
> >index 6dc7e93ece75..cea7e2be2cbc 100644
> >--- a/drivers/dpll/dpll_core.c
> >+++ b/drivers/dpll/dpll_core.c
> >@@ -1031,14 +1031,19 @@
> EXPORT_SYMBOL_GPL(dpll_pin_on_pin_register);
> > void dpll_pin_on_pin_unregister(struct dpll_pin *parent, struct dpll_pin
> >*pin,
> > 				const struct dpll_pin_ops *ops, void *priv)
> > {
> >+	struct dpll_pin_registration *reg;
> > 	struct dpll_pin_ref *ref;
> > 	unsigned long i;
> >
> > 	mutex_lock(&dpll_lock);
> > 	dpll_pin_delete_ntf(pin);
> 
> Hey Grzegorz,
> 
> This line is redundant after 7/13?
> 

Hi Arek,
Yes, in general. 
The reason for which I put it as separate patches (applies to all new 'Fixes' patches) is:
- to identify atomic changes and
- to make it easy to backport to other branches ('old' ones) if the call is to backport it

Regards

Grzegorz

> > 	dpll_xa_ref_pin_del(&pin->parent_refs, parent, ops, priv, pin);
> >-	xa_for_each(&pin->dpll_refs, i, ref)
> >+	xa_for_each(&pin->dpll_refs, i, ref) {
> >+		reg = dpll_pin_registration_find(ref, ops, priv, parent);
> >+		if (!reg)
> >+			continue;
> > 		__dpll_pin_unregister(ref->dpll, pin, ops, priv, parent);
> >+	}
> > 	mutex_unlock(&dpll_lock);
> > }
> > EXPORT_SYMBOL_GPL(dpll_pin_on_pin_unregister);
> >--
> >2.39.3