netdev - Re: [PATCH net-next 3/3] net: phy: bcm7xxx: request and manage GPHY clock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ccfa67f5-d3dd-26a6-1bb8-9772e2434d82@gmail.com>
Date:   Fri, 4 Sep 2020 08:37:30 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Marco Felsch <m.felsch@...gutronix.de>
Cc:     netdev@...r.kernel.org, andrew@...n.ch, adam.rudzinski@....net.pl,
        hkallweit1@...il.com, richard.leitner@...data.com,
        zhengdejin5@...il.com, devicetree@...r.kernel.org,
        kernel@...gutronix.de, kuba@...nel.org, robh+dt@...nel.org
Subject: Re: [PATCH net-next 3/3] net: phy: bcm7xxx: request and manage GPHY
 clock



On 9/3/2020 11:15 PM, Marco Felsch wrote:
> Hi Florian,
> 
> On 20-09-02 21:39, Florian Fainelli wrote:
>> The internal Gigabit PHY on Broadcom STB chips has a digital clock which
>> drives its MDIO interface among other things, the driver now requests
>> and manage that clock during .probe() and .remove() accordingly.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@...il.com>
>> ---
>>   drivers/net/phy/bcm7xxx.c | 18 +++++++++++++++++-
>>   1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/phy/bcm7xxx.c b/drivers/net/phy/bcm7xxx.c
>> index 692048d86ab1..f0ffcdcaef03 100644
>> --- a/drivers/net/phy/bcm7xxx.c
>> +++ b/drivers/net/phy/bcm7xxx.c
>> @@ -11,6 +11,7 @@
>>   #include "bcm-phy-lib.h"
>>   #include <linux/bitops.h>
>>   #include <linux/brcmphy.h>
>> +#include <linux/clk.h>
>>   #include <linux/mdio.h>
>>   
>>   /* Broadcom BCM7xxx internal PHY registers */
>> @@ -39,6 +40,7 @@
>>   
>>   struct bcm7xxx_phy_priv {
>>   	u64	*stats;
>> +	struct clk *clk;
>>   };
>>   
>>   static int bcm7xxx_28nm_d0_afe_config_init(struct phy_device *phydev)
>> @@ -534,7 +536,19 @@ static int bcm7xxx_28nm_probe(struct phy_device *phydev)
>>   	if (!priv->stats)
>>   		return -ENOMEM;
>>   
>> -	return 0;
>> +	priv->clk = devm_clk_get_optional(&phydev->mdio.dev, NULL);
> 
> Since the clock is binded to the mdio-dev here..
> 
>> +	if (IS_ERR(priv->clk))
>> +		return PTR_ERR(priv->clk);
>> +
>> +	return clk_prepare_enable(priv->clk);
> 
> clould we use devm_add_action_or_reset() here so we don't have to
> register the .remove() hook?

Maybe, more on that below.

> 
>> +}
>> +
>> +static void bcm7xxx_28nm_remove(struct phy_device *phydev)
>> +{
>> +	struct bcm7xxx_phy_priv *priv = phydev->priv;
>> +
>> +	clk_disable_unprepare(priv->clk);
>> +	devm_clk_put(&phydev->mdio.dev, priv->clk);
> 
> Is this really necessary? The devm_clk_get_optional() function already
> registers the devm_clk_release() hook.

Yes, because you can unbind the PHY driver from sysfs, and if you want 
to bind that driver again, which will call .probe() again, you must undo 
strictly everything that .probe() did. The embedded mdio_device does not 
go away, so there will be no automatic freeing of resources. Using 
devm_* may be confusing, so using just the plain clk_get() and clk_put() 
may be clearer here.
-- 
Florian