Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753025AbcD0NYi (ORCPT ); Wed, 27 Apr 2016 09:24:38 -0400 Received: from regular1.263xmail.com ([211.150.99.138]:35586 "EHLO regular1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752820AbcD0NYf (ORCPT ); Wed, 27 Apr 2016 09:24:35 -0400 X-263anti-spam: BIG:0;ABS:1;DNS:0;ATT:0;SPF:S;KSV:0; X-MAIL-GRAY: 0 X-MAIL-DELIVERY: 1 X-ABS-CHECKED: 1 X-SKE-CHECKED: 1 X-ADDR-CHECKED: 0 X-KSVirus-check: 0 X-RL-SENDER: andy.yan@rock-chips.com X-FST-TO: linux-pm@vger.kernel.org X-SENDER-IP: 121.15.173.1 X-LOGIN-NAME: andy.yan@rock-chips.com X-UNIQUE-TAG: <8364919cca10481cead14b7d04744ba3> X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Subject: Re: [PATCH v8 2/4] power: reset: add reboot mode driver To: Krzysztof Kozlowski , robh+dt@kernel.org, heiko@sntech.de, sre@kernel.org References: <1461567051-21734-1-git-send-email-andy.yan@rock-chips.com> <1461567336-21834-1-git-send-email-andy.yan@rock-chips.com> <571DF49F.6000706@samsung.com> <571DFFB0.4010409@rock-chips.com> <571E122B.2070401@samsung.com> Cc: john.stultz@linaro.org, arnd@arndb.de, pawel.moll@arm.com, mark.rutland@arm.com, ijc+devicetree@hellion.org.uk, galak@codeaurora.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, dbaryshkov@gmail.com, dwmw2@infradead.org, alexandre.belloni@free-electrons.com, moritz.fischer@ettus.com, f.fainelli@gmail.com, linux-pm@vger.kernel.org From: Andy Yan Message-ID: <5720BD85.5030302@rock-chips.com> Date: Wed, 27 Apr 2016 21:24:21 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <571E122B.2070401@samsung.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8491 Lines: 235 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 >>>> Reviewed-by: Moritz Fischer >>>> Tested-by: John Stultz >>>> Acked-by: John Stultz >>>> Signed-off-by: Andy Yan >>>> >>>> --- >>>> >>>> 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 >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> +#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... I check the code of register_reboot_notifier, this function only return 0. So Maybe I don't need to check the return value here. > > Best regards, > Krzysztof > > >