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

Hi Andrew,
在 2026/8/9 23:24, Andrew Lunn 写道:

quoted

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.

You need to be careful with devm_ it can be a footgun. devm_ works
best when all resources are devm_. It gets messy when you need to mix
devm_ resources with resources which are not devm_.

So the rules are, if .probe() fails, the probe needs to cleanup
whatever it did, because as you said, .remove is not called.

quoted

If I switch to phy_package_join() and release the package explicitly
during driver removal, that failure path will retain the package
reference.

It should not, if you correctly undo what you did. So if the first PHY
to probe calls phy_package_join(), and creates the shared clock, it
needs to destroy the shared clock and do a phy_package_leave() if
probe fails. In this situation, when the second PHY probes, it will
call phy_package_join(), find that phy_package_init_once() is true,
and create the shared clock. Now, i've not audited the phy_package
code, it might be broken, and you might need to fix it. probe()
failing does not happen to often, so it is not tested well.

quoted

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?

Maybe you actually need a devm_phy_package_init_once() where you pass
as a parameter the function to call as the release function.

I _think_ that will work, but i don't actually like it. I personally
would throw away of the devm_ calls and do the cleanup manually. It is
much easier to reason about.

I agree. In the next version, I will remove the devm_ calls and manage
the resources explicitly with phy_package_join() and
phy_package_leave().

If the DP83640 driver's .probe() returns successfully but a later PHY
core initialization step fails, the current PHY core does not call the
driver's .remove() callback. This is a generic PHY core rollback issue,
which I will address separately in a follow-up patch rather than include
in this series.

quoted

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?

I did not realise the list was used in other places. So yes, i don't
think you need it for create/destroy of the shared clock.

quoted

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.

This is wrong. They should be using __phy_package_write() &
__phy_package_read() since you are accessing a package resource, not
an individual PHY resources.

quoted

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?

Yes, the clock should be a package resource, so should have the same
lifetime as the package.

I revisited this part. Extending the lifetime of the registered PTP clock
until the last package member leaves would require broader changes.

The current PTP clock relies on clock->chosen to provide the actual PHY.
For example, ptp_dp83640_enable() and periodic_output() use
clock->chosen->phydev for non-broadcast writes that configure the EXTS
and PEROUT GPIOs.

If the chosen PHY leaves before the last package member while the PTP
clock remains registered, the driver would need to define the behavior of
all callbacks after the chosen PHY has left. clock->chosen is also used
by recalibrate() as the calibration reference, so this is more than a
change to the register accessors; it requires redefining the hardware
selection and synchronization semantics.

Given the additional scope, would it be acceptable to retain the existing
MDIO access paths and keep the PTP clock lifetime tied to the chosen PHY,
rather than keep it registered until the last package member leaves?

That would introduce the PHY package only to manage the lifetime of the
shared struct dp83640_clock. This would keep the change focused on the two
lifetime bugs described in the commit message.

I've sent v5 with these changes.

Thanks,
Xuanqiang