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?
> 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;
