linux-kernel - Re: [PATCH net-next v3 07/19] net: ravb: Move reference clock enable/disable on runtime PM APIs

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] [day] [month] [year] [list]

Date: Tue, 23 Jan 2024 23:06:56 +0300
From: Sergey Shtylyov <s.shtylyov@....ru>
To: claudiu beznea <claudiu.beznea@...on.dev>, <davem@...emloft.net>,
	<edumazet@...gle.com>, <kuba@...nel.org>, <pabeni@...hat.com>,
	<richardcochran@...il.com>, <p.zabel@...gutronix.de>,
	<yoshihiro.shimoda.uh@...esas.com>, <wsa+renesas@...g-engineering.com>
CC: <netdev@...r.kernel.org>, <linux-renesas-soc@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <geert+renesas@...der.be>, Claudiu Beznea
	<claudiu.beznea.uj@...renesas.com>
Subject: Re: [PATCH net-next v3 07/19] net: ravb: Move reference clock
 enable/disable on runtime PM APIs

On 1/8/24 11:03 AM, claudiu beznea wrote:

   Oops, looks like I forgot to reply to this one...

[...]
>>> From: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
>>>
>>> Reference clock could be or not part of the power domain. If it is part of
>>> the power domain, the power domain takes care of propertly setting it. In
>>> case it is not part of the power domain and full runtime PM support is
>>> available in driver the clock will not be propertly disabled/enabled at
>>> runtime. For this, keep the prepare/unprepare operations in the driver's
>>> probe()/remove() functions and move the enable/disable in runtime PM
>>> functions.
>>>
>>> Along with it, the other clock request operations were moved close to
>>> reference clock request and prepare to have all the clock requests
>>> specific code grouped together.
>>>
>>> Reviewed-by: Sergey Shtylyov <s.shtylyov@....ru>

[...]

>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
[...]

>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>> index 844ac3306e93..4673cc2faec0 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
>>> +	pm_runtime_enable(&pdev->dev);
>>> +	error = pm_runtime_resume_and_get(&pdev->dev);
>>> +	if (error < 0)
>>> +		goto out_rpm_disable;
>>> +
>>>  	priv->addr = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
>>>  	if (IS_ERR(priv->addr)) {
>>>  		error = PTR_ERR(priv->addr);
>>> -		goto out_release;
>>> +		goto out_rpm_put;
>>>  	}
>>>  
>>>  	/* The Ether-specific entries in the device structure. */
[...]
>>> @@ -3060,21 +3058,27 @@ static int ravb_resume(struct device *dev)
>>>  	return ret;
>>>  }
>>>  
>>> -static int ravb_runtime_nop(struct device *dev)
>>> +static int ravb_runtime_suspend(struct device *dev)
>>>  {
>>> -	/* Runtime PM callback shared between ->runtime_suspend()
>>> -	 * and ->runtime_resume(). Simply returns success.
>>> -	 *
>>> -	 * This driver re-initializes all registers after
>>> -	 * pm_runtime_get_sync() anyway so there is no need
>>> -	 * to save and restore registers here.
>>> -	 */
>>
>>    Perhaps even worth a separate patch to completely remove this function
>> which doesn't seem to make sense?
> 
> Why? With that the refclk will not be properly enabled/disabled when it
> will not be part of the power domain. Take

   That's what you are adding in this patch, right? Before this patch
this was ravb_runtime_nop(), always returning 0. Did it make any sense?

> https://elixir.bootlin.com/linux/v6.7/source/arch/arm64/boot/dts/renesas/beacon-renesom-som.dtsi#L57
> as an example. Here refclk is from an external source (not part of power
> domain).
> 
> Thank you,
> Claudiu Beznea

[...]

MBR, Sergey