Messages in this thread | | | Date | Sun, 30 Sep 2018 20:58:49 +0530 | From | Sibi Sankar <> | Subject | Re: [RFC PATCH] soc: qcom: rmtfs_mem: Control remoteproc from rmtfs_mem |
| |
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@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;
-- -- Sibi Sankar -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
| |