Message-ID: <a0181586-817c-e093-990f-5f95c10aadbd@linaro.org>
Date:   Thu, 16 Mar 2023 18:27:33 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH v2 3/4] firmware: qcom_scm: Refactor code to support
 multiple download mode
Content-Language: en-GB
To:     Mukesh Ojha <quic_mojha@quicinc.com>, agross@kernel.org,
        andersson@kernel.org, konrad.dybcio@linaro.org
Cc:     linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org
References: <1677664555-30191-1-git-send-email-quic_mojha@quicinc.com>
 <1677664555-30191-4-git-send-email-quic_mojha@quicinc.com>
 <469b4a3a-ea40-ad6b-2848-210325b8914c@linaro.org>
 <fe07806e-e77b-1198-1ffb-be8e2df53dd6@quicinc.com>
From:   Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
In-Reply-To: <fe07806e-e77b-1198-1ffb-be8e2df53dd6@quicinc.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 03/03/2023 09:46, Mukesh Ojha wrote:
> Thanks for your time in checking this..
> 
> On 3/1/2023 4:29 PM, Dmitry Baryshkov wrote:
>> On 01/03/2023 11:55, Mukesh Ojha wrote:
>>> Currently on Qualcomm SoC, download_mode is enabled if
>>> CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT is selected.
>>>
>>> Refactor the code such that it supports multiple download
>>> modes and drop CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT config
>>> instead, give interface to set the download mode from
>>> module parameter.
>>>
>>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>>> ---
>>> Changes in v2:
>>>   - Passed download mode as parameter
>>>   - Accept human accepatable download mode string.
>>>   - enable = !!dload_mode
>>>   - Shifted module param callback to somewhere down in
>>>     the file so that it no longer need to know the
>>>     prototype of qcom_scm_set_download_mode()
>>>   - updated commit text.
>>>
>>>   drivers/firmware/Kconfig    | 11 --------
>>>   drivers/firmware/qcom_scm.c | 65 
>>> ++++++++++++++++++++++++++++++++++++++-------
>>>   2 files changed, 56 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
>>> index b59e304..ff7e9f3 100644
>>> --- a/drivers/firmware/Kconfig
>>> +++ b/drivers/firmware/Kconfig
>>> @@ -215,17 +215,6 @@ config MTK_ADSP_IPC
>>>   config QCOM_SCM
>>>       tristate
>>> -config QCOM_SCM_DOWNLOAD_MODE_DEFAULT
>>> -    bool "Qualcomm download mode enabled by default"
>>> -    depends on QCOM_SCM
>>> -    help
>>> -      A device with "download mode" enabled will upon an unexpected
>>> -      warm-restart enter a special debug mode that allows the user to
>>> -      "download" memory content over USB for offline postmortem 
>>> analysis.
>>> -      The feature can be enabled/disabled on the kernel command line.
>>> -
>>> -      Say Y here to enable "download mode" by default.
>>> -
>>>   config SYSFB
>>>       bool
>>>       select BOOT_VESA_SUPPORT
>>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
>>> index c9f1fad..ca07208 100644
>>> --- a/drivers/firmware/qcom_scm.c
>>> +++ b/drivers/firmware/qcom_scm.c
>>> @@ -17,17 +17,22 @@
>>>   #include <linux/clk.h>
>>>   #include <linux/reset-controller.h>
>>>   #include <linux/arm-smccc.h>
>>> +#include <linux/kstrtox.h>
>>>   #include "qcom_scm.h"
>>> -static bool download_mode = 
>>> IS_ENABLED(CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT);
>>> -module_param(download_mode, bool, 0);
>>> -
>>>   #define SCM_HAS_CORE_CLK    BIT(0)
>>>   #define SCM_HAS_IFACE_CLK    BIT(1)
>>>   #define SCM_HAS_BUS_CLK        BIT(2)
>>>   #define QCOM_DOWNLOAD_MODE_MASK 0x30
>>> +#define QCOM_DOWNLOAD_FULLDUMP    0x10
>>> +#define QCOM_DOWNLOAD_NODUMP    0x0
>>> +
>>> +#define MAX_DLOAD_LEN    8
>>> +
>>> +static char download_mode[] = "off";
>>> +static u32 dload_mode;
>>>   struct qcom_scm {
>>>       struct device *dev;
>>> @@ -417,8 +422,9 @@ static int __qcom_scm_set_dload_mode(struct 
>>> device *dev, bool enable)
>>>       return qcom_scm_call_atomic(__scm->dev, &desc, NULL);
>>>   }
>>> -static void qcom_scm_set_download_mode(bool enable)
>>> +static void qcom_scm_set_download_mode(u32 dload_mode)
>>>   {
>>> +    bool enable = !!dload_mode;
>>>       bool avail;
>>>       int ret = 0;
>>>       u32 val;
>>> @@ -438,7 +444,7 @@ static void qcom_scm_set_download_mode(bool enable)
>>>           val &= ~QCOM_DOWNLOAD_MODE_MASK;
>>>           if (enable)
>>> -            val |= QCOM_SCM_BOOT_SET_DLOAD_MODE;
>>> +            val |= dload_mode;
>>>           ret = qcom_scm_io_writel(__scm->dload_mode_addr, val);
>>>       } else {
>>> @@ -1338,6 +1344,47 @@ bool qcom_scm_is_available(void)
>>>   }
>>>   EXPORT_SYMBOL(qcom_scm_is_available);
>>> +static int set_dload_mode(const char *val, const struct kernel_param 
>>> *kp)
>>> +{
>>> +    int ret;
>>> +
>>> +    if (!strncmp(val, "full", strlen("full"))) {
>>> +        dload_mode = QCOM_DOWNLOAD_FULLDUMP;
>>> +    } else if (!strncmp(val, "off", strlen("off"))) {
>>> +        dload_mode = QCOM_DOWNLOAD_NODUMP;
>>> +    } else {
>>> +        if (kstrtouint(val, 0, &dload_mode) ||
>>> +             !(dload_mode == 0 || dload_mode == 1)) {
>>> +            pr_err("unknown download mode\n");
>>> +            return -EINVAL;
>>> +        }
>>> +
>>> +    }
>>> +
>>> +    ret = param_set_copystring(val, kp);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    if (__scm)
>>> +        qcom_scm_set_download_mode(dload_mode);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static const struct kernel_param_ops dload_mode_param_ops = {
>>> +    .set = set_dload_mode,
>>> +    .get = param_get_string,
>>
>> Please follow the param_get_bool approach and return sanitized data 
>> here. In other words, "full" / "none" depending on the dload_mode 
>> instead of storing the passed string and returning it later.
>>
> 
> This could be done.
> 
>>> +};
>>> +
>>> +static struct kparam_string dload_mode_param = {
>>> +    .string = download_mode,
>>> +    .maxlen = MAX_DLOAD_LEN,
>>
>> I think writing "full" to this param might overwrite some kernel data. 
>> "00000000" should be even worse.
> 
> There is check in param_set_copystring() which would avoid that.


No. The check will validate the value's length against MAX_DLOAD_LEN. 
But it will not safeguard your code. download_mode's size is less than 
MAX_DLOAD_LEN.

-- 
With best wishes
Dmitry