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

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

[Bjorn Andersson]

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.

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/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.txt b/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.txt
index 8562ba1dce69..95b209e7f5d1 100644
--- a/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.txt
+++ b/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.txt
@@ -32,6 +32,13 @@ access block device data using the Remote Filesystem protocol.
 	Value type: <u32>
 	Definition: vmid of the remote processor, to set up memory protection.
 
+- rproc:
+	Usage: optional
+	Value type: <phandle>
+	Definition: reference to a remoteproc node, that should be powered up
+		    while the remote file system memory instance is ready to
+		    handle requests from the remote subsystem.
+
 = EXAMPLE
 The following example shows the remote filesystem memory setup for APQ8016,
 with the rmtfs region for the Hexagon DSP (id #1) located at 0x86700000.
diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index d7a4b9eca5d2..1445a38e8b34 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -1142,6 +1142,7 @@ static int q6v5_probe(struct platform_device *pdev)
 	qproc = (struct q6v5 *)rproc->priv;
 	qproc->dev = &pdev->dev;
 	qproc->rproc = rproc;
+	rproc->auto_boot = false;
 	platform_set_drvdata(pdev, qproc);
 
 	ret = q6v5_init_mem(qproc, pdev);
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 01fb6aba61d2..1109272479b9 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -88,6 +88,7 @@ config QCOM_QMI_HELPERS
 config QCOM_RMTFS_MEM
 	tristate "Qualcomm Remote Filesystem memory driver"
 	depends on ARCH_QCOM
+	depends on REMOTEPROC
 	select QCOM_SCM
 	help
 	  The Qualcomm remote filesystem memory driver is used for allocating
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);
+		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;
-- 
2.18.0

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

[Brian Norris]

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

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

[Sibi Sankar]

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.

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

[Bjorn Andersson]

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;

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

[Brian Norris]

Hi Bjorn,

Sorry for getting back to this late.

On Tue, Oct 02, 2018 at 12:34:45PM -0700, Bjorn Andersson wrote:
> On Tue 25 Sep 10:29 PDT 2018, Brian Norris wrote:
> 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.

It could, if you had rmtfs always power cycle the modem on startup. But
we'd be in a bad situation in the meantime, so that might not be great.

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

I believe Sibi Sankar already sent a v2 with the rmtfs driver doing this
instead. He also replied to this email with slightly different
suggestions; I'll try to reply to both of those.

Thanks,
Brian

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

[Brian Norris]

Hi Sibi,

On Sun, Sep 30, 2018 at 08:58:49PM +0530, Sibi Sankar wrote:
> On 2018-09-25 22:59, Brian Norris wrote:
> > On Tue, Sep 25, 2018 at 01:06:07AM -0700, Bjorn Andersson wrote:
> > 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

How exactly is the firmware configured for this? Isn't this something
that's just determined at (firmware) build time? I seem to recall seeing
early firmware releases that booted OK without rmtfs (and therefore
probably had an internal FS?) and later ones that didn't.

Anyway, this sounds even more like an argument for:
(1) not describing this in the device tree; and
(2) as a result of (1), probably not handled in the kernel either

The reasoning for (1): this is purely a software configuration, and does
not really describe the hardware. There's nothing about the hardware
that says the modem won't have an internal FS; so you have to choose
whether to add a rmtfs-to-rproc phandle based on which firmware you're
building in.

On second thought, that might even be an argument for killing the entire
rmtfs_mem node in DT actually -- and as a matter of fact, I think you
have the same "configuration" (i.e., DT isn't just describing hardware)
problem with the rmtfs_mem node. AIUI, you may have to choose a
different memory region size based on the number of firmware features
a particular product supports. That's typically a Device Tree no-no.

But I don't know how far down that rabbit hole we should go...

> 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.

OK, sure. But as per the above, the whole software-configuration-in-DT
thing is undesirable as well.

Anyway, I'll take a look at your v2.

> 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.

I'm not so sure. Well, sure, it's probably quicker. But this is a known
bug in Android that you've clearly added Android workarounds for. Fixing
it in later firmware doesn't preclude you also continuing to use your
hack on Android.

And can you explain: why can't this fix be applied on all new firmware?
Why does it have to be a "one-off" for dragon boards?

Brian