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