[RFC] device-tree: resets properties, reset topology, and shared resets

[RFC] device-tree: resets properties, reset topology, and shared resets

[Geert Uytterhoeven]

	Hi Rob, Mark,

Best Wishes for 2019!

Can you please provide your opinion on parsing the whole device tree for
"resets" phandle properties to find shared resets, cfr. patch [1] below?

This is a prerequisite for safe generic reset handling for
virtualization on DT-based platforms, cfr. patch [2].

Many thanks in advance!

References:
  1. [PATCH v3] reset: Exclusive resets must be dedicated to a single
     hardware block
     https://lore.kernel.org/lkml/20181113133520.20889-1-geert+renesas@glider.be/
  2. [PATCH v5] vfio: platform: Add generic reset controller support
     https://lore.kernel.org/lkml/20181113131508.18246-1-geert+renesas@glider.be/

From 96418942472531d736dffd64ff5ae90d055d71b0 Mon Sep 17 00:00:00 2001
From: Geert Uytterhoeven <geert+renesas@glider.be>
Date: Mon, 17 Sep 2018 16:34:26 +0200
Subject: [PATCH v3] reset: Exclusive resets must be dedicated to a single
 hardware block

In some SoCs multiple hardware blocks may share a reset control.
The reset control API for shared resets will only assert such a reset
when the drivers for all hardware blocks agree.
The exclusive reset control API still allows to assert such a reset, but
that impacts all other hardware blocks sharing the reset.

While the kernel doc comments clearly state that the API for shared
resets applies to reset controls which are shared between hardware
blocks, the exact meaning of exclusive resets is not documented.
Fix the semantic ambiguity with respect to exclusive access vs.
exclusive reset lines by:
  1. Clarifying that exclusive resets really are intended for use with
     reset controls which are dedicated to a single hardware block,
  2. Ensuring that obtaining an exclusive reset control will fail if the
     reset is shared by multiple hardware blocks, for both DT-based and
     lookup-based reset controls.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Question from Philipp for the DT maintainers:

   "I'd still like to hear the device tree maintainers' opinion on
    parsing the whole DT for "resets" phandle properties to find shared
    resets like this."

Thanks!

v3:
  - Make args parameter of __of_reset_is_exclusive() a pointer,
  - Print a warning when detecting a shared reset,
  - Rebase on top of of_node_put() move,

v2:
  - Fix wrong variable in __reset_is_dedicated() loop,
  - Add missing of_node_put() in __of_reset_is_dedicated(),
  - Document that exclusive reset controls imply they are dedicated to a
    single hardware block,
  - Drop new dedicated flag and new API reset_control_get_dedicated(),
    as exclusive already implies dedicated,
  - Rename {__of_,}reset_is_dedicated() to {__of_,}reset_is_exclusive(),
  - Reword description.

Note: Exclusive lookup-based reset controls were not tested.
---
 drivers/reset/core.c  | 67 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/reset.h |  5 +++-
 2 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index bce2d6aefef98131..022740cfab9f429e 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -459,6 +459,42 @@ static void __reset_control_put_internal(struct reset_control *rstc)
 	kref_put(&rstc->refcnt, __reset_control_release);
 }
 
+static bool __of_reset_is_exclusive(const struct device_node *node,
+				    const struct of_phandle_args *args,
+				    const char *id)
+{
+	struct of_phandle_args args2;
+	struct device_node *node2;
+	int index, ret;
+	bool eq;
+
+	for_each_node_with_property(node2, "resets") {
+		if (node == node2)
+			continue;
+
+		for (index = 0; ; index++) {
+			ret = of_parse_phandle_with_args(node2, "resets",
+							 "#reset-cells", index,
+							 &args2);
+			if (ret)
+				break;
+
+			eq = (args2.np == args.np &&
+			      args2.args_count == args.args_count &&
+			      !memcmp(args2.args, args.args,
+				      args.args_count * sizeof(args.args[0])));
+			of_node_put(args2.np);
+			if (eq) {
+				pr_warn("%pOF requests exclusive control over reset %s shared with %pOF on %pOF\n",
+					node, id, node2, args->np);
+				return false;
+			}
+		}
+	}
+
+	return true;
+}
+
 struct reset_control *__of_reset_control_get(struct device_node *node,
 				     const char *id, int index, bool shared,
 				     bool optional)
