Date:   Tue, 16 Nov 2021 19:09:32 -0800
From:   Brian Norris <briannorris@chromium.org>
To:     Matthias Kaehlcke <mka@chromium.org>
Cc:     Sandeep Maheswaram <quic_c_sanm@quicinc.com>,
        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>,
        linux-arm-msm@vger.kernel.org, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org, quic_pkondeti@quicinc.com,
        quic_ppratap@quicinc.com
Subject: Re: [PATCH v9 2/5] usb: dwc3: core: Host wake up support from system
 suspend
Message-ID: <YZRybAkx1YLiVvfl@google.com>
References: <1635753224-23975-1-git-send-email-quic_c_sanm@quicinc.com>
 <1635753224-23975-3-git-send-email-quic_c_sanm@quicinc.com>
 <YZRMoNEZTy8XimIx@google.com>
 <YZRXZs5B08SaBqMx@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YZRXZs5B08SaBqMx@google.com>
Precedence: bulk

On Tue, Nov 16, 2021 at 05:14:14PM -0800, Matthias Kaehlcke wrote:
> On Tue, Nov 16, 2021 at 04:28:16PM -0800, Brian Norris wrote:
> > On Mon, Nov 01, 2021 at 01:23:41PM +0530, Sandeep Maheswaram wrote:
> > > +		if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(&dwc->xhci->dev)) {
> > 
> > I still find it odd to use device_may_wakeup(), since that's something
> > controlled by user space (sysfs) and doesn't fully factor in hardware
> > support. But that's what xhci-plat.c is doing, so I guess I see why
> > you're imitating it...
> > ...still, feels wrong to me. But so does a lot of how dwc3 works.
> 
> device_may_wakeup() actually factors in hardware support, at least if the
> driver does the right thing (TM).

Well in theory, maybe. But the latter half of the sentence is the key :)

In particular, xhci-plat does the Wrong Thing before this series:

	device_set_wakeup_capable(&pdev->dev, true);

i.e., it doesn't factor any "capability" in at all; it just assumes it.

And per your thoughts below, it's still Wrong after this series.

> The (current) implementation is:
> 
> static inline bool device_may_wakeup(struct device *dev)
> {
> 	return dev->power.can_wakeup && !!dev->power.wakeup;
> }
> 
> '.can_wakeup' should describe the hardware capability to wake up and the
> other flag whether wakeup is enabled (which can be altered by userspace).
> 
> What this series currently does with the .can_wakeup flag is still wrong
> though IMO. Patch "[1/5] usb: host: xhci: plat: Add suspend quirk for dwc3
> controller" [1] dynamically sets the flag with a value that depends on what
> is connected to the bus, so it doesn't specify any longer whether the
> hardware supports wakeup or not.
> 
> [1] https://patchwork.kernel.org/project/linux-usb/patch/1635753224-23975-2-git-send-email-quic_c_sanm@quicinc.com/

I'm not sure either your patch nor Sandeep's patch really get at the
heart of my problem here, which is that neither dwc3 nor xhci-plat are
trying to reflect wakeup capability of the host controller at all. (And
if the host controller doesn't suppor wakeup, it doesn't really matter
what any of its children think.) It seems that
drivers/usb/dwc3/dwc3-imx8mp.c is the only one that actually sets the
correct wakeup flag at the level that we _really_ know what's up -- the
platform/"glue" driver.

Maybe we need to do a little of both: teach the glue drivers (e.g.,
dwc3-qcom.c) to reflect their wakeup capability properly, and then look
at *that* capability (as well as any children, recursively, but only if
the glue driver supports it) when trying to make wakeup decisions.

It still feels wrong that there are 3 separate "can wakeup"
determinations for the host controller though: 1 dwc3-{glue}, 1
dwc3(core), and 1 xhci-plat. But maybe we have to hold our noses on that
one.

Brian