Re: [PATCH v2] reset: Exclusive resets must be dedicated to a single hardware block

[Philipp Zabel <p.zabel@pengutronix.de>]

Hi Geert,

Thank you for the patch. I'd still like to hear the device tree
maintainers' (added to Cc:) opinion on parsing the whole DT for "resets"
phandle properties to find shared resets like this.

On Thu, 2018-09-27 at 20:00 +0200, Geert Uytterhoeven wrote:
> 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>
> ---
> This is v2 of "[RFC] reset: Add support for dedicated reset controls":
>   - 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  | 58 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/reset.h |  5 +++-
>  2 files changed, 62 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index 225e34c56b94a2e3..2f5b61226c7964eb 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -459,6 +459,38 @@ 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)
> +{
> +	struct of_phandle_args args2;
> +	struct device_node *node2;
> +	int index, ret;
> +	bool eq;

I suppose it is very unlikely to get false positives where an arbitrary
node contains a "resets" property that looks like a proper phandle to an
actual reset-controller node.
Are we allowed though to scan the whole tree for "resets" properties
regardless of the nodes' bindings or compatible properties like this?

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

Emitting a loud warning here could be very helpful if it contains
both the reset controller node and the reset index, as well as the
consumer nodes: node and node2.

> +				return false;
> +		}
> +	}
> +	return true;
> +}
> +
>  struct reset_control *__of_reset_control_get(struct device_node *node,
>  				     const char *id, int index, bool shared,
>  				     bool optional)
> @@ -514,6 +546,11 @@ struct reset_control *__of_reset_control_get(struct device_node *node,
>  		return ERR_PTR(rstc_id);
>  	}
>  
> +	if (!shared && !__of_reset_is_exclusive(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,6 +578,22 @@ __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)
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
>  static struct reset_control *
>  __reset_control_get_from_lookup(struct device *dev, const char *con_id,
>  				bool shared, bool optional)
> @@ -562,6 +615,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 29af6d6b2f4b8103..5881d2594761e48f 100644
> --- a/include/linux/reset.h
> +++ b/include/linux/reset.h
> @@ -116,8 +116,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.

regards
Philipp