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

[Sibi Sankar <sibis@codeaurora.org>]

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.