Re: [RFC PATCH] soc: qcom: rmtfs_mem: Control remoteproc from rmtfs_mem
From: Sibi Sankar
Date: Sun Sep 30 2018 - 11:28:56 EST
On 2018-09-25 22:59, 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.
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...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?
Hi Brian,
It might be more logical to make that "somebody" the rmtfs_mem driver
itself, since
the modem as such does not have any direct functional dependency on
rmtfs_mem i.e
the firmware can be configured to run on rmtfs_mem or internal fs. So in
such cases
where the modem is running on internal fs, it would be undesirable to
have a hard
coded dependency for rmtfs_mem in remoteproc modem itself.
Wouldn't it be simpler/quicker to fix this in kernel than churning out
new firmware
releases. A fix in firmware will also mean that this becomes one-off fix
for dragon
boards diverging the firmware branch from whats used in android for
8916/8974/8996.
Signed-off-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
---
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;
--
-- Sibi Sankar --
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.