lkml.org 
[lkml]   [2023]   [Mar]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v2 3/4] firmware: qcom_scm: Refactor code to support multiple download mode
From
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.

>
>> +};
>> +
>> +module_param_cb(download_mode, &dload_mode_param_ops,
>> &dload_mode_param, 0644);
>> +MODULE_PARM_DESC(download_mode,
>> +         "Download mode: off/full or 0/1/off/on for existing users");
>
> Nit: on is not supported even for existing users.

Agree. just 0/1 would do fine there.

-Mukesh
>
>> +
>>   static int qcom_scm_probe(struct platform_device *pdev)
>>   {
>>       struct qcom_scm *scm;
>> @@ -1418,12 +1465,12 @@ static int qcom_scm_probe(struct
>> platform_device *pdev)
>>       __get_convention();
>>       /*
>> -     * If requested enable "download mode", from this point on warmboot
>> +     * If download mode is requested, from this point on warmboot
>>        * will cause the boot stages to enter download mode, unless
>>        * disabled below by a clean shutdown/reboot.
>>        */
>> -    if (download_mode)
>> -        qcom_scm_set_download_mode(true);
>> +    if (dload_mode)
>> +        qcom_scm_set_download_mode(dload_mode);
>>       return 0;
>>   }
>> @@ -1431,7 +1478,7 @@ static int qcom_scm_probe(struct platform_device
>> *pdev)
>>   static void qcom_scm_shutdown(struct platform_device *pdev)
>>   {
>>       /* Clean shutdown, disable download mode to allow normal restart */
>> -    qcom_scm_set_download_mode(false);
>> +    qcom_scm_set_download_mode(QCOM_DOWNLOAD_NODUMP);
>>   }
>>   static const struct of_device_id qcom_scm_dt_match[] = {
>

\
 
 \ /
  Last update: 2023-03-27 00:42    [W:0.122 / U:0.192 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site