Message ID | 1541000943-24251-1-git-send-email-patrick.delaunay@st.com |
---|---|
State | Accepted |
Commit | de5bab9c59807b109f89a39855c9eacfe4d08822 |
Delegated to: | Tom Rini |
Headers | show |
Series | [U-Boot,v2] syscon: update syscon_node_to_regmap to use the DM functions | expand |
Hi Tom, > From: Patrick DELAUNAY > Sent: mercredi 31 octobre 2018 16:49 > > Subject: [PATCH v2] 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 > > > Changes in v2: > - minor update after Simon review (remove variable ofnode > and add one comment) > > 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..3864f13 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; > 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); > + /* bound to driver with same ofnode or to root if not found */ > + if (device_find_global_by_ofnode(node, &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); > } > -- > 2.7.4 Juste a little reminder because I don't see this patch in v2019.01-rc1. http://patchwork.ozlabs.org/patch/991519/ That fix a regression on stm32mp1 board detected lately since v2018.11-rc1. It could also impact the other boards using CONFIG_SYSRESET_SYSCON and CONFIG_SYSRESET. Tell me if I can do anything on my side, you need v3 with rebase. Patrick
On Wed, Oct 31, 2018 at 04:49:03PM +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> Applied to u-boot/master, thanks!
diff --git a/drivers/core/syscon-uclass.c b/drivers/core/syscon-uclass.c index 303e166..3864f13 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; 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); + /* bound to driver with same ofnode or to root if not found */ + if (device_find_global_by_ofnode(node, &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 Changes in v2: - minor update after Simon review (remove variable ofnode and add one comment) drivers/core/syscon-uclass.c | 55 ++++++++++++++------------------------------ 1 file changed, 17 insertions(+), 38 deletions(-)