linux-kernel - Re: [v15 2/6] usb: host: xhci-plat: Enable wakeup based on children wakeup status

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: <YnvcMe+irsndtcV0@google.com>
Date:   Wed, 11 May 2022 08:54:25 -0700
From:   Matthias Kaehlcke <mka@...omium.org>
To:     Pavan Kondeti <quic_pkondeti@...cinc.com>
Cc:     Krishna Kurapati <quic_kriskura@...cinc.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Felipe Balbi <balbi@...nel.org>,
        Stephen Boyd <swboyd@...omium.org>,
        Doug Anderson <dianders@...omium.org>,
        Mathias Nyman <mathias.nyman@...el.com>,
        devicetree@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-pm@...r.kernel.org, quic_ppratap@...cinc.com,
        quic_vpulyala@...cinc.com
Subject: Re: [v15 2/6] usb: host: xhci-plat: Enable wakeup based on children
 wakeup status

On Wed, May 11, 2022 at 07:21:01AM +0530, Pavan Kondeti wrote:
> On Mon, May 09, 2022 at 09:08:43AM +0530, Pavan Kondeti wrote:
> > On Fri, May 06, 2022 at 08:36:31AM -0700, Matthias Kaehlcke wrote:
> > > On Thu, May 05, 2022 at 02:26:09PM +0530, Krishna Kurapati wrote:
> > > > device_wakeup_path() tells if any of the children devices needs
> > > > wakeup. Use this hint to enable/disable wakeup of our device. This
> > > > helps the parent device of xhci-plat (like sysdev) to retrieve
> > > > the wakeup setting via device_wakeup_path().
> > > > 
> > > > Signed-off-by: Krishna Kurapati <quic_kriskura@...cinc.com>
> > > > ---
> > > >  drivers/usb/host/xhci-plat.c | 8 ++++++++
> > > >  1 file changed, 8 insertions(+)
> > > > 
> > > > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > > > index 649ffd8..ad585fa 100644
> > > > --- a/drivers/usb/host/xhci-plat.c
> > > > +++ b/drivers/usb/host/xhci-plat.c
> > > > @@ -415,6 +415,14 @@ static int __maybe_unused xhci_plat_suspend(struct device *dev)
> > > >  	if (pm_runtime_suspended(dev))
> > > >  		pm_runtime_resume(dev);
> > > >  
> > > > +	if (device_wakeup_path(dev)) {
> > > > +		if (!device_may_wakeup(dev))
> > > > +			device_wakeup_enable(dev);
> > > > +	} else {
> > > > +		if (device_may_wakeup(dev))
> > > > +			device_wakeup_disable(dev);
> > > > +	}
> > > 
> > > This code is not self-explantatory and deserves a comment.
> > > 
> > > Enabling/disabling wakeup for the purpose if signalling is a bit of a
> > > hack. It might be an acceptable hack as long as it has no side effects.
> > > However with the current implementation the wakeup state of the xHCI can
> > > be different after resuming than it was before going to suspend:
> > > 
> > > after boot
> > >   grep -h xhci /sys/class/wakeup/*/name
> > >     => xhci-hcd.14.auto
> > > 
> > > after suspend w/o wakeup capable device
> > >   grep -h xhci /sys/class/wakeup/*/name
> > >     => no results
> > > 
> > > after suspend with wakeup capable device
> > >   grep -h xhci /sys/class/wakeup/*/name
> > >     => xhci-hcd.14.auto
> > > 
> > > The hack shouldn't alter the wakeup state 'persistently', i.e. you'll have
> > > to restore it on resume, as in Pavan does in his reply to '[PATCH v14 2/7]
> > > PM / wakeup: Add device_children_wakeup_capable()' (it needs to be done
> > > conditionally though).
> > 
> > I am worried that we are not doing the right thing here. why should the
> > xhci-plat goes against the wishes of the user space policy here? Can we NOT
> > just do anything here? If some one wants xhci-plat to wakeup all the time,
> > dwc3 will be configured to wakeup the system provided that the support is
> > available. This way we don't break any existing users of xhci-plat i.e not
> > enabling wakeup from the kernel.
> > 
> Krishna,
> 
> can we please drop this patch and use device_wakeup_path() and verify the
> following cases.
> 
> 1. one of the downstream USB device supports wakeup and xhci-plat wakeup is enabled
> 2. one of the downstream USB device supports wakeup and xhci-plat wakeup is
> disabled
> 3. none of the downstream USB device supports wakeup (or disable) and
> xhci-plat wakeup is enabled.
> 4. none of the downstream USB device supports wakeup (or disable) and
> xhci-plat wakeup is disabled.

I wonder if we couldn't keep this simpler: if the dwc3 is wakeup capable keep
the PHYs/core powered, otherwise power them down. Similar to what commit
689bf72c6e0d ("usb: dwc3: Don't reinitialize core during host
bus-suspend/resume") intended, but with the additonal check for wakeup
capability. We now know that the PHYs need to be powered down on some SoCs
to allow the SoC to reach its low power mode during suspend:


  commit c4a5153e87fdf6805f63ff57556260e2554155a5
  Author: Manu Gautam <mgautam@...eaurora.org>
  Date:   Thu Jan 18 16:54:30 2018 +0530

  usb: dwc3: core: Power-off core/PHYs on system_suspend in host mode

  Commit 689bf72c6e0d ("usb: dwc3: Don't reinitialize core during
  host bus-suspend/resume") updated suspend/resume routines to not
  power_off and reinit PHYs/core for host mode.
  It broke platforms that rely on DWC3 core to power_off PHYs to
  enter low power state on system suspend.


With wakeup capable controllers this is apparently not an issue, otherwise
the SoC wouldn't be able to enter its low power state when wakeup is
enabled.