| Message ID | 1680076012-10785-6-git-send-email-quic_mojha@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Refactor to support multiple download mode | expand |
Wed, Mar 29, 2023 at 01:16:52PM +0530, Mukesh Ojha kirjoitti: > Currently, scm driver only supports full dump when download > mode is selected. Add support to enable minidump as well as > enable it along with fulldump. ... > #define QCOM_DOWNLOAD_MODE_MASK 0x30 > #define QCOM_DOWNLOAD_FULLDUMP 0x1 > +#define QCOM_DOWNLOAD_MINIDUMP 0x2 > +#define QCOM_DOWNLOAD_BOTHDUMP (QCOM_DOWNLOAD_FULLDUMP | QCOM_DOWNLOAD_MINIDUMP) Now order is broken. > #define QCOM_DOWNLOAD_NODUMP 0x0 ... > @@ -1420,13 +1422,16 @@ static irqreturn_t qcom_scm_irq_handler(int irq, void *data) > return IRQ_HANDLED; > } > > - Stray change and ping-pong style at the same time. ... > if (download_mode == QCOM_DOWNLOAD_FULLDUMP) > len = sysfs_emit(buffer, "full\n"); > + else if (download_mode == QCOM_DOWNLOAD_MINIDUMP) > + len = sysfs_emit(buffer, "mini\n"); > + else if (download_mode == QCOM_DOWNLOAD_BOTHDUMP) > + len = sysfs_emit(buffer, "full,mini\n"); Why not "both" ? > else if (download_mode == QCOM_DOWNLOAD_NODUMP) > len = sysfs_emit(buffer, "off\n"); With an array (for streq_match_string() call suggested earlier) this become as simple as if (mode >= ARRAY_SIZE(...)) return sysfs_emit("Oh heh!\n"); return sysfs_emit("%s\n", array[mode]); ... > - if (sysfs_streq(val, "full")) { Why changing this line? > + if (sysfs_streq(val, "full,mini") || sysfs_streq(val, "mini,full")) { > + download_mode = QCOM_DOWNLOAD_BOTHDUMP; It's way too hard, esp. taking into account that once user enters wrong order, user can't simply validate this by reading value back. Use "both" and that's it. > + } else if (sysfs_streq(val, "full")) { > download_mode = QCOM_DOWNLOAD_FULLDUMP; > + } else if (sysfs_streq(val, "mini")) { > + download_mode = QCOM_DOWNLOAD_MINIDUMP; ... > module_param_cb(download_mode, &download_mode_param_ops, NULL, 0644); > MODULE_PARM_DESC(download_mode, > - "Download mode: off/full or 0/1 for existing users"); > + "download mode: off/full/mini/full,mini or mini,full and 0/1 for existing users"); You really must be consistent with at least a couple of things: 1) capitalization; 2) indentation.
On Sat, May 27, 2023 at 01:14:50AM +0300, andy.shevchenko@gmail.com wrote: > Wed, Mar 29, 2023 at 01:16:52PM +0530, Mukesh Ojha kirjoitti: > > Currently, scm driver only supports full dump when download > > mode is selected. Add support to enable minidump as well as > > enable it along with fulldump. > > ... > > > #define QCOM_DOWNLOAD_MODE_MASK 0x30 > > #define QCOM_DOWNLOAD_FULLDUMP 0x1 > > +#define QCOM_DOWNLOAD_MINIDUMP 0x2 > > +#define QCOM_DOWNLOAD_BOTHDUMP (QCOM_DOWNLOAD_FULLDUMP | QCOM_DOWNLOAD_MINIDUMP) > > Now order is broken. > > > #define QCOM_DOWNLOAD_NODUMP 0x0 > > ... > > > @@ -1420,13 +1422,16 @@ static irqreturn_t qcom_scm_irq_handler(int irq, void *data) > > return IRQ_HANDLED; > > } > > > > - > > Stray change and ping-pong style at the same time. > > ... > > > if (download_mode == QCOM_DOWNLOAD_FULLDUMP) > > len = sysfs_emit(buffer, "full\n"); > > + else if (download_mode == QCOM_DOWNLOAD_MINIDUMP) > > + len = sysfs_emit(buffer, "mini\n"); > > + else if (download_mode == QCOM_DOWNLOAD_BOTHDUMP) > > > + len = sysfs_emit(buffer, "full,mini\n"); > > Why not "both" ? > "both" isn't very future proof (and I think we've had additional variations in the past already), so I asked for this form. Many thanks for your thorough review, Andy! Regards, Bjorn > > else if (download_mode == QCOM_DOWNLOAD_NODUMP) > > len = sysfs_emit(buffer, "off\n"); > > > With an array (for streq_match_string() call suggested earlier) this become as > simple as > > if (mode >= ARRAY_SIZE(...)) > return sysfs_emit("Oh heh!\n"); > > return sysfs_emit("%s\n", array[mode]); > > ... > > > - if (sysfs_streq(val, "full")) { > > Why changing this line? > > > + if (sysfs_streq(val, "full,mini") || sysfs_streq(val, "mini,full")) { > > + download_mode = QCOM_DOWNLOAD_BOTHDUMP; > > It's way too hard, esp. taking into account that once user enters wrong order, > user can't simply validate this by reading value back. > > Use "both" and that's it. > > > + } else if (sysfs_streq(val, "full")) { > > download_mode = QCOM_DOWNLOAD_FULLDUMP; > > + } else if (sysfs_streq(val, "mini")) { > > + download_mode = QCOM_DOWNLOAD_MINIDUMP; > > ... > > > module_param_cb(download_mode, &download_mode_param_ops, NULL, 0644); > > MODULE_PARM_DESC(download_mode, > > - "Download mode: off/full or 0/1 for existing users"); > > + "download mode: off/full/mini/full,mini or mini,full and 0/1 for existing users"); > > You really must be consistent with at least a couple of things: > 1) capitalization; > 2) indentation. > > -- > With Best Regards, > Andy Shevchenko > >
On Sat, May 27, 2023 at 2:32 AM Bjorn Andersson <andersson@kernel.org> wrote: > On Sat, May 27, 2023 at 01:14:50AM +0300, andy.shevchenko@gmail.com wrote: > > Wed, Mar 29, 2023 at 01:16:52PM +0530, Mukesh Ojha kirjoitti: ... > > > if (download_mode == QCOM_DOWNLOAD_FULLDUMP) > > > len = sysfs_emit(buffer, "full\n"); > > > + else if (download_mode == QCOM_DOWNLOAD_MINIDUMP) > > > + len = sysfs_emit(buffer, "mini\n"); > > > + else if (download_mode == QCOM_DOWNLOAD_BOTHDUMP) > > > > > + len = sysfs_emit(buffer, "full,mini\n"); > > > > Why not "both" ? > "both" isn't very future proof (and I think we've had additional > variations in the past already), so I asked for this form. Okay, so this should be the bit flags and we should parse a list of the values. In that case I may agree with the approach. > > if (mode >= ARRAY_SIZE(...)) > > return sysfs_emit("Oh heh!\n"); > > > > return sysfs_emit("%s\n", array[mode]); ... > > > + if (sysfs_streq(val, "full,mini") || sysfs_streq(val, "mini,full")) { > > > + download_mode = QCOM_DOWNLOAD_BOTHDUMP; > > > > It's way too hard, esp. taking into account that once user enters wrong order, > > user can't simply validate this by reading value back. > > > > Use "both" and that's it. > > > > > + } else if (sysfs_streq(val, "full")) { > > > download_mode = QCOM_DOWNLOAD_FULLDUMP; > > > + } else if (sysfs_streq(val, "mini")) { > > > + download_mode = QCOM_DOWNLOAD_MINIDUMP; As per above.
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c index fa6502a..780e50a 100644 --- a/drivers/firmware/qcom_scm.c +++ b/drivers/firmware/qcom_scm.c @@ -32,6 +32,8 @@ static u32 download_mode; #define QCOM_DOWNLOAD_MODE_MASK 0x30 #define QCOM_DOWNLOAD_FULLDUMP 0x1 +#define QCOM_DOWNLOAD_MINIDUMP 0x2 +#define QCOM_DOWNLOAD_BOTHDUMP (QCOM_DOWNLOAD_FULLDUMP | QCOM_DOWNLOAD_MINIDUMP) #define QCOM_DOWNLOAD_NODUMP 0x0 struct qcom_scm { @@ -1420,13 +1422,16 @@ static irqreturn_t qcom_scm_irq_handler(int irq, void *data) return IRQ_HANDLED; } - static int get_download_mode(char *buffer, const struct kernel_param *kp) { int len = 0; if (download_mode == QCOM_DOWNLOAD_FULLDUMP) len = sysfs_emit(buffer, "full\n"); + else if (download_mode == QCOM_DOWNLOAD_MINIDUMP) + len = sysfs_emit(buffer, "mini\n"); + else if (download_mode == QCOM_DOWNLOAD_BOTHDUMP) + len = sysfs_emit(buffer, "full,mini\n"); else if (download_mode == QCOM_DOWNLOAD_NODUMP) len = sysfs_emit(buffer, "off\n"); @@ -1437,8 +1442,12 @@ static int set_download_mode(const char *val, const struct kernel_param *kp) { u32 old = download_mode; - if (sysfs_streq(val, "full")) { + if (sysfs_streq(val, "full,mini") || sysfs_streq(val, "mini,full")) { + download_mode = QCOM_DOWNLOAD_BOTHDUMP; + } else if (sysfs_streq(val, "full")) { download_mode = QCOM_DOWNLOAD_FULLDUMP; + } else if (sysfs_streq(val, "mini")) { + download_mode = QCOM_DOWNLOAD_MINIDUMP; } else if (sysfs_streq(val, "off")) { download_mode = QCOM_DOWNLOAD_NODUMP; } else if (kstrtouint(val, 0, &download_mode) || @@ -1461,7 +1470,7 @@ static const struct kernel_param_ops download_mode_param_ops = { module_param_cb(download_mode, &download_mode_param_ops, NULL, 0644); MODULE_PARM_DESC(download_mode, - "Download mode: off/full or 0/1 for existing users"); + "download mode: off/full/mini/full,mini or mini,full and 0/1 for existing users"); static int qcom_scm_probe(struct platform_device *pdev) {
Currently, scm driver only supports full dump when download mode is selected. Add support to enable minidump as well as enable it along with fulldump. Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> --- drivers/firmware/qcom_scm.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)