@@ -513,6 +549,11 @@ struct reset_control *__of_reset_control_get(struct device_node *node,
 		goto out;
 	}
 
+	if (!shared && !__of_reset_is_exclusive(node, &args, id)) {
+		rstc = ERR_PTR(-EINVAL);
+		goto out;
+	}
+
 	/* reset_list_mutex also protects the rcdev's reset_control list */
 	rstc = __reset_control_get_internal(rcdev, rstc_id, shared);
 
@@ -542,6 +583,27 @@ __reset_controller_by_name(const char *name)
 	return NULL;
 }
 
+static bool __reset_is_exclusive(const struct reset_control_lookup *lookup)
+{
+	const struct reset_control_lookup *lookup2;
+
+	list_for_each_entry(lookup2, &reset_lookup_list, list) {
+		if (lookup2 == lookup)
+			continue;
+
+		if (lookup2->provider == lookup->provider &&
+		    lookup2->index == lookup->index) {
+			pr_warn("%s/%s requests exclusive control over reset %s:%u shared with %s/%s",
+				lookup->dev_id, lookup->con_id,
+				lookup->provider, lookup->index,
+				lookup2->dev_id, lookup2->con_id);
+			return false;
+		}
+	}
+
+	return true;
+}
+
 static struct reset_control *
 __reset_control_get_from_lookup(struct device *dev, const char *con_id,
 				bool shared, bool optional)
