Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 31 Mar 2020 10:55:47 -0700
From:   Bjorn Andersson <bjorn.andersson@linaro.org>
To:     Mathieu Poirier <mathieu.poirier@linaro.org>
Cc:     Rishabh Bhatnagar <rishabhb@codeaurora.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-remoteproc <linux-remoteproc@vger.kernel.org>,
        Ohad Ben-Cohen <ohad@wizery.com>, psodagud@codeaurora.org,
        tsoni@codeaurora.org, Siddharth Gupta <sidgup@codeaurora.org>
Subject: Re: [PATCH 1/2] remoteproc: Add userspace char device driver
Message-ID: <20200331175547.GB254911@minitux>
References: <1585241440-7572-1-git-send-email-rishabhb@codeaurora.org>
 <1585241440-7572-2-git-send-email-rishabhb@codeaurora.org>
 <20200330221245.GA17782@xps15>
 <20200330224554.GD215915@minitux>
 <CANLsYkxEA66kGZh1rToSn79fpgPHqEjMZsSw74Rx3OLwG2k35w@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CANLsYkxEA66kGZh1rToSn79fpgPHqEjMZsSw74Rx3OLwG2k35w@mail.gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue 31 Mar 09:47 PDT 2020, Mathieu Poirier wrote:

> On Mon, 30 Mar 2020 at 16:45, Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> >
> > On Mon 30 Mar 15:12 PDT 2020, Mathieu Poirier wrote:
> > [..]
> > > > +   struct rproc *rproc;
> > > > +
> > > > +   rproc = container_of(inode->i_cdev, struct rproc, char_dev);
> > > > +   if (!rproc)
> > > > +           return -EINVAL;
> > > > +
> > > > +   rproc_shutdown(rproc);
> > >
> > > The scenario I see here is that a userspace program will call
> > > open(/dev/rproc_xyz, SOME_OPTION) when it is launched.  The file stays open
> > > until either the application shuts down, in which case it calls close() or it
> > > crashes.  In that case the system will automatically close all file descriptors
> > > that were open by the application, which will also call rproc_shutdown().
> > >
> > > To me the same functionality can be achieved with the current functionality
> > > provided by sysfs.
> > >
> > > When the application starts it needs to read
> > > "/sys/class/remoteproc/remoteprocX/state".  If the state is "offline" then
> > > "start" should be written to "/sys/.../state".  If the state is "running" the
> > > application just crashed and got restarted.  In which case it needs to stop the
> > > remote processor and start it again.
> > >
> >
> > A case when this would be useful is the Qualcomm modem, which relies on
> > disk access through a "remote file system service" [1].
> >
> > Today we register the service (a few layers ontop of rpmsg/GLINK) then
> > find the modem remoteproc and write "start" into the state sysfs file.
> >
> > When we get a signal for termination we write "stop" into state to stop
> > the remoteproc before exiting.
> >
> > There is however no way for us to indicate to the modem that rmtfs just
> > died, e.g. a kill -9 on the process will result in the modem continue
> > and the next IO request will fail which in most cases will be fatal.
> 
> The modem will crash when attempting an IO while rmtfs is down?
> 

In certain cases there's nothing else to do.

> >
> > So instead having rmtfs holding /dev/rproc_foo open would upon its
> > termination cause the modem to be stopped automatically, and as the
> > system respawns rmtfs the modem would be started anew and the two sides
> > would be synced up again.
> 
> I have a better idea of what is going on now - thanks for writing this up.
> 
> I would make this feature a kernel configurable option as some people
> may not want it.

Sounds reasonable.

> I also think having "/dev/remoteprocX" is fine, so
> no need to change anything currently visible in sysfs.
> 

I agree, it sure is annoying that remoteproc%d isn't stable, but it's
what we have and consistency is important.

Regards,
Bjorn