| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <37621d4fadff8e3a0bbe034119351d16@codeaurora.org>
Date: Sun, 30 Sep 2018 20:58:49 +0530
From: Sibi Sankar <sibis@...eaurora.org>
To: Brian Norris <briannorris@...omium.org>,
Bjorn Andersson <bjorn.andersson@...aro.org>
Cc: Rob Herring <robh+dt@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Andy Gross <andy.gross@...aro.org>,
David Brown <david.brown@...aro.org>,
Avaneesh Kumar Dwivedi <akdwived@...eaurora.org>,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-msm@...r.kernel.org, linux-soc@...r.kernel.org,
linux-arm-msm-owner@...r.kernel.org
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@...aro.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.
Powered by blists - more mailing lists