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

On Thu, 30 Jul 2026 14:44:51 +0800 [email protected] wrote:
> From: Xuanqiang Luo <[email protected]>
> 
> Commit 42e2a9e11a1d ("net: phy: dp83640: improve phydev and driver
> removal handling") moved per-bus clock cleanup from module exit to the
> remove path. This leaves two lifetime problems.
> 
> dp83640_clock_get_bus() publishes a newly allocated clock before the
> driver allocates its per-PHY data and registers the PTP clock. If either
> operation fails, no PHY is bound and the remove callback cannot release
> the clock, leaking the clock, its pin configuration, and the MII bus
> device reference.
> 
> The remove path can also free a clock after dropping clock_lock. A
> concurrent probe may already have found the clock under
> phyter_clocks_lock and be waiting for clock_lock, allowing it to acquire
> a freed mutex and access the freed clock.
> 
> Use the PHY package infrastructure for the per-bus clock. The package
> table is scoped to each MII bus and holds the shared object until the
> last joined PHY leaves. Serialize the one-time clock initialization with
> the bus shared lock because phy_package_probe_once() elects an
> initializer but does not wait for initialization to finish.
> 
> Manage both the package reference and the per-PHY state with devres.
> This is needed because dp83640_probe() may succeed before later PHY core
> initialization fails, and the driver remove callback is not called for
> that failure. Register the per-PHY cleanup action after the package
> reference so probe unwinding first unregisters the PTP clock or removes
> the PHY from the clock list, then releases the shared clock. Release the
> same action from the normal remove path.
> 
> 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.

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

>  		goto no_memory;
> @@ -1450,24 +1432,31 @@ static int dp83640_probe(struct phy_device *phydev)
>  	} else
>  		list_add_tail(&dp83640->list, &clock->phylist);
>  
> -	dp83640_clock_put(clock);
> +	mutex_unlock(&clock->clock_lock);
> +
> +	err = devm_add_action_or_reset(&phydev->mdio.dev,
> +				       dp83640_phy_release, dp83640);
> +	if (err)
> +		return err;
> +
>  	return 0;
>  
>  no_register:
>  	clock->chosen = NULL;
> +	clock->ptp_clock = NULL;
> +	phydev->default_timestamp = false;
> +	phydev->mii_ts = NULL;
> +	phydev->priv = NULL;
>  	kfree(dp83640);
>  no_memory:
> -	dp83640_clock_put(clock);
> +	mutex_unlock(&clock->clock_lock);
>  no_clock:
>  	return err;
>  }
>  
>  static void dp83640_remove(struct phy_device *phydev)
>  {
> -	struct dp83640_clock *clock;
> -	struct list_head *this, *next;
> -	struct dp83640_private *tmp, *dp83640 = phydev->priv;
> -	bool remove_clock = false;
> +	struct dp83640_private *dp83640 = phydev->priv;
>  
>  	if (phydev->mdio.addr == BROADCAST_ADDR)
>  		return;
> @@ -1475,43 +1464,8 @@ static void dp83640_remove(struct phy_device *phydev)
>  	phydev->mii_ts = NULL;
>  
>  	enable_status_frames(phydev, false);
> -	cancel_delayed_work_sync(&dp83640->ts_work);
> -
> -	skb_queue_purge(&dp83640->rx_queue);
> -	skb_queue_purge(&dp83640->tx_queue);
> -
> -	clock = dp83640_clock_get(dp83640->clock);
>  
> -	if (dp83640 == clock->chosen) {
> -		ptp_clock_unregister(clock->ptp_clock);
> -		clock->chosen = NULL;
> -	} else {
> -		list_for_each_safe(this, next, &clock->phylist) {
> -			tmp = list_entry(this, struct dp83640_private, list);
> -			if (tmp == dp83640) {
> -				list_del_init(&tmp->list);
> -				break;
> -			}
> -		}
> -	}
> -
> -	if (!clock->chosen && list_empty(&clock->phylist))
> -		remove_clock = true;
> -
> -	dp83640_clock_put(clock);
> -	kfree(dp83640);
> -
> -	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.
-- 
pw-bot: cr