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

From: Matthias Kaehlcke <mka@chromium.org>
To: Krishna Kurapati <quic_kriskura@quicinc.com>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Rob Herring <robh+dt@kernel.org>, Andy Gross <agross@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Felipe Balbi <balbi@kernel.org>,
	Stephen Boyd <swboyd@chromium.org>,
	Doug Anderson <dianders@chromium.org>,
	Mathias Nyman <mathias.nyman@intel.com>,
	devicetree@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-pm@vger.kernel.org, quic_pkondeti@quicinc.com,
	quic_ppratap@quicinc.com, quic_vpulyala@quicinc.com
Subject: Re: [v15 2/6] usb: host: xhci-plat: Enable wakeup based on children wakeup status
Date: Fri, 6 May 2022 08:36:31 -0700	[thread overview]
Message-ID: <YnVAZSZYQvIJxOHv@google.com> (raw)
In-Reply-To: <1651740973-7944-3-git-send-email-quic_kriskura@quicinc.com>

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@quicinc.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).