| Message ID | 20180917163955.19023-2-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio: platform: Add generic reset controller support | expand |
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
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); > } > > /* >
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
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
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
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 >
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
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
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); } /*
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(-)