linux-kernel - Re: [PATCH 2/2] usb: dwc3: of_simple: don't call pm_runtime_set

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: <4a4fd59c-84a6-61c6-5567-50660b07302a@ti.com>
Date:   Wed, 13 Jun 2018 14:15:11 +0300
From:   Roger Quadros <rogerq@...com>
To:     Felipe Balbi <balbi@...nel.org>, Tero Kristo <t-kristo@...com>,
        "Gerlach, Dave" <d-gerlach@...com>
CC:     <linux-usb@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        Nishanth Menon <nm@...com>, "Nori, Sekhar" <nsekhar@...com>,
        Alan Stern <stern@...land.harvard.edu>,
        Johan Hovold <johan@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH 2/2] usb: dwc3: of_simple: don't call
 pm_runtime_set_active()

On 31/05/18 10:59, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@...com> writes:
> 
>> On 30/05/18 15:31, Felipe Balbi wrote:
>>> Roger Quadros <rogerq@...com> writes:
>>>
>>>> Don't call pm_runtime_set_active() as it will prevent the device
>>>> from being activated in the next pm_runtime_get_sync() call.
>>>>
>>>> Also call pm_runtime_get_sync() before of_platform_populate().
>>>>
>>>> Signed-off-by: Roger Quadros <rogerq@...com>
>>>
>>> This patch is wrong.
>>>
>>>> ---
>>>>  drivers/usb/dwc3/dwc3-of-simple.c | 7 +++----
>>>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
>>>> index e98d221..2cbb5c0 100644
>>>> --- a/drivers/usb/dwc3/dwc3-of-simple.c
>>>> +++ b/drivers/usb/dwc3/dwc3-of-simple.c
>>>> @@ -121,6 +121,9 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
>>>>  	if (ret)
>>>>  		goto err_resetc_assert;
>>>>  
>>>> +	pm_runtime_enable(dev);
>>>> +	pm_runtime_get_sync(dev);
>>>
>>> No, this is the wrong way to do things. My device should be enabled
>>> already from probe, specially since I have already enabled clocks.
>>
>> As far as I understood just enabling clocks (which might not include bus clock)
>> doesn't ensure device is enabled.
>>
>> Did you mean that I don't need to do a pm_runtime_get_sync() to enable my device in probe?
>> Who is enabling by device for me then? Is device core supposed to do it?
> 
> Not device core, but the bus code. Look at how PCI handles it. IIRC,
> only TI's omap_device behaves peculiarly WRT probe & pm runtime.

PCI does it doesn't mean it is a rule that bus code has to enable the device before probe.
At least platform bus doesn't seem to do it as part of of_platform_populate().

see __device_attach() and driver_probe_device() in drivers/base/dd.c

It does a pm_runtime_get_sync() on dev->parent but not on dev.

Also, from section 5 of Documentation/power/runtime_pm.txt

"In addition to that, the initial runtime PM status of all devices is
'suspended', but it need not reflect the actual physical state of the device.
Thus, if the device is initially active (i.e. it is able to process I/O), its
runtime PM status must be changed to 'active', with the help of
pm_runtime_set_active(), before pm_runtime_enable() is called for the device."

"Note, if the device may execute pm_runtime calls during the probe (such as
if it is registers with a subsystem that may call back in) then the
pm_runtime_get_sync() call paired with a pm_runtime_put() call will be
appropriate to ensure that the device is not put back to sleep during the
probe. This can happen with systems such as the network device layer."

So looks like we can't assume that the device is "active" when probe() is called.
Which means, we need to do

pm_runtime_get_sync();
enable optional clocks;

pm_runtime_forbid(); if we don't yet want runtime suspend/resume.
OR
pm_runtime_put(); if we want runtime suspend/resume support.

I don't think we should be calling pm_runtime_set_active() as we can't be sure of the
initial state of the device for different buses.
> 
>> The issue I was facing is that without this patch my device wasn't being enabled
>> as pm_runtime_set_active() is being done _before_ pm_runtime_get_sync().
>> It could be an issue with the platform's PM domain code as well.
> 
> Could be
> 

-- 
cheers,
-roger

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki