Re: [RFC PATCH] soc: qcom: rmtfs_mem: Control remoteproc from rmtfs_mem

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Tue 25 Sep 10:29 PDT 2018, Brian Norris wrote:

> Hi Bjorn,
> 
> On Tue, Sep 25, 2018 at 01:06:07AM -0700, Bjorn Andersson wrote:
> > rmtfs_mem provides access to physical storage and is crucial for the
> > operation of the Qualcomm modem subsystem.
> > 
> > The rmtfs_mem implementation must be available before the modem
> > subsystem is booted and a solution where the modem remoteproc will
> > verify that the rmtfs_mem is available has been discussed in the past.
> > But this would not handle the case where the rmtfs_mem provider is
> > restarted, which would cause fatal loss of access to the storage device
> > for the modem.
> > 
> > The suggestion is therefor to link the rmtfs_mem to its associated
> > remote processor instance and control it based on the availability of
> > the rmtfs_mem implementation.
> 
> But what does "availability" mean? If I'm reading your rmtfs daemon
> properly, "availability" should mean that the daemon is up and has
> registered a RMTFS_QMI_SERVICE. But in this patch, you're keying off of
> the open() call, which sounds like you're introducing a race condition
> -- we might have open()ed the RMTFS memory but we're not actually
> completely ready to service requests.
> 

You're right. The modem will fail to load if the RMTFS_QMI_SERVICE is
not present, it doesn't care about this thing (rmtfs) has "opened"
rmtfs_mem.

> So rather than looking for open(), I think somebody needs to be looking
> for the appearance and disappearance of the RMTFS_QMI_SERVICE. (Looking
> for disappearance would resolve the daemon restart issue, no?) That
> "somebody" could be the remoteproc driver I suppose (qmi_add_lookup()?),
> or

Right, thinking about it some more we could make the remoteproc driver
start and stop itself as the RMTFS_QMI_SERVICE get
registered/unregistered.

> ...couldn't it just be the modem itself? Do you actually need to
> restart the entire modem when the RMTFS service goes away, or do you
> just need to pause storage activity?
> 

Unfortunately the protocol isn't stateless; a handle to the partition is
acquired by an "open" call and then read/write operations are performed
on that handle. So unless the modem explicitly reopens the partitions as
the rmtfs service is restarted this won't work - and I haven't observed
this behavior.


For the record; I did consider making the rmtfs implementation the one
driving the remoteproc state through /sys/class/remoteproc, but that
would not cope with abnormal termination of the rmtfs implementation.

I will work up a patch making the remoteproc driver observe the presence
of the RMTFS_QMI_SERVICE and see how that looks.

Thanks for your feedback!

Regards,
Bjorn

> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> > 
> > The currently implemented workaround in the Linaro QCOMLT releases is to
> > blacklist the qcom_q6v5_pil kernel module and load this explicitly after rmtfs
> > has been started.
> > 
> > With this patch the modem module can be loaded automatically by the
> > platform_bus and will only be booted as the rmtfs becomes available. Performing
> > actions such as upgrading (and restarting) the rmtfs service will cause the
> > modem to automatically restart and hence continue to function after the
> > upgrade.
> > 
> >  .../reserved-memory/qcom,rmtfs-mem.txt        |  7 ++++++
> >  drivers/remoteproc/qcom_q6v5_pil.c            |  1 +
> >  drivers/soc/qcom/Kconfig                      |  1 +
> >  drivers/soc/qcom/rmtfs_mem.c                  | 23 ++++++++++++++++++-
> >  4 files changed, 31 insertions(+), 1 deletion(-)
> > 
> ...
> > diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c
> > index 8a3678c2e83c..8b08be310397 100644
> > --- a/drivers/soc/qcom/rmtfs_mem.c
> > +++ b/drivers/soc/qcom/rmtfs_mem.c
> > @@ -18,6 +18,7 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/of.h>
> >  #include <linux/of_reserved_mem.h>
> > +#include <linux/remoteproc.h>
> >  #include <linux/dma-mapping.h>
> >  #include <linux/slab.h>
> >  #include <linux/uaccess.h>
> > @@ -39,6 +40,8 @@ struct qcom_rmtfs_mem {
> >  	unsigned int client_id;
> >  
> >  	unsigned int perms;
> > +
> > +	struct rproc *rproc;
> >  };
> >  
> >  static ssize_t qcom_rmtfs_mem_show(struct device *dev,
> > @@ -80,11 +83,18 @@ static int qcom_rmtfs_mem_open(struct inode *inode, struct file *filp)
> >  	struct qcom_rmtfs_mem *rmtfs_mem = container_of(inode->i_cdev,
> >  							struct qcom_rmtfs_mem,
> >  							cdev);
> > +	int ret = 0;
> >  
> >  	get_device(&rmtfs_mem->dev);
> >  	filp->private_data = rmtfs_mem;
> >  
> > -	return 0;
> > +	if (rmtfs_mem->rproc) {
> > +		ret = rproc_boot(rmtfs_mem->rproc);
> > +		if (ret)
> > +			put_device(&rmtfs_mem->dev);
> > +	}
> > +
> > +	return ret;
> >  }
> >  static ssize_t qcom_rmtfs_mem_read(struct file *filp,
> >  			      char __user *buf, size_t count, loff_t *f_pos)
> > @@ -127,6 +137,9 @@ static int qcom_rmtfs_mem_release(struct inode *inode, struct file *filp)
> >  {
> >  	struct qcom_rmtfs_mem *rmtfs_mem = filp->private_data;
> >  
> > +	if (rmtfs_mem->rproc)
> > +		rproc_shutdown(rmtfs_mem->rproc);
> > +
> >  	put_device(&rmtfs_mem->dev);
> >  
> >  	return 0;
> > @@ -156,6 +169,7 @@ static int qcom_rmtfs_mem_probe(struct platform_device *pdev)
> >  	struct qcom_scm_vmperm perms[2];
> >  	struct reserved_mem *rmem;
> >  	struct qcom_rmtfs_mem *rmtfs_mem;
> > +	phandle rproc_phandle;
> >  	u32 client_id;
> >  	u32 vmid;
> >  	int ret;
> > @@ -181,6 +195,13 @@ static int qcom_rmtfs_mem_probe(struct platform_device *pdev)
> >  	rmtfs_mem->client_id = client_id;
> >  	rmtfs_mem->size = rmem->size;
> >  
> > +	ret = of_property_read_u32(node, "rproc", &rproc_phandle);
> > +	if (!ret) {
> > +		rmtfs_mem->rproc = rproc_get_by_phandle(rproc_phandle);
> 
> You're doing an rproc_get(), so you need to do a rproc_put() in
> remove().
> 
> Brian
> 
> > +		if (!rmtfs_mem->rproc)
> > +			return -EPROBE_DEFER;
> > +	}
> > +
> >  	device_initialize(&rmtfs_mem->dev);
> >  	rmtfs_mem->dev.parent = &pdev->dev;
> >  	rmtfs_mem->dev.groups = qcom_rmtfs_mem_groups;