Re: [PATCH v7 2/4] power: reset: add reboot mode driver
Hi Krzysztof: Thanks for you kindly suggestion. I will follow it in next version. On 2016年04月12日 19:18, Krzysztof Kozlowski wrote: On 04/12/2016 11:27 AM, Andy Yan wrote: + +static const struct of_device_id syscon_reboot_mode_of_match[] = { +{ .compatible = "syscon-reboot-mode" }, +{} +}; + +static struct platform_driver syscon_reboot_mode_driver = { +.probe = syscon_reboot_mode_probe, Cleanup needed. What will happen after device unbind? Memory will be released (devm-*()) but reboot notifier won't thus leading to OOPS on reboot. From the kernel_restart_prepare function, the reboot notifier will be called before device_shutdown. Is there any other case the device unbind before reboot notifier called? This is a regular module platform driver so unbind can happen any time initiated by user, either by unbind command or by module removal. User can then re-bind device or not - probably does not matter. Anyway after such first unbind, the restart will kaboom instead of do a restart. I just need to do clean up in remove? Yes, you need to clean up everything in remove() callback (in a reversed order). Best regards, Krzysztof
Re: [PATCH v7 2/4] power: reset: add reboot mode driver
Hi Krzysztof: Thanks for you kindly suggestion. I will follow it in next version. On 2016年04月12日 19:18, Krzysztof Kozlowski wrote: On 04/12/2016 11:27 AM, Andy Yan wrote: + +static const struct of_device_id syscon_reboot_mode_of_match[] = { +{ .compatible = "syscon-reboot-mode" }, +{} +}; + +static struct platform_driver syscon_reboot_mode_driver = { +.probe = syscon_reboot_mode_probe, Cleanup needed. What will happen after device unbind? Memory will be released (devm-*()) but reboot notifier won't thus leading to OOPS on reboot. From the kernel_restart_prepare function, the reboot notifier will be called before device_shutdown. Is there any other case the device unbind before reboot notifier called? This is a regular module platform driver so unbind can happen any time initiated by user, either by unbind command or by module removal. User can then re-bind device or not - probably does not matter. Anyway after such first unbind, the restart will kaboom instead of do a restart. I just need to do clean up in remove? Yes, you need to clean up everything in remove() callback (in a reversed order). Best regards, Krzysztof
Re: [PATCH v7 2/4] power: reset: add reboot mode driver
On 04/12/2016 11:27 AM, Andy Yan wrote: > + > +static const struct of_device_id syscon_reboot_mode_of_match[] = { > +{ .compatible = "syscon-reboot-mode" }, > +{} > +}; > + > +static struct platform_driver syscon_reboot_mode_driver = { > +.probe = syscon_reboot_mode_probe, Cleanup needed. What will happen after device unbind? Memory will be released (devm-*()) but reboot notifier won't thus leading to OOPS on reboot. >>> From the kernel_restart_prepare function, the reboot notifier will >>> be called before device_shutdown. Is there any other case the device >>> unbind before reboot notifier >>> called? >> This is a regular module platform driver so unbind can happen any time >> initiated by user, either by unbind command or by module removal. User >> can then re-bind device or not - probably does not matter. Anyway after >> such first unbind, the restart will kaboom instead of do a restart. > > I just need to do clean up in remove? Yes, you need to clean up everything in remove() callback (in a reversed order). Best regards, Krzysztof
Re: [PATCH v7 2/4] power: reset: add reboot mode driver
On 04/12/2016 11:27 AM, Andy Yan wrote: > + > +static const struct of_device_id syscon_reboot_mode_of_match[] = { > +{ .compatible = "syscon-reboot-mode" }, > +{} > +}; > + > +static struct platform_driver syscon_reboot_mode_driver = { > +.probe = syscon_reboot_mode_probe, Cleanup needed. What will happen after device unbind? Memory will be released (devm-*()) but reboot notifier won't thus leading to OOPS on reboot. >>> From the kernel_restart_prepare function, the reboot notifier will >>> be called before device_shutdown. Is there any other case the device >>> unbind before reboot notifier >>> called? >> This is a regular module platform driver so unbind can happen any time >> initiated by user, either by unbind command or by module removal. User >> can then re-bind device or not - probably does not matter. Anyway after >> such first unbind, the restart will kaboom instead of do a restart. > > I just need to do clean up in remove? Yes, you need to clean up everything in remove() callback (in a reversed order). Best regards, Krzysztof
Re: [PATCH v7 2/4] power: reset: add reboot mode driver
Hi Krzysztof: On 2016年04月06日 09:00, Krzysztof Kozlowski wrote: On 06.04.2016 09:50, Andy Yan wrote: (...) +return -ENOMEM; + +info->mode = kstrdup_const(prop->name + len, GFP_KERNEL); +if (of_property_read_u32(np, prop->name, >magic)) { +dev_err(dev, "reboot mode %s without magic number\n", +info->mode); +devm_kfree(dev, info); +continue; +} +list_add_tail(>list, >head); +} +of_node_put(np); If you of_node_put() here, there is no sense in getting it before. I mentioned of_node_get() only because I am not sure about life-cycle of nodes in case of DT overlays and you are storing the pointer to string from DT. The doubts I have are concerning only the case of freeing nodes from overlay. I don't know if of_node_get() is needed but of_node_get()+of_node_put() seems useless. I am also not sure about it. Maybe just drop of_node_get/put ? OK, let's drop both get() and put(). (...) + +static const struct of_device_id syscon_reboot_mode_of_match[] = { +{ .compatible = "syscon-reboot-mode" }, +{} +}; + +static struct platform_driver syscon_reboot_mode_driver = { +.probe = syscon_reboot_mode_probe, Cleanup needed. What will happen after device unbind? Memory will be released (devm-*()) but reboot notifier won't thus leading to OOPS on reboot. From the kernel_restart_prepare function, the reboot notifier will be called before device_shutdown. Is there any other case the device unbind before reboot notifier called? This is a regular module platform driver so unbind can happen any time initiated by user, either by unbind command or by module removal. User can then re-bind device or not - probably does not matter. Anyway after such first unbind, the restart will kaboom instead of do a restart. I just need to do clean up in remove? Beside that, you always should clean up, regardless of restart or not. If you do not want unbind (thus no need of cleanup) then forbid it by making it a non-module with suppressed bind. Best regards, Krzysztof
Re: [PATCH v7 2/4] power: reset: add reboot mode driver
Hi Krzysztof: On 2016年04月06日 09:00, Krzysztof Kozlowski wrote: On 06.04.2016 09:50, Andy Yan wrote: (...) +return -ENOMEM; + +info->mode = kstrdup_const(prop->name + len, GFP_KERNEL); +if (of_property_read_u32(np, prop->name, >magic)) { +dev_err(dev, "reboot mode %s without magic number\n", +info->mode); +devm_kfree(dev, info); +continue; +} +list_add_tail(>list, >head); +} +of_node_put(np); If you of_node_put() here, there is no sense in getting it before. I mentioned of_node_get() only because I am not sure about life-cycle of nodes in case of DT overlays and you are storing the pointer to string from DT. The doubts I have are concerning only the case of freeing nodes from overlay. I don't know if of_node_get() is needed but of_node_get()+of_node_put() seems useless. I am also not sure about it. Maybe just drop of_node_get/put ? OK, let's drop both get() and put(). (...) + +static const struct of_device_id syscon_reboot_mode_of_match[] = { +{ .compatible = "syscon-reboot-mode" }, +{} +}; + +static struct platform_driver syscon_reboot_mode_driver = { +.probe = syscon_reboot_mode_probe, Cleanup needed. What will happen after device unbind? Memory will be released (devm-*()) but reboot notifier won't thus leading to OOPS on reboot. From the kernel_restart_prepare function, the reboot notifier will be called before device_shutdown. Is there any other case the device unbind before reboot notifier called? This is a regular module platform driver so unbind can happen any time initiated by user, either by unbind command or by module removal. User can then re-bind device or not - probably does not matter. Anyway after such first unbind, the restart will kaboom instead of do a restart. I just need to do clean up in remove? Beside that, you always should clean up, regardless of restart or not. If you do not want unbind (thus no need of cleanup) then forbid it by making it a non-module with suppressed bind. Best regards, Krzysztof
Re: [PATCH v7 2/4] power: reset: add reboot mode driver
On 06.04.2016 09:50, Andy Yan wrote: (...) >>> +return -ENOMEM; >>> + >>> +info->mode = kstrdup_const(prop->name + len, GFP_KERNEL); >>> +if (of_property_read_u32(np, prop->name, >magic)) { >>> +dev_err(dev, "reboot mode %s without magic number\n", >>> +info->mode); >>> +devm_kfree(dev, info); >>> +continue; >>> +} >>> +list_add_tail(>list, >head); >>> +} >>> +of_node_put(np); >> If you of_node_put() here, there is no sense in getting it before. I >> mentioned of_node_get() only because I am not sure about life-cycle of >> nodes in case of DT overlays and you are storing the pointer to string >> from DT. >> >> The doubts I have are concerning only the case of freeing nodes from >> overlay. >> >> I don't know if of_node_get() is needed but of_node_get()+of_node_put() >> seems useless. > > > I am also not sure about it. Maybe just drop of_node_get/put ? OK, let's drop both get() and put(). (...) >>> + >>> +static const struct of_device_id syscon_reboot_mode_of_match[] = { >>> +{ .compatible = "syscon-reboot-mode" }, >>> +{} >>> +}; >>> + >>> +static struct platform_driver syscon_reboot_mode_driver = { >>> +.probe = syscon_reboot_mode_probe, >> Cleanup needed. What will happen after device unbind? Memory will be >> released (devm-*()) but reboot notifier won't thus leading to OOPS on >> reboot. > > From the kernel_restart_prepare function, the reboot notifier will > be called before device_shutdown. Is there any other case the device > unbind before reboot notifier > called? This is a regular module platform driver so unbind can happen any time initiated by user, either by unbind command or by module removal. User can then re-bind device or not - probably does not matter. Anyway after such first unbind, the restart will kaboom instead of do a restart. Beside that, you always should clean up, regardless of restart or not. If you do not want unbind (thus no need of cleanup) then forbid it by making it a non-module with suppressed bind. Best regards, Krzysztof
Re: [PATCH v7 2/4] power: reset: add reboot mode driver
On 06.04.2016 09:50, Andy Yan wrote: (...) >>> +return -ENOMEM; >>> + >>> +info->mode = kstrdup_const(prop->name + len, GFP_KERNEL); >>> +if (of_property_read_u32(np, prop->name, >magic)) { >>> +dev_err(dev, "reboot mode %s without magic number\n", >>> +info->mode); >>> +devm_kfree(dev, info); >>> +continue; >>> +} >>> +list_add_tail(>list, >head); >>> +} >>> +of_node_put(np); >> If you of_node_put() here, there is no sense in getting it before. I >> mentioned of_node_get() only because I am not sure about life-cycle of >> nodes in case of DT overlays and you are storing the pointer to string >> from DT. >> >> The doubts I have are concerning only the case of freeing nodes from >> overlay. >> >> I don't know if of_node_get() is needed but of_node_get()+of_node_put() >> seems useless. > > > I am also not sure about it. Maybe just drop of_node_get/put ? OK, let's drop both get() and put(). (...) >>> + >>> +static const struct of_device_id syscon_reboot_mode_of_match[] = { >>> +{ .compatible = "syscon-reboot-mode" }, >>> +{} >>> +}; >>> + >>> +static struct platform_driver syscon_reboot_mode_driver = { >>> +.probe = syscon_reboot_mode_probe, >> Cleanup needed. What will happen after device unbind? Memory will be >> released (devm-*()) but reboot notifier won't thus leading to OOPS on >> reboot. > > From the kernel_restart_prepare function, the reboot notifier will > be called before device_shutdown. Is there any other case the device > unbind before reboot notifier > called? This is a regular module platform driver so unbind can happen any time initiated by user, either by unbind command or by module removal. User can then re-bind device or not - probably does not matter. Anyway after such first unbind, the restart will kaboom instead of do a restart. Beside that, you always should clean up, regardless of restart or not. If you do not want unbind (thus no need of cleanup) then forbid it by making it a non-module with suppressed bind. Best regards, Krzysztof
Re: [PATCH v7 2/4] power: reset: add reboot mode driver
Hi Krzysztof: Thanks for you comments. On 2016年03月30日 11:36, Krzysztof Kozlowski wrote: On 30.03.2016 11:20, 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 BruggerReviewed-by: Moritz Fischer Tested-by: John Stultz Acked-by: John Stultz Signed-off-by: Andy Yan --- Changes in v7: - address some suggestions from Krzysztof, make syscon-reboot-mode driver data self-contained. Thanks for changes, few comments below. (...) diff --git a/drivers/power/reset/reboot-mode.c b/drivers/power/reset/reboot-mode.c new file mode 100644 index 000..ae6f931 --- /dev/null +++ b/drivers/power/reset/reboot-mode.c @@ -0,0 +1,115 @@ +/* + * 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; +}; + +struct reboot_mode_driver { + struct device *dev; + struct list_head head; + int (*write)(struct device *dev, int magic); + struct notifier_block reboot_notifier; +}; + +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, >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->dev, magic); + + return NOTIFY_DONE; +} + +int reboot_mode_register(struct device *dev, int (*write)(struct device *, int)) +{ + struct reboot_mode_driver *reboot; + struct mode_info *info; + struct property *prop; + struct device_node *np; + size_t len = strlen(PREFIX); + int ret; + + reboot = devm_kzalloc(dev, sizeof(*reboot), GFP_KERNEL); + if (!reboot) + return -ENOMEM; + + reboot->dev = dev; + reboot->write = write; + INIT_LIST_HEAD(>head); + + np = of_node_get(dev->of_node); + for_each_property_of_node(np, prop) { + if (len > strlen(prop->name) || strncmp(prop->name, PREFIX, len)) + continue; + + info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL); + if (!info) Clean up needed: of_node_put() and kfree_const() for items stored on the list from previous iterations. Okay, I will add it in V8. + return -ENOMEM; + + info->mode = kstrdup_const(prop->name + len, GFP_KERNEL); + if (of_property_read_u32(np, prop->name, >magic)) { + dev_err(dev, "reboot mode %s without magic number\n", + info->mode); + devm_kfree(dev, info); + continue; + } + list_add_tail(>list, >head); + } + of_node_put(np); If you of_node_put() here, there is no sense in getting it before. I mentioned of_node_get() only because I am not sure about life-cycle of nodes in case of DT overlays and you are storing the pointer to string from DT. The doubts I have are concerning only the case of freeing nodes from overlay. I don't know if of_node_get() is needed but of_node_get()+of_node_put() seems useless. I am also not sure about it. Maybe just drop of_node_get/put ? + + reboot->reboot_notifier.notifier_call = reboot_mode_notify; + ret = register_reboot_notifier(>reboot_notifier); + if (ret) + dev_err(dev, "can't register reboot
Re: [PATCH v7 2/4] power: reset: add reboot mode driver
Hi Krzysztof: Thanks for you comments. On 2016年03月30日 11:36, Krzysztof Kozlowski wrote: On 30.03.2016 11:20, 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 v7: - address some suggestions from Krzysztof, make syscon-reboot-mode driver data self-contained. Thanks for changes, few comments below. (...) diff --git a/drivers/power/reset/reboot-mode.c b/drivers/power/reset/reboot-mode.c new file mode 100644 index 000..ae6f931 --- /dev/null +++ b/drivers/power/reset/reboot-mode.c @@ -0,0 +1,115 @@ +/* + * 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; +}; + +struct reboot_mode_driver { + struct device *dev; + struct list_head head; + int (*write)(struct device *dev, int magic); + struct notifier_block reboot_notifier; +}; + +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, >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->dev, magic); + + return NOTIFY_DONE; +} + +int reboot_mode_register(struct device *dev, int (*write)(struct device *, int)) +{ + struct reboot_mode_driver *reboot; + struct mode_info *info; + struct property *prop; + struct device_node *np; + size_t len = strlen(PREFIX); + int ret; + + reboot = devm_kzalloc(dev, sizeof(*reboot), GFP_KERNEL); + if (!reboot) + return -ENOMEM; + + reboot->dev = dev; + reboot->write = write; + INIT_LIST_HEAD(>head); + + np = of_node_get(dev->of_node); + for_each_property_of_node(np, prop) { + if (len > strlen(prop->name) || strncmp(prop->name, PREFIX, len)) + continue; + + info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL); + if (!info) Clean up needed: of_node_put() and kfree_const() for items stored on the list from previous iterations. Okay, I will add it in V8. + return -ENOMEM; + + info->mode = kstrdup_const(prop->name + len, GFP_KERNEL); + if (of_property_read_u32(np, prop->name, >magic)) { + dev_err(dev, "reboot mode %s without magic number\n", + info->mode); + devm_kfree(dev, info); + continue; + } + list_add_tail(>list, >head); + } + of_node_put(np); If you of_node_put() here, there is no sense in getting it before. I mentioned of_node_get() only because I am not sure about life-cycle of nodes in case of DT overlays and you are storing the pointer to string from DT. The doubts I have are concerning only the case of freeing nodes from overlay. I don't know if of_node_get() is needed but of_node_get()+of_node_put() seems useless. I am also not sure about it. Maybe just drop of_node_get/put ? + + reboot->reboot_notifier.notifier_call = reboot_mode_notify; + ret = register_reboot_notifier(>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 +#include +#include
Re: [PATCH v7 2/4] power: reset: add reboot mode driver
On 30.03.2016 11:20, 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 v7: > - address some suggestions from Krzysztof, make syscon-reboot-mode driver > data self-contained. > Thanks for changes, few comments below. (...) > diff --git a/drivers/power/reset/reboot-mode.c > b/drivers/power/reset/reboot-mode.c > new file mode 100644 > index 000..ae6f931 > --- /dev/null > +++ b/drivers/power/reset/reboot-mode.c > @@ -0,0 +1,115 @@ > +/* > + * 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; > +}; > + > +struct reboot_mode_driver { > + struct device *dev; > + struct list_head head; > + int (*write)(struct device *dev, int magic); > + struct notifier_block reboot_notifier; > +}; > + > +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, >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->dev, magic); > + > + return NOTIFY_DONE; > +} > + > +int reboot_mode_register(struct device *dev, int (*write)(struct device *, > int)) > +{ > + struct reboot_mode_driver *reboot; > + struct mode_info *info; > + struct property *prop; > + struct device_node *np; > + size_t len = strlen(PREFIX); > + int ret; > + > + reboot = devm_kzalloc(dev, sizeof(*reboot), GFP_KERNEL); > + if (!reboot) > + return -ENOMEM; > + > + reboot->dev = dev; > + reboot->write = write; > + INIT_LIST_HEAD(>head); > + > + np = of_node_get(dev->of_node); > + for_each_property_of_node(np, prop) { > + if (len > strlen(prop->name) || strncmp(prop->name, PREFIX, > len)) > + continue; > + > + info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL); > + if (!info) Clean up needed: of_node_put() and kfree_const() for items stored on the list from previous iterations. > + return -ENOMEM; > + > + info->mode = kstrdup_const(prop->name + len, GFP_KERNEL); > + if (of_property_read_u32(np, prop->name, >magic)) { > + dev_err(dev, "reboot mode %s without magic number\n", > + info->mode); > + devm_kfree(dev, info); > + continue; > + } > + list_add_tail(>list, >head); > + } > + of_node_put(np); If you of_node_put() here, there is no sense in getting it before. I mentioned of_node_get() only because I am not sure about life-cycle of nodes in case of DT overlays and you are storing the pointer to string from DT. The doubts I have are concerning only the case of freeing nodes from overlay. I don't know if of_node_get() is needed but of_node_get()+of_node_put() seems useless. > + > + reboot->reboot_notifier.notifier_call = reboot_mode_notify; > + ret = register_reboot_notifier(>reboot_notifier); > + if (ret) > + dev_err(dev, "can't register reboot notifier\n"); > + > + return ret; > +} >
Re: [PATCH v7 2/4] power: reset: add reboot mode driver
On 30.03.2016 11:20, 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 v7: > - address some suggestions from Krzysztof, make syscon-reboot-mode driver > data self-contained. > Thanks for changes, few comments below. (...) > diff --git a/drivers/power/reset/reboot-mode.c > b/drivers/power/reset/reboot-mode.c > new file mode 100644 > index 000..ae6f931 > --- /dev/null > +++ b/drivers/power/reset/reboot-mode.c > @@ -0,0 +1,115 @@ > +/* > + * 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; > +}; > + > +struct reboot_mode_driver { > + struct device *dev; > + struct list_head head; > + int (*write)(struct device *dev, int magic); > + struct notifier_block reboot_notifier; > +}; > + > +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, >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->dev, magic); > + > + return NOTIFY_DONE; > +} > + > +int reboot_mode_register(struct device *dev, int (*write)(struct device *, > int)) > +{ > + struct reboot_mode_driver *reboot; > + struct mode_info *info; > + struct property *prop; > + struct device_node *np; > + size_t len = strlen(PREFIX); > + int ret; > + > + reboot = devm_kzalloc(dev, sizeof(*reboot), GFP_KERNEL); > + if (!reboot) > + return -ENOMEM; > + > + reboot->dev = dev; > + reboot->write = write; > + INIT_LIST_HEAD(>head); > + > + np = of_node_get(dev->of_node); > + for_each_property_of_node(np, prop) { > + if (len > strlen(prop->name) || strncmp(prop->name, PREFIX, > len)) > + continue; > + > + info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL); > + if (!info) Clean up needed: of_node_put() and kfree_const() for items stored on the list from previous iterations. > + return -ENOMEM; > + > + info->mode = kstrdup_const(prop->name + len, GFP_KERNEL); > + if (of_property_read_u32(np, prop->name, >magic)) { > + dev_err(dev, "reboot mode %s without magic number\n", > + info->mode); > + devm_kfree(dev, info); > + continue; > + } > + list_add_tail(>list, >head); > + } > + of_node_put(np); If you of_node_put() here, there is no sense in getting it before. I mentioned of_node_get() only because I am not sure about life-cycle of nodes in case of DT overlays and you are storing the pointer to string from DT. The doubts I have are concerning only the case of freeing nodes from overlay. I don't know if of_node_get() is needed but of_node_get()+of_node_put() seems useless. > + > + reboot->reboot_notifier.notifier_call = reboot_mode_notify; > + ret = register_reboot_notifier(>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"); >
[PATCH v7 2/4] power: reset: add reboot mode driver
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 BruggerReviewed-by: Moritz Fischer Tested-by: John Stultz Acked-by: John Stultz Signed-off-by: Andy Yan --- 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| 115 +++ drivers/power/reset/reboot-mode.h| 6 ++ drivers/power/reset/syscon-reboot-mode.c | 86 +++ 5 files changed, 222 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 000..ae6f931 --- /dev/null +++ b/drivers/power/reset/reboot-mode.c @@ -0,0 +1,115 @@ +/* + * 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; +}; + +struct reboot_mode_driver { + struct device *dev; + struct list_head head; + int (*write)(struct device *dev, int magic); + struct notifier_block reboot_notifier; +}; + +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, >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->dev,
[PATCH v7 2/4] power: reset: add reboot mode driver
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 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| 115 +++ drivers/power/reset/reboot-mode.h| 6 ++ drivers/power/reset/syscon-reboot-mode.c | 86 +++ 5 files changed, 222 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 000..ae6f931 --- /dev/null +++ b/drivers/power/reset/reboot-mode.c @@ -0,0 +1,115 @@ +/* + * 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; +}; + +struct reboot_mode_driver { + struct device *dev; + struct list_head head; + int (*write)(struct device *dev, int magic); + struct notifier_block reboot_notifier; +}; + +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, >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->dev, magic); + + return NOTIFY_DONE; +} + +int reboot_mode_register(struct device *dev, int (*write)(struct device *,