LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Doug Anderson <dianders@chromium.org>
To: Sibi Sankar <sibis@codeaurora.org>
Cc: Stephen Boyd <swboyd@chromium.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Alex Elder <elder@linaro.org>,
	Matthias Kaehlcke <mka@chromium.org>,
	Andy Gross <agross@kernel.org>, Vinod Koul <vkoul@kernel.org>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/2] soc: qcom: aoss: Don't wait for IRQ if we might be in suspend/resume noirq
Date: Tue, 11 Aug 2020 14:21:33 -0700
Message-ID: <CAD=FV=WWYA-hxdAVdLHj9Ej9nCWohOHsh9GA1GZ3Mg-XuJfeyA@mail.gmail.com> (raw)
In-Reply-To: <8450aff2d16aed092295c61a8e4ca850@codeaurora.org>

Hi,

On Thu, Aug 6, 2020 at 10:33 AM Sibi Sankar <sibis@codeaurora.org> wrote:
>
> On 2020-08-06 22:40, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Aug 6, 2020 at 7:36 AM Sibi Sankar <sibis@codeaurora.org>
> > wrote:
> >>
> >> On 2020-08-06 04:32, Stephen Boyd wrote:
> >> > +Sibi who wrote the code
> >> >
> >> > Quoting Doug Anderson (2020-08-05 13:24:06)
> >> >>
> >> >> On Wed, Aug 5, 2020 at 10:36 AM Stephen Boyd <swboyd@chromium.org>
> >> >> wrote:
> >> >> >
> >> >> > Why is the genpd being powered off at all? It looks like the driver is
> >> >> > written in a way that it doesn't expect this to happen. See where
> >> >> > adsp_pds_disable() is called from. Looks like the remoteproc "stop"
> >> >> > callback should be called or the driver should be detached.
> >> >> >
> >> >> > It sort of looks like the genpd is expected to be at the max level all
> >> >> > the time (it sets INT_MAX in adsp_pds_enable(), cool).
> >> >>
> >> >> In general in Linux there are some things that, at suspend time, get
> >> >> done behind a driver's back.  The regulator API, for instance, allows
> >> >> for regulators to be turned off in suspend even if a driver leaves
> >> >> them on.  Sure, it's good practice for a driver to be explicit but the
> >> >> regulator suspend states do allow for the more heavy-handed approach.
> >> >>
> >> >> I guess I assume that genpd is a bit similar.  If a driver leaves a
> >> >> genpd on all the time then it will still be turned off at suspend time
> >> >> and then turned back on at resume time.  It seems like it must be part
> >> >> of the genpd API.  Specifically genpd_sync_power_off() says: "Check if
> >> >> the given PM domain can be powered off (during system suspend or
> >> >> hibernation) and do that if so."  That makes it seem like it's how
> >> >> genpd works.
> >> >>
> >> >> Reading all the descriptions of things like GENPD_FLAG_ALWAYS_ON,
> >> >> GENPD_FLAG_ACTIVE_WAKEUP, GENPD_FLAG_RPM_ALWAYS_ON makes me even more
> >> >> convinced that it's normal (unless otherwise specified) for genpds to
> >> >> get turned off in suspend even if a driver just blindly left them on.
> >> >>
> >> >> Presumably if this "modem" genpd is supposed to stay on in suspend
> >> >> time it should have been marked "always on"?  I'd guess we'd need to
> >> >> add "GENPD_FLAG_ALWAYS_ON" in some (or all?) cases in qmp_pd_add() if
> >> >> this was true?
> >> >
> >> > Agreed. I can't read the mind of Sibi so I can only guess that Sibi
> >> > wasn't expecting this behavior by reading the driver structure. That
> >> > could be a wrong assumption.
> >> >
> >> >>
> >> >>
> >> >> > Maybe we need to
> >> >> > add some sort of suspend hooks to the remote proc driver instead? Where
> >> >> > those suspend hooks are called earlier and drop the genpd performance
> >> >> > state request but otherwise leave it enabled across suspend?
> >> >>
> >> >> I think you're saying:
> >> >>
> >> >> a) You think it's a bug today that the "modem" genpd is being powered
> >> >> off in suspend.  Any evidence to back this up?
> >> >>
> >> >> b) Assuming it's a bug today, we should mark the "modem" as
> >> >> GENPD_FLAG_ALWAYS_ON.
> >> >>
> >> >> c) If there are genpds that sometimes should be left on in suspend but
> >> >> sometimes not (and that doesn't match up with what
> >> >> GENPD_FLAG_ACTIVE_WAKEUP does), then we'd have to pass
> >> >> GENPD_FLAG_ALWAYS_ON as a flag and then add suspend hooks to make the
> >> >> decision for us.
> >>
> >> Doug/Stephen,
> >>
> >> Yes this is a bug, we wouldn't want
> >> to disable aoss_qmp genpd for modem
> >> during suspend (when the modem is
> >> running). The qmp send for modem
> >> is the primary means through which
> >> aoss determines whether to wait for
> >> modem before proceeding to sleep. So
> >> looks like updating the flag with
> >> GENPD_FLAG_ACTIVE_WAKEUP is the way
> >> to go. But introducing another flag
> >> that doesn't touch genpd's during
> >> suspend/resume should also work.
> >
> > OK, sounds good.  As per out-of-band conversation:
> >
> > * You'll plan to post a patch updating the flag.
> >
> > * There's still nothing here that says my patch is the wrong thing to
> > do also.  It seems like genpd poweroff routine are expected to be able
> > to run at "noirq" time so we should make sure we are able to do that.
> >
> > I'm also curious: my patch doesn't affect the behavior.  The genpd
> > would be powered off with or without my patch, my patch just removes a
> > pointless 1 second delay.  Therefore I guess today there is some type
> > of bug because the genpd is being turned off.  What would be the
> > visible impact of that bug?  ...or is it somehow masked by something
> > else keeping this power on so it wasn't an issue right now?
>
> I've been told AOSS decides to wait
> for modem suspend if its been notified
> that modem is on through qmp_send. AFAIK
> we never ran into this because AOSS sleep
> sequence starts after xo-shutdown which
> wont be reached in the presence of active
> rpmh votes from modem.
>
> Regardless we definitely want this genpd left
> untouched during suspend/resume.

