LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Sibi Sankar <sibis@codeaurora.org>
To: Brian Norris <briannorris@chromium.org>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>,
	Ramon Fried <ramon.fried@gmail.com>,
	linux-remoteproc@vger.kernel.org,
	Linux Kernel <linux-kernel@vger.kernel.org>,
	linux-kernel-owner@vger.kernel.org
Subject: Re: [PATCH] remoteproc: qcom_q6v5: don't auto boot remote processor
Date: Sat, 19 Jan 2019 09:47:39 +0530
Message-ID: <04e98c14b5d4113a466879ee9cc3d6d6@codeaurora.org> (raw)
In-Reply-To: <CA+ASDXPAVfX=EiOKY_t-OoQFuKssbqiK4fAUstOdvO0=69kskg@mail.gmail.com>

Hi Brian,
Thanks for the review

On 2019-01-19 02:34, Brian Norris wrote:
> Hi Sibi,
> 
> On Fri, Jan 18, 2019 at 11:46 AM Sibi Sankar <sibis@codeaurora.org> 
> wrote:
>> On 2019-01-19 00:05, Brian Norris wrote:
>> > On Thu, Jan 17, 2019 at 11:04 PM Sibi Sankar <sibis@codeaurora.org>
>> >> After experimenting with in kernel solutions for
>> >> three revisions and observing problems on graceful
>> >> shutdown usecase,
>> >
>> > What exactly were the problems again? e.g., what were the deficiencies
>> > with having the remoteproc device listen for the REMOTEFS_QMI_SVC_ID
>> > service again? Sorry, but I sort of dropped off on reviewing that
>> > stuff, and now I see this. I'd mildly prefer something that is
>> > actually automatic, but if I'm missing some aspects, I'd like to hear
>> > that. (And, I'd like to see them explained in the commit message, if
>> > this is ever to be merged.)
>> 
>> bringing down the modem after the RMTFS server
>> goes down leaves the modem in limbo (It has a few
>> pending rmtfs transactions that cannot go through)
>> which results in sysmon graceful shutdown failing.
> 
> Makes sense. Probably should be described in a re-send of this patch,
> if we're going with that.
> 
>> And we have to do a modem force-stop to proceed
>> which we want to avoid in graceful shutdown cases.
> 
> Shouldn't you do the "force-stop" in the kernel too? e.g., if rmtfs
> daemon dies without doing a properly-timed stop, then we should still
> force a stop in the kernel, no? Basically, why not do both mechanisms:
> REMOTEFS_QMI_SVC-activated start/stop in the kernel, and manual stop
> (and start? this likely might still be redundant) in the daemon?

yeah we already have that implemented in the kernel
i.e first we try graceful shutdown followed by
force-stop which will just timeout if graceful
shutdown was already completed. We would just call
stop from rmtfs without worrying about the underlying
details.

> 
>> This is overcome by starting rproc mss from rmtfs
>> after REMOTEFS_QMI service is up and stopping
>> rproc mss from rmtfs on SIGKILL/SIGINT and other
>> program error signals before bringing down the
>> RMTFS_QMI service i.e before exiting the rmtfs
>> server loop.
> 
> That's still not really a sure-fire way of doing things. For one, you
> can't catch SIGKILL. Similarly, you can't really clean up from OOM,
> segfault, etc. So it would still be helpful to hook into the QMI
> service notifications in the kernel, I think.

yes we would want a fail safe in the kernel
as well. Sorry I meant SIGTERM instead of
SIGKILL earlier

> 
>> >> switching to controlling the
>> >> remoteproc mss through rmtfs seems to solve all
>> >> the known issues.
>> >
>> > How so? It explicitly does NOT help at all if RMTFS crashes.
>> > Because...who's going to stop the modem in that case? (It works if you
>> > automatically respawn a new RMTFS daemon, to toggle the modem. But
>> > that's kind of cheating, and you can do that anyway, even without this
>> > patch.) On the contrary, your patch *would* resolve that, since the
>> > modem would notice when the RMTFS server goes away, and it would stop
>> > itself.
>> 
>> yeah we would want to mimic what the kernel
>> patch did with the exception of stopping modem
>> before bringing down the rmtfs server (not toggle
>> rproc state but start on rmtfs service up and stop
>> before rmtfs server exit). So in that case we would
>> not want the modem to auto-boot.
> 
> See above. You can't really mimic what the kernel patch was doing
> completely. You can try (which is probably a good idea), but you
> should still have some fallback in the kernel, I expect.

yeah

> 
>> >> https://patchwork.kernel.org/patch/10662395/
>> >>
>> >> we should probably get this merged in, now that
>> >> we are planning to start/stop mss through
>> >> rmtfs.
>> >
>> > Sorry, who's planning to stop mss through rmtfs? Did I miss something?
>> 
>> I have a working patch which I'll soon send
>> upstream for review, after it clears the internal
>> reviews/processes
> 
> OK.
> 
> Brian

-- 
-- Sibi Sankar --
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.

  reply index

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-24 19:21 Ramon Fried
2018-05-29  4:20 ` Bjorn Andersson
2018-05-29 11:31   ` Ramon Fried
2019-01-18  7:04   ` Sibi Sankar
2019-01-18 18:35     ` Brian Norris
2019-01-18 19:46       ` Sibi Sankar
2019-01-18 21:04         ` Brian Norris
2019-01-19  4:17           ` Sibi Sankar [this message]
2019-01-30 21:07 ` Bjorn Andersson

Reply instructions:

You may reply publically 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=04e98c14b5d4113a466879ee9cc3d6d6@codeaurora.org \
    --to=sibis@codeaurora.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=briannorris@chromium.org \
    --cc=linux-kernel-owner@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-remoteproc@vger.kernel.org \
    --cc=ramon.fried@gmail.com \
    /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

	# 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 linux-kernel@archiver.kernel.org
	public-inbox-index lkml


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