Re: [PATCH v8 2/4] power: reset: add reboot mode driver
From: Andy Yan
Date: Wed Apr 27 2016 - 08:57:32 EST
Hi Krzysztof:
On 2016å04æ25æ 20:48, Krzysztof Kozlowski wrote:
On 04/25/2016 01:29 PM, Andy Yan wrote:
Hi Krzysztof:
On 2016å04æ25æ 18:42, Krzysztof Kozlowski wrote:
On 04/25/2016 08:55 AM, Andy Yan wrote:
This driver parses the reboot commands like "reboot bootloader"
and "reboot recovery" to get a boot mode described in the
device tree , then call the write interfae to store the boot
mode in some place like special register or sram, which can
be read by the bootloader after system reboot, then the bootloader
can take different action according to the mode stored.
This is commonly used on Android based devices, in order to
reboot the device into fastboot or recovery mode.
Reviewed-by: Matthias Brugger <matthias.bgg@xxxxxxxxx>
Reviewed-by: Moritz Fischer <moritz.fischer@xxxxxxxxx>
Tested-by: John Stultz <john.stultz@xxxxxxxxxx>
Acked-by: John Stultz <john.stultz@xxxxxxxxxx>
Signed-off-by: Andy Yan <andy.yan@xxxxxxxxxxxxxx>
---
Changes in v8:
- do some cleanup when driver ubind
Changes in v7:
- address some suggestions from Krzysztof, make syscon-reboot-mode
driver data self-contained.
Changes in v6: None
Changes in v5:
- use two blank space under help in Kconfig
- use unsigned int instead of int for member magic in struct mode_info
Changes in v4:
- make this driver depends on OF to avoid kbuild test error
Changes in v3:
- scan multi properities
- add mask value for some platform which only use some bits of the
register
to store boot mode magic value
Changes in v2:
- move to dir drivers/power/reset/
- make syscon-reboot-mode a generic driver
Changes in v1:
- fix the embarrassed compile warning
- correct the maskrom magic number
- check for the normal reboot
drivers/power/reset/Kconfig | 13 ++++
drivers/power/reset/Makefile | 2 +
drivers/power/reset/reboot-mode.c | 118
+++++++++++++++++++++++++++++++
drivers/power/reset/reboot-mode.h | 14 ++++
drivers/power/reset/syscon-reboot-mode.c | 99
++++++++++++++++++++++++++
5 files changed, 246 insertions(+)
create mode 100644 drivers/power/reset/reboot-mode.c
create mode 100644 drivers/power/reset/reboot-mode.h
create mode 100644 drivers/power/reset/syscon-reboot-mode.c
diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
index 1131cf7..cf50630 100644
--- a/drivers/power/reset/Kconfig
+++ b/drivers/power/reset/Kconfig
@@ -173,5 +173,18 @@ config POWER_RESET_ZX
help
Reboot support for ZTE SoCs.
+config REBOOT_MODE
+ tristate
+
+config SYSCON_REBOOT_MODE
+ bool "Generic SYSCON regmap reboot mode driver"
+ depends on OF
+ select REBOOT_MODE
+ help
+ Say y here will enable reboot mode driver. This will
+ get reboot mode arguments and store it in SYSCON mapped
+ register, then the bootloader can read it to take different
+ action according to the mode.
+
endif
diff --git a/drivers/power/reset/Makefile
b/drivers/power/reset/Makefile
index 096fa67..a63865b 100644
--- a/drivers/power/reset/Makefile
+++ b/drivers/power/reset/Makefile
@@ -20,3 +20,5 @@ obj-$(CONFIG_POWER_RESET_SYSCON) += syscon-reboot.o
obj-$(CONFIG_POWER_RESET_SYSCON_POWEROFF) += syscon-poweroff.o
obj-$(CONFIG_POWER_RESET_RMOBILE) += rmobile-reset.o
obj-$(CONFIG_POWER_RESET_ZX) += zx-reboot.o
+obj-$(CONFIG_REBOOT_MODE) += reboot-mode.o
+obj-$(CONFIG_SYSCON_REBOOT_MODE) += syscon-reboot-mode.o
diff --git a/drivers/power/reset/reboot-mode.c
b/drivers/power/reset/reboot-mode.c
new file mode 100644
index 0000000..cdc4d72
--- /dev/null
+++ b/drivers/power/reset/reboot-mode.c
@@ -0,0 +1,118 @@
+/*
+ * Copyright (c) 2016, Fuzhou Rockchip Electronics Co., Ltd
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/reboot.h>
+#include "reboot-mode.h"
+
+#define PREFIX "mode-"
+
+struct mode_info {
+ const char *mode;
+ unsigned int magic;
+ struct list_head list;
+};
+
+static int get_reboot_mode_magic(struct reboot_mode_driver *reboot,
+ const char *cmd)
+{
+ const char *normal = "normal";
+ int magic = 0;
+ struct mode_info *info;
+
+ if (!cmd)
+ cmd = normal;
+
+ list_for_each_entry(info, &reboot->head, list) {
+ if (!strcmp(info->mode, cmd)) {
+ magic = info->magic;
+ break;
+ }
+ }
+
+ return magic;
+}
+
+static int reboot_mode_notify(struct notifier_block *this,
+ unsigned long mode, void *cmd)
+{
+ struct reboot_mode_driver *reboot;
+ int magic;
+
+ reboot = container_of(this, struct reboot_mode_driver,
reboot_notifier);
+ magic = get_reboot_mode_magic(reboot, cmd);
+ if (magic)
+ reboot->write(reboot, magic);
+
+ return NOTIFY_DONE;
+}
+
+int reboot_mode_register(struct reboot_mode_driver *reboot)
+{
+ struct mode_info *info;
+ struct property *prop;
+ struct device_node *np = reboot->dev->of_node;
+ size_t len = strlen(PREFIX);
+ int ret;
+
+ INIT_LIST_HEAD(&reboot->head);
+
+ for_each_property_of_node(np, prop) {
+ if (len > strlen(prop->name) || strncmp(prop->name, PREFIX,
len))
+ continue;
+
+ info = devm_kzalloc(reboot->dev, sizeof(*info), GFP_KERNEL);
+ if (!info) {
+ ret = -ENOMEM;
+ goto error;
+ }
+
+ info->mode = kstrdup_const(prop->name + len, GFP_KERNEL);
+ if (of_property_read_u32(np, prop->name, &info->magic)) {
+ dev_err(reboot->dev, "reboot mode %s without magic
number\n",
+ info->mode);
+ devm_kfree(reboot->dev, info);
kfree_const(). The duplicated string won't be added to the list so it
won't be freed in error path or in unregister().
Each mode has a mode_info, if one of them gets an error whith
devm_kzalloc, they other mode_info add to the list before should be freed.
Please, write it again. I don't understand.
You are allocating 'info'. Then you are const-allocating 'info->mode'.
In this error path you are freeing 'info'. Where is the kfree_const()
for 'info->mode'?
Sorry, I got it, it's my fault.Thanks for your review.
+ continue;
+ }
+ list_add_tail(&info->list, &reboot->head);
+ }
+
+ reboot->reboot_notifier.notifier_call = reboot_mode_notify;
+ ret = register_reboot_notifier(&reboot->reboot_notifier);
+ if (ret)
+ dev_err(reboot->dev, "can't register reboot notifier\n");
I don't understand your error paths. In previous patches they were
buggy... they still look buggy, I think. It's 8th iteration and such
basic things are still present.
If this is an error then shouldn't everything be cleaned up? You are
returning error code thus the reboot_mode_register() caller will fail.
This means the probe will fail. So you need to clean up - go to error?
If this is an error, I think only the duplicated string should be
clean up,because other resources allocated by devm. If there is
something else, please let me know.
Just goto error...
Best regards,
Krzysztof