With Sibi's patch [1] then ${SUBJECT} patch is no longer needed since
we are no longer called during "noirq" / "syscore" time.  Assuming
Sibi's patches (or something similar to them) are OK, we can consider
this patch abandoned.  I'll re-post patch #2 on its own once we get
confirmation that Sibi's patches are OK w/ folks.

[1] https://lore.kernel.org/r/20200811190252.10559-2-sibis@codeaurora.org

-Doug

      reply index

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-05 16:16 Douglas Anderson
2020-08-05 16:16 ` [PATCH 2/2] soc: qcom: aoss: qmp_send() can't handle interruptions so stop pretending Douglas Anderson
2020-08-05 17:28   ` Stephen Boyd
2020-08-05 20:25     ` Doug Anderson
2020-08-05 17:36 ` [PATCH 1/2] soc: qcom: aoss: Don't wait for IRQ if we might be in suspend/resume noirq Stephen Boyd
2020-08-05 20:24   ` Doug Anderson
2020-08-05 23:02     ` Stephen Boyd
2020-08-06 14:34       ` Sibi Sankar
2020-08-06 17:10         ` Doug Anderson
2020-08-06 17:33           ` Sibi Sankar
2020-08-11 21:21             ` Doug Anderson [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='CAD=FV=WWYA-hxdAVdLHj9Ej9nCWohOHsh9GA1GZ3Mg-XuJfeyA@mail.gmail.com' \
    --to=dianders@chromium.org \
    --cc=agross@kernel.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=elder@linaro.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mka@chromium.org \
    --cc=sibis@codeaurora.org \
    --cc=swboyd@chromium.org \
    --cc=vkoul@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

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git