[U-Boot] [PATCH] syscon: update syscon_node_to_regmap to use the DM functions
+ Update the function syscon_node_to_regmap() to force bound on syscon uclass and directly use the list of device from DM. + Remove the static list syscon_list.
This patch avoid issue (crash) when syscon_node_to_regmap() is called before and after reallocation (list content is invalid).
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com --- Hi
This patch is a modified (after Simon Glass review) and rebased version on v2018.11-rc3 for [U-Boot] syscon: reset node list syscon_list after relocation http://patchwork.ozlabs.org/patch/983139/
This patch correct a crash see on v2018.11-rc1 with my board stm32mp1 for the command "reset", reproduced on v2018.11-rc3.
The crash is a side effect of 2 patches 1f6ca3f42f6edf143473159297f4c515b1cf36f6 sysreset: syscon: update regmap access to syscon
23471aed5c33e104d6fa64575932577618543bec board_f: Add reset status printing
With the first patch the syscon_node_to_regmap() is called each time that the class sysreset_syscon is probed.
=> in v2018.09 probe is done only when reset is requested
NB: for stm32mp1, the node rcc_reboot in tagged pre-relocated to support reset in pre-reloc phases (allow panic).
With the second patch, U-Boot probes all the sysreset uclass in preloc phase to allow to print the reset status, and the list is initialized in board_f / pre-reloc:
-> print_resetinfo --> uclass_first_device_err(UCLASS_SYSRESET, &dev); ---> syscon_reboot_probe() ----> syscon_node_to_regmap(node) -----> of_syscon_register() -------> list_add_tail(&syscon->list, &syscon_list);
During relocation, the content of syscon_list (static) is updated but the list use pointers allocated by malloc in pre-reloc phasis
And when I execute the reset command -> do_reset() --> reset_cpu() ---> sysreset_walk_halt(SYSRESET_COLD); ----> loop on device UCLASS_SYSRESET -----> syscon_reboot_probe() ------> syscon_node_to_regmap(node) -------> list_for_each_entry(entry, &syscon_list, list)
I have a crash here because the pointer syscon_list.next is invalid.
This patch solves the issue by using DM list for syscon uclass.
Tested on my board, the initial issue is solved and I check the behavior with dm_dump_all(): the RCC driver is not binded/probed as syscon uclass by default (checked with dm tree) and correctly bounded and probed when it is necessary (before relocation: print_resetinfo() or after relocation for command reset).
Regards
Patrick
drivers/core/syscon-uclass.c | 55 ++++++++++++++------------------------------ 1 file changed, 17 insertions(+), 38 deletions(-)
diff --git a/drivers/core/syscon-uclass.c b/drivers/core/syscon-uclass.c index 303e166..aeb3583 100644 --- a/drivers/core/syscon-uclass.c +++ b/drivers/core/syscon-uclass.c @@ -123,52 +123,31 @@ U_BOOT_DRIVER(generic_syscon) = { * The syscon node can be bound to another driver, but still works * as a syscon provider. */ -static LIST_HEAD(syscon_list); - -struct syscon { - ofnode node; - struct regmap *regmap; - struct list_head list; -}; - -static struct syscon *of_syscon_register(ofnode node) +struct regmap *syscon_node_to_regmap(ofnode node) { - struct syscon *syscon; + struct udevice *dev, *parent; + ofnode ofnode = node; int ret;
+ if (!uclass_get_device_by_ofnode(UCLASS_SYSCON, node, &dev)) + return syscon_get_regmap(dev); + if (!ofnode_device_is_compatible(node, "syscon")) return ERR_PTR(-EINVAL);
- syscon = malloc(sizeof(*syscon)); - if (!syscon) - return ERR_PTR(-ENOMEM); + if (device_find_global_by_ofnode(ofnode, &parent)) + parent = dm_root();
- ret = regmap_init_mem(node, &syscon->regmap); - if (ret) { - free(syscon); + /* force bound to syscon class */ + ret = device_bind_driver_to_node(parent, "syscon", + ofnode_get_name(node), + node, &dev); + if (ret) return ERR_PTR(ret); - } - - list_add_tail(&syscon->list, &syscon_list); - - return syscon; -}
-struct regmap *syscon_node_to_regmap(ofnode node) -{ - struct syscon *entry, *syscon = NULL; - - list_for_each_entry(entry, &syscon_list, list) - if (ofnode_equal(entry->node, node)) { - syscon = entry; - break; - } - - if (!syscon) - syscon = of_syscon_register(node); - - if (IS_ERR(syscon)) - return ERR_CAST(syscon); + ret = device_probe(dev); + if (ret) + return ERR_PTR(ret);
- return syscon->regmap; + return syscon_get_regmap(dev); }
On Tue, Oct 30, 2018 at 02:44:29PM +0100, Patrick Delaunay wrote:
- Update the function syscon_node_to_regmap() to force bound on syscon uclass and directly use the list of device from DM.
- Remove the static list syscon_list.
This patch avoid issue (crash) when syscon_node_to_regmap() is called before and after reallocation (list content is invalid).
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com
Hi
This patch is a modified (after Simon Glass review) and rebased version on v2018.11-rc3 for [U-Boot] syscon: reset node list syscon_list after relocation http://patchwork.ozlabs.org/patch/983139/
This patch correct a crash see on v2018.11-rc1 with my board stm32mp1 for the command "reset", reproduced on v2018.11-rc3.
The crash is a side effect of 2 patches 1f6ca3f42f6edf143473159297f4c515b1cf36f6 sysreset: syscon: update regmap access to syscon
23471aed5c33e104d6fa64575932577618543bec board_f: Add reset status printing
With the first patch the syscon_node_to_regmap() is called each time that the class sysreset_syscon is probed.
=> in v2018.09 probe is done only when reset is requested
NB: for stm32mp1, the node rcc_reboot in tagged pre-relocated to support reset in pre-reloc phases (allow panic).
With the second patch, U-Boot probes all the sysreset uclass in preloc phase to allow to print the reset status, and the list is initialized in board_f / pre-reloc:
-> print_resetinfo --> uclass_first_device_err(UCLASS_SYSRESET, &dev); ---> syscon_reboot_probe() ----> syscon_node_to_regmap(node) -----> of_syscon_register() -------> list_add_tail(&syscon->list, &syscon_list);
During relocation, the content of syscon_list (static) is updated but the list use pointers allocated by malloc in pre-reloc phasis
And when I execute the reset command -> do_reset() --> reset_cpu() ---> sysreset_walk_halt(SYSRESET_COLD); ----> loop on device UCLASS_SYSRESET -----> syscon_reboot_probe() ------> syscon_node_to_regmap(node) -------> list_for_each_entry(entry, &syscon_list, list)
I have a crash here because the pointer syscon_list.next is invalid.
This patch solves the issue by using DM list for syscon uclass.
Tested on my board, the initial issue is solved and I check the behavior with dm_dump_all(): the RCC driver is not binded/probed as syscon uclass by default (checked with dm tree) and correctly bounded and probed when it is necessary (before relocation: print_resetinfo() or after relocation for command reset).
So this is a regression we should fix, to be clear, yes? Thanks!
Hi Tom
From: Tom Rini trini@konsulko.com Sent: mardi 30 octobre 2018 14:52 Subject: Re: [PATCH] syscon: update syscon_node_to_regmap to use the DM functions
On Tue, Oct 30, 2018 at 02:44:29PM +0100, Patrick Delaunay wrote:
- Update the function syscon_node_to_regmap() to force bound on syscon uclass and directly use the list of device from DM.
- Remove the static list syscon_list.
This patch avoid issue (crash) when syscon_node_to_regmap() is called before and after reallocation (list content is invalid).
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com
Hi
This patch is a modified (after Simon Glass review) and rebased version on v2018.11-rc3 for [U-Boot] syscon: reset node list syscon_list after relocation http://patchwork.ozlabs.org/patch/983139/
This patch correct a crash see on v2018.11-rc1 with my board stm32mp1 for the command "reset", reproduced on v2018.11-rc3.
The crash is a side effect of 2 patches 1f6ca3f42f6edf143473159297f4c515b1cf36f6 sysreset: syscon: update regmap access to syscon
23471aed5c33e104d6fa64575932577618543bec board_f: Add reset status printing
With the first patch the syscon_node_to_regmap() is called each time that the class sysreset_syscon is probed.
=> in v2018.09 probe is done only when reset is requested
NB: for stm32mp1, the node rcc_reboot in tagged pre-relocated to support reset in pre-reloc phases (allow panic).
With the second patch, U-Boot probes all the sysreset uclass in preloc phase to allow to print the reset status, and the list is initialized in board_f / pre-reloc:
-> print_resetinfo --> uclass_first_device_err(UCLASS_SYSRESET, &dev); ---> syscon_reboot_probe() ----> syscon_node_to_regmap(node) -----> of_syscon_register() -------> list_add_tail(&syscon->list, &syscon_list);
During relocation, the content of syscon_list (static) is updated but the list use pointers allocated by malloc in pre-reloc phasis
And when I execute the reset command -> do_reset() --> reset_cpu() ---> sysreset_walk_halt(SYSRESET_COLD); ----> loop on device UCLASS_SYSRESET -----> syscon_reboot_probe() ------> syscon_node_to_regmap(node) -------> list_for_each_entry(entry, &syscon_list, list)
I have a crash here because the pointer syscon_list.next is invalid.
This patch solves the issue by using DM list for syscon uclass.
Tested on my board, the initial issue is solved and I check the behavior with dm_dump_all(): the RCC driver is not binded/probed as syscon uclass by default (checked with dm tree) and correctly bounded and probed when it is necessary (before relocation: print_resetinfo() or after relocation for command reset).
So this is a regression we should fix, to be clear, yes? Thanks!
Yes it is a regression. It could also impact the boards using CONFIG_SYSRESET_SYSCON and CONFIG_SYSRESET.
Ragards
-- Tom
Patrick
HI Patrick,
On 30 October 2018 at 07:44, Patrick Delaunay patrick.delaunay@st.com wrote:
- Update the function syscon_node_to_regmap() to force bound on syscon uclass and directly use the list of device from DM.
- Remove the static list syscon_list.
This patch avoid issue (crash) when syscon_node_to_regmap() is called before and after reallocation (list content is invalid).
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com
Hi
This patch is a modified (after Simon Glass review) and rebased version on v2018.11-rc3 for [U-Boot] syscon: reset node list syscon_list after relocation http://patchwork.ozlabs.org/patch/983139/
This patch correct a crash see on v2018.11-rc1 with my board stm32mp1 for the command "reset", reproduced on v2018.11-rc3.
The crash is a side effect of 2 patches 1f6ca3f42f6edf143473159297f4c515b1cf36f6 sysreset: syscon: update regmap access to syscon
23471aed5c33e104d6fa64575932577618543bec board_f: Add reset status printing
With the first patch the syscon_node_to_regmap() is called each time that the class sysreset_syscon is probed.
=> in v2018.09 probe is done only when reset is requested
NB: for stm32mp1, the node rcc_reboot in tagged pre-relocated to support reset in pre-reloc phases (allow panic).
With the second patch, U-Boot probes all the sysreset uclass in preloc phase to allow to print the reset status, and the list is initialized in board_f / pre-reloc:
-> print_resetinfo --> uclass_first_device_err(UCLASS_SYSRESET, &dev); ---> syscon_reboot_probe() ----> syscon_node_to_regmap(node) -----> of_syscon_register() -------> list_add_tail(&syscon->list, &syscon_list);
During relocation, the content of syscon_list (static) is updated but the list use pointers allocated by malloc in pre-reloc phasis
And when I execute the reset command -> do_reset() --> reset_cpu() ---> sysreset_walk_halt(SYSRESET_COLD); ----> loop on device UCLASS_SYSRESET -----> syscon_reboot_probe() ------> syscon_node_to_regmap(node) -------> list_for_each_entry(entry, &syscon_list, list)
I have a crash here because the pointer syscon_list.next is invalid.
This patch solves the issue by using DM list for syscon uclass.
Tested on my board, the initial issue is solved and I check the behavior with dm_dump_all(): the RCC driver is not binded/probed as syscon uclass by default (checked with dm tree) and correctly bounded and probed when it is necessary (before relocation: print_resetinfo() or after relocation for command reset).
Regards
Patrick
drivers/core/syscon-uclass.c | 55 ++++++++++++++------------------------------ 1 file changed, 17 insertions(+), 38 deletions(-)
diff --git a/drivers/core/syscon-uclass.c b/drivers/core/syscon-uclass.c index 303e166..aeb3583 100644 --- a/drivers/core/syscon-uclass.c +++ b/drivers/core/syscon-uclass.c @@ -123,52 +123,31 @@ U_BOOT_DRIVER(generic_syscon) = {
- The syscon node can be bound to another driver, but still works
- as a syscon provider.
*/ -static LIST_HEAD(syscon_list);
-struct syscon {
ofnode node;struct regmap *regmap;struct list_head list;-};
-static struct syscon *of_syscon_register(ofnode node) +struct regmap *syscon_node_to_regmap(ofnode node) {
struct syscon *syscon;
struct udevice *dev, *parent;ofnode ofnode = node;
What is the purpose of this variable if it is always set to 'node'?
int ret;
if (!uclass_get_device_by_ofnode(UCLASS_SYSCON, node, &dev))return syscon_get_regmap(dev);if (!ofnode_device_is_compatible(node, "syscon")) return ERR_PTR(-EINVAL);
syscon = malloc(sizeof(*syscon));if (!syscon)return ERR_PTR(-ENOMEM);
if (device_find_global_by_ofnode(ofnode, &parent))parent = dm_root();
So if you fail to find the node, you use the root? Is this so that you have somewhere to 'hang' the device? I think this code could all use a few comments.
ret = regmap_init_mem(node, &syscon->regmap);if (ret) {free(syscon);
/* force bound to syscon class */ret = device_bind_driver_to_node(parent, "syscon",ofnode_get_name(node),node, &dev);if (ret) return ERR_PTR(ret);
}list_add_tail(&syscon->list, &syscon_list);return syscon;-}
-struct regmap *syscon_node_to_regmap(ofnode node) -{
struct syscon *entry, *syscon = NULL;list_for_each_entry(entry, &syscon_list, list)if (ofnode_equal(entry->node, node)) {syscon = entry;break;}if (!syscon)syscon = of_syscon_register(node);if (IS_ERR(syscon))return ERR_CAST(syscon);
ret = device_probe(dev);if (ret)return ERR_PTR(ret);
return syscon->regmap;
return syscon_get_regmap(dev);}
2.7.4
Regards, Simon
Hi Simon,
From: sjg@google.com sjg@google.com On Behalf Of Simon Glass Sent: mercredi 31 octobre 2018 01:11
HI Patrick,
On 30 October 2018 at 07:44, Patrick Delaunay patrick.delaunay@st.com wrote:
- Update the function syscon_node_to_regmap() to force bound on syscon uclass and directly use the list of device from DM.
- Remove the static list syscon_list.
This patch avoid issue (crash) when syscon_node_to_regmap() is called before and after reallocation (list content is invalid).
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com
drivers/core/syscon-uclass.c | 55 ++++++++++++++------------------------------ 1 file changed, 17 insertions(+), 38 deletions(-)
diff --git a/drivers/core/syscon-uclass.c b/drivers/core/syscon-uclass.c index 303e166..aeb3583 100644 --- a/drivers/core/syscon-uclass.c +++ b/drivers/core/syscon-uclass.c @@ -123,52 +123,31 @@ U_BOOT_DRIVER(generic_syscon) = {
- The syscon node can be bound to another driver, but still works
- as a syscon provider.
*/ -static LIST_HEAD(syscon_list);
-struct syscon {
ofnode node;struct regmap *regmap;struct list_head list;-};
-static struct syscon *of_syscon_register(ofnode node) +struct regmap *syscon_node_to_regmap(ofnode node) {
struct syscon *syscon;
struct udevice *dev, *parent;ofnode ofnode = node;What is the purpose of this variable if it is always set to 'node'?
No need, I remove it.
int ret;
if (!uclass_get_device_by_ofnode(UCLASS_SYSCON, node, &dev))return syscon_get_regmap(dev);if (!ofnode_device_is_compatible(node, "syscon")) return ERR_PTR(-EINVAL);
syscon = malloc(sizeof(*syscon));if (!syscon)return ERR_PTR(-ENOMEM);
if (device_find_global_by_ofnode(ofnode, &parent))parent = dm_root();So if you fail to find the node, you use the root? Is this so that you have somewhere to 'hang' the device? I think this code could all use a few comments.
Yes exactly. it is to avoid binding error when the node with "syscon" compatible is not associated to a other U-Boot driver. But It is more a protection, normally if the driver is not bound to other U-boot driver, it should be bound to "syscon" at least.
I will add comment.
ret = regmap_init_mem(node, &syscon->regmap);if (ret) {free(syscon);
/* force bound to syscon class */ret = device_bind_driver_to_node(parent, "syscon",ofnode_get_name(node),node, &dev);if (ret) return ERR_PTR(ret);
}list_add_tail(&syscon->list, &syscon_list);return syscon;-}
-struct regmap *syscon_node_to_regmap(ofnode node) -{
struct syscon *entry, *syscon = NULL;list_for_each_entry(entry, &syscon_list, list)if (ofnode_equal(entry->node, node)) {syscon = entry;break;}if (!syscon)syscon = of_syscon_register(node);if (IS_ERR(syscon))return ERR_CAST(syscon);
ret = device_probe(dev);if (ret)return ERR_PTR(ret);
return syscon->regmap;
return syscon_get_regmap(dev);}
2.7.4
Regards, Simon
Regards
Patrick
participants (4)
-
Patrick DELAUNAY -
Patrick Delaunay
-
Simon Glass -
Tom Rini