| Message ID | 1540907069-18958-1-git-send-email-patrick.delaunay@st.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [U-Boot] syscon: update syscon_node_to_regmap to use the DM functions | expand |
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
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); }
+ 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(-)