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: <CAMz4ku+NhwYTF8X_hrK2d1v1RVg+RVUruHhF2+N_Z+goQ3+ZcQ@mail.gmail.com>
Date:   Wed, 22 Mar 2017 18:40:21 +0800
From:   Baolin Wang <baolin.wang@...aro.org>
To:     Felipe Balbi <balbi@...nel.org>
Cc:     Mathias Nyman <mathias.nyman@...ux.intel.com>,
        Mathias Nyman <mathias.nyman@...el.com>,
        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

Hi,

On 22 March 2017 at 17:00, Felipe Balbi <balbi@...nel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@...aro.org> writes:
>>>>>> I don't yet understand why we can't just keep runtime pm disabled as a
>>>>>> default for xhci platform devices.
>>>>>> It could be enabled by whatever creates the platform device by setting some
>>>>>> device property
>>>>>> (or equivalent), which would be checked in xhci_plat_probe() before enabling
>>>>>> runtime pm. It
>>>>>> could then optionally be set in sysfs via power/control entry.
>>>>>
>>>>> I think runtime pm is not one hardware property, is it suitable if we
>>>>> introduce one device property to enable/disable runtime pm?
>>>>
>>>> As I said, runtime pm is not one hardware property, I think it is not
>>>> suitable if we introduce one device property to enable/disable runtime
>>>> pm.
>>>
>>> we already this functionality exposed on sysfs.
>>
>> From my understanding, Mathias suggested me to add one device property
>> (name like "usb-host-runtimePM") by platform_device_add_properties()
>> to enable/disable runtime PM when creating platform device, like
>> usb3_lpm_capable:
>>
>> if (dwc->usb3_lpm_capable)
>>       props[prop_idx++].name = "usb3-lpm-capable";
>>
>> ret = platform_device_add_properties(xhci, props);
>> if (ret) {
>>       dev_err(dwc->dev, "failed to add properties to xHCI\n");
>>       goto err1;
>> }
>>
>> What I think It is not suitable to introduce one device property like
>> above to enable/disable runtime PM, it is not one hardware attribute.
>
> yeah, that's silly. We already have means for doing that:
>
>         my_probe()
>         {
>                 [...]
>
>                 pm_runtime_enable(dev);
>                 pm_runtime_forbid(dev);

That's same with getting the usage counter.

>
>                 [...]
>
>                 return 0;
>         }
>
>>>> Secondly, we only can resume the xhci platform device by getting the
>>>> xhci usage counter from gadget driver, since the cable plug in/out
>>>> events only can be notified to glue layer of gadget driver(like dwc3
>>>> glue layer). That means if we want to suspend xhci platform device, we
>>>
>>> this is a problem with the glue layer, IMO. It should be something like
>>> so:
>>>
>>> static irqreturn_t dwc3_foobar_wakeup(int irq, void *_glue)
>>> {
>>>         struct dwc3_foobar_glue *glue = _glue;
>>>         u32 reg;
>>>
>>>         reg = real(glue->base, OFFSET);
>>>         if (reg & CONNECT)
>>>                 pm_runtime_resume(&glue->dwc3);
>>>
>>>         return IRQ_HANDLED;
>>> }
>>>
>>> then dwc3's ->runtime_resume() should check if the event is supposed to
>>> be handled by host or peripheral by checking which mode it was before
>>> suspend and making the assumption that we don't change modes while
>>> suspended. Something like:
>>>
>>> static int dwc3_runtime_resume(struct device *dev)
>>> {
>>>         struct dwc3 *dwc = dev_get_drvdata(dev);
>>>
>>>         [...]
>>>
>>>         if (dwc->is_host)
>>>                 pm_runtime_resume(dwc->xhci.dev);
>>>
>>>         [...]
>>>
>>>         return 0;
>>> }
>>
>> Yeah, if we don't need to get xhci usage counter in xhci_plat_probe()
>> to avoid affecting other controller's runtime PM, we can do like this
>> and do not need to get/put counter.
>
> why do you need to get xhci's usage counter in xhci_plat_probe() ?
>
> And why would one xhci affect the other? They are different struct
> device instances and, thus, have different pm usage counter. How would
> one xhci's pm_runtime affect another?

What I mean is if another USB controller's driver did not implement
runtime pm callbacks but system enables CONFIG_PM, that will make xhci
device auto-suspended when after probing xhci-plat if we did not get
xhci device usage counter, but gadget driver can not resume xhci
without implementing runtime PM callbacks.

If we want to implement xhci-plat runtime resume/suspend without
getting usage counter, we should assume every driver using xhci-plat
should implement their runtime PM callbacks. Is this right?

>
> PM runtime is not per driver, it's per device.

Correct.

-- 
Baolin.wang
Best Regards