[PATCH/RFC v4 0/2] vfio: platform: Add generic reset controller support

[PATCH/RFC v4 0/2] vfio: platform: Add generic reset controller support

[Geert Uytterhoeven]

	Hi all,

This patch series implements generic reset controller support for
vfio-platform, for devices that are connected to an SoC-internal reset
controller and can be reset in a generic way.  This avoids having to
write/change a vfio-specific reset driver for each and every device to be
passed-through to a guest.

It consists of two patches:
  - The first patch enhances the reset controller subsystem to add support
    for dedicated resets.
    This addresses the issue that even reset_control_get_exclusive() does
    not guarantee that we have an exclusive wire between the device and
    the reset controller.
  - The second patch enhances vfio-platform to add generic reset controller
    support.

Changes compared to v3:
  - Add Reviewed-by,
  - New patch "reset: Add support for dedicated reset controls",
  - Use new RFC reset_control_get_dedicated() instead of
    of_reset_control_get_exclusive(), to (a) make it more generic, and
    (b) make sure the reset returned is really a dedicated reset, and
    does not affect other devices,
  - Drop vfio-platform reset leak fix, which has been applied.

Changes compared to v2:
  - Add Reviewed-by,
  - Merge similar checks in vfio_platform_has_reset().

Changes compared to v1:
  - Add Reviewed-by,
  - Don't store error values in vdev->reset_control,
  - Use of_reset_control_get_exclusive() instead of
    __of_reset_control_get(),
  - Improve description.

This has been tested on R-Car H3/Salvator-XS:
  - SATA can be exported, as it has a dedicated reset,
  - PWM1 cannot, as its reset is shared with all other PWM modules:
    "VFIO: No reset function found for device e6e31000.pwm".

Thanks for your comments!

Geert Uytterhoeven (2):
  [RFC] reset: Add support for dedicated reset controls
  [RFC] vfio: platform: Add generic reset controller support

 drivers/reset/core.c                          | 76 ++++++++++++++++---
 drivers/vfio/platform/vfio_platform_common.c  | 20 ++++-
 drivers/vfio/platform/vfio_platform_private.h |  1 +
 include/linux/reset.h                         | 60 ++++++++++-----
 4 files changed, 126 insertions(+), 31 deletions(-)

-- 
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

[PATCH/RFC v4 1/2] reset: Add support for dedicated reset controls

[Geert Uytterhoeven]

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

Sometimes a driver needs to reset a specific hardware block, and be 100%
sure it has no impact on other hardware blocks.  This is e.g. the case
for virtualization with device pass-through, where the host wants to
reset any exported device before and after exporting it for use by the
guest, for isolation.

Hence a new flag for dedicated resets is added to the internal methods,
with a new public reset_control_get_dedicated() method, to obtain an
exclusive handle to a reset that is dedicated to one specific hardware
block.

This supports both DT-based and lookup-based reset controls.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v4:
  - New.

Notes:
  - Dedicated lookup-based reset controls were not tested,
  - Several internal functions now take 3 boolean flags, and should
    probably be converted to take a bitmask instead,
  - I think __device_reset() should call __reset_control_get() with
    dedicated=true.  However, that will impact existing users,
  - Should a different error than -EINVAL be returned on failure?
---
 drivers/reset/core.c  | 76 ++++++++++++++++++++++++++++++++++++++-----
 include/linux/reset.h | 60 ++++++++++++++++++++++------------
 2 files changed, 107 insertions(+), 29 deletions(-)

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index 225e34c56b94a2e3..5bc4eeca70c0fcc2 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -459,9 +459,38 @@ static void __reset_control_put_internal(struct reset_control *rstc)
 	kref_put(&rstc->refcnt, __reset_control_release);
 }
 
+static bool __of_reset_is_dedicated(const struct device_node *node,
+				    const struct of_phandle_args args)
+{
+	struct of_phandle_args args2;
+	struct device_node *node2;
+	int index, ret;
+
+	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;
+
+			if (args2.np == args.np &&
+			    args2.args_count == args.args_count &&
+			    !memcmp(args2.args, args.args,
+				    args.args_count * sizeof(args.args[0])))
+				return false;
+		}
+	}
+
+	return true;
+}
+
 struct reset_control *__of_reset_control_get(struct device_node *node,
 				     const char *id, int index, bool shared,
-				     bool optional)
+				     bool optional, bool dedicated)
 {
 	struct reset_control *rstc;
 	struct reset_controller_dev *r, *rcdev;
@@ -514,6 +543,11 @@ struct reset_control *__of_reset_control_get(struct device_node *node,
 		return ERR_PTR(rstc_id);
 	}
 
+	if (dedicated && !__of_reset_is_dedicated(node, args)) {
+		mutex_unlock(&reset_list_mutex);
+		return ERR_PTR(-EINVAL);
+	}
+
 	/* reset_list_mutex also protects the rcdev's reset_control list */
 	rstc = __reset_control_get_internal(rcdev, rstc_id, shared);
 
@@ -541,9 +575,25 @@ __reset_controller_by_name(const char *name)
 	return NULL;
 }
 
+static bool __reset_is_dedicated(const struct reset_control_lookup *lookup)
+{
+	const struct reset_control_lookup *lookup2;
+
+	list_for_each_entry(lookup, &reset_lookup_list, list) {
+		if (lookup2 == lookup)
+			continue;
+
+		if (lookup2->provider == lookup->provider &&
+		    lookup2->index == lookup->index)
+			return false;
+	}
+
+	return true;
+}
+
 static struct reset_control *
 __reset_control_get_from_lookup(struct device *dev, const char *con_id,
-				bool shared, bool optional)
+				bool shared, bool optional, bool dedicated)
 {
 	const struct reset_control_lookup *lookup;
 	struct reset_controller_dev *rcdev;
@@ -562,6 +612,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 (dedicated && !__reset_is_dedicated(lookup)) {
+				mutex_unlock(&reset_lookup_mutex);
+				return ERR_PTR(-EINVAL);
+			}
+
 			mutex_lock(&reset_list_mutex);
 			rcdev = __reset_controller_by_name(lookup->provider);
 			if (!rcdev) {
@@ -588,13 +643,15 @@ __reset_control_get_from_lookup(struct device *dev, const char *con_id,
 }
 
 struct reset_control *__reset_control_get(struct device *dev, const char *id,
-					  int index, bool shared, bool optional)
+					  int index, bool shared,
+					  bool optional, bool dedicated)
 {
 	if (dev->of_node)
 		return __of_reset_control_get(dev->of_node, id, index, shared,
-					      optional);
+					      optional, dedicated);
 
-	return __reset_control_get_from_lookup(dev, id, shared, optional);
+	return __reset_control_get_from_lookup(dev, id, shared, optional,
+					       dedicated);
 }
 EXPORT_SYMBOL_GPL(__reset_control_get);
 
