From mboxrd@z Thu Jan  1 00:00:00 1970
From: Simon Glass <sjg@chromium.org>
Date: Thu, 18 Oct 2018 21:25:11 -0600
Subject: [U-Boot] [PATCH] syscon: reset node list syscon_list after
	relocation
In-Reply-To: <1539357985-8753-1-git-send-email-patrick.delaunay@st.com>
References: <1539357985-8753-1-git-send-email-patrick.delaunay@st.com>
Message-ID: <CAPnjgZ3UWDexQ7x1cTxkg1t+YKokS6iHrw_bswyH92+=hv21AQ@mail.gmail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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