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'? >> >>> + 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html