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

[luoxuanqiang <xuanqiang.luo@linux.dev>]

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

>> 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.

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.

>> 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.
>
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