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

Powered by blists - more mailing lists