[U-Boot] [PATCH v2] syscon: update syscon_node_to_regmap to use the DM functions

[U-Boot] [PATCH v2] syscon: update syscon_node_to_regmap to use the DM functions

[Patrick Delaunay]

+ 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

[U-Boot] [PATCH v2] syscon: update syscon_node_to_regmap to use the DM functions

[Patrick DELAUNAY]

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

[U-Boot] [U-Boot, v2] syscon: update syscon_node_to_regmap to use the DM functions

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181217/4e6cabb5/attachment.sig>