From: Zijun Hu <quic_zijuhu@quicinc.com>

For devm_of_phy_provider_unregister(), its comment says it needs to invoke
of_phy_provider_unregister() to unregister the phy provider, but it does
not invoke the function actually since devres_destroy() will not call
devm_phy_provider_release() at all which will call the function, and the
missing of_phy_provider_unregister() call will case:

- The phy provider fails to be unregistered.
- Leak both memory and the OF node refcount.

Fixed by using devres_release() instead of devres_destroy() within the API.

Fixes: ff764963479a ("drivers: phy: add generic PHY framework")
Cc: stable@vger.kernel.org
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/phy/phy-core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index f190d7126613..de07e1616b34 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -1259,12 +1259,12 @@ EXPORT_SYMBOL_GPL(of_phy_provider_unregister);
  * of_phy_provider_unregister to unregister the phy provider.
  */
 void devm_of_phy_provider_unregister(struct device *dev,
-	struct phy_provider *phy_provider)
+				     struct phy_provider *phy_provider)
 {
 	int r;
 
-	r = devres_destroy(dev, devm_phy_provider_release, devm_phy_match,
-		phy_provider);
+	r = devres_release(dev, devm_phy_provider_release, devm_phy_match,
+			   phy_provider);
 	dev_WARN_ONCE(dev, r, "couldn't find PHY provider device resource\n");
 }
 EXPORT_SYMBOL_GPL(devm_of_phy_provider_unregister);

-- 
2.34.1

On Thu, Oct 24, 2024 at 10:39:27PM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> For devm_of_phy_provider_unregister(), its comment says it needs to invoke
> of_phy_provider_unregister() to unregister the phy provider, but it does
> not invoke the function actually since devres_destroy() will not call
> devm_phy_provider_release() at all which will call the function, and the
> missing of_phy_provider_unregister() call will case:

Please split this up in two sentences as well.

> - The phy provider fails to be unregistered.
> - Leak both memory and the OF node refcount.

Perhaps a comment about there not being any in-tree users of this API is
in place here?

And you could consider dropping the function altogether as well.

> Fixed by using devres_release() instead of devres_destroy() within the API.
> 
> Fixes: ff764963479a ("drivers: phy: add generic PHY framework")
> Cc: stable@vger.kernel.org
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>

Looks good otherwise.

Johan

On 2024/10/29 21:43, Johan Hovold wrote:
> On Thu, Oct 24, 2024 at 10:39:27PM +0800, Zijun Hu wrote:
>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>
>> For devm_of_phy_provider_unregister(), its comment says it needs to invoke
>> of_phy_provider_unregister() to unregister the phy provider, but it does
>> not invoke the function actually since devres_destroy() will not call
>> devm_phy_provider_release() at all which will call the function, and the
>> missing of_phy_provider_unregister() call will case:
> 
> Please split this up in two sentences as well.
> 

good suggestions. will do it.

>> - The phy provider fails to be unregistered.
>> - Leak both memory and the OF node refcount.
> 
> Perhaps a comment about there not being any in-tree users of this API is
> in place here?
> 

okay, will do it as you suggest.

> And you could consider dropping the function altogether as well.
>

Remove the API devm_of_phy_provider_unregister()?

i prefer fixing it instead of removing it based on below considerations.

1) it is simper. just about one line change.
2) the API may be used in future. the similar API of [PATCH 1/6] have 2
usages.

>> Fixed by using devres_release() instead of devres_destroy() within the API.
>>
>> Fixes: ff764963479a ("drivers: phy: add generic PHY framework")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> Looks good otherwise.
> 
> Johan

On Tue, Oct 29, 2024 at 11:35:48PM +0800, Zijun Hu wrote:
> On 2024/10/29 21:43, Johan Hovold wrote:

> > And you could consider dropping the function altogether as well.
> 
> Remove the API devm_of_phy_provider_unregister()?
> 
> i prefer fixing it instead of removing it based on below considerations.
> 
> 1) it is simper. just about one line change.
> 2) the API may be used in future. the similar API of [PATCH 1/6] have 2
> usages.

We typically don't carry APIs that no one uses (or has ever used), but
sure, that can be done separately later if anyone cares.

Johan