From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754654AbcC1IFr (ORCPT ); Mon, 28 Mar 2016 04:05:47 -0400 Received: from mailout3.w1.samsung.com ([210.118.77.13]:52332 "EHLO mailout3.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754246AbcC1IFi (ORCPT ); Mon, 28 Mar 2016 04:05:38 -0400 MIME-version: 1.0 Content-type: text/plain; charset=utf-8 X-AuditID: cbfec7f5-f792a6d000001302-c5-56f8e5cd255d Content-transfer-encoding: 8BIT 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> <56F8D078.6040503@samsung.com> <56F8DFED.50905@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: <56F8E5C4.5010000@samsung.com> Date: Mon, 28 Mar 2016 17:05:24 +0900 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 In-reply-to: <56F8DFED.50905@rock-chips.com> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrLIsWRmVeSWpSXmKPExsVy+t/xy7pnn/4IMzj/ydSi49piJov+8zcZ Lf5OOsZu8X5ZD6PFpCfvmS3mHznHajFx5WRmi1/vjrBb9L9ZyGrx/9FrVotzr1YyWpz5rWux 5MlDdovXLwwtNj2+xmpxedccNovPvUcYLT49+M9scfsyr8Wb3y/YLZZev8hk0dRibLFg8hNW izMNERanrn9ms5gwfS2LReteoMmnd5dYvPx4gsWivXkZk4Ocx5p5axg9Wpp72Dx+/5rE6HG5 r5fJY+apLeweTzZdZPTYOesuu8fK5V/YPDav0PLYtKqTzePOtT1A3pJ6jysnmlg9/s7az+LR t2UVo8f2a/OYA4SiuGxSUnMyy1KL9O0SuDL+/3zIXLBIuKLj8Xa2BsZnvF2MnBwSAiYSV59u YIKwxSQu3FvP1sXIxSEksJRR4mPTOrAEr4CgxI/J91i6GDk4mAXkJY5cyoYw1SWmTMmFKH/K KDHt3kl2kHJhAQeJT1cWgbWKCKhJ/Lx5kwWiaC2TxKrVb1hBHGaBLawSd352MIJUsQkYS2xe voQNYpmWxJ+fv8EmsQioShzZ1QU2SVQgQuLJ3JNg9ZwCOhJbeg+wTGAUmIXkvlkI981CuG8B I/MqRtHU0uSC4qT0XCO94sTc4tK8dL3k/NxNjJBE8XUH49JjVocYBTgYlXh4Myx/hAmxJpYV V+YeYpTgYFYS4b36BCjEm5JYWZValB9fVJqTWnyIUZqDRUmcd+au9yFCAumJJanZqakFqUUw WSYOTqkGRlfdT14MbytW82Twcgde2/JcPX/H3ejnn72ELTdP3rFV8tO6jP6E4hlN16K+8fYu vxH34dPU8KWybU8fr1Z4vDP6jkuUaMuDOYpxhqeanrwJXt7k90ePvUpur0vbfbOFNztjDz87 U65y/9iPO1xqDSczBOr+d3Ne26+qOTt67t2pl3Tbt13ddlKJpTgj0VCLuag4EQDJQyWJEAMA AA== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 28.03.2016 16:40, Andy Yan wrote: > Hi Krzysztof : > > On 2016年03月28日 14:34, Krzysztof Kozlowski wrote: >> 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 seems that the value '0' cannot be used. How about mentioning it in bindings documentation? (...) >>>>> + 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'? > > What I mean is set info->mode as a pointer point to prop->name + len > > struct mode_info { > char *mode; > .......... > ......... > } > > info->mode = prop->name + len Ahh, I get it. Then I guess you should also do of_node_get() and of_node_put()... and use kstrdup_const(). Looks good but I am not familiar with overlays and life-cycle of OF nodes (documentation for the life-cycle is a todo list item: Documentation/devicetree/todo.txt). 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 17:05:24 +0900 Subject: [PATCH v6 2/4] power: reset: add reboot mode driver In-Reply-To: <56F8DFED.50905@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> <56F8D078.6040503@samsung.com> <56F8DFED.50905@rock-chips.com> Message-ID: <56F8E5C4.5010000@samsung.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 28.03.2016 16:40, Andy Yan wrote: > Hi Krzysztof : > > On 2016?03?28? 14:34, Krzysztof Kozlowski wrote: >> 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 seems that the value '0' cannot be used. How about mentioning it in bindings documentation? (...) >>>>> + 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'? > > What I mean is set info->mode as a pointer point to prop->name + len > > struct mode_info { > char *mode; > .......... > ......... > } > > info->mode = prop->name + len Ahh, I get it. Then I guess you should also do of_node_get() and of_node_put()... and use kstrdup_const(). Looks good but I am not familiar with overlays and life-cycle of OF nodes (documentation for the life-cycle is a todo list item: Documentation/devicetree/todo.txt). Best regards, Krzysztof