Received-SPF: pass (google.com: domain of linux-kernel+bounces-62327-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Subject: Re: [PATCH net-next v2 5/5] net: ravb: Add runtime PM support
To: claudiu beznea <claudiu.beznea@tuxon.dev>, <davem@davemloft.net>,
	<edumazet@google.com>, <kuba@kernel.org>, <pabeni@redhat.com>
CC: <netdev@vger.kernel.org>, <linux-renesas-soc@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, Claudiu Beznea
	<claudiu.beznea.uj@bp.renesas.com>
References: <20240209170459.4143861-1-claudiu.beznea.uj@bp.renesas.com>
 <20240209170459.4143861-6-claudiu.beznea.uj@bp.renesas.com>
 <3808dee0-b623-b870-7d96-94cc5fc12350@omp.ru>
 <7d0ae75d-2fdb-47cb-b57b-20ee477d6081@tuxon.dev>
From: Sergey Shtylyov <s.shtylyov@omp.ru>
Organization: Open Mobile Platform
Message-ID: <1990e269-44fe-b45f-09b5-0c84f21778fc@omp.ru>
Date: Mon, 12 Feb 2024 23:19:43 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.10.1
Precedence: bulk
MIME-Version: 1.0
In-Reply-To: <7d0ae75d-2fdb-47cb-b57b-20ee477d6081@tuxon.dev>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
X-KSE-Attachment-Filter-Triggered-Filters: Clean
X-KSE-BulkMessagesFiltering-Scan-Result: InTheLimit

On 2/12/24 10:56 AM, claudiu beznea wrote:

[...]

>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>
>>> Add runtime PM support for the ravb driver. As the driver is used by
>>> different IP variants, with different behaviors, to be able to have the
>>> runtime PM support available for all devices, the preparatory commits
>>> moved all the resources parsing and allocations in the driver's probe
>>> function and kept the settings for ravb_open(). This is due to the fact
>>> that on some IP variants-platforms tuples disabling/enabling the clocks
>>> will switch the IP to the reset operation mode where registers' content is
>>
>>    This pesky "registers' content" somehow evaded me -- should be "register
>> contents" as well...
>>
>>> lost and reconfiguration needs to be done. For this the rabv_open()
>>> function enables the clocks, switches the IP to configuration mode, applies
>>> all the registers settings and switches the IP to the operational mode. At
>>> the end of ravb_open() IP is ready to send/receive data.
>>>
>>> In ravb_close() necessary reverts are done (compared with ravb_open()), the
>>> IP is switched to reset mode and clocks are disabled.
>>>
>>> The ethtool APIs or IOCTLs that might execute while the interface is down
>>> are either cached (and applied in ravb_open()) or rejected (as at that time
>>> the IP is in reset mode). Keeping the IP in the reset mode also increases
>>> the power saved (according to the hardware manual).
>>>
>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>> [...]
>>
>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>> index f4be08f0198d..5bbfdfeef8a9 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>> @@ -1939,16 +1939,21 @@ static int ravb_open(struct net_device *ndev)
>>>  {
>>>  	struct ravb_private *priv = netdev_priv(ndev);
>>>  	const struct ravb_hw_info *info = priv->info;
>>> +	struct device *dev = &priv->pdev->dev;
>>>  	int error;
>>>  
>>>  	napi_enable(&priv->napi[RAVB_BE]);
>>>  	if (info->nc_queues)
>>>  		napi_enable(&priv->napi[RAVB_NC]);
>>>  
>>> +	error = pm_runtime_resume_and_get(dev);
>>> +	if (error < 0)
>>> +		goto out_napi_off;
>>
>>    Well, s/error/ret/ -- it would fit better here...
> 
> Using error is the "trademark" of this driver, it is used all around the
> driver. I haven't introduced it here, I don't like it. The variable error

   Heh, because it's my usual style. Too bad you don't like it... :-)

> in this particular function is here from the beginning of the driver.

   I think it's well suited for the functions returning either 0 or a
(negative) error code. It's *if* (error < 0) that confuses me (as this
API can return positive numbers in case of success...

> So, I don't consider changing error to ret is the scope of this series.

   OK, you're probably right... are you going to respin the series because
of Biju's comments?

[...]
>>> @@ -3066,6 +3089,12 @@ static void ravb_remove(struct platform_device *pdev)
>>>  	struct net_device *ndev = platform_get_drvdata(pdev);
>>>  	struct ravb_private *priv = netdev_priv(ndev);
>>>  	const struct ravb_hw_info *info = priv->info;
>>> +	struct device *dev = &priv->pdev->dev;
>>> +	int error;
>>> +
>>> +	error = pm_runtime_resume_and_get(dev);
>>> +	if (error < 0)
>>> +		return;
>>
>>    Again, s/erorr/ret/ in this case.
> 
> error was used here to comply with the rest of the driver. So, if you still
> want me to change it here and in ravb_remove() please confirm.

   No, we are good enough without that; I'll consider doing a cleanup
when/if I have time. :-)

> Thank you,
> Claudiu Beznea

MBR, Sergey