@@ -635,7 +692,7 @@ static void devm_reset_control_release(struct device *dev, void *res)
 
 struct reset_control *__devm_reset_control_get(struct device *dev,
 				     const char *id, int index, bool shared,
-				     bool optional)
+				     bool optional, bool dedicated)
 {
 	struct reset_control **ptr, *rstc;
 
@@ -644,7 +701,7 @@ struct reset_control *__devm_reset_control_get(struct device *dev,
 	if (!ptr)
 		return ERR_PTR(-ENOMEM);
 
-	rstc = __reset_control_get(dev, id, index, shared, optional);
+	rstc = __reset_control_get(dev, id, index, shared, optional, dedicated);
 	if (!IS_ERR(rstc)) {
 		*ptr = rstc;
 		devres_add(dev, ptr);
@@ -671,7 +728,7 @@ int __device_reset(struct device *dev, bool optional)
 	struct reset_control *rstc;
 	int ret;
 
-	rstc = __reset_control_get(dev, NULL, 0, 0, optional);
+	rstc = __reset_control_get(dev, NULL, 0, false, optional, false);
 	if (IS_ERR(rstc))
 		return PTR_ERR(rstc);
 
@@ -735,7 +792,8 @@ of_reset_control_array_get(struct device_node *np, bool shared, bool optional)
 		return ERR_PTR(-ENOMEM);
 
 	for (i = 0; i < num; i++) {
-		rstc = __of_reset_control_get(np, NULL, i, shared, optional);
+		rstc = __of_reset_control_get(np, NULL, i, shared, optional,
+					      false);
 		if (IS_ERR(rstc))
 			goto err_rst;
 		resets->rstc[i] = rstc;
diff --git a/include/linux/reset.h b/include/linux/reset.h
index 09732c36f3515a1e..6ca6e108b612f923 100644
--- a/include/linux/reset.h
+++ b/include/linux/reset.h
@@ -17,15 +17,15 @@ int reset_control_status(struct reset_control *rstc);
 
 struct reset_control *__of_reset_control_get(struct device_node *node,
 				     const char *id, int index, bool shared,
-				     bool optional);
+				     bool optional, bool dedicated);
 struct reset_control *__reset_control_get(struct device *dev, const char *id,
 					  int index, bool shared,
-					  bool optional);
+					  bool optional, bool dedicated);
 void reset_control_put(struct reset_control *rstc);
 int __device_reset(struct device *dev, bool optional);
 struct reset_control *__devm_reset_control_get(struct device *dev,
 				     const char *id, int index, bool shared,
-				     bool optional);
+				     bool optional, bool dedicated);
 
 struct reset_control *devm_reset_control_array_get(struct device *dev,
 						   bool shared, bool optional);
@@ -66,21 +66,23 @@ static inline int __device_reset(struct device *dev, bool optional)
 static inline struct reset_control *__of_reset_control_get(
 					struct device_node *node,
 					const char *id, int index, bool shared,
-					bool optional)
+					bool optional, bool dedicated)
 {
 	return optional ? NULL : ERR_PTR(-ENOTSUPP);
 }
 
 static inline struct reset_control *__reset_control_get(
 					struct device *dev, const char *id,
-					int index, bool shared, bool optional)
+					int index, bool shared, bool optional,
+					bool dedicated)
 {
 	return optional ? NULL : ERR_PTR(-ENOTSUPP);
 }
 
 static inline struct reset_control *__devm_reset_control_get(
 					struct device *dev, const char *id,
-					int index, bool shared, bool optional)
+					int index, bool shared, bool optional,
+					bool dedicated)
 {
 	return optional ? NULL : ERR_PTR(-ENOTSUPP);
 }
