| 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: <82de11c77b8b46dcdfec4a2d2569ac95@codeaurora.org>
Date: Thu, 01 Jul 2021 01:01:25 +0530
From: Sibi Sankar <sibis@...eaurora.org>
To: Matthias Kaehlcke <mka@...omium.org>
Cc: bjorn.andersson@...aro.org, robh+dt@...nel.org,
swboyd@...omium.org, ulf.hansson@...aro.org, rjw@...ysocki.net,
agross@...nel.org, ohad@...ery.com, mathieu.poirier@...aro.org,
linux-arm-msm@...r.kernel.org, linux-remoteproc@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
dianders@...omium.org, rishabhb@...eaurora.org,
sidgup@...eaurora.org
Subject: Re: [PATCH v3 04/13] remoteproc: qcom: q6v5: Use qmp_send to update
co-processor load state
Hey Matthias,
Thanks for taking time to review the series.
On 2021-06-26 02:41, Matthias Kaehlcke wrote:
> On Fri, Jun 25, 2021 at 12:21:58AM +0530, Sibi Sankar wrote:
>> The power domains exposed by the AOSS QMP driver control the load
>> state
>> resources linked to modem, adsp, cdsp remoteprocs. These are used to
>> notify the Always on Subsystem (AOSS) that a particular co-processor
>> is
>> up/down. AOSS uses this information to wait for the co-processors to
>> suspend before starting its sleep sequence.
>>
>> These co-processors enter low-power modes independent to that of the
>> application processor and the load state resources linked to them are
>> expected to remain unaltered across system suspend/resume cycles. To
>> achieve this behavior lets stop using the power-domains exposed by the
>> AOSS QMP node and replace them with generic qmp_send interface
>> instead.
>>
>> Signed-off-by: Sibi Sankar <sibis@...eaurora.org>
>> ---
>> drivers/remoteproc/qcom_q6v5.c | 56 +++++++++++++++++++++++++-
>> drivers/remoteproc/qcom_q6v5.h | 7 +++-
>> drivers/remoteproc/qcom_q6v5_adsp.c | 7 +++-
>> drivers/remoteproc/qcom_q6v5_mss.c | 44 ++++----------------
>> drivers/remoteproc/qcom_q6v5_pas.c | 80
>> +++++++++----------------------------
>> drivers/remoteproc/qcom_q6v5_wcss.c | 4 +-
>> 6 files changed, 94 insertions(+), 104 deletions(-)
>>
>> diff --git a/drivers/remoteproc/qcom_q6v5.c
>> b/drivers/remoteproc/qcom_q6v5.c
>> index 9627a950928e..4a9a481c211b 100644
>> --- a/drivers/remoteproc/qcom_q6v5.c
>> +++ b/drivers/remoteproc/qcom_q6v5.c
>> @@ -16,8 +16,28 @@
>> #include "qcom_common.h"
>> #include "qcom_q6v5.h"
>>
>> +#define Q6V5_LOAD_STATE_MSG_LEN 64
>> #define Q6V5_PANIC_DELAY_MS 200
>>
>> +static int q6v5_load_state_toggle(struct qcom_q6v5 *q6v5, bool
>> enable)
>> +{
>> + char buf[Q6V5_LOAD_STATE_MSG_LEN] = {};
>> + int ret;
>> +
>> + if (IS_ERR(q6v5->qmp))
>> + return 0;
>> +
>> + snprintf(buf, sizeof(buf),
>> + "{class: image, res: load_state, name: %s, val: %s}",
>> + q6v5->load_state, enable ? "on" : "off");
>> +
>> + ret = qmp_send(q6v5->qmp, buf, sizeof(buf));
>> + if (ret)
>> + dev_err(q6v5->dev, "failed to toggle load state\n");
>> +
>> + return ret;
>> +}
>> +
>> /**
>> * qcom_q6v5_prepare() - reinitialize the qcom_q6v5 context before
>> start
>> * @q6v5: reference to qcom_q6v5 context to be reinitialized
>> @@ -26,6 +46,12 @@
>> */
>> int qcom_q6v5_prepare(struct qcom_q6v5 *q6v5)
>> {
>> + int ret;
>> +
>> + ret = q6v5_load_state_toggle(q6v5, true);
>> + if (ret)
>> + return ret;
>> +
>> reinit_completion(&q6v5->start_done);
>> reinit_completion(&q6v5->stop_done);
>>
>> @@ -47,6 +73,7 @@ EXPORT_SYMBOL_GPL(qcom_q6v5_prepare);
>> int qcom_q6v5_unprepare(struct qcom_q6v5 *q6v5)
>> {
>> disable_irq(q6v5->handover_irq);
>> + q6v5_load_state_toggle(q6v5, false);
>>
>> return !q6v5->handover_issued;
>> }
>> @@ -196,12 +223,13 @@ EXPORT_SYMBOL_GPL(qcom_q6v5_panic);
>> * @pdev: platform_device reference for acquiring resources
>> * @rproc: associated remoteproc instance
>> * @crash_reason: SMEM id for crash reason string, or 0 if none
>> + * @load_state: load state resource string
>> * @handover: function to be called when proxy resources should be
>> released
>> *
>> * Return: 0 on success, negative errno on failure
>> */
>> int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct platform_device
>> *pdev,
>> - struct rproc *rproc, int crash_reason,
>> + struct rproc *rproc, int crash_reason, const char *load_state,
>> void (*handover)(struct qcom_q6v5 *q6v5))
>> {
>> int ret;
>> @@ -210,6 +238,7 @@ int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct
>> platform_device *pdev,
>> q6v5->dev = &pdev->dev;
>> q6v5->crash_reason = crash_reason;
>> q6v5->handover = handover;
>> + q6v5->load_state = kstrdup_const(load_state, GFP_KERNEL);
>>
>> init_completion(&q6v5->start_done);
>> init_completion(&q6v5->stop_done);
>> @@ -286,9 +315,34 @@ int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct
>> platform_device *pdev,
>> return PTR_ERR(q6v5->state);
>> }
>>
>> + q6v5->qmp = qmp_get(&pdev->dev);
>> + if (IS_ERR(q6v5->qmp)) {
>> + if (PTR_ERR(q6v5->qmp) != -ENODEV) {
>> + if (PTR_ERR(q6v5->qmp) != -EPROBE_DEFER)
>> + dev_err(&pdev->dev, "failed to acquire load state\n");
>> + return PTR_ERR(q6v5->qmp);
>> + }
>> + } else {
>> + if (!q6v5->load_state) {
>> + dev_err(&pdev->dev, "load state resource string empty\n");
>> + return -EINVAL;
>> + }
>> + }
>> +
>> return 0;
>> }
>> EXPORT_SYMBOL_GPL(qcom_q6v5_init);
>>
>> +/**
>> + * qcom_q6v5_deinit() - deinitialize the q6v5 common struct
>> + * @q6v5: reference to qcom_q6v5 context to be deinitialized
>> + * @pdev: platform_device reference for acquiring resources
>> + */
>> +void qcom_q6v5_deinit(struct qcom_q6v5 *q6v5, struct platform_device
>> *pdev)
>> +{
>
> pdev isn't used, remove it?
thanks for catching this. It was an
artefact of the recent qmp api changes.
>
> kfree_const(q6v5->load_state);
>
>> + qmp_put(q6v5->qmp);
>> +}
>> +EXPORT_SYMBOL_GPL(qcom_q6v5_deinit);
>> +
>> MODULE_LICENSE("GPL v2");
>> MODULE_DESCRIPTION("Qualcomm Peripheral Image Loader for Q6V5");
>> diff --git a/drivers/remoteproc/qcom_q6v5.h
>> b/drivers/remoteproc/qcom_q6v5.h
>> index 1c212f670cbc..3d9f525cb4ec 100644
>> --- a/drivers/remoteproc/qcom_q6v5.h
>> +++ b/drivers/remoteproc/qcom_q6v5.h
>> @@ -5,6 +5,7 @@
>>
>> #include <linux/kernel.h>
>> #include <linux/completion.h>
>> +#include <linux/soc/qcom/qcom_aoss.h>
>>
>> struct rproc;
>> struct qcom_smem_state;
>> @@ -15,6 +16,8 @@ struct qcom_q6v5 {
>> struct rproc *rproc;
>>
>> struct qcom_smem_state *state;
>> + struct qmp *qmp;
>> +
>> unsigned stop_bit;
>>
>> int wdog_irq;
>> @@ -32,12 +35,14 @@ struct qcom_q6v5 {
>>
>> bool running;
>>
>> + const char *load_state;
>> void (*handover)(struct qcom_q6v5 *q6v5);
>> };
>>
>> int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct platform_device
>> *pdev,
>> - struct rproc *rproc, int crash_reason,
>> + struct rproc *rproc, int crash_reason, const char *load_state,
>> void (*handover)(struct qcom_q6v5 *q6v5));
>> +void qcom_q6v5_deinit(struct qcom_q6v5 *q6v5, struct platform_device
>> *pdev);
>>
>> int qcom_q6v5_prepare(struct qcom_q6v5 *q6v5);
>> int qcom_q6v5_unprepare(struct qcom_q6v5 *q6v5);
>> diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c
>> b/drivers/remoteproc/qcom_q6v5_adsp.c
>> index 8b0d8bbacd2e..0f5e0fd216b4 100644
>> --- a/drivers/remoteproc/qcom_q6v5_adsp.c
>> +++ b/drivers/remoteproc/qcom_q6v5_adsp.c
>> @@ -185,7 +185,9 @@ static int adsp_start(struct rproc *rproc)
>> int ret;
>> unsigned int val;
>>
>> - qcom_q6v5_prepare(&adsp->q6v5);
>> + ret = qcom_q6v5_prepare(&adsp->q6v5);
>> + if (ret)
>> + return ret;
>>
>> ret = clk_prepare_enable(adsp->xo);
>> if (ret)
>> @@ -465,7 +467,7 @@ static int adsp_probe(struct platform_device
>> *pdev)
>> if (ret)
>> goto disable_pm;
>>
>> - ret = qcom_q6v5_init(&adsp->q6v5, pdev, rproc,
>> desc->crash_reason_smem,
>> + ret = qcom_q6v5_init(&adsp->q6v5, pdev, rproc,
>> desc->crash_reason_smem, NULL,
>> qcom_adsp_pil_handover);
>
> Doesn't passing a load_state of NULL cause qcom_q6v5_init() to fail
> with -EINVAL?
qmp_get is expected to error out with
-ENODEV since we don't mention a qmp
node in dt. When a client needs to use
qmp in the future we would replace NULL
with the proper load_state string value.
>
>> --- a/drivers/remoteproc/qcom_q6v5_wcss.c
>> +++ b/drivers/remoteproc/qcom_q6v5_wcss.c
>> @@ -1044,8 +1044,7 @@ static int q6v5_wcss_probe(struct
>> platform_device *pdev)
>> if (ret)
>> goto free_rproc;
>>
>> - ret = qcom_q6v5_init(&wcss->q6v5, pdev, rproc,
>> desc->crash_reason_smem,
>> - NULL);
>> + ret = qcom_q6v5_init(&wcss->q6v5, pdev, rproc,
>> desc->crash_reason_smem, NULL, NULL);
>
> Same as for adsp_probe(), doesn't a load_state of NULL cause _init() to
> fail?
it won't for the same reason explained
above.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.
Powered by blists - more mailing lists