Message-ID: <c5cde3e5a6b129197e723e88b5df9ede@codeaurora.org>
Date:   Wed, 31 Oct 2018 20:02:24 +0530
From:   Sibi Sankar <sibis@...eaurora.org>
To:     Brian Norris <briannorris@...omium.org>
Cc:     bjorn.andersson@...aro.org, david.brown@...aro.org,
        robh+dt@...nel.org, mark.rutland@....com, andy.gross@...aro.org,
        akdwived@...eaurora.org, clew@...eaurora.org,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-soc@...r.kernel.org, linux-arm-msm-owner@...r.kernel.org,
        linux-kernel-owner@...r.kernel.org
Subject: Re: [RFC PATCH v2] soc: qcom: rmtfs_mem: Control remoteproc from
 rmtfs_mem

Hi Brian,
Thanks for the review!

On 2018-10-18 06:24, Brian Norris wrote:
> Hi Sibi,
> 
> On Sun, Sep 30, 2018 at 09:26:46PM +0530, Sibi Sankar wrote:
>> From: Bjorn Andersson <bjorn.andersson@...aro.org>
>> 
>> 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 therefore 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@...aro.org>
>> [sibis: Added qmi lookup for Remote file system service]
>> Signed-off-by: Sibi Sankar <sibis@...eaurora.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.
>> 
>> v2:
>>   Remove rproc_boot/shutdown from rmtfs_mem open/release and add
>>   qmi lookup for Remote file system service to address Brian's
>>   race concerns.
>> 
>>  .../reserved-memory/qcom,rmtfs-mem.txt        |  7 ++
>>  drivers/remoteproc/qcom_q6v5_pil.c            |  1 +
>>  drivers/soc/qcom/Kconfig                      |  2 +
>>  drivers/soc/qcom/rmtfs_mem.c                  | 65 
>> ++++++++++++++++++-
>>  4 files changed, 72 insertions(+), 3 deletions(-)
>> 
>> 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.
>> +
> 
> I'll repeat my comment here: this is straying far into the territory of
> putting software configuration in the device tree. Per your own
> comments, the modem firmware can be configured to run with or without a
> remote FS, and now you're assuming that the device tree will include
> this property or not, based on how you configured said firmware. That's
> not how device tree is supposed to work.
> 

Yes makes sense, will remove all dt dependencies in the next re-spin

>>  = 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;
> 
> So how is it supposed to work when you have an internal filesystem for
> the modem? User space just knows about this, and manually starts the
> remoteproc?
> 

I somehow missed this

Since the default firmware configuration for 8916/8996/845 has
rmtfs dependency I plan on adding the qmi lookup by default till
we get a platform that needs rmtfs disabled by default for which
I could easily add a flag for rmtfs dependency in
rproc_hexagon_res in qcom_q6v5_mss driver and do qmi lookup only
if rmtfs is supported.