@@ -127,7 +129,25 @@ static inline int device_reset_optional(struct device *dev)
 static inline struct reset_control *
 __must_check reset_control_get_exclusive(struct device *dev, const char *id)
 {
-	return __reset_control_get(dev, id, 0, false, false);
+	return __reset_control_get(dev, id, 0, false, false, false);
+}
+
+/**
+ * reset_control_get_dedicated - Lookup and obtain an exclusive reference
+ *                               to a dedicated reset controller.
+ * @dev: device to be reset by the controller
+ * @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
+ * return -EBUSY.
+ *
+ * Use of id names is optional.
+ */
+static inline struct reset_control *
+__must_check reset_control_get_dedicated(struct device *dev, const char *id)
+{
+	return __reset_control_get(dev, id, 0, false, false, true);
 }
 
 /**
@@ -155,19 +175,19 @@ __must_check reset_control_get_exclusive(struct device *dev, const char *id)
 static inline struct reset_control *reset_control_get_shared(
 					struct device *dev, const char *id)
 {
-	return __reset_control_get(dev, id, 0, true, false);
+	return __reset_control_get(dev, id, 0, true, false, false);
 }
 
 static inline struct reset_control *reset_control_get_optional_exclusive(
 					struct device *dev, const char *id)
 {
-	return __reset_control_get(dev, id, 0, false, true);
+	return __reset_control_get(dev, id, 0, false, true, false);
 }
 
 static inline struct reset_control *reset_control_get_optional_shared(
 					struct device *dev, const char *id)
 {
-	return __reset_control_get(dev, id, 0, true, true);
+	return __reset_control_get(dev, id, 0, true, true, false);
 }
 
 /**
@@ -183,7 +203,7 @@ static inline struct reset_control *reset_control_get_optional_shared(
 static inline struct reset_control *of_reset_control_get_exclusive(
 				struct device_node *node, const char *id)
 {
-	return __of_reset_control_get(node, id, 0, false, false);
+	return __of_reset_control_get(node, id, 0, false, false, false);
 }
 
 /**
@@ -208,7 +228,7 @@ static inline struct reset_control *of_reset_control_get_exclusive(
 static inline struct reset_control *of_reset_control_get_shared(
 				struct device_node *node, const char *id)
 {
-	return __of_reset_control_get(node, id, 0, true, false);
+	return __of_reset_control_get(node, id, 0, true, false, false);
 }
 
 /**
@@ -225,7 +245,7 @@ static inline struct reset_control *of_reset_control_get_shared(
 static inline struct reset_control *of_reset_control_get_exclusive_by_index(
 					struct device_node *node, int index)
 {
-	return __of_reset_control_get(node, NULL, index, false, false);
+	return __of_reset_control_get(node, NULL, index, false, false, false);
 }
 
 /**
@@ -253,7 +273,7 @@ static inline struct reset_control *of_reset_control_get_exclusive_by_index(
 static inline struct reset_control *of_reset_control_get_shared_by_index(
 					struct device_node *node, int index)
 {
-	return __of_reset_control_get(node, NULL, index, true, false);
+	return __of_reset_control_get(node, NULL, index, true, false, false);
 }
 
 /**
@@ -272,7 +292,7 @@ static inline struct reset_control *
 __must_check devm_reset_control_get_exclusive(struct device *dev,
 					      const char *id)
 {
-	return __devm_reset_control_get(dev, id, 0, false, false);
+	return __devm_reset_control_get(dev, id, 0, false, false, false);
 }
 
 /**
@@ -287,19 +307,19 @@ __must_check devm_reset_control_get_exclusive(struct device *dev,
 static inline struct reset_control *devm_reset_control_get_shared(
 					struct device *dev, const char *id)
 {
-	return __devm_reset_control_get(dev, id, 0, true, false);
+	return __devm_reset_control_get(dev, id, 0, true, false, false);
 }
 
 static inline struct reset_control *devm_reset_control_get_optional_exclusive(
 					struct device *dev, const char *id)
 {
-	return __devm_reset_control_get(dev, id, 0, false, true);
+	return __devm_reset_control_get(dev, id, 0, false, true, false);
 }
 
 static inline struct reset_control *devm_reset_control_get_optional_shared(
 					struct device *dev, const char *id)
 {
-	return __devm_reset_control_get(dev, id, 0, true, true);
+	return __devm_reset_control_get(dev, id, 0, true, true, false);
 }
 
 /**
@@ -317,7 +337,7 @@ static inline struct reset_control *devm_reset_control_get_optional_shared(
 static inline struct reset_control *
 devm_reset_control_get_exclusive_by_index(struct device *dev, int index)
 {
-	return __devm_reset_control_get(dev, NULL, index, false, false);
+	return __devm_reset_control_get(dev, NULL, index, false, false, false);
 }
 
 /**
@@ -333,7 +353,7 @@ devm_reset_control_get_exclusive_by_index(struct device *dev, int index)
 static inline struct reset_control *
 devm_reset_control_get_shared_by_index(struct device *dev, int index)
 {
-	return __devm_reset_control_get(dev, NULL, index, true, false);
+	return __devm_reset_control_get(dev, NULL, index, true, false, false);
 }
 
 /*
-- 
2.17.1

Re: [PATCH/RFC v4 1/2] reset: Add support for dedicated reset controls

[Geert Uytterhoeven]

On Mon, Sep 17, 2018 at 6:40 PM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
> In some SoCs multiple hardware blocks may share a reset control.
> The existing reset control API for shared resets will only assert such a
> reset when the drivers for all hardware blocks agree.
> The existing exclusive reset control API still allows to assert such a
> reset, but that impacts all other hardware blocks sharing the reset.
>
> Sometimes a driver needs to reset a specific hardware block, and be 100%
> sure it has no impact on other hardware blocks.  This is e.g. the case
> for virtualization with device pass-through, where the host wants to
> reset any exported device before and after exporting it for use by the
> guest, for isolation.
>
> Hence a new flag for dedicated resets is added to the internal methods,
> with a new public reset_control_get_dedicated() method, to obtain an
> exclusive handle to a reset that is dedicated to one specific hardware
> block.
>
> This supports both DT-based and lookup-based reset controls.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v4:
>   - New.
>
> Notes:
>   - Dedicated lookup-based reset controls were not tested,

And untested code is buggy...

> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c

> @@ -541,9 +575,25 @@ __reset_controller_by_name(const char *name)
>         return NULL;
>  }
>
> +static bool __reset_is_dedicated(const struct reset_control_lookup *lookup)
> +{
> +       const struct reset_control_lookup *lookup2;
> +
> +       list_for_each_entry(lookup, &reset_lookup_list, list) {

... as the list_for_each() should use "lookup2" instead of "lookup" (warning
reported by kbuild robot).

> +               if (lookup2 == lookup)
> +                       continue;
> +
> +               if (lookup2->provider == lookup->provider &&
> +                   lookup2->index == lookup->index)
> +                       return false;
> +       }
> +
> +       return true;
> +}

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: [PATCH/RFC v4 1/2] reset: Add support for dedicated reset controls

[Auger Eric]

Hi Geert,

On 9/17/18 6:39 PM, Geert Uytterhoeven wrote:
> In some SoCs multiple hardware blocks may share a reset control.
> The existing reset control API for shared resets will only assert such a
> reset when the drivers for all hardware blocks agree.
> The existing exclusive reset control API still allows to assert such a
> reset, but that impacts all other hardware blocks sharing the reset.
> 
> Sometimes a driver needs to reset a specific hardware block, and be 100%
> sure it has no impact on other hardware blocks.  This is e.g. the case
> for virtualization with device pass-through, where the host wants to
> reset any exported device before and after exporting it for use by the
> guest, for isolation.
> 
> Hence a new flag for dedicated resets is added to the internal methods,
> with a new public reset_control_get_dedicated() method, to obtain an
> exclusive handle to a reset that is dedicated to one specific hardware
> block.
> 
> This supports both DT-based and lookup-based reset controls.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v4:
>   - New.
> 
> Notes:
>   - Dedicated lookup-based reset controls were not tested,
>   - Several internal functions now take 3 boolean flags, and should
>     probably be converted to take a bitmask instead,
>   - I think __device_reset() should call __reset_control_get() with
>     dedicated=true.  However, that will impact existing users,
why should it?
>   - Should a different error than -EINVAL be returned on failure?
> ---
>  drivers/reset/core.c  | 76 ++++++++++++++++++++++++++++++++++++++-----
>  include/linux/reset.h | 60 ++++++++++++++++++++++------------
>  2 files changed, 107 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index 225e34c56b94a2e3..5bc4eeca70c0fcc2 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -459,9 +459,38 @@ static void __reset_control_put_internal(struct reset_control *rstc)
>  	kref_put(&rstc->refcnt, __reset_control_release);
>  }
>  
> +static bool __of_reset_is_dedicated(const struct device_node *node,
> +				    const struct of_phandle_args args)
> +{
> +	struct of_phandle_args args2;
> +	struct device_node *node2;
> +	int index, ret;
> +
> +	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;
> +
> +			if (args2.np == args.np &&
> +			    args2.args_count == args.args_count &&
> +			    !memcmp(args2.args, args.args,
> +				    args.args_count * sizeof(args.args[0])))
> +				return false;
You need to call of_node_put(args2.np) (see of_parse_phandle_with_args
kernel doc)

Isn't it sufficient to check device_node handles are equal?

Thanks

Eric

> +		}
> +	}
> +
> +	return true;
> +}
> +
>  struct reset_control *__of_reset_control_get(struct device_node *node,
>  				     const char *id, int index, bool shared,
> -				     bool optional)
> +				     bool optional, bool dedicated)
>  {
>  	struct reset_control *rstc;
>  	struct reset_controller_dev *r, *rcdev;
> @@ -514,6 +543,11 @@ struct reset_control *__of_reset_control_get(struct device_node *node,
>  		return ERR_PTR(rstc_id);
>  	}
>  
> +	if (dedicated && !__of_reset_is_dedicated(node, args)) {
> +		mutex_unlock(&reset_list_mutex);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
>  	/* reset_list_mutex also protects the rcdev's reset_control list */
>  	rstc = __reset_control_get_internal(rcdev, rstc_id, shared);
>  
> @@ -541,9 +575,25 @@ __reset_controller_by_name(const char *name)
>  	return NULL;
>  }
>  
> +static bool __reset_is_dedicated(const struct reset_control_lookup *lookup)
> +{
> +	const struct reset_control_lookup *lookup2;
> +
> +	list_for_each_entry(lookup, &reset_lookup_list, list) {
> +		if (lookup2 == lookup)
> +			continue;
> +
> +		if (lookup2->provider == lookup->provider &&
> +		    lookup2->index == lookup->index)
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
>  static struct reset_control *
>  __reset_control_get_from_lookup(struct device *dev, const char *con_id,
> -				bool shared, bool optional)
> +				bool shared, bool optional, bool dedicated)
>  {
>  	const struct reset_control_lookup *lookup;
>  	struct reset_controller_dev *rcdev;
> @@ -562,6 +612,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 (dedicated && !__reset_is_dedicated(lookup)) {
> +				mutex_unlock(&reset_lookup_mutex);
> +				return ERR_PTR(-EINVAL);
> +			}
> +
>  			mutex_lock(&reset_list_mutex);
>  			rcdev = __reset_controller_by_name(lookup->provider);
>  			if (!rcdev) {
> @@ -588,13 +643,15 @@ __reset_control_get_from_lookup(struct device *dev, const char *con_id,
>  }
>  
>  struct reset_control *__reset_control_get(struct device *dev, const char *id,
> -					  int index, bool shared, bool optional)
> +					  int index, bool shared,
> +					  bool optional, bool dedicated)
>  {
>  	if (dev->of_node)
>  		return __of_reset_control_get(dev->of_node, id, index, shared,
> -					      optional);
> +					      optional, dedicated);
>  
> -	return __reset_control_get_from_lookup(dev, id, shared, optional);
> +	return __reset_control_get_from_lookup(dev, id, shared, optional,
> +					       dedicated);
>  }
>  EXPORT_SYMBOL_GPL(__reset_control_get);
>  
> @@ -635,7 +692,7 @@ static void devm_reset_control_release(struct device *dev, void *res)
>  
>  struct reset_control *__devm_reset_control_get(struct device *dev,
>  				     const char *id, int index, bool shared,
> -				     bool optional)
> +				     bool optional, bool dedicated)
>  {
>  	struct reset_control **ptr, *rstc;
>  
> @@ -644,7 +701,7 @@ struct reset_control *__devm_reset_control_get(struct device *dev,
>  	if (!ptr)
>  		return ERR_PTR(-ENOMEM);
>  
> -	rstc = __reset_control_get(dev, id, index, shared, optional);
> +	rstc = __reset_control_get(dev, id, index, shared, optional, dedicated);
>  	if (!IS_ERR(rstc)) {
>  		*ptr = rstc;
>  		devres_add(dev, ptr);
> @@ -671,7 +728,7 @@ int __device_reset(struct device *dev, bool optional)
>  	struct reset_control *rstc;
>  	int ret;
>  
> -	rstc = __reset_control_get(dev, NULL, 0, 0, optional);
> +	rstc = __reset_control_get(dev, NULL, 0, false, optional, false);
>  	if (IS_ERR(rstc))
>  		return PTR_ERR(rstc);
>  
> @@ -735,7 +792,8 @@ of_reset_control_array_get(struct device_node *np, bool shared, bool optional)
>  		return ERR_PTR(-ENOMEM);
>  
>  	for (i = 0; i < num; i++) {
> -		rstc = __of_reset_control_get(np, NULL, i, shared, optional);
> +		rstc = __of_reset_control_get(np, NULL, i, shared, optional,
> +					      false);
>  		if (IS_ERR(rstc))
>  			goto err_rst;
>  		resets->rstc[i] = rstc;
> diff --git a/include/linux/reset.h b/include/linux/reset.h
> index 09732c36f3515a1e..6ca6e108b612f923 100644
> --- a/include/linux/reset.h
> +++ b/include/linux/reset.h
> @@ -17,15 +17,15 @@ int reset_control_status(struct reset_control *rstc);
>  
>  struct reset_control *__of_reset_control_get(struct device_node *node,
>  				     const char *id, int index, bool shared,
> -				     bool optional);
> +				     bool optional, bool dedicated);
>  struct reset_control *__reset_control_get(struct device *dev, const char *id,
>  					  int index, bool shared,
> -					  bool optional);
> +					  bool optional, bool dedicated);
>  void reset_control_put(struct reset_control *rstc);
>  int __device_reset(struct device *dev, bool optional);
>  struct reset_control *__devm_reset_control_get(struct device *dev,
>  				     const char *id, int index, bool shared,
> -				     bool optional);
> +				     bool optional, bool dedicated);
>  
>  struct reset_control *devm_reset_control_array_get(struct device *dev,
>  						   bool shared, bool optional);
> @@ -66,21 +66,23 @@ static inline int __device_reset(struct device *dev, bool optional)
>  static inline struct reset_control *__of_reset_control_get(
>  					struct device_node *node,
>  					const char *id, int index, bool shared,
> -					bool optional)
> +					bool optional, bool dedicated)
>  {
>  	return optional ? NULL : ERR_PTR(-ENOTSUPP);
>  }
>  
>  static inline struct reset_control *__reset_control_get(
>  					struct device *dev, const char *id,
> -					int index, bool shared, bool optional)
> +					int index, bool shared, bool optional,
> +					bool dedicated)
>  {
>  	return optional ? NULL : ERR_PTR(-ENOTSUPP);
>  }
>  
>  static inline struct reset_control *__devm_reset_control_get(
>  					struct device *dev, const char *id,
> -					int index, bool shared, bool optional)
> +					int index, bool shared, bool optional,
> +					bool dedicated)
>  {
>  	return optional ? NULL : ERR_PTR(-ENOTSUPP);
>  }
> @@ -127,7 +129,25 @@ static inline int device_reset_optional(struct device *dev)
>  static inline struct reset_control *
>  __must_check reset_control_get_exclusive(struct device *dev, const char *id)
>  {
> -	return __reset_control_get(dev, id, 0, false, false);
> +	return __reset_control_get(dev, id, 0, false, false, false);
> +}
> +
> +/**
> + * reset_control_get_dedicated - Lookup and obtain an exclusive reference
> + *                               to a dedicated reset controller.
> + * @dev: device to be reset by the controller
> + * @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
> + * return -EBUSY.
> + *
> + * Use of id names is optional.
> + */
> +static inline struct reset_control *
> +__must_check reset_control_get_dedicated(struct device *dev, const char *id)
> +{
> +	return __reset_control_get(dev, id, 0, false, false, true);
>  }
>  
>  /**
> @@ -155,19 +175,19 @@ __must_check reset_control_get_exclusive(struct device *dev, const char *id)
>  static inline struct reset_control *reset_control_get_shared(
>  					struct device *dev, const char *id)
>  {
> -	return __reset_control_get(dev, id, 0, true, false);
> +	return __reset_control_get(dev, id, 0, true, false, false);
>  }
>  
>  static inline struct reset_control *reset_control_get_optional_exclusive(
>  					struct device *dev, const char *id)
>  {
> -	return __reset_control_get(dev, id, 0, false, true);
> +	return __reset_control_get(dev, id, 0, false, true, false);
>  }
>  
>  static inline struct reset_control *reset_control_get_optional_shared(
>  					struct device *dev, const char *id)
>  {
> -	return __reset_control_get(dev, id, 0, true, true);
> +	return __reset_control_get(dev, id, 0, true, true, false);
>  }
>  
>  /**
> @@ -183,7 +203,7 @@ static inline struct reset_control *reset_control_get_optional_shared(
>  static inline struct reset_control *of_reset_control_get_exclusive(
>  				struct device_node *node, const char *id)
>  {
> -	return __of_reset_control_get(node, id, 0, false, false);
> +	return __of_reset_control_get(node, id, 0, false, false, false);
>  }
>  
>  /**
> @@ -208,7 +228,7 @@ static inline struct reset_control *of_reset_control_get_exclusive(
>  static inline struct reset_control *of_reset_control_get_shared(
>  				struct device_node *node, const char *id)
>  {
> -	return __of_reset_control_get(node, id, 0, true, false);
> +	return __of_reset_control_get(node, id, 0, true, false, false);
>  }
>  
>  /**
> @@ -225,7 +245,7 @@ static inline struct reset_control *of_reset_control_get_shared(
>  static inline struct reset_control *of_reset_control_get_exclusive_by_index(
>  					struct device_node *node, int index)
>  {
> -	return __of_reset_control_get(node, NULL, index, false, false);
> +	return __of_reset_control_get(node, NULL, index, false, false, false);
>  }
>  
>  /**
> @@ -253,7 +273,7 @@ static inline struct reset_control *of_reset_control_get_exclusive_by_index(
>  static inline struct reset_control *of_reset_control_get_shared_by_index(
>  					struct device_node *node, int index)
>  {
> -	return __of_reset_control_get(node, NULL, index, true, false);
> +	return __of_reset_control_get(node, NULL, index, true, false, false);
>  }
>  
>  /**
> @@ -272,7 +292,7 @@ static inline struct reset_control *
>  __must_check devm_reset_control_get_exclusive(struct device *dev,
>  					      const char *id)
>  {
> -	return __devm_reset_control_get(dev, id, 0, false, false);
> +	return __devm_reset_control_get(dev, id, 0, false, false, false);
>  }
>  
>  /**
> @@ -287,19 +307,19 @@ __must_check devm_reset_control_get_exclusive(struct device *dev,
>  static inline struct reset_control *devm_reset_control_get_shared(
>  					struct device *dev, const char *id)
>  {
> -	return __devm_reset_control_get(dev, id, 0, true, false);
> +	return __devm_reset_control_get(dev, id, 0, true, false, false);
>  }
>  
>  static inline struct reset_control *devm_reset_control_get_optional_exclusive(
>  					struct device *dev, const char *id)
>  {
> -	return __devm_reset_control_get(dev, id, 0, false, true);
> +	return __devm_reset_control_get(dev, id, 0, false, true, false);
>  }
>  
>  static inline struct reset_control *devm_reset_control_get_optional_shared(
>  					struct device *dev, const char *id)
>  {
> -	return __devm_reset_control_get(dev, id, 0, true, true);
> +	return __devm_reset_control_get(dev, id, 0, true, true, false);
>  }
>  
>  /**
> @@ -317,7 +337,7 @@ static inline struct reset_control *devm_reset_control_get_optional_shared(
>  static inline struct reset_control *
>  devm_reset_control_get_exclusive_by_index(struct device *dev, int index)
>  {
> -	return __devm_reset_control_get(dev, NULL, index, false, false);
> +	return __devm_reset_control_get(dev, NULL, index, false, false, false);
>  }
>  
>  /**
> @@ -333,7 +353,7 @@ devm_reset_control_get_exclusive_by_index(struct device *dev, int index)
>  static inline struct reset_control *
>  devm_reset_control_get_shared_by_index(struct device *dev, int index)
>  {
> -	return __devm_reset_control_get(dev, NULL, index, true, false);
> +	return __devm_reset_control_get(dev, NULL, index, true, false, false);
>  }
>  
>  /*
> 

Re: [PATCH/RFC v4 1/2] reset: Add support for dedicated reset controls

[Geert Uytterhoeven]

Hi Eric,

On Wed, Sep 19, 2018 at 2:09 PM Auger Eric <eric.auger@redhat.com> wrote:
> On 9/17/18 6:39 PM, Geert Uytterhoeven wrote:
> > In some SoCs multiple hardware blocks may share a reset control.
> > The existing reset control API for shared resets will only assert such a
> > reset when the drivers for all hardware blocks agree.
> > The existing exclusive reset control API still allows to assert such a
> > reset, but that impacts all other hardware blocks sharing the reset.
> >
> > Sometimes a driver needs to reset a specific hardware block, and be 100%
> > sure it has no impact on other hardware blocks.  This is e.g. the case
> > for virtualization with device pass-through, where the host wants to
> > reset any exported device before and after exporting it for use by the
> > guest, for isolation.
> >
> > Hence a new flag for dedicated resets is added to the internal methods,
> > with a new public reset_control_get_dedicated() method, to obtain an
> > exclusive handle to a reset that is dedicated to one specific hardware
> > block.
> >
> > This supports both DT-based and lookup-based reset controls.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > v4:
> >   - New.
> >
> > Notes:
> >   - Dedicated lookup-based reset controls were not tested,
> >   - Several internal functions now take 3 boolean flags, and should
> >     probably be converted to take a bitmask instead,
> >   - I think __device_reset() should call __reset_control_get() with
> >     dedicated=true.  However, that will impact existing users,
>
> why should it?

device_reset{,_optional}() are supposed to reset the passed device.
If the reset is not dedicated, doing so will reset other devices, too.

> > --- a/drivers/reset/core.c
> > +++ b/drivers/reset/core.c
> > @@ -459,9 +459,38 @@ static void __reset_control_put_internal(struct reset_control *rstc)
> >       kref_put(&rstc->refcnt, __reset_control_release);
> >  }
> >
> > +static bool __of_reset_is_dedicated(const struct device_node *node,
> > +                                 const struct of_phandle_args args)
> > +{
> > +     struct of_phandle_args args2;
> > +     struct device_node *node2;
> > +     int index, ret;
> > +
> > +     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;
> > +
> > +                     if (args2.np == args.np &&
> > +                         args2.args_count == args.args_count &&
> > +                         !memcmp(args2.args, args.args,
> > +                                 args.args_count * sizeof(args.args[0])))
> > +                             return false;
> You need to call of_node_put(args2.np) (see of_parse_phandle_with_args
> kernel doc)

Thanks, nice catch!

> Isn't it sufficient to check device_node handles are equal?

That would make it work with #reset-cells == 0 only.
If #reset-cells > 0, the reset line specifier includes extra arguments.

On the Renesas SoCs I'm using, there's a single reset controller, so
args.np is always the same.  The actual reset line is specified by
args.args[0].  See the "resets" properties in e.g.
https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git/tree/arch/arm64/boot/dts/renesas/r8a7795.dtsi

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: [PATCH/RFC v4 1/2] reset: Add support for dedicated reset controls

[Auger Eric]

Hi Geert,

On 9/19/18 3:16 PM, Geert Uytterhoeven wrote:
> Hi Eric,
> 
> On Wed, Sep 19, 2018 at 2:09 PM Auger Eric <eric.auger@redhat.com> wrote:
>> On 9/17/18 6:39 PM, Geert Uytterhoeven wrote:
>>> In some SoCs multiple hardware blocks may share a reset control.
>>> The existing reset control API for shared resets will only assert such a
>>> reset when the drivers for all hardware blocks agree.
>>> The existing exclusive reset control API still allows to assert such a
>>> reset, but that impacts all other hardware blocks sharing the reset.
>>>
>>> Sometimes a driver needs to reset a specific hardware block, and be 100%
>>> sure it has no impact on other hardware blocks.  This is e.g. the case
>>> for virtualization with device pass-through, where the host wants to
>>> reset any exported device before and after exporting it for use by the
>>> guest, for isolation.
>>>
>>> Hence a new flag for dedicated resets is added to the internal methods,
>>> with a new public reset_control_get_dedicated() method, to obtain an
>>> exclusive handle to a reset that is dedicated to one specific hardware
>>> block.
>>>
>>> This supports both DT-based and lookup-based reset controls.
>>>
>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>> ---
>>> v4:
>>>   - New.
>>>
>>> Notes:
>>>   - Dedicated lookup-based reset controls were not tested,
>>>   - Several internal functions now take 3 boolean flags, and should
>>>     probably be converted to take a bitmask instead,
>>>   - I think __device_reset() should call __reset_control_get() with
>>>     dedicated=true.  However, that will impact existing users,
>>
>> why should it?
> 
> device_reset{,_optional}() are supposed to reset the passed device.
> If the reset is not dedicated, doing so will reset other devices, too.
ok, that's not obvious too me but I am not familiar enough with the API
and existing callers.
> 
>>> --- a/drivers/reset/core.c
>>> +++ b/drivers/reset/core.c
>>> @@ -459,9 +459,38 @@ static void __reset_control_put_internal(struct reset_control *rstc)
>>>       kref_put(&rstc->refcnt, __reset_control_release);
>>>  }
>>>
>>> +static bool __of_reset_is_dedicated(const struct device_node *node,
>>> +                                 const struct of_phandle_args args)
>>> +{
>>> +     struct of_phandle_args args2;
>>> +     struct device_node *node2;
>>> +     int index, ret;
>>> +
>>> +     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;
>>> +
>>> +                     if (args2.np == args.np &&
>>> +                         args2.args_count == args.args_count &&
>>> +                         !memcmp(args2.args, args.args,
>>> +                                 args.args_count * sizeof(args.args[0])))
>>> +                             return false;
>> You need to call of_node_put(args2.np) (see of_parse_phandle_with_args
>> kernel doc)
> 
> Thanks, nice catch!
> 
>> Isn't it sufficient to check device_node handles are equal?
> 
> That would make it work with #reset-cells == 0 only.
> If #reset-cells > 0, the reset line specifier includes extra arguments.
> 
> On the Renesas SoCs I'm using, there's a single reset controller, so
> args.np is always the same.  The actual reset line is specified by
> args.args[0].  See the "resets" properties in e.g.
> https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git/tree/arch/arm64/boot/dts/renesas/r8a7795.dtsi
OK get it now. Thank you for the explanations.

Best Regards

Eric
> 
> 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: [PATCH/RFC v4 1/2] reset: Add support for dedicated reset controls

[Philipp Zabel]

On Mon, 2018-09-17 at 18:39 +0200, Geert Uytterhoeven wrote:
> In some SoCs multiple hardware blocks may share a reset control.
> The existing reset control API for shared resets will only assert such a
> reset when the drivers for all hardware blocks agree.
> The existing exclusive reset control API still allows to assert such a
> reset, but that impacts all other hardware blocks sharing the reset.

I consider requesting exclusive access to a shared reset line a misuse
of the API. Are there such cases? Can they be fixed?

> Sometimes a driver needs to reset a specific hardware block, and be 100%
> sure it has no impact on other hardware blocks.  This is e.g. the case
> for virtualization with device pass-through, where the host wants to
> reset any exported device before and after exporting it for use by the
> guest, for isolation.
> 
> Hence a new flag for dedicated resets is added to the internal methods,
> with a new public reset_control_get_dedicated() method, to obtain an
> exclusive handle to a reset that is dedicated to one specific hardware
> block.

I'm not sure a new flag is necessary, this is what exclusive resets
should be.
Also I fear there will be confusion about the difference between
exclusive (refering to the reset control) and dedicated (refering to
the reset line) reset controls.

> This supports both DT-based and lookup-based reset controls.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v4:
>   - New.
> 
> Notes:
>   - Dedicated lookup-based reset controls were not tested,
>   - Several internal functions now take 3 boolean flags, and should
>     probably be converted to take a bitmask instead,

In case we have to add more flags, yes.

>   - I think __device_reset() should call __reset_control_get() with
>     dedicated=true.  However, that will impact existing users,

Which ones? And how?

>   - Should a different error than -EINVAL be returned on failure?
> ---
>  drivers/reset/core.c  | 76 ++++++++++++++++++++++++++++++++++++++-----
>  include/linux/reset.h | 60 ++++++++++++++++++++++------------
>  2 files changed, 107 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index 225e34c56b94a2e3..5bc4eeca70c0fcc2 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -459,9 +459,38 @@ static void __reset_control_put_internal(struct reset_control *rstc)
>  	kref_put(&rstc->refcnt, __reset_control_release);
>  }
>  
> +static bool __of_reset_is_dedicated(const struct device_node *node,
> +				    const struct of_phandle_args args)
> +{
> +	struct of_phandle_args args2;
> +	struct device_node *node2;
> +	int index, ret;
> +
> +	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;
> +
> +			if (args2.np == args.np &&
> +			    args2.args_count == args.args_count &&
> +			    !memcmp(args2.args, args.args,
> +				    args.args_count * sizeof(args.args[0])))
> +				return false;
> +		}
> +	}
> +
> +	return true;
> +}

I want to hear the device tree maintainers' opinion about this.
I'd very much like to have such a check for exclusive resets, but my
understanding is that we are not allowed to make the assumption that
other nodes' "reset" properties follow the common reset signal device
tree bindings.

Maybe this is something that should be checked in a device tree
validation step?

regards
Philipp

Re: [PATCH/RFC v4 1/2] reset: Add support for dedicated reset controls

[Geert Uytterhoeven]

Hi Philipp,

CC Mark & Rob (DT)

On Wed, Sep 19, 2018 at 4:58 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> On Mon, 2018-09-17 at 18:39 +0200, Geert Uytterhoeven wrote:
> > In some SoCs multiple hardware blocks may share a reset control.
> > The existing reset control API for shared resets will only assert such a
> > reset when the drivers for all hardware blocks agree.
> > The existing exclusive reset control API still allows to assert such a
> > reset, but that impacts all other hardware blocks sharing the reset.
>
> I consider requesting exclusive access to a shared reset line a misuse
> of the API. Are there such cases? Can they be fixed?

I guess there are plenty.  Whether a line is shared or dedicated depends
on SoC integration.

The issue is that a driver requesting exclusive access has no way to know
if the reset line is dedicated to its device or not.  If no other
driver requested
the reset control (most drivers don't use reset controls), it will succeed.

> > Sometimes a driver needs to reset a specific hardware block, and be 100%
> > sure it has no impact on other hardware blocks.  This is e.g. the case
> > for virtualization with device pass-through, where the host wants to
> > reset any exported device before and after exporting it for use by the
> > guest, for isolation.
> >
> > Hence a new flag for dedicated resets is added to the internal methods,
> > with a new public reset_control_get_dedicated() method, to obtain an
> > exclusive handle to a reset that is dedicated to one specific hardware
> > block.
>
> I'm not sure a new flag is necessary, this is what exclusive resets
> should be.

So perhaps the check should be done for the existing exclusive resets
instead, without adding a new flag?

> Also I fear there will be confusion about the difference between
> exclusive (refering to the reset control) and dedicated (refering to
> the reset line) reset controls.

Indeed, exclusive has multiple meanings here:
  1. Exclusive vs. shared access to the reset control,
  2. Reset line is dedicated to a single device, or shared with multiple
     devices.

If we can simplify (exclusive == dedicated, 1.shared == 2.shared), life
can become simpler.

> >   - I think __device_reset() should call __reset_control_get() with
> >     dedicated=true.  However, that will impact existing users,
>
> Which ones? And how?

I didn't actually check which drivers.
If a reset is not dedicated, device_reset{,_optional}() will suddenly
start to fail if
a reset turns out to be not dedicated.
Well, currently the device will be reset multiple times, so people would
already have noticed...

> >   - Should a different error than -EINVAL be returned on failure?
> > ---
> >  drivers/reset/core.c  | 76 ++++++++++++++++++++++++++++++++++++++-----
> >  include/linux/reset.h | 60 ++++++++++++++++++++++------------
> >  2 files changed, 107 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> > index 225e34c56b94a2e3..5bc4eeca70c0fcc2 100644
> > --- a/drivers/reset/core.c
> > +++ b/drivers/reset/core.c
> > @@ -459,9 +459,38 @@ static void __reset_control_put_internal(struct reset_control *rstc)
> >       kref_put(&rstc->refcnt, __reset_control_release);
> >  }
> >
> > +static bool __of_reset_is_dedicated(const struct device_node *node,
> > +                                 const struct of_phandle_args args)
> > +{
> > +     struct of_phandle_args args2;
> > +     struct device_node *node2;
> > +     int index, ret;
> > +
> > +     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;
> > +
> > +                     if (args2.np == args.np &&
> > +                         args2.args_count == args.args_count &&
> > +                         !memcmp(args2.args, args.args,
> > +                                 args.args_count * sizeof(args.args[0])))
> > +                             return false;
> > +             }
> > +     }
> > +
> > +     return true;
> > +}
>
> I want to hear the device tree maintainers' opinion about this.
> I'd very much like to have such a check for exclusive resets, but my
> understanding is that we are not allowed to make the assumption that
> other nodes' "reset" properties follow the common reset signal device
> tree bindings.
>
> Maybe this is something that should be checked in a device tree
> validation step?

We already have SoCs where reset lines are shared among multiple on-chip
devices. So dtc validation won't work, and a runtime check is needed.

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: [PATCH/RFC v4 1/2] reset: Add support for dedicated reset controls

[Philipp Zabel]

Hi Geert,

On Wed, 2018-09-19 at 17:24 +0200, Geert Uytterhoeven wrote:
> On Wed, Sep 19, 2018 at 4:58 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
[...]
> > I consider requesting exclusive access to a shared reset line a misuse
> > of the API. Are there such cases? Can they be fixed?
> 
> I guess there are plenty.

I did a cursory search for drivers that request exclusive reset controls
for resets that have multiple phandle references in the corresponding
DT. So far I have found none.

> Whether a line is shared or dedicated depends on SoC integration.
>
> The issue is that a driver requesting exclusive access has no way to know
> if the reset line is dedicated to its device or not. If no other
> driver requested the reset control (most drivers don't use reset
> controls), it will succeed.

True. It would be great to have a way to make sure an exclusive request
for a shared reset line never succeeds.

> > > Sometimes a driver needs to reset a specific hardware block, and be 100%
> > > sure it has no impact on other hardware blocks.  This is e.g. the case
> > > for virtualization with device pass-through, where the host wants to
> > > reset any exported device before and after exporting it for use by the
> > > guest, for isolation.
> > > 
> > > Hence a new flag for dedicated resets is added to the internal methods,
> > > with a new public reset_control_get_dedicated() method, to obtain an
> > > exclusive handle to a reset that is dedicated to one specific hardware
> > > block.
> > 
> > I'm not sure a new flag is necessary, this is what exclusive resets
> > should be.
> 
> So perhaps the check should be done for the existing exclusive resets
> instead, without adding a new flag?

That would be my preference, if possible.

> > Also I fear there will be confusion about the difference between
> > exclusive (refering to the reset control) and dedicated (refering to
> > the reset line) reset controls.
> 
> Indeed, exclusive has multiple meanings here:
>   1. Exclusive vs. shared access to the reset control,
>   2. Reset line is dedicated to a single device, or shared with multiple
>      devices.

2. is the more important factor, and that's what I have in mind when
talking about exclusive vs. shared resets. Admittedly, the kernel doc
comments only mention 1.

> If we can simplify (exclusive == dedicated, 1.shared == 2.shared), life
> can become simpler.

Well, it still has to be possible for drivers to request 1.shared
control over a dedicated reset line, just because the same driver may
work on multiple SoCs, only some of which have that reset line 2.shared.
But if we could make sure that exclusive requests are only possible for
dedicated reset lines, I'd be happier.

> > >   - I think __device_reset() should call __reset_control_get() with
> > >     dedicated=true.  However, that will impact existing users,
> > 
> > Which ones? And how?
> 
> I didn't actually check which drivers.
> If a reset is not dedicated, device_reset{,_optional}() will suddenly
> start to fail if
> a reset turns out to be not dedicated.
> Well, currently the device will be reset multiple times, so people would
> already have noticed...

Exactly. Naive exclusive control, as currently implemented, is bound to
fail if the reset line is shared. I am not aware of any cases where this
currently happens.
Of course there could always be those fragile cases where something just
works by accident and lucky timing.

[...]
> > I want to hear the device tree maintainers' opinion about this.
> > I'd very much like to have such a check for exclusive resets, but my
> > understanding is that we are not allowed to make the assumption that
> > other nodes' "reset" properties follow the common reset signal device
> > tree bindings.
> > 
> > Maybe this is something that should be checked in a device tree
> > validation step?
> 
> We already have SoCs where reset lines are shared among multiple on-chip
> devices. So dtc validation won't work, and a runtime check is needed.

Right, the dtb would have to be validated against driver code to get the
information whether a given phandle might be requested exclusively at
some point.

It would be better if this could be checked at runtime.

regards
Philipp

Re: [PATCH/RFC v4 1/2] reset: Add support for dedicated reset controls

[Geert Uytterhoeven]

Hi Philip,

On Thu, Sep 20, 2018 at 11:27 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> On Wed, 2018-09-19 at 17:24 +0200, Geert Uytterhoeven wrote:
> > On Wed, Sep 19, 2018 at 4:58 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> [...]
> > > I consider requesting exclusive access to a shared reset line a misuse
> > > of the API. Are there such cases? Can they be fixed?
> >
> > I guess there are plenty.
>
> I did a cursory search for drivers that request exclusive reset controls
> for resets that have multiple phandle references in the corresponding
> DT. So far I have found none.

OK.

> > Whether a line is shared or dedicated depends on SoC integration.
> >
> > The issue is that a driver requesting exclusive access has no way to know
> > if the reset line is dedicated to its device or not. If no other
> > driver requested the reset control (most drivers don't use reset
> > controls), it will succeed.
>
> True. It would be great to have a way to make sure an exclusive request
> for a shared reset line never succeeds.
>
> > > > Sometimes a driver needs to reset a specific hardware block, and be 100%
> > > > sure it has no impact on other hardware blocks.  This is e.g. the case
> > > > for virtualization with device pass-through, where the host wants to
> > > > reset any exported device before and after exporting it for use by the
> > > > guest, for isolation.
> > > >
> > > > Hence a new flag for dedicated resets is added to the internal methods,
> > > > with a new public reset_control_get_dedicated() method, to obtain an
> > > > exclusive handle to a reset that is dedicated to one specific hardware
> > > > block.
> > >
> > > I'm not sure a new flag is necessary, this is what exclusive resets
> > > should be.
> >
> > So perhaps the check should be done for the existing exclusive resets
> > instead, without adding a new flag?
>
> That would be my preference, if possible.

OK, will make it so.

> > > Also I fear there will be confusion about the difference between
> > > exclusive (refering to the reset control) and dedicated (refering to
> > > the reset line) reset controls.
> >
> > Indeed, exclusive has multiple meanings here:
> >   1. Exclusive vs. shared access to the reset control,
> >   2. Reset line is dedicated to a single device, or shared with multiple
> >      devices.
>
> 2. is the more important factor, and that's what I have in mind when
> talking about exclusive vs. shared resets. Admittedly, the kernel doc
> comments only mention 1.

OK. That did contribute to my confustion...

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

[PATCH/RFC v4 2/2] vfio: platform: Add generic reset controller support

[Geert Uytterhoeven]

Vfio-platform requires dedicated reset support, provided either by ACPI,
or, on DT platforms, by a device-specific reset driver matching against
the device's compatible value.

On many SoCs, devices are connected to an SoC-internal reset controller.
If the reset hierarchy is described in DT using "resets" properties, or
in lookup tables in platform code, such devices can be reset in a
generic way through the reset controller subsystem.  Hence add support
for this, avoiding the need to write device-specific reset drivers for
each single device on affected SoCs.

Devices that do require a more complex reset procedure can still provide
a device-specific reset driver, as that takes precedence.

Note that this functionality depends on CONFIG_RESET_CONTROLLER=y, and
becomes a no-op (as in: "No reset function found for device") if reset
controller support is disabled.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
---
v4:
  - Add Reviewed-by,
  - Use new RFC reset_control_get_dedicated() instead of
    of_reset_control_get_exclusive(), to (a) make it more generic, and
    (b) make sure the reset returned is really a dedicated reset, and
    does not affect other devices,

v3:
  - Add Reviewed-by,
  - Merge similar checks in vfio_platform_has_reset(),

v2:
  - Don't store error values in vdev->reset_control,
  - Use of_reset_control_get_exclusive() instead of
    __of_reset_control_get(),
  - Improve description.

---
 drivers/vfio/platform/vfio_platform_common.c  | 20 +++++++++++++++++--
 drivers/vfio/platform/vfio_platform_private.h |  1 +
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index c0cd824be2b767be..eb77fe87f3663e3e 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -18,6 +18,7 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/pm_runtime.h>
+#include <linux/reset.h>
 #include <linux/slab.h>
 #include <linux/types.h>
 #include <linux/uaccess.h>
@@ -113,11 +114,13 @@ static bool vfio_platform_has_reset(struct vfio_platform_device *vdev)
 	if (VFIO_PLATFORM_IS_ACPI(vdev))
 		return vfio_platform_acpi_has_reset(vdev);
 
-	return vdev->of_reset ? true : false;
+	return vdev->of_reset || vdev->reset_control;
 }
 
 static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
 {
+	struct reset_control *rstc;
+
 	if (VFIO_PLATFORM_IS_ACPI(vdev))
 		return vfio_platform_acpi_has_reset(vdev) ? 0 : -ENOENT;
 
@@ -128,8 +131,16 @@ static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
 		vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
 							&vdev->reset_module);
 	}
+	if (vdev->of_reset)
+		return 0;
+
+	rstc = reset_control_get_dedicated(vdev->device, NULL);
+	if (!IS_ERR(rstc)) {
+		vdev->reset_control = rstc;
+		return 0;
+	}
 
-	return vdev->of_reset ? 0 : -ENOENT;
+	return PTR_ERR(rstc);
 }
 
 static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
@@ -139,6 +150,8 @@ static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
 
 	if (vdev->of_reset)
 		module_put(vdev->reset_module);
+
+	reset_control_put(vdev->reset_control);
 }
 
 static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
@@ -218,6 +231,9 @@ static int vfio_platform_call_reset(struct vfio_platform_device *vdev,
 	} else if (vdev->of_reset) {
 		dev_info(vdev->device, "reset\n");
 		return vdev->of_reset(vdev);
+	} else if (vdev->reset_control) {
+		dev_info(vdev->device, "reset\n");
+		return reset_control_reset(vdev->reset_control);
 	}
 
 	dev_warn(vdev->device, "no reset function found!\n");
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index 85ffe5d9d1abd94e..a56e80ae5986540b 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -60,6 +60,7 @@ struct vfio_platform_device {
 	const char			*compat;
 	const char			*acpihid;
 	struct module			*reset_module;
+	struct reset_control		*reset_control;
 	struct device			*device;
 
 	/*
-- 
2.17.1

Re: [PATCH/RFC v4 2/2] vfio: platform: Add generic reset controller support

[Auger Eric]

Hi Geert,

On 9/17/18 6:39 PM, Geert Uytterhoeven wrote:
> Vfio-platform requires dedicated reset support, provided either by ACPI,
> or, on DT platforms, by a device-specific reset driver matching against
> the device's compatible value.
> 
> On many SoCs, devices are connected to an SoC-internal reset controller.
> If the reset hierarchy is described in DT using "resets" properties, or
> in lookup tables in platform code, such devices can be reset in a
> generic way through the reset controller subsystem.  Hence add support
> for this, avoiding the need to write device-specific reset drivers for
> each single device on affected SoCs.
> 
> Devices that do require a more complex reset procedure can still provide
> a device-specific reset driver, as that takes precedence.
> 
> Note that this functionality depends on CONFIG_RESET_CONTROLLER=y, and
> becomes a no-op (as in: "No reset function found for device") if reset
> controller support is disabled.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
> Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
> ---
> v4:
>   - Add Reviewed-by,
>   - Use new RFC reset_control_get_dedicated() instead of
>     of_reset_control_get_exclusive(), to (a) make it more generic, and
>     (b) make sure the reset returned is really a dedicated reset, and
>     does not affect other devices,
> 
> v3:
>   - Add Reviewed-by,
>   - Merge similar checks in vfio_platform_has_reset(),
> 
> v2:
>   - Don't store error values in vdev->reset_control,
>   - Use of_reset_control_get_exclusive() instead of
>     __of_reset_control_get(),
>   - Improve description.
> 
> ---
>  drivers/vfio/platform/vfio_platform_common.c  | 20 +++++++++++++++++--
>  drivers/vfio/platform/vfio_platform_private.h |  1 +
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> index c0cd824be2b767be..eb77fe87f3663e3e 100644
> --- a/drivers/vfio/platform/vfio_platform_common.c
> +++ b/drivers/vfio/platform/vfio_platform_common.c
> @@ -18,6 +18,7 @@
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/reset.h>
>  #include <linux/slab.h>
>  #include <linux/types.h>
>  #include <linux/uaccess.h>
> @@ -113,11 +114,13 @@ static bool vfio_platform_has_reset(struct vfio_platform_device *vdev)
>  	if (VFIO_PLATFORM_IS_ACPI(vdev))
>  		return vfio_platform_acpi_has_reset(vdev);
>  
> -	return vdev->of_reset ? true : false;
> +	return vdev->of_reset || vdev->reset_control;
>  }
>  
>  static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
>  {
> +	struct reset_control *rstc;
> +
>  	if (VFIO_PLATFORM_IS_ACPI(vdev))
>  		return vfio_platform_acpi_has_reset(vdev) ? 0 : -ENOENT;
>  
> @@ -128,8 +131,16 @@ static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
>  		vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
>  							&vdev->reset_module);
>  	}
> +	if (vdev->of_reset)
> +		return 0;
> +
> +	rstc = reset_control_get_dedicated(vdev->device, NULL);
> +	if (!IS_ERR(rstc)) {
> +		vdev->reset_control = rstc;
> +		return 0;
> +	}
>  
> -	return vdev->of_reset ? 0 : -ENOENT;
> +	return PTR_ERR(rstc);
This changes the returned value as seen by the user (probe returned
valud). Can we keep -ENOENT in case of no reset controller found?

Otherwise looks good to me with the new "dedicated" reset semantics.

Thanks

Eric
>  }
>  
>  static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
> @@ -139,6 +150,8 @@ static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
>  
>  	if (vdev->of_reset)
>  		module_put(vdev->reset_module);
> +
> +	reset_control_put(vdev->reset_control);
>  }
>  
>  static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
> @@ -218,6 +231,9 @@ static int vfio_platform_call_reset(struct vfio_platform_device *vdev,
>  	} else if (vdev->of_reset) {
>  		dev_info(vdev->device, "reset\n");
>  		return vdev->of_reset(vdev);
> +	} else if (vdev->reset_control) {
> +		dev_info(vdev->device, "reset\n");
> +		return reset_control_reset(vdev->reset_control);
>  	}
>  
>  	dev_warn(vdev->device, "no reset function found!\n");
> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
> index 85ffe5d9d1abd94e..a56e80ae5986540b 100644
> --- a/drivers/vfio/platform/vfio_platform_private.h
> +++ b/drivers/vfio/platform/vfio_platform_private.h
> @@ -60,6 +60,7 @@ struct vfio_platform_device {
>  	const char			*compat;
>  	const char			*acpihid;
>  	struct module			*reset_module;
> +	struct reset_control		*reset_control;
>  	struct device			*device;
>  
>  	/*
> 

Re: [PATCH/RFC v4 2/2] vfio: platform: Add generic reset controller support

[Geert Uytterhoeven]

Hi Eric,

On Wed, Sep 19, 2018 at 2:36 PM Auger Eric <eric.auger@redhat.com> wrote:
> On 9/17/18 6:39 PM, Geert Uytterhoeven wrote:
> > Vfio-platform requires dedicated reset support, provided either by ACPI,
> > or, on DT platforms, by a device-specific reset driver matching against
> > the device's compatible value.
> >
> > On many SoCs, devices are connected to an SoC-internal reset controller.
> > If the reset hierarchy is described in DT using "resets" properties, or
> > in lookup tables in platform code, such devices can be reset in a
> > generic way through the reset controller subsystem.  Hence add support
> > for this, avoiding the need to write device-specific reset drivers for
> > each single device on affected SoCs.
> >
> > Devices that do require a more complex reset procedure can still provide
> > a device-specific reset driver, as that takes precedence.
> >
> > Note that this functionality depends on CONFIG_RESET_CONTROLLER=y, and
> > becomes a no-op (as in: "No reset function found for device") if reset
> > controller support is disabled.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
> > Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

> > --- a/drivers/vfio/platform/vfio_platform_common.c
> > +++ b/drivers/vfio/platform/vfio_platform_common.c

> > @@ -128,8 +131,16 @@ static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
> >               vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
> >                                                       &vdev->reset_module);
> >       }
> > +     if (vdev->of_reset)
> > +             return 0;
> > +
> > +     rstc = reset_control_get_dedicated(vdev->device, NULL);
> > +     if (!IS_ERR(rstc)) {
> > +             vdev->reset_control = rstc;
> > +             return 0;
> > +     }
> >
> > -     return vdev->of_reset ? 0 : -ENOENT;
> > +     return PTR_ERR(rstc);
> This changes the returned value as seen by the user (probe returned
> valud). Can we keep -ENOENT in case of no reset controller found?

On success, it still returns 0.
On failure, it forwards the error from reset_control_get_dedicated(), which
is IMHO better than replacing it by -ENOENT: we try to propagate error
codes as much as possible.  It could e.g. return -EPROBE_DEFER.

Is there anything that relies on the function returning -ENOENT?

> Otherwise looks good to me with the new "dedicated" reset semantics.

Thanks!

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: [PATCH/RFC v4 2/2] vfio: platform: Add generic reset controller support

[Auger Eric]

Hi Geert,

On 9/19/18 2:54 PM, Geert Uytterhoeven wrote:
> Hi Eric,
> 
> On Wed, Sep 19, 2018 at 2:36 PM Auger Eric <eric.auger@redhat.com> wrote:
>> On 9/17/18 6:39 PM, Geert Uytterhoeven wrote:
>>> Vfio-platform requires dedicated reset support, provided either by ACPI,
>>> or, on DT platforms, by a device-specific reset driver matching against
>>> the device's compatible value.
>>>
>>> On many SoCs, devices are connected to an SoC-internal reset controller.
>>> If the reset hierarchy is described in DT using "resets" properties, or
>>> in lookup tables in platform code, such devices can be reset in a
>>> generic way through the reset controller subsystem.  Hence add support
>>> for this, avoiding the need to write device-specific reset drivers for
>>> each single device on affected SoCs.
>>>
>>> Devices that do require a more complex reset procedure can still provide
>>> a device-specific reset driver, as that takes precedence.
>>>
>>> Note that this functionality depends on CONFIG_RESET_CONTROLLER=y, and
>>> becomes a no-op (as in: "No reset function found for device") if reset
>>> controller support is disabled.
>>>
>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
>>> Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
> 
>>> --- a/drivers/vfio/platform/vfio_platform_common.c
>>> +++ b/drivers/vfio/platform/vfio_platform_common.c
> 
>>> @@ -128,8 +131,16 @@ static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
>>>               vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
>>>                                                       &vdev->reset_module);
>>>       }
>>> +     if (vdev->of_reset)
>>> +             return 0;
>>> +
>>> +     rstc = reset_control_get_dedicated(vdev->device, NULL);
>>> +     if (!IS_ERR(rstc)) {
>>> +             vdev->reset_control = rstc;
>>> +             return 0;
>>> +     }
>>>
>>> -     return vdev->of_reset ? 0 : -ENOENT;
>>> +     return PTR_ERR(rstc);
>> This changes the returned value as seen by the user (probe returned
>> valud). Can we keep -ENOENT in case of no reset controller found?
> 
> On success, it still returns 0.
> On failure, it forwards the error from reset_control_get_dedicated(), which
> is IMHO better than replacing it by -ENOENT: we try to propagate error
> codes as much as possible.  It could e.g. return -EPROBE_DEFER.
> 
> Is there anything that relies on the function returning -ENOENT?
None I am aware of actually. I was afraid about compatibility break but
here we would change an errno by another one so maybe that's not a big
deal at that stage of vfio_platform usage?

Thanks

Eric
> 
>> Otherwise looks good to me with the new "dedicated" reset semantics.
> 
> Thanks!
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

Re: [PATCH/RFC v4 2/2] vfio: platform: Add generic reset controller support

[Alex Williamson]

On Wed, 19 Sep 2018 17:31:43 +0200
Auger Eric <eric.auger@redhat.com> wrote:

> Hi Geert,
> 
> On 9/19/18 2:54 PM, Geert Uytterhoeven wrote:
> > Hi Eric,
> > 
> > On Wed, Sep 19, 2018 at 2:36 PM Auger Eric <eric.auger@redhat.com> wrote:  
> >> On 9/17/18 6:39 PM, Geert Uytterhoeven wrote:  
> >>> Vfio-platform requires dedicated reset support, provided either by ACPI,
> >>> or, on DT platforms, by a device-specific reset driver matching against
> >>> the device's compatible value.
> >>>
> >>> On many SoCs, devices are connected to an SoC-internal reset controller.
> >>> If the reset hierarchy is described in DT using "resets" properties, or
> >>> in lookup tables in platform code, such devices can be reset in a
> >>> generic way through the reset controller subsystem.  Hence add support
> >>> for this, avoiding the need to write device-specific reset drivers for
> >>> each single device on affected SoCs.
> >>>
> >>> Devices that do require a more complex reset procedure can still provide
> >>> a device-specific reset driver, as that takes precedence.
> >>>
> >>> Note that this functionality depends on CONFIG_RESET_CONTROLLER=y, and
> >>> becomes a no-op (as in: "No reset function found for device") if reset
> >>> controller support is disabled.
> >>>
> >>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >>> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
> >>> Reviewed-by: Simon Horman <horms+renesas@verge.net.au>  
> >   
> >>> --- a/drivers/vfio/platform/vfio_platform_common.c
> >>> +++ b/drivers/vfio/platform/vfio_platform_common.c  
> >   
> >>> @@ -128,8 +131,16 @@ static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
> >>>               vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
> >>>                                                       &vdev->reset_module);
> >>>       }
> >>> +     if (vdev->of_reset)
> >>> +             return 0;
> >>> +
> >>> +     rstc = reset_control_get_dedicated(vdev->device, NULL);
> >>> +     if (!IS_ERR(rstc)) {
> >>> +             vdev->reset_control = rstc;
> >>> +             return 0;
> >>> +     }
> >>>
> >>> -     return vdev->of_reset ? 0 : -ENOENT;
> >>> +     return PTR_ERR(rstc);  
> >> This changes the returned value as seen by the user (probe returned
> >> valud). Can we keep -ENOENT in case of no reset controller found?  
> > 
> > On success, it still returns 0.
> > On failure, it forwards the error from reset_control_get_dedicated(), which
> > is IMHO better than replacing it by -ENOENT: we try to propagate error
> > codes as much as possible.  It could e.g. return -EPROBE_DEFER.
> > 
> > Is there anything that relies on the function returning -ENOENT?  
> None I am aware of actually. I was afraid about compatibility break but
> here we would change an errno by another one so maybe that's not a big
> deal at that stage of vfio_platform usage?

Yeah, I don't see that one errno vs another really matters in the grand
scheme of things.  I also don't see that propagating this particular
errno adds much value, but it is good general practice, so seems ok to
me unless there are other concerns.  Thanks,

Alex