From: Jun Li <jun.li@nxp.com>
To: Peter Chen <peter.chen@kernel.org>,
Mathias Nyman <mathias.nyman@linux.intel.com>
Cc: Frank Li <frank.li@nxp.com>,
"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
Zhi Li <lznuaa@gmail.com>
Subject: RE: xhci crash at xhci_disable_hub_port_wake when system suspend.
Date: Wed, 12 Jan 2022 13:36:02 +0000 [thread overview]
Message-ID: <VI1PR04MB4333F89471E833AA5BAEFED389529@VI1PR04MB4333.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <20220112124820.GA3796@Peter>
> -----Original Message-----
> From: Peter Chen <peter.chen@kernel.org>
> Sent: Wednesday, January 12, 2022 8:48 PM
> To: Mathias Nyman <mathias.nyman@linux.intel.com>
> Cc: Frank Li <frank.li@nxp.com>; linux-usb@vger.kernel.org; Jun Li
> <jun.li@nxp.com>; Zhi Li <lznuaa@gmail.com>
> Subject: Re: xhci crash at xhci_disable_hub_port_wake when system suspend.
>
> On 22-01-10 23:07:48, Mathias Nyman wrote:
> > On 10.1.2022 14.33, Peter Chen wrote:
> > > On 22-01-07 15:58:26, Frank Li wrote:
> > >> Mathias Nyman
> > >>
> > >> Recently we found a crash at xhci_disable_hub_port_wake when system
> suspend if enable remote wake up.
> > >>
> > >> Basial flow is.
> > >>
> > >> 1. run time suspend call xhci_suspend, xhci parent devices gate the
> clock.
> > >> 2. echo mem >/sys/power/state, system _device_suspend call
> xhci_suspend
> > >> 3. xhci_suspend call xhci_disable_hub_port_wake, which access
> register, but clock already gated by run time pm.
> > >>
> > >> Why find this issue now, that is because previous power domain driver
> will call run time resume before it. But the below commit remove it.
> > >>
> > >> c1df456d0f06eb9275c1cd4c66548fc5738ea428
> > >> Author: Ulf Hansson ulf.hansson@linaro.org
> > >> Date: Thu Mar 4 20:28:43 2021 +0100
> > >>
> > >> PM: domains: Don't runtime resume devices at genpd_prepare()
> > >>
> > >>
> > >>
> > >> According to HCD_FLAG_HW_ACCESSIBLE logic, xhci should not access
> hardware when second time call xhci_suspend without call xhci_resume.
> > >>
> > >> xhci_disable_hub_port_wake(xhci, &xhci->usb3_rhub,
> do_wakeup);
> > >> xhci_disable_hub_port_wake(xhci, &xhci->usb2_rhub,
> > >> do_wakeup);
> > >>
> > >> if (!HCD_HW_ACCESSIBLE(hcd))
> > >> return 0;
> > >>
> > >> .....
> > >> clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> > >> clear_bit(HCD_FLAG_HW_ACCESSIBLE,
> > >> &xhci->shared_hcd->flags);
> > >>
> > >> I am not sure if it is safe to move xhci_disable_hub_port_wake after
> HCD_HW_ACCESSIBLE check, Or need add additional run_time_resume before it.
> >
> > We probably need to runtime resume first in case we need to adjust the
> > wakeup settings
> >
> > >
> > > Frank, I prefer adding runtime resume at xhci-plat.c like below,
> > > let's see what Mathias says.
> > >
> > >
> > > diff --git a/drivers/usb/host/xhci-plat.c
> > > b/drivers/usb/host/xhci-plat.c index c1edcc9b13ce..47a5a10381a7
> > > 100644
> > > --- a/drivers/usb/host/xhci-plat.c
> > > +++ b/drivers/usb/host/xhci-plat.c
> > > @@ -440,6 +440,9 @@ static int __maybe_unused xhci_plat_suspend(struct
> device *dev)
> > > ret = xhci_priv_suspend_quirk(hcd);
> > > if (ret)
> > > return ret;
> > > +
> > > + if (pm_runtime_suspended(dev))
> > > + pm_runtime_resume(dev);
> > > /*
> > > * xhci_suspend() needs `do_wakeup` to know whether host is allowed
> > > * to do wakeup during suspend.
> > >
> >
> > Yes, looks like a solution to me.
> > Just checked that driver-api/pm/devices.rst also suggest calling
> > pm_runtime_resume() in ->suspend callback if device needs to adjust
> > wakeup capabilities.
> >
> > Frank Li, does this work for you?
> >
> > Peter, if we now make sure xhci host is not runtime suspended at
> > system suspend, does it mean that the !HCD_HW_ACCESSIBLE(hcd) check
> > you added to xhci_suspend() is no longer needed?
>
> Yes, it is no longer needed if xhci host is not runtime suspended.
Looks like existing non xhci-plat user(xhci-tegra) is also calling xhci_resume()
before calling xhci_suspend(), so it's safe to remove it.
Li Jun
>
> --
>
> Thanks,
> Peter Chen
prev parent reply other threads:[~2022-01-12 13:36 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-07 15:58 xhci crash at xhci_disable_hub_port_wake when system suspend Frank Li
2022-01-10 12:33 ` Peter Chen
2022-01-10 21:07 ` Mathias Nyman
2022-01-10 21:37 ` Zhi Li
2022-01-12 12:48 ` Peter Chen
2022-01-12 13:36 ` Jun Li [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=VI1PR04MB4333F89471E833AA5BAEFED389529@VI1PR04MB4333.eurprd04.prod.outlook.com \
--to=jun.li@nxp.com \
--cc=frank.li@nxp.com \
--cc=linux-usb@vger.kernel.org \
--cc=lznuaa@gmail.com \
--cc=mathias.nyman@linux.intel.com \
--cc=peter.chen@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).