Re: [PATCH net v4 4/4] net: phy: dp83640: fix per-bus clock lifetime

Hi Andrew,

在 2026/8/7 21:59, Andrew Lunn 写道:
>> -static int dp83640_probe(struct phy_device *phydev)
>> +static void dp83640_phy_release(void *data)
>>   {
> ...
>
>> +	mutex_lock(&clock->clock_lock);
>> +	if (dp83640 == clock->chosen) {
>> +		ptp_clock_unregister(clock->ptp_clock);
>> +		clock->ptp_clock = NULL;
>> +		clock->chosen = NULL;
>> +	} else {
> Probe has:
>
>> +	/* Ensure other PHY probes wait for shared clock initialization. */
>> +	phy_package_lock(phydev);
>> +	if (phy_package_probe_once(phydev))
>> +		dp83640_clock_init(clock);
>> +	phy_package_unlock(phydev);
> It seems like a phy_package_release_once(phydev) would help keep probe
> and release being symmetric. The problem is getting the semantics
> correct. phy_package_probe_once() will be true for the first PHY
> probed. You want phy_package_release_once() to be true when the last
> PHY is removed from the package. It probably needs to look at
> phydev->shared->refcnt. However that probably also requires using
> phy_package_join() not devm_phy_package_join().
>
> I then _think_ all the list manipulation can go away, and the driver
> will look cleaner.
>
>      Andrew
>
> ---
> pw-bot: cr

Thanks. I tried to work through the suggested design, but I am still not
sure I understand how the different lifetime requirements are meant to
fit together.

The reason v4 uses devm_phy_package_join() is that dp83640_probe() can
return successfully and phy_probe() can fail later. The driver's
.remove() callback is not called on that path, so devres is needed to
release both the per-PHY state and the package reference.

If I switch to phy_package_join() and release the package explicitly
during driver removal, that failure path will retain the package
reference. If I keep the managed join, however, the per-PHY devres action
runs before the managed package leave. A release_once check from that
action is therefore not atomic with dropping the package reference. Two
concurrent removals could both observe a non-final reference count before
either managed leave runs.

I do not see how to pair a phy_package_release_once() helper with package
leave without losing the managed probe unwind. Am I missing an existing
mechanism here?

There are also two DP83640-specific details I am unsure how to handle
under this model.

The PHY list is not only used during release. recalibrate() uses it to
enumerate the fully initialized non-chosen PHYs, read their timestamps,
and adjust their clock offsets. The package API does not currently
provide equivalent member enumeration. When you mentioned that all list
manipulation could go away, did you mean only the list handling for the
final member?

The PTP clock is also tied to clock->chosen rather than to the final
package member. For example, ptp_dp83640_gettime() obtains the PHY device
directly from clock->chosen->phydev and uses it for the subsequent MDIO
accesses. The chosen PHY is not necessarily removed last, so keeping the
PTP clock registered after that PHY has been removed would leave this
callback dereferencing stale per-PHY state.

Should release_once govern only the lifetime of the package-private
storage, or do you expect the PTP clock to remain registered until the
last package member leaves as well? The latter would also require
handling chosen-PHY migration, and I am not sure whether that is intended
to be part of this fix.

Thanks,
Xuanqiang