Re: [PATCH net v2] net: phy: dp83640: fix per-bus clock lifetime

在 2026/8/4 10:00, Jakub Kicinski 写道:

> On Thu, 30 Jul 2026 14:44:51 +0800 [email protected] wrote:
>>
>> Embed the pin configuration in the package private data so the final
>> package leave releases all clock storage.
>>
>> Fixes: 42e2a9e11a1d ("net: phy: dp83640: improve phydev and driver removal handling")
>> Suggested-by: Jakub Kicinski <[email protected]>
> No need to add Suggested-by tags for review comments.
> Only if the v1 was suggested by the person.
>
Understood, I will drop it in v3.

>> +static int dp83640_probe(struct phy_device *phydev)
>> +{
>>   	struct dp83640_private *dp83640;
>> +	struct dp83640_clock *clock;
>>   	int err = -ENOMEM, i;
>>   
>>   	if (phydev->mdio.addr == BROADCAST_ADDR)
>>   		return 0;
>>   
>> -	clock = dp83640_clock_get_bus(phydev->mdio.bus);
>> -	if (!clock)
>> +	err = devm_phy_package_join(&phydev->mdio.dev, phydev,
>> +				    BROADCAST_ADDR, sizeof(*clock));
>> +	if (err)
>>   		goto no_clock;
>>   
>> +	clock = phy_package_get_priv(phydev);
>> +	/* Ensure other PHY probes wait for shared clock initialization. */
>> +	mutex_lock(&phydev->mdio.bus->shared_lock);
>> +	if (phy_package_probe_once(phydev))
>> +		dp83640_clock_init(clock);
>> +	mutex_unlock(&phydev->mdio.bus->shared_lock);
>> +
>> +	mutex_lock(&clock->clock_lock);
>> +
>>   	dp83640 = kzalloc_obj(struct dp83640_private);
>>   	if (!dp83640)
> AI reviewer points out that this error path is now missing setting
> the err variable. Please clean this up as part of your change.
> Remove the init to -ENOMEM and have every goto x; path set err
> if it needs to

Thanks, this is a serious oversight. I will fix it in v3.

>>   	
>> -	if (remove_clock) {
>> -		mutex_lock(&phyter_clocks_lock);
>> -		list_del(&clock->list);
>> -		mutex_unlock(&phyter_clocks_lock);
>> -
>> -		mutex_destroy(&clock->extreg_lock);
>> -		mutex_destroy(&clock->clock_lock);
>> -		put_device(&clock->bus->dev);
>> -		kfree(clock->caps.pin_config);
>> -		kfree(clock);
>> -	}
>> +	devm_release_action(&phydev->mdio.dev, dp83640_phy_release, dp83640);
> Why the explicit call? if you're using devm_ you should let it handle
> the unwind.. If you want to have an explicit call, don't use dev_ APIs.

I used the explicit call because I was concerned about leaving the
entire unwind to devres. After the driver's .remove() callback returns,
phylib asserts reset, and the subsequent devres cleanup calls
ptp_clock_unregister(), which may issue MDIO writes while disabling PTP
events.

However, the same ordering can also occur when the driver .probe()
succeeds but later PHY core initialization fails: phy_probe() asserts
reset before devres cleanup unregisters the PTP clock.

As you point out, explicitly releasing a managed action makes the
cleanup semantics awkward, and I currently have no evidence that the
post-reset MDIO writes cause an actual problem. I will therefore drop
the explicit release in v3 and let devres handle the cleanup.

Thanks,
Xuanqiang