linux-kernel - Re: [PATCH v5 1/2] usb: host: plat: Enable xhci plat runtime PM

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: <58908DBC.5010208@linux.intel.com>
Date:   Tue, 31 Jan 2017 15:14:36 +0200
From:   Mathias Nyman <mathias.nyman@...ux.intel.com>
To:     Baolin Wang <baolin.wang@...aro.org>,
        Mathias Nyman <mathias.nyman@...el.com>,
        Felipe Balbi <balbi@...nel.org>
Cc:     Greg KH <gregkh@...uxfoundation.org>,
        Mark Brown <broonie@...nel.org>,
        USB <linux-usb@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Alan Stern <stern@...land.harvard.edu>
Subject: Re: [PATCH v5 1/2] usb: host: plat: Enable xhci plat runtime PM

On 16.01.2017 12:56, Baolin Wang wrote:
> Hi Mathias,

Hi

Sorry about the long review delay
CC Alan in case my pm assumptions need to be corrected

>
> On 13 December 2016 at 15:49, Baolin Wang <baolin.wang@...aro.org> wrote:
>> Enable the xhci plat runtime PM for parent device to suspend/resume xhci.
>> Also call pm_runtime_get_noresume() in probe() function in case the parent
>> device doesn't call suspend/resume callback by runtime PM now.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@...aro.org>
>> ---
>> Changes since v4:
>>   - No updates.
>>
>> Changes since v3:
>>   - Fix kbuild error.
>>
>> Changes since v2:
>>   - Add pm_runtime_get_noresume() in probe() function.
>>   - Add pm_runtime_set_suspended()/pm_runtime_put_noidle() in remove() function.
>>
>> Changes since v1:
>>   - No updates.
>> ---
>
> Do you have any comments about this patch? Thanks.
>
>>   drivers/usb/host/xhci-plat.c |   41 ++++++++++++++++++++++++++++++++++++-----
>>   1 file changed, 36 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
>> index ed56bf9..5805c6a 100644
>> --- a/drivers/usb/host/xhci-plat.c
>> +++ b/drivers/usb/host/xhci-plat.c
>> @@ -246,6 +246,10 @@ static int xhci_plat_probe(struct platform_device *pdev)
>>          if (ret)
>>                  goto dealloc_usb2_hcd;
>>
>> +       pm_runtime_get_noresume(&pdev->dev);
>> +       pm_runtime_set_active(&pdev->dev);
>> +       pm_runtime_enable(&pdev->dev);
>> +
>>          return 0;
>>
>>

To me this looks like we are not really enabling runtime pm for xhci, we increment the
usage count in probe, and keep it until remove is called.

This would normally prevent parent dwc3 from runtime suspending, but I see that in patch 2/2 adds
pm_suspend_ignore_children(dev, true) to dwc3, and, that dwc3 actually controls xhci runtime
pm by calling pm_runtime_get/put for xhci in its own dwc3 runtime_suspend/resume callbacks.

Looks a bit upside down to me, what's the reason for this?

what prevents calling pm_runtime_put_* before leaving probe in xhci and let pm code handle
the runtime suspend and parent/child relationships?

-Mathias