| Message ID | 1539357985-8753-1-git-send-email-patrick.delaunay@st.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | [U-Boot] syscon: reset node list syscon_list after relocation | expand |
Hi Patrick, On 12 October 2018 at 09:26, Patrick Delaunay <patrick.delaunay@st.com> wrote: > Reset the list head after the reallocation because the list syscon_list > use allocated pointer and they are no more valid. > This patch avoid issue (crash) when syscon_node_to_regmap() is called > before and after reallocation. > > Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com> > --- > Hi > > This patch correct a crash see on v2018.11-rc1 with my board stm32mp1 > for the command "reset". > > 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) We have a similar issue with the timer - we set gd->timer to NULL in initr_dm(). I am not 100% what is going on here, but using pre-reloc devices (i.e. which were set up before relocation) after relocation is bad. We need to avoid that. The syscon_list struct should be in the syscon uclass, i.e. declared as uclass priv data in UCLASS_DRIVER(syscon). We should not have this as free-standard static data. That's not how DM works... So I guess Masahiro's patch needs to be adjusted. Regards, Simon
Hi Simon, > From: sjg@google.com <sjg@google.com> On Behalf Of Simon Glass > Sent: vendredi 19 octobre 2018 05:25 > > Hi Patrick, > > On 12 October 2018 at 09:26, Patrick Delaunay <patrick.delaunay@st.com> > wrote: > > Reset the list head after the reallocation because the list > > syscon_list use allocated pointer and they are no more valid. > > This patch avoid issue (crash) when syscon_node_to_regmap() is called > > before and after reallocation. > > > > Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com> > > --- > > Hi > > > > This patch correct a crash see on v2018.11-rc1 with my board stm32mp1 > > for the command "reset". > > > > 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) > > We have a similar issue with the timer - we set gd->timer to NULL in initr_dm(). > > I am not 100% what is going on here, but using pre-reloc devices (i.e. > which were set up before relocation) after relocation is bad. We need to avoid > that. Ok, I agree with you. > The syscon_list struct should be in the syscon uclass, i.e. declared as uclass priv > data in UCLASS_DRIVER(syscon). We should not have this as free-standard static > data. That's not how DM works... > So I guess Masahiro's patch needs to be adjusted. Today, I read your comment and I start to move the list in syscon uclass priv data (I just discovered the uclass priv data). But after some time spent with this list, I prefer completely remove it and have the same behavior with DM functions (as a list is already managed for the drivers). It is more that only a adjustment, I completely update the function syscon_node_to_regmap(): force bound to syscon driver and probe it when the syscon driver don't yet exist..... => Do you think that is a good solution ? The function change to: struct regmap *syscon_node_to_regmap(ofnode node) { struct udevice *dev; struct udevice *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); if (device_find_global_by_ofnode(ofnode, &parent)) parent = dm_root(); /* 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); ret = device_probe(dev); if (ret) return ERR_PTR(ret); return syscon_get_regmap(dev); } Tested on my board, the initial issue is sole and I check the behavior with dm_dump_all(); It seems OK for my case (RCC driver) . before relocation => syscon is correctly probed Class index Probed Driver Name ----------------------------------------- root 0 [ + ] root_drive root_driver misc 0 [ ] stm32mp_bs |-- stm32mp_bsec simple_bus 0 [ + ] generic_si |-- soc serial 0 [ + ] serial_stm | |-- serial@40010000 misc 1 [ + ] stm32-rcc | |-- rcc@50000000 clk 0 [ + ] stm32mp1_c | | |-- stm32mp1_clk reset 0 [ ] stm32_rcc_ | | |-- stm32_rcc_reset syscon 1 [ + ] syscon | | `-- rcc@50000000 <<<< SYSCON PROBED sysreset 0 [ + ] syscon_reb | |-- rcc-reboot@50000000 mmc 0 [ ] stm32_sdmm | |-- sdmmc@58005000 blk 0 [ ] mmc_blk | | `-- sdmmc@58005000.blk And it is also the case for syscon reset (when I execute the command reset) STM32MP> reset resetting ... STM32MP> reset resetting ... Class index Probed Driver Name ----------------------------------------- root 0 [ + ] root_drive root_driver misc 0 [ + ] stm32mp_bs |-- stm32mp_bsec simple_bus 0 [ + ] generic_si |-- soc serial 0 [ + ] serial_stm | |-- serial@40010000 i2c 0 [ ] stm32f7-i2 | |-- i2c@40013000 i2c 1 [ ] stm32f7-i2 | |-- i2c@40015000 usb 0 [ ] dwc2_usb | |-- usb-otg@49000000 misc 1 [ + ] stm32-rcc | |-- rcc@50000000 clk 0 [ + ] stm32mp1_c | | |-- stm32mp1_clk reset 0 [ + ] stm32_rcc_ | | |-- stm32_rcc_reset syscon 4 [ + ] syscon | | `-- rcc@50000000 <<<<< SYSCON sysreset 0 [ + ] syscon_reb | |-- rcc-reboot@50000000 And the driver is not binded/probed by default (checked with dm tree) STM32MP> dm tree Class index Probed Driver Name ----------------------------------------- root 0 [ + ] root_drive root_driver misc 0 [ + ] stm32mp_bs |-- stm32mp_bsec simple_bus 0 [ + ] generic_si |-- soc serial 0 [ + ] serial_stm | |-- serial@40010000 i2c 0 [ ] stm32f7-i2 | |-- i2c@40013000 i2c 1 [ ] stm32f7-i2 | |-- i2c@40015000 usb 0 [ ] dwc2_usb | |-- usb-otg@49000000 misc 1 [ + ] stm32-rcc | |-- rcc@50000000 <<<<< NO SYSCON child clk 0 [ + ] stm32mp1_c | | |-- stm32mp1_clk reset 0 [ + ] stm32_rcc_ | | `-- stm32_rcc_reset sysreset 0 [ ] syscon_reb | |-- rcc-reboot@50000000 syscon 0 [ ] stmp32mp_s | |-- pwr@50001000 > Regards, > Simon Regards Patrick
diff --git a/drivers/core/syscon-uclass.c b/drivers/core/syscon-uclass.c index 303e166..aacb43a 100644 --- a/drivers/core/syscon-uclass.c +++ b/drivers/core/syscon-uclass.c @@ -156,8 +156,15 @@ static struct syscon *of_syscon_register(ofnode node) struct regmap *syscon_node_to_regmap(ofnode node) { + static int reloc_done; struct syscon *entry, *syscon = NULL; + /* content of list is not relocated (malloc): reinit when needed */ + if (!reloc_done && (gd->flags & GD_FLG_RELOC)) { + INIT_LIST_HEAD(&syscon_list); + reloc_done++; + } + list_for_each_entry(entry, &syscon_list, list) if (ofnode_equal(entry->node, node)) { syscon = entry;
Reset the list head after the reallocation because the list syscon_list use allocated pointer and they are no more valid. This patch avoid issue (crash) when syscon_node_to_regmap() is called before and after reallocation. Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com> --- Hi This patch correct a crash see on v2018.11-rc1 with my board stm32mp1 for the command "reset". 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. I solve the issue by resetting the list after reloc and it is working but I don't know if a more elegant solution exist (keep the list in .bss section to be set to 0). Regards Patrick drivers/core/syscon-uclass.c | 7 +++++++ 1 file changed, 7 insertions(+)