@@ -563,6 +625,11 @@ __reset_control_get_from_lookup(struct device *dev, const char *con_id,
 		if ((!con_id && !lookup->con_id) ||
 		    ((con_id && lookup->con_id) &&
 		     !strcmp(con_id, lookup->con_id))) {
+			if (!shared && !__reset_is_exclusive(lookup)) {
+				mutex_unlock(&reset_lookup_mutex);
+				return ERR_PTR(-EINVAL);
+			}
+
 			mutex_lock(&reset_list_mutex);
 			rcdev = __reset_controller_by_name(lookup->provider);
 			if (!rcdev) {
diff --git a/include/linux/reset.h b/include/linux/reset.h
index b7f3ad691ee9773e..2698b36bd1eb3e0c 100644
--- a/include/linux/reset.h
+++ b/include/linux/reset.h
@@ -123,8 +123,11 @@ static inline int device_reset_optional(struct device *dev)
  * @id: reset line name
  *
  * Returns a struct reset_control or IS_ERR() condition containing errno.
- * If this function is called more than once for the same reset_control it will
+ * If this function is called more than once for the same reset control it will
  * return -EBUSY.
+ * This function is intended for use with reset controls which are dedicated
+ * to a single hardware block.  If called for a reset control shared among
+ * multiple hardware blocks, it will return -EINVAL.
  *
  * See reset_control_get_shared for details on shared references to
  * reset-controls.
-- 
2.17.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [RFC] device-tree: resets properties, reset topology, and shared resets

[Geert Uytterhoeven]

On Mon, Jan 7, 2019 at 6:51 PM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -459,6 +459,42 @@ static void __reset_control_put_internal(struct reset_control *rstc)
>         kref_put(&rstc->refcnt, __reset_control_release);
>  }
>
> +static bool __of_reset_is_exclusive(const struct device_node *node,
> +                                   const struct of_phandle_args *args,
> +                                   const char *id)
> +{
> +       struct of_phandle_args args2;
> +       struct device_node *node2;
> +       int index, ret;
> +       bool eq;
> +
> +       for_each_node_with_property(node2, "resets") {
> +               if (node == node2)
> +                       continue;
> +
> +               for (index = 0; ; index++) {
> +                       ret = of_parse_phandle_with_args(node2, "resets",
> +                                                        "#reset-cells", index,
> +                                                        &args2);
> +                       if (ret)
> +                               break;
> +
> +                       eq = (args2.np == args.np &&
> +                             args2.args_count == args.args_count &&
> +                             !memcmp(args2.args, args.args,
> +                                     args.args_count * sizeof(args.args[0])));

This is embarrasing: as kbuild test robot pointed out, these should be
"args->" instead of "args.".

I failed to notice, as I have a local follow-up patch extracting this
comparison into a common helper.

Nevertheless, this doesn't change anything w.r.t. the principle behind
this patch.
Thanks again!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC] device-tree: resets properties, reset topology, and shared resets

[Geert Uytterhoeven]

Hi Rob, Mark,

Ping, can you please provide your feedback?
Even if you have no comments, please say so.

Thanks again!

On Mon, Jan 7, 2019 at 6:51 PM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
> Best Wishes for 2019!
>
> Can you please provide your opinion on parsing the whole device tree for
> "resets" phandle properties to find shared resets, cfr. patch [1] below?
>
> This is a prerequisite for safe generic reset handling for
> virtualization on DT-based platforms, cfr. patch [2].
>
> Many thanks in advance!
>
> References:
>   1. [PATCH v3] reset: Exclusive resets must be dedicated to a single
>      hardware block
>      https://lore.kernel.org/lkml/20181113133520.20889-1-geert+renesas@glider.be/
>   2. [PATCH v5] vfio: platform: Add generic reset controller support
>      https://lore.kernel.org/lkml/20181113131508.18246-1-geert+renesas@glider.be/
>
> From 96418942472531d736dffd64ff5ae90d055d71b0 Mon Sep 17 00:00:00 2001
> From: Geert Uytterhoeven <geert+renesas@glider.be>
> Date: Mon, 17 Sep 2018 16:34:26 +0200
> Subject: [PATCH v3] reset: Exclusive resets must be dedicated to a single
>  hardware block
>
> In some SoCs multiple hardware blocks may share a reset control.
> The reset control API for shared resets will only assert such a reset
> when the drivers for all hardware blocks agree.
> The exclusive reset control API still allows to assert such a reset, but
> that impacts all other hardware blocks sharing the reset.
>
> While the kernel doc comments clearly state that the API for shared
> resets applies to reset controls which are shared between hardware
> blocks, the exact meaning of exclusive resets is not documented.
> Fix the semantic ambiguity with respect to exclusive access vs.
> exclusive reset lines by:
>   1. Clarifying that exclusive resets really are intended for use with
>      reset controls which are dedicated to a single hardware block,
>   2. Ensuring that obtaining an exclusive reset control will fail if the
>      reset is shared by multiple hardware blocks, for both DT-based and
>      lookup-based reset controls.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Question from Philipp for the DT maintainers:
>
>    "I'd still like to hear the device tree maintainers' opinion on
>     parsing the whole DT for "resets" phandle properties to find shared
>     resets like this."
>
> Thanks!
>
> v3:
>   - Make args parameter of __of_reset_is_exclusive() a pointer,
>   - Print a warning when detecting a shared reset,
>   - Rebase on top of of_node_put() move,
>
> v2:
>   - Fix wrong variable in __reset_is_dedicated() loop,
>   - Add missing of_node_put() in __of_reset_is_dedicated(),
>   - Document that exclusive reset controls imply they are dedicated to a
>     single hardware block,
>   - Drop new dedicated flag and new API reset_control_get_dedicated(),
>     as exclusive already implies dedicated,
>   - Rename {__of_,}reset_is_dedicated() to {__of_,}reset_is_exclusive(),
>   - Reword description.
>
> Note: Exclusive lookup-based reset controls were not tested.
> ---
>  drivers/reset/core.c  | 67 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/reset.h |  5 +++-
>  2 files changed, 71 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index bce2d6aefef98131..022740cfab9f429e 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -459,6 +459,42 @@ static void __reset_control_put_internal(struct reset_control *rstc)
>         kref_put(&rstc->refcnt, __reset_control_release);
>  }
>
> +static bool __of_reset_is_exclusive(const struct device_node *node,
> +                                   const struct of_phandle_args *args,
> +                                   const char *id)
> +{
> +       struct of_phandle_args args2;
> +       struct device_node *node2;
> +       int index, ret;
> +       bool eq;
> +
> +       for_each_node_with_property(node2, "resets") {
> +               if (node == node2)
> +                       continue;
> +
> +               for (index = 0; ; index++) {
> +                       ret = of_parse_phandle_with_args(node2, "resets",
> +                                                        "#reset-cells", index,
> +                                                        &args2);
> +                       if (ret)
> +                               break;
> +
> +                       eq = (args2.np == args.np &&
> +                             args2.args_count == args.args_count &&
> +                             !memcmp(args2.args, args.args,
> +                                     args.args_count * sizeof(args.args[0])));
> +                       of_node_put(args2.np);
> +                       if (eq) {
> +                               pr_warn("%pOF requests exclusive control over reset %s shared with %pOF on %pOF\n",
> +                                       node, id, node2, args->np);
> +                               return false;
> +                       }
> +               }
> +       }
> +
> +       return true;
> +}
> +
>  struct reset_control *__of_reset_control_get(struct device_node *node,
>                                      const char *id, int index, bool shared,
>                                      bool optional)
> @@ -513,6 +549,11 @@ struct reset_control *__of_reset_control_get(struct device_node *node,
>                 goto out;
>         }
>
> +       if (!shared && !__of_reset_is_exclusive(node, &args, id)) {
> +               rstc = ERR_PTR(-EINVAL);
> +               goto out;
> +       }
> +
>         /* reset_list_mutex also protects the rcdev's reset_control list */
>         rstc = __reset_control_get_internal(rcdev, rstc_id, shared);
>
> @@ -542,6 +583,27 @@ __reset_controller_by_name(const char *name)
>         return NULL;
>  }
>
> +static bool __reset_is_exclusive(const struct reset_control_lookup *lookup)
> +{
> +       const struct reset_control_lookup *lookup2;
> +
> +       list_for_each_entry(lookup2, &reset_lookup_list, list) {
> +               if (lookup2 == lookup)
> +                       continue;
> +
> +               if (lookup2->provider == lookup->provider &&
> +                   lookup2->index == lookup->index) {
> +                       pr_warn("%s/%s requests exclusive control over reset %s:%u shared with %s/%s",
> +                               lookup->dev_id, lookup->con_id,
> +                               lookup->provider, lookup->index,
> +                               lookup2->dev_id, lookup2->con_id);
> +                       return false;
> +               }
> +       }
> +
> +       return true;
> +}
> +
>  static struct reset_control *
>  __reset_control_get_from_lookup(struct device *dev, const char *con_id,
>                                 bool shared, bool optional)
> @@ -563,6 +625,11 @@ __reset_control_get_from_lookup(struct device *dev, const char *con_id,
>                 if ((!con_id && !lookup->con_id) ||
>                     ((con_id && lookup->con_id) &&
>                      !strcmp(con_id, lookup->con_id))) {
> +                       if (!shared && !__reset_is_exclusive(lookup)) {
> +                               mutex_unlock(&reset_lookup_mutex);
> +                               return ERR_PTR(-EINVAL);
> +                       }
> +
>                         mutex_lock(&reset_list_mutex);
>                         rcdev = __reset_controller_by_name(lookup->provider);
>                         if (!rcdev) {
> diff --git a/include/linux/reset.h b/include/linux/reset.h
> index b7f3ad691ee9773e..2698b36bd1eb3e0c 100644
> --- a/include/linux/reset.h
> +++ b/include/linux/reset.h
> @@ -123,8 +123,11 @@ static inline int device_reset_optional(struct device *dev)
>   * @id: reset line name
>   *
>   * Returns a struct reset_control or IS_ERR() condition containing errno.
> - * If this function is called more than once for the same reset_control it will
> + * If this function is called more than once for the same reset control it will
>   * return -EBUSY.
> + * This function is intended for use with reset controls which are dedicated
> + * to a single hardware block.  If called for a reset control shared among
> + * multiple hardware blocks, it will return -EINVAL.
>   *
>   * See reset_control_get_shared for details on shared references to
>   * reset-controls.
> --
> 2.17.1

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds