linux-kernel - Re: [RFC PATCH 3/5] net: macb: Add pm runtime support

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: <87bd6e9f-be18-b267-a01d-c4efc2adc55d@microchip.com>
Date:   Thu, 3 May 2018 15:59:01 +0300
From:   Claudiu Beznea <Claudiu.Beznea@...rochip.com>
To:     Harini Katakam <harinik@...inx.com>
CC:     Nicolas Ferre <nicolas.ferre@...el.com>,
        David Miller <davem@...emloft.net>, <netdev@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <michals@...inx.com>,
        <appanad@...inx.com>,
        Shubhrajyoti Datta <shubhrajyoti.datta@...inx.com>
Subject: Re: [RFC PATCH 3/5] net: macb: Add pm runtime support



On 03.05.2018 14:13, Harini Katakam wrote:
> Hi Claudiu,
> 
> On Thu, May 3, 2018 at 3:39 PM, Claudiu Beznea
> <Claudiu.Beznea@...rochip.com> wrote:
>>
>>
>> On 22.03.2018 15:51, harinikatakamlinux@...il.com wrote:
>>> From: Harini Katakam <harinik@...inx.com>
> <snip>
>> I would use a "goto" instruction, e.g.:
>>                 value = -ETIMEDOUT;
>>                 goto out;
>>
> 
> Will do
> 
>>
>> Below, in macb_open() you have a return err; case:
>>         err = macb_alloc_consistent(bp);
>>         if (err) {
>>                 netdev_err(dev, "Unable to allocate DMA memory (error %d)\n",
>>                            err);
>>                 return err;
>>         }
>>
>> You have to undo pm_runtime_get_sync() with pm_runtime_put_sync() or something
>> similar to decrement dev->power.usage_count.
> 
> Will do
> 
> <snip>
>>> @@ -4104,11 +4145,16 @@ static int macb_remove(struct platform_device *pdev)
>>>               mdiobus_free(bp->mii_bus);
>>>
>>>               unregister_netdev(dev);
>>> -             clk_disable_unprepare(bp->tx_clk);
>>> -             clk_disable_unprepare(bp->hclk);
>>> -             clk_disable_unprepare(bp->pclk);
>>> -             clk_disable_unprepare(bp->rx_clk);
>>> -             clk_disable_unprepare(bp->tsu_clk);
>>> +             pm_runtime_disable(&pdev->dev);
>>> +             pm_runtime_dont_use_autosuspend(&pdev->dev);
>>> +             if (!pm_runtime_suspended(&pdev->dev)) {
>>> +                     clk_disable_unprepare(bp->tx_clk);
>>> +                     clk_disable_unprepare(bp->hclk);
>>> +                     clk_disable_unprepare(bp->pclk);
>>> +                     clk_disable_unprepare(bp->rx_clk);
>>> +                     clk_disable_unprepare(bp->tsu_clk);
>>> +                     pm_runtime_set_suspended(&pdev->dev);
>>
>> This is driver remove function. Shouldn't clocks be removed?
> 
> clk_disable_unprepare IS being done here.
> The check for !pm_runtime_suspended is just to make sure the
> clocks are not already removed (in runtime_suspend).

Could this happen? Looking over code, starting with 
platform_driver_unregister() it looks to me that the runtime resume
is called just before driver remove is called.

platform_driver_unregister() ->
driver_unregister() ->
bus_remove_driver() ->
driver_detach() ->
device_release_driver_internal() ->
__device_release_driver()
{
	// ...
        pm_runtime_get_sync(dev);		// runtime resume                                       
        pm_runtime_clean_up_links(dev);                                 
 
	// ...

	pm_runtime_put_sync(dev);                                       
                                                                                
        if (dev->bus && dev->bus->remove)                               
        	dev->bus->remove(dev);                                  
        else if (drv->remove)                                           
                drv->remove(dev);                                       

	// ...
}

Thank you,
Claudiu

> 
> Regards,
> Harini
>