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

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.

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.

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.

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.

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.

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.

	 Andrew