From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754788AbcC1Gew (ORCPT ); Mon, 28 Mar 2016 02:34:52 -0400 Received: from mailout2.w1.samsung.com ([210.118.77.12]:31964 "EHLO mailout2.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754536AbcC1Gep (ORCPT ); Mon, 28 Mar 2016 02:34:45 -0400 X-AuditID: cbfec7f4-f796c6d000001486-af-56f8d080b8b4 Subject: Re: [PATCH v6 2/4] power: reset: add reboot mode driver To: Andy Yan References: <1458646525-491-1-git-send-email-andy.yan@rock-chips.com> <1458646642-591-1-git-send-email-andy.yan@rock-chips.com> <56F39F36.5050702@rock-chips.com> Cc: robh+dt@kernel.org, sre@kernel.org, heiko@sntech.de, john.stultz@linaro.org, arnd@arndb.de, galak@codeaurora.org, ijc+devicetree@hellion.org.uk, catalin.marinas@arm.com, olof@lixom.net, alexandre.belloni@free-electrons.com, dbaryshkov@gmail.com, jun.nie@linaro.org, pawel.moll@arm.com, will.deacon@arm.com, linux-rockchip@lists.infradead.org, matthias.bgg@gmail.com, devicetree@vger.kernel.org, linux-pm@vger.kernel.org, f.fainelli@gmail.com, linux@arm.linux.org.uk, mbrugger@suse.com, linux-arm-kernel@lists.infradead.org, lorenzo.pieralisi@arm.com, moritz.fischer@ettus.com, linux-kernel@vger.kernel.org, wxt@rock-chips.com, dwmw2@infradead.org, mark.rutland@arm.com From: Krzysztof Kozlowski Message-id: <56F8D078.6040503@samsung.com> Date: Mon, 28 Mar 2016 15:34:32 +0900 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: <56F39F36.5050702@rock-chips.com> Content-type: text/plain; charset=utf-8 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA02SfSzUcRzH+97v0a1f+3Wh79ioW7Tddldi9qVmtvzxy9aqrUytTYffEHd0 h8WahKTm+aHWLZLHSDHUFWJDyFNEZxZWnCF087DlIenOreW/1/vzfX0/738+NCbaxm3oEGUk r1LKw8SkEO/506mT3hlY8zle2iRAKbpiAcr4NArQVnYHhQxlqQBl6w0YetreT6CsihwMbfxs p1DGwjMCbU/OE6j/RwVAvZtSVKL/TqH5WSdUO6Uj0FDDExKtpLUDtPxtG0Nfhxi0sDlLodKR QQFKSHJGhTl6AvXG+6LukRUSZT56iaO7742bexoj0dxSF47uJZYJPO24qoIqwCUlppLc5kY2 4IbS0wTc4+56itPXDgLunWac4irKV0mu7rmEq628T3JjuiZjKrnNDXclENyWpgXn0usrAafV FWDnRVeEpwL5sJBoXnXM45ow+E3jEohYPnJzJu01FQ+SbB4ACxqyLlD7uI00szUcmKg2spAW saUAVjUZBOYwDWB30wpusg6wnnB5uEhgYkvWEa6PjuJmaQHAjS8tlClgbD0Bx9ZTgMkiWWdY V16y08GwEqjNW9hhnHWA1Z8nCRNbsb5Qn/8RmJ39cC1nYqfNgpXBwoJfxjbauPQozM1VmMYY aw/rqhaxTMBqdv3Q/Lc0u6xCgFUCKz4qIELtH6RwkqnlCnWUMkgWEK6oBeazWH0LijvcWwFL A/FeJthtzUdEyKPVMYpWAGlMbMlM9hlHTKA8JpZXhfuposJ4dSuwpXHxQSa/wXBRxAbJI/lQ no/gVf9eBbSFTTzgf3vXu9YcWrVwlmm9Fudjb1V6n3aXUvFX+4qyPKYlDfai5hfVocXUGTeJ XbJ9nOOykly/wbNzM5f1VlIf9dylwaTD4+eSNw0jcdbZJXs+CP0ftmnZ/OsnmcJux7x0B9sa iYvMOejE2eZ596l9flKVq3K1cyjYK+VCO/OqmAkT4+pguZMEU6nlfwGkl/P9EgMAAA== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 24.03.2016 17:03, Andy Yan wrote: > Hi Krzystof: (...) >>> +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; >> In absence of 'normal' mode (it is not described as required property) >> the magic will be '0'. It would be nice to document that in bindings. >> Imagine someone forgets about this and will wonder why 0x0 is written >> to his precious register on normal reboot... > > If the magic value is '0', we won't touch the register, please see > reboot_mode_notify bellow. Ah, indeed... so we cannot use value of '0' for magic (e.g. to clear any existing value for normal reboot)? >> It would be nice to document that 'mode-normal' has a special >> (hard-coded) meaning. >> >>> +} >>> + >>> +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(magic); >>> + >>> + return NOTIFY_DONE; >>> +} >>> + >>> +int reboot_mode_register(struct device *dev, int (*write)(int)) >>> +{ >>> + struct reboot_mode_driver *reboot; >>> + struct mode_info *info; >>> + struct property *prop; >>> + size_t len = strlen(PREFIX); >>> + int ret; >>> + >>> + reboot = devm_kzalloc(dev, sizeof(*reboot), GFP_KERNEL); >>> + if (!reboot) >>> + return -ENOMEM; >>> + >>> + reboot->write = write; >>> + INIT_LIST_HEAD(&reboot->head); >>> + for_each_property_of_node(dev->of_node, prop) { >>> + if (len > strlen(prop->name) || strncmp(prop->name, >>> PREFIX, len)) >>> + continue; >> New line please for readability. > > Okay >> >>> + info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL); >>> + if (!info) >>> + return -ENOMEM; >> Ditto. > > Okay >> >>> + strcpy(info->mode, prop->name + len); >> Ehm, and how do you protect that name of mode is shorter than 32 >> characters? > > How about info->mode = prop->name + len ? I don't get your answer. As fair as I read the code, the prop->name can be very long and you are copying it from 5 character. If the name of the mode has bazillion characters then again my question: how do you protect that it will fit in 32 bytes of 'mode'? >> >>> + if (of_property_read_u32(dev->of_node, prop->name, >>> &info->magic)) { >>> + dev_err(dev, "reboot mode %s without magic >>> number\n", >>> + info->mode); >>> + devm_kfree(dev, info); >>> + 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(dev, "can't register reboot notifier\n"); >>> + >>> + return ret; >>> +} >>> +EXPORT_SYMBOL_GPL(reboot_mode_register); >>> + >>> +MODULE_AUTHOR("Andy Yan >> +MODULE_DESCRIPTION("System reboot mode driver"); >>> +MODULE_LICENSE("GPL v2"); >>> diff --git a/drivers/power/reset/reboot-mode.h >>> b/drivers/power/reset/reboot-mode.h >>> new file mode 100644 >>> index 0000000..44ed34f >>> --- /dev/null >>> +++ b/drivers/power/reset/reboot-mode.h >>> @@ -0,0 +1,6 @@ >>> +#ifndef __REBOOT_MODE_H__ >>> +#define __REBOOT_MODE_H__ >>> + >>> +int reboot_mode_register(struct device *dev, int (*write)(int)); >> Documentation would be appreciated. Although it is local header but >> you decoupled them and you are exporting the function. > > I think I should remove the EXPORT_SYMBOL_GPL. You need to export symbols which are located in modules. Is it located in module? Best regards, Krzysztof From mboxrd@z Thu Jan 1 00:00:00 1970 From: k.kozlowski@samsung.com (Krzysztof Kozlowski) Date: Mon, 28 Mar 2016 15:34:32 +0900 Subject: [PATCH v6 2/4] power: reset: add reboot mode driver In-Reply-To: <56F39F36.5050702@rock-chips.com> References: <1458646525-491-1-git-send-email-andy.yan@rock-chips.com> <1458646642-591-1-git-send-email-andy.yan@rock-chips.com> <56F39F36.5050702@rock-chips.com> Message-ID: <56F8D078.6040503@samsung.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 24.03.2016 17:03, Andy Yan wrote: > Hi Krzystof: (...) >>> +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; >> In absence of 'normal' mode (it is not described as required property) >> the magic will be '0'. It would be nice to document that in bindings. >> Imagine someone forgets about this and will wonder why 0x0 is written >> to his precious register on normal reboot... > > If the magic value is '0', we won't touch the register, please see > reboot_mode_notify bellow. Ah, indeed... so we cannot use value of '0' for magic (e.g. to clear any existing value for normal reboot)? >> It would be nice to document that 'mode-normal' has a special >> (hard-coded) meaning. >> >>> +} >>> + >>> +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(magic); >>> + >>> + return NOTIFY_DONE; >>> +} >>> + >>> +int reboot_mode_register(struct device *dev, int (*write)(int)) >>> +{ >>> + struct reboot_mode_driver *reboot; >>> + struct mode_info *info; >>> + struct property *prop; >>> + size_t len = strlen(PREFIX); >>> + int ret; >>> + >>> + reboot = devm_kzalloc(dev, sizeof(*reboot), GFP_KERNEL); >>> + if (!reboot) >>> + return -ENOMEM; >>> + >>> + reboot->write = write; >>> + INIT_LIST_HEAD(&reboot->head); >>> + for_each_property_of_node(dev->of_node, prop) { >>> + if (len > strlen(prop->name) || strncmp(prop->name, >>> PREFIX, len)) >>> + continue; >> New line please for readability. > > Okay >> >>> + info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL); >>> + if (!info) >>> + return -ENOMEM; >> Ditto. > > Okay >> >>> + strcpy(info->mode, prop->name + len); >> Ehm, and how do you protect that name of mode is shorter than 32 >> characters? > > How about info->mode = prop->name + len ? I don't get your answer. As fair as I read the code, the prop->name can be very long and you are copying it from 5 character. If the name of the mode has bazillion characters then again my question: how do you protect that it will fit in 32 bytes of 'mode'? >> >>> + if (of_property_read_u32(dev->of_node, prop->name, >>> &info->magic)) { >>> + dev_err(dev, "reboot mode %s without magic >>> number\n", >>> + info->mode); >>> + devm_kfree(dev, info); >>> + 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(dev, "can't register reboot notifier\n"); >>> + >>> + return ret; >>> +} >>> +EXPORT_SYMBOL_GPL(reboot_mode_register); >>> + >>> +MODULE_AUTHOR("Andy Yan >> +MODULE_DESCRIPTION("System reboot mode driver"); >>> +MODULE_LICENSE("GPL v2"); >>> diff --git a/drivers/power/reset/reboot-mode.h >>> b/drivers/power/reset/reboot-mode.h >>> new file mode 100644 >>> index 0000000..44ed34f >>> --- /dev/null >>> +++ b/drivers/power/reset/reboot-mode.h >>> @@ -0,0 +1,6 @@ >>> +#ifndef __REBOOT_MODE_H__ >>> +#define __REBOOT_MODE_H__ >>> + >>> +int reboot_mode_register(struct device *dev, int (*write)(int)); >> Documentation would be appreciated. Although it is local header but >> you decoupled them and you are exporting the function. > > I think I should remove the EXPORT_SYMBOL_GPL. You need to export symbols which are located in modules. Is it located in module? Best regards, Krzysztof