>>  	platform_set_drvdata(pdev, qproc);
>> 
>>  	ret = q6v5_init_mem(qproc, pdev);
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index 8a7b8dea6990..4e3345944325 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -86,7 +86,9 @@ config QCOM_QMI_HELPERS
>>  config QCOM_RMTFS_MEM
>>  	tristate "Qualcomm Remote Filesystem memory driver"
>>  	depends on ARCH_QCOM
>> +	depends on REMOTEPROC
>>  	select QCOM_SCM
>> +	select QCOM_QMI_HELPERS
>>  	help
>>  	  The Qualcomm remote filesystem memory driver is used for 
>> allocating
>>  	  and exposing regions of shared memory with remote processors for 
>> the
>> diff --git a/drivers/soc/qcom/rmtfs_mem.c 
>> b/drivers/soc/qcom/rmtfs_mem.c
>> index 97bb5989aa21..757e30083f67 100644
>> --- a/drivers/soc/qcom/rmtfs_mem.c
>> +++ b/drivers/soc/qcom/rmtfs_mem.c
>> @@ -18,11 +18,13 @@
>>  #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>
>>  #include <linux/io.h>
>>  #include <linux/qcom_scm.h>
>> +#include <linux/soc/qcom/qmi.h>
>> 
>>  #define QCOM_RMTFS_MEM_DEV_MAX	(MINORMASK + 1)
>> 
>> @@ -31,6 +33,7 @@ static dev_t qcom_rmtfs_mem_major;
>>  struct qcom_rmtfs_mem {
>>  	struct device dev;
>>  	struct cdev cdev;
>> +	struct qmi_handle rmtfs_hdl;
>> 
>>  	void *base;
>>  	phys_addr_t addr;
>> @@ -39,6 +42,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,
>> @@ -141,6 +146,36 @@ static const struct file_operations 
>> qcom_rmtfs_mem_fops = {
>>  	.llseek = default_llseek,
>>  };
>> 
>> +static int rmtfs_new_server(struct qmi_handle *qmi,
>> +				 struct qmi_service *service)
>> +{
>> +	int ret = 0;
>> +	struct qcom_rmtfs_mem *rmtfs_mem = container_of(qmi,
>> +							struct qcom_rmtfs_mem,
>> +							rmtfs_hdl);
>> +
>> +	if (rmtfs_mem->rproc)
> 
> Couldn't you avoid registering these callbacks entirely, if there's no
> rproc device/phandle?
> 

will remove all dt dependencies in the next re-spin

>> +		ret = rproc_boot(rmtfs_mem->rproc);
>> +
>> +	return ret;
>> +};
>> +
>> +static void rmtfs_del_server(struct qmi_handle *qmi,
>> +				  struct qmi_service *service)
>> +{
>> +	struct qcom_rmtfs_mem *rmtfs_mem = container_of(qmi,
>> +							struct qcom_rmtfs_mem,
>> +							rmtfs_hdl);
>> +
>> +	if (rmtfs_mem->rproc)
>> +		rproc_shutdown(rmtfs_mem->rproc);
>> +};
>> +
>> +static struct qmi_ops rmtfs_lookup_ops = {
>> +	.new_server = rmtfs_new_server,
>> +	.del_server = rmtfs_del_server,
>> +};
>> +
>>  static void qcom_rmtfs_mem_release_device(struct device *dev)
>>  {
>>  	struct qcom_rmtfs_mem *rmtfs_mem = container_of(dev,
>> @@ -156,6 +191,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 +217,22 @@ 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;
>> +	}
>> +
>> +	ret = qmi_handle_init(&rmtfs_mem->rmtfs_hdl, 0,
>> +			      &rmtfs_lookup_ops, NULL);
> 
> Similar to the above comment: this should just be under the "if rproc"
> condition -- also because in remove(), you only unregister these
> callbacks if you have an rproc device.
> 

I'll be moving qmi_lookup logic to qcom_q6v5_mss driver will fix
it there

>> +	if (ret < 0)
>> +		goto put_rproc;
> 
> You've got the error handling wrong here. You're doing the
> rmtfs_mem->dev cleanup under the 'put_rproc' label, but you haven't 
> even
> started to initialize that device by now.
> 
>> +
>> +	ret = qmi_add_lookup(&rmtfs_mem->rmtfs_hdl, 14, 0, 0);
> 
> I can see there are some bad examples out there already to cheat off
> of...but please don't just use magic nubmers like '14' here. There
> should be a defined constant for this.
> 

Yes, I'll make sure I add comments and the corresponding define

> And while we're at it: why isn't there a common header for QMI service
> IDs? Would be nice to list all the IDs that the kernel might be using,
> in one place.

I can probably take this up as a separate task if its something
Bjorn wants cleaned up?

> 
>> +	if (ret < 0)
>> +		goto err_release_qmi_handle;
>> +
>>  	device_initialize(&rmtfs_mem->dev);
>>  	rmtfs_mem->dev.parent = &pdev->dev;
>>  	rmtfs_mem->dev.groups = qcom_rmtfs_mem_groups;
>> @@ -191,7 +243,7 @@ static int qcom_rmtfs_mem_probe(struct 
>> platform_device *pdev)
>>  	if (IS_ERR(rmtfs_mem->base)) {
>>  		dev_err(&pdev->dev, "failed to remap rmtfs_mem region\n");
>>  		ret = PTR_ERR(rmtfs_mem->base);
>> -		goto put_device;
>> +		goto err_release_qmi_handle;
>>  	}
>> 
>>  	cdev_init(&rmtfs_mem->cdev, &qcom_rmtfs_mem_fops);
>> @@ -204,7 +256,7 @@ static int qcom_rmtfs_mem_probe(struct 
>> platform_device *pdev)
>>  	ret = cdev_device_add(&rmtfs_mem->cdev, &rmtfs_mem->dev);
>>  	if (ret) {
>>  		dev_err(&pdev->dev, "failed to add cdev: %d\n", ret);
>> -		goto put_device;
>> +		goto err_release_qmi_handle;
>>  	}
>> 
>>  	ret = of_property_read_u32(node, "qcom,vmid", &vmid);
>> @@ -237,7 +289,10 @@ static int qcom_rmtfs_mem_probe(struct 
>> platform_device *pdev)
>> 
>>  remove_cdev:
>>  	cdev_device_del(&rmtfs_mem->cdev, &rmtfs_mem->dev);
>> -put_device:
>> +err_release_qmi_handle:
>> +	qmi_handle_release(&rmtfs_mem->rmtfs_hdl);
>> +put_rproc:
>> +	rproc_put(rmtfs_mem->rproc);
>>  	put_device(&rmtfs_mem->dev);
> 
> As mentioned above, this is in the wrong order. You probably will need
> an additional exit label too.
> 

yes missed that but will move the qmi lookup logic to qcom_q6v5_mss
driver. will fix it there

>> 
>>  	return ret;
>> @@ -257,6 +312,10 @@ static int qcom_rmtfs_mem_remove(struct 
>> platform_device *pdev)
>>  	}
>> 
>>  	cdev_device_del(&rmtfs_mem->cdev, &rmtfs_mem->dev);
>> +	if (rmtfs_mem->rproc) {
>> +		qmi_handle_release(&rmtfs_mem->rmtfs_hdl);
> 
> As noted above, this doesn't match with probe().
> 
> Brian
> 
>> +		rproc_put(rmtfs_mem->rproc);
>> +	}
>>  	put_device(&rmtfs_mem->dev);
>> 
>>  	return 0;

-- 
-- Sibi Sankar --
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.

Powered by blists - more mailing lists