| Message ID | 20180927180038.24599-1-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] reset: Exclusive resets must be dedicated to a single hardware block | expand |
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
Hi Philipp, On Thu, Oct 4, 2018 at 12:15 PM Philipp Zabel <p.zabel@pengutronix.de> wrote: > 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) Oops, this should take *args, not 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? Given "resets" is a more-or-less standard property, I'd say yes. Especially given of_parse_phandle_with_args() does verify that the target node has #reset-cells, and that the number of parameters matches that. > > + 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]))); As there's at least one other function in -next that compares of_phandle_args, I will add a helper of_phandle_args_eq(). > > + 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. Indeed, will do, also for lookup resets. We already have of_print_phandle_args(), but that is a bit inflexible. Adding support for "%pOFa" looks like the modern thing to do. Gr{oetje,eeting}s, Geert
On Fri, Oct 5, 2018 at 10:55 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Thu, Oct 4, 2018 at 12:15 PM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > 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) > > Oops, this should take *args, not 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? > > Given "resets" is a more-or-less standard property, I'd say yes. > Especially given of_parse_phandle_with_args() does verify that the target > node has #reset-cells, and that the number of parameters matches that. > > > > + 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]))); > > As there's at least one other function in -next that compares of_phandle_args, > I will add a helper of_phandle_args_eq(). > > > > + 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 Actually on DT-based systems, the index is a driver-specific implementation detail, and may differ from the actual reset specifier in DT. E.g. on R-Car systems, DT uses a human-readable representation matching the datasheet, while internally, the driver uses a packed representation. Hence printing the index may confuse the user. For lookup-based systems, this is different. > > consumer nodes: node and node2. > > Indeed, will do, also for lookup resets. > > We already have of_print_phandle_args(), but that is a bit inflexible. > Adding support for "%pOFa" looks like the modern thing to do. Scrap that: of_phandle_args is not derived from a device_node, so %pOFa is not appropriate (and would crash instead of fall back to a pointer before %pOFa support is implemented). And without more users, it doesn't make much sense to go for a new type (e.g. "%pOA")... Actually, printing the full reset specifier is not needed. A message like /soc/pwm@e6e31000 and /soc/pwm@e6e30000 share a reset on /soc/clock-controller@e6150000 should give sufficient clue to the user. Gr{oetje,eeting}s, Geert
On Fri, 2018-10-05 at 14:31 +0200, Geert Uytterhoeven wrote: [...] > > > > + eq = (args2.np == args.np && > > > > + args2.args_count == args.args_count && > > > > + !memcmp(args2.args, args.args, > > > > + args.args_count * sizeof(args.args[0]))); > > > > As there's at least one other function in -next that compares of_phandle_args, > > I will add a helper of_phandle_args_eq(). > > > > > > + 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 > > Actually on DT-based systems, the index is a driver-specific > implementation detail, and may differ from the actual reset specifier in DT. > E.g. on R-Car systems, DT uses a human-readable representation matching > the datasheet, while internally, the driver uses a packed representation. > Hence printing the index may confuse the user. > > For lookup-based systems, this is different. Correct. I'm so used to #reset-cells = <1>, it's hard to remember the exceptions. So let's not try to print indices or args. > > > consumer nodes: node and node2. > > > > Indeed, will do, also for lookup resets. > > > > We already have of_print_phandle_args(), but that is a bit inflexible. > > Adding support for "%pOFa" looks like the modern thing to do. > > Scrap that: of_phandle_args is not derived from a device_node, so %pOFa > is not appropriate (and would crash instead of fall back to a pointer before > %pOFa support is implemented). And without more users, it doesn't make much > sense to go for a new type (e.g. "%pOA")... > > Actually, printing the full reset specifier is not needed. A message like > > /soc/pwm@e6e31000 and /soc/pwm@e6e30000 share a reset on > /soc/clock-controller@e6150000 > > should give sufficient clue to the user. Yes. You could also pass con_id into __of_reset_is_exclusive and print that. It would be nice to indicate which consumer requested exclusive access. regards Philipp
Hi Philipp, On Fri, Oct 5, 2018 at 5:16 PM Philipp Zabel <p.zabel@pengutronix.de> wrote: > On Fri, 2018-10-05 at 14:31 +0200, Geert Uytterhoeven wrote: > [...] > > > > > + eq = (args2.np == args.np && > > > > > + args2.args_count == args.args_count && > > > > > + !memcmp(args2.args, args.args, > > > > > + args.args_count * sizeof(args.args[0]))); > > > > > > As there's at least one other function in -next that compares of_phandle_args, > > > I will add a helper of_phandle_args_eq(). > > > > > > > > + 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 > > > > Actually on DT-based systems, the index is a driver-specific > > implementation detail, and may differ from the actual reset specifier in DT. > > E.g. on R-Car systems, DT uses a human-readable representation matching > > the datasheet, while internally, the driver uses a packed representation. > > Hence printing the index may confuse the user. > > > > For lookup-based systems, this is different. > > Correct. I'm so used to #reset-cells = <1>, it's hard to remember the > exceptions. So let's not try to print indices or args. > > > > > consumer nodes: node and node2. > > > > > > Indeed, will do, also for lookup resets. > > > > > > We already have of_print_phandle_args(), but that is a bit inflexible. > > > Adding support for "%pOFa" looks like the modern thing to do. > > > > Scrap that: of_phandle_args is not derived from a device_node, so %pOFa > > is not appropriate (and would crash instead of fall back to a pointer before > > %pOFa support is implemented). And without more users, it doesn't make much > > sense to go for a new type (e.g. "%pOA")... > > > > Actually, printing the full reset specifier is not needed. A message like > > > > /soc/pwm@e6e31000 and /soc/pwm@e6e30000 share a reset on > > /soc/clock-controller@e6150000 > > > > should give sufficient clue to the user. > > Yes. You could also pass con_id into __of_reset_is_exclusive and print > that. It would be nice to indicate which consumer requested exclusive > access. con_id is used for lookup-based resets only? But the value passed there is the "id" parameter of reset_control_get_exclusive(). However, that is not the consumer name, and usually NULL. I'm afraid the only way to know the consumer is to print a backtrace with WARN()? Gr{oetje,eeting}s, Geert
On Mon, 2018-10-08 at 11:59 +0200, Geert Uytterhoeven wrote: > Hi Philipp, > > On Fri, Oct 5, 2018 at 5:16 PM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > On Fri, 2018-10-05 at 14:31 +0200, Geert Uytterhoeven wrote: > > [...] > > > > > > + eq = (args2.np == args.np && > > > > > > + args2.args_count == args.args_count && > > > > > > + !memcmp(args2.args, args.args, > > > > > > + args.args_count * sizeof(args.args[0]))); > > > > > > > > As there's at least one other function in -next that compares of_phandle_args, > > > > I will add a helper of_phandle_args_eq(). > > > > > > > > > > + 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 > > > > > > Actually on DT-based systems, the index is a driver-specific > > > implementation detail, and may differ from the actual reset specifier in DT. > > > E.g. on R-Car systems, DT uses a human-readable representation matching > > > the datasheet, while internally, the driver uses a packed representation. > > > Hence printing the index may confuse the user. > > > > > > For lookup-based systems, this is different. > > > > Correct. I'm so used to #reset-cells = <1>, it's hard to remember the > > exceptions. So let's not try to print indices or args. > > > > > > > consumer nodes: node and node2. > > > > > > > > Indeed, will do, also for lookup resets. > > > > > > > > We already have of_print_phandle_args(), but that is a bit inflexible. > > > > Adding support for "%pOFa" looks like the modern thing to do. > > > > > > Scrap that: of_phandle_args is not derived from a device_node, so %pOFa > > > is not appropriate (and would crash instead of fall back to a pointer before > > > %pOFa support is implemented). And without more users, it doesn't make much > > > sense to go for a new type (e.g. "%pOA")... > > > > > > Actually, printing the full reset specifier is not needed. A message like > > > > > > /soc/pwm@e6e31000 and /soc/pwm@e6e30000 share a reset on > > > /soc/clock-controller@e6150000 > > > > > > should give sufficient clue to the user. > > > > Yes. You could also pass con_id into __of_reset_is_exclusive and print > > that. It would be nice to indicate which consumer requested exclusive > > access. > > con_id is used for lookup-based resets only? > > But the value passed there is the "id" parameter of > reset_control_get_exclusive(). Sorry, I did mean the id parameter in the __of_reset_control_get case. > However, that is not the consumer name, It is the name of the reset signal from point of view of the consumer. It specifies, via its position in the reset-names property, which of potentially multiple reset phandles in the resets property is the one causing the conflict. > and usually NULL. In which case the resets property usually only contains one phandle, so it is not needed to determine the conflicting reset control. > I'm afraid the only way to know the consumer is to print a backtrace with > WARN()? I'm just suggesting to augment the warning message with the reset id, if available. For example: /soc/pwm@e6e31000 requests exclusive control over reset "pwm" shared with /soc/pwm@e6e30000 on /soc/clock-controller@e6150000 regards Philipp
Hi Philipp, On Mon, Oct 8, 2018 at 12:57 PM Philipp Zabel <p.zabel@pengutronix.de> wrote: > On Mon, 2018-10-08 at 11:59 +0200, Geert Uytterhoeven wrote: > > On Fri, Oct 5, 2018 at 5:16 PM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > > On Fri, 2018-10-05 at 14:31 +0200, Geert Uytterhoeven wrote: > > > [...] > > > > > > > + eq = (args2.np == args.np && > > > > > > > + args2.args_count == args.args_count && > > > > > > > + !memcmp(args2.args, args.args, > > > > > > > + args.args_count * sizeof(args.args[0]))); > > > > > > > > > > As there's at least one other function in -next that compares of_phandle_args, > > > > > I will add a helper of_phandle_args_eq(). > > > > > > > > > > > > + 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 > > > > > > > > Actually on DT-based systems, the index is a driver-specific > > > > implementation detail, and may differ from the actual reset specifier in DT. > > > > E.g. on R-Car systems, DT uses a human-readable representation matching > > > > the datasheet, while internally, the driver uses a packed representation. > > > > Hence printing the index may confuse the user. > > > > > > > > For lookup-based systems, this is different. > > > > > > Correct. I'm so used to #reset-cells = <1>, it's hard to remember the > > > exceptions. So let's not try to print indices or args. > > > > > > > > > consumer nodes: node and node2. > > > > > > > > > > Indeed, will do, also for lookup resets. > > > > > > > > > > We already have of_print_phandle_args(), but that is a bit inflexible. > > > > > Adding support for "%pOFa" looks like the modern thing to do. > > > > > > > > Scrap that: of_phandle_args is not derived from a device_node, so %pOFa > > > > is not appropriate (and would crash instead of fall back to a pointer before > > > > %pOFa support is implemented). And without more users, it doesn't make much > > > > sense to go for a new type (e.g. "%pOA")... > > > > > > > > Actually, printing the full reset specifier is not needed. A message like > > > > > > > > /soc/pwm@e6e31000 and /soc/pwm@e6e30000 share a reset on > > > > /soc/clock-controller@e6150000 > > > > > > > > should give sufficient clue to the user. > > > > > > Yes. You could also pass con_id into __of_reset_is_exclusive and print > > > that. It would be nice to indicate which consumer requested exclusive > > > access. > > > > con_id is used for lookup-based resets only? > > > > But the value passed there is the "id" parameter of > > reset_control_get_exclusive(). > > Sorry, I did mean the id parameter in the __of_reset_control_get case. OK. > > However, that is not the consumer name, > > It is the name of the reset signal from point of view of the consumer. > It specifies, via its position in the reset-names property, which of > potentially multiple reset phandles in the resets property is the one > causing the conflict. > > > and usually NULL. > > In which case the resets property usually only contains one phandle, so > it is not needed to determine the conflicting reset control. For a device-specific driver knowing about all resets, that is indeed the case. For the generic case (e.g. my vfio-platform use case), it just wants to reset the device, and thus passes NULL. Perhaps I should use devm_reset_control_array_get_exclusive() instead of reset_control_get_exclusive(), to assert all resets? However, that makes the detection of shared resets more tricky, as it needs to consider all combinations. Currently e.g. resets = <&rst1> <&rst2>; and resets = <&rst2> <&rst1>; is not detected as reset sharing... As device_reset() also uses the first reset only, I'll keep it that way... > > I'm afraid the only way to know the consumer is to print a backtrace with > > WARN()? > > I'm just suggesting to augment the warning message with the reset id, if > available. For example: > > /soc/pwm@e6e31000 requests exclusive control over reset "pwm" > shared with /soc/pwm@e6e30000 on /soc/clock-controller@e6150000 Thanks, consider it done for next version. Gr{oetje,eeting}s, Geert
On Mon, 2018-10-08 at 13:47 +0200, Geert Uytterhoeven wrote: > Hi Philipp, > > On Mon, Oct 8, 2018 at 12:57 PM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > On Mon, 2018-10-08 at 11:59 +0200, Geert Uytterhoeven wrote: > > > On Fri, Oct 5, 2018 at 5:16 PM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > > > On Fri, 2018-10-05 at 14:31 +0200, Geert Uytterhoeven wrote: > > > > [...] > > > > > > > > + eq = (args2.np == args.np && > > > > > > > > + args2.args_count == args.args_count && > > > > > > > > + !memcmp(args2.args, args.args, > > > > > > > > + args.args_count * sizeof(args.args[0]))); > > > > > > > > > > > > As there's at least one other function in -next that compares of_phandle_args, > > > > > > I will add a helper of_phandle_args_eq(). > > > > > > > > > > > > > > + 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 > > > > > > > > > > Actually on DT-based systems, the index is a driver-specific > > > > > implementation detail, and may differ from the actual reset specifier in DT. > > > > > E.g. on R-Car systems, DT uses a human-readable representation matching > > > > > the datasheet, while internally, the driver uses a packed representation. > > > > > Hence printing the index may confuse the user. > > > > > > > > > > For lookup-based systems, this is different. > > > > > > > > Correct. I'm so used to #reset-cells = <1>, it's hard to remember the > > > > exceptions. So let's not try to print indices or args. > > > > > > > > > > > consumer nodes: node and node2. > > > > > > > > > > > > Indeed, will do, also for lookup resets. > > > > > > > > > > > > We already have of_print_phandle_args(), but that is a bit inflexible. > > > > > > Adding support for "%pOFa" looks like the modern thing to do. > > > > > > > > > > Scrap that: of_phandle_args is not derived from a device_node, so %pOFa > > > > > is not appropriate (and would crash instead of fall back to a pointer before > > > > > %pOFa support is implemented). And without more users, it doesn't make much > > > > > sense to go for a new type (e.g. "%pOA")... > > > > > > > > > > Actually, printing the full reset specifier is not needed. A message like > > > > > > > > > > /soc/pwm@e6e31000 and /soc/pwm@e6e30000 share a reset on > > > > > /soc/clock-controller@e6150000 > > > > > > > > > > should give sufficient clue to the user. > > > > > > > > Yes. You could also pass con_id into __of_reset_is_exclusive and print > > > > that. It would be nice to indicate which consumer requested exclusive > > > > access. > > > > > > con_id is used for lookup-based resets only? > > > > > > But the value passed there is the "id" parameter of > > > reset_control_get_exclusive(). > > > > Sorry, I did mean the id parameter in the __of_reset_control_get case. > > OK. > > > > However, that is not the consumer name, > > > > It is the name of the reset signal from point of view of the consumer. > > It specifies, via its position in the reset-names property, which of > > potentially multiple reset phandles in the resets property is the one > > causing the conflict. > > > > > and usually NULL. > > > > In which case the resets property usually only contains one phandle, so > > it is not needed to determine the conflicting reset control. > > For a device-specific driver knowing about all resets, that is indeed the case. > For the generic case (e.g. my vfio-platform use case), it just wants to reset > the device, and thus passes NULL. I fear in general, you can't know how to reset arbitrary hardware just from the "resets" property. Imagine a hypothetical device with two resets A and B that deadlocks if reset B is deasserted too early in a time window after reset A is deasserted. > Perhaps I should use devm_reset_control_array_get_exclusive() instead of > reset_control_get_exclusive(), to assert all resets? The safe thing to do would be to let vfio-platfrom handle devices only if they have no more than one reset. > However, that makes the detection of shared resets more tricky, as it needs > to consider all combinations. Currently e.g. > resets = <&rst1> <&rst2>; > and > resets = <&rst2> <&rst1>; > is not detected as reset sharing... > > As device_reset() also uses the first reset only, I'll keep it that way... Yes, device_reset() has the same issue. regards Philipp
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; + + 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) + 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.
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(-)