drivers/reset/core.c | 30 +++++++++--------------------- 1 file changed, 9 insertions(+), 21 deletions(-)
Use guard(mutex) to automatically unlock mutexes when going out of
scope. Simplify error paths by removing a goto and manual mutex
unlocking in multiple places.
Follow-up to commit 3ec21e7fa854 ("reset: simplify locking with
guard()").
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
drivers/reset/core.c | 30 +++++++++---------------------
1 file changed, 9 insertions(+), 21 deletions(-)
diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index 4d509d41456a..6fbc6f3c14c9 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -676,25 +676,20 @@ int reset_control_acquire(struct reset_control *rstc)
if (reset_control_is_array(rstc))
return reset_control_array_acquire(rstc_to_array(rstc));
- mutex_lock(&reset_list_mutex);
+ guard(mutex)(&reset_list_mutex);
- if (rstc->acquired) {
- mutex_unlock(&reset_list_mutex);
+ if (rstc->acquired)
return 0;
- }
list_for_each_entry(rc, &rstc->rcdev->reset_control_head, list) {
if (rstc != rc && rstc->id == rc->id) {
- if (rc->acquired) {
- mutex_unlock(&reset_list_mutex);
+ if (rc->acquired)
return -EBUSY;
- }
}
}
rstc->acquired = true;
- mutex_unlock(&reset_list_mutex);
return 0;
}
EXPORT_SYMBOL_GPL(reset_control_acquire);
@@ -1041,29 +1036,27 @@ __of_reset_control_get(struct device_node *node, const char *id, int index,
}
}
- mutex_lock(&reset_list_mutex);
+ guard(mutex)(&reset_list_mutex);
rcdev = __reset_find_rcdev(&args, gpio_fallback);
if (!rcdev) {
rstc = ERR_PTR(-EPROBE_DEFER);
- goto out_unlock;
+ goto out_put;
}
if (WARN_ON(args.args_count != rcdev->of_reset_n_cells)) {
rstc = ERR_PTR(-EINVAL);
- goto out_unlock;
+ goto out_put;
}
rstc_id = rcdev->of_xlate(rcdev, &args);
if (rstc_id < 0) {
rstc = ERR_PTR(rstc_id);
- goto out_unlock;
+ goto out_put;
}
/* reset_list_mutex also protects the rcdev's reset_control list */
rstc = __reset_control_get_internal(rcdev, rstc_id, shared, acquired);
-out_unlock:
- mutex_unlock(&reset_list_mutex);
out_put:
of_node_put(args.np);
@@ -1098,7 +1091,7 @@ __reset_control_get_from_lookup(struct device *dev, const char *con_id,
const char *dev_id = dev_name(dev);
struct reset_control *rstc = NULL;
- mutex_lock(&reset_lookup_mutex);
+ guard(mutex)(&reset_lookup_mutex);
list_for_each_entry(lookup, &reset_lookup_list, list) {
if (strcmp(lookup->dev_id, dev_id))
@@ -1107,11 +1100,9 @@ __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))) {
- mutex_lock(&reset_list_mutex);
+ guard(mutex)(&reset_list_mutex);
rcdev = __reset_controller_by_name(lookup->provider);
if (!rcdev) {
- mutex_unlock(&reset_list_mutex);
- mutex_unlock(&reset_lookup_mutex);
/* Reset provider may not be ready yet. */
return ERR_PTR(-EPROBE_DEFER);
}
@@ -1119,13 +1110,10 @@ __reset_control_get_from_lookup(struct device *dev, const char *con_id,
rstc = __reset_control_get_internal(rcdev,
lookup->index,
shared, acquired);
- mutex_unlock(&reset_list_mutex);
break;
}
}
- mutex_unlock(&reset_lookup_mutex);
-
if (!rstc)
return optional ? NULL : ERR_PTR(-ENOENT);
---
base-commit: 487b1b32e317b85c2948eb4013f3e089a0433d49
change-id: 20240927-reset-guard-c42dfd2a26c7
Best regards,
--
Philipp Zabel <p.zabel@pengutronix.de>> Use guard(mutex) to automatically unlock mutexes when going out of
> scope. Simplify error paths by removing a goto and manual mutex
> unlocking in multiple places.
…
> +++ b/drivers/reset/core.c
…
@@ -1041,29 +1036,27 @@ __of_reset_control_get(struct device_node *node, const char *id, int index,
}
}
- mutex_lock(&reset_list_mutex);
+ guard(mutex)(&reset_list_mutex);
rcdev = __reset_find_rcdev(&args, gpio_fallback);
…
rstc = __reset_control_get_internal(rcdev, rstc_id, shared, acquired);
-out_unlock:
- mutex_unlock(&reset_list_mutex);
out_put:
of_node_put(args.np);
…
Would you like to preserve the same lock scope (which ended before this function call)?
@@ -1098,7 +1091,7 @@ __reset_control_get_from_lookup(struct device *dev, const char *con_id,
const char *dev_id = dev_name(dev);
struct reset_control *rstc = NULL;
- mutex_lock(&reset_lookup_mutex);
+ guard(mutex)(&reset_lookup_mutex);
list_for_each_entry(lookup, &reset_lookup_list, list) {
…
break;
}
}
- mutex_unlock(&reset_lookup_mutex);
-
if (!rstc)
return optional ? NULL : ERR_PTR(-ENOENT);
…
Would you really like to increase the lock scope here?
Regards,
Markus
On So, 2024-09-29 at 12:45 +0200, Markus Elfring wrote:
> > Use guard(mutex) to automatically unlock mutexes when going out of
> > scope. Simplify error paths by removing a goto and manual mutex
> > unlocking in multiple places.
> …
> > +++ b/drivers/reset/core.c
> …
> @@ -1041,29 +1036,27 @@ __of_reset_control_get(struct device_node
> *node, const char *id, int index,
> }
> }
>
> - mutex_lock(&reset_list_mutex);
> + guard(mutex)(&reset_list_mutex);
> rcdev = __reset_find_rcdev(&args, gpio_fallback);
> …
> rstc = __reset_control_get_internal(rcdev, rstc_id, shared,
> acquired);
>
> -out_unlock:
> - mutex_unlock(&reset_list_mutex);
> out_put:
> of_node_put(args.np);
> …
>
> Would you like to preserve the same lock scope (which ended before
> this function call)?
Thank you for pointing this out. Yes, and this should have alerted me
to the issue with goto out_put from before the locked region.
> @@ -1098,7 +1091,7 @@ __reset_control_get_from_lookup(struct device
> *dev, const char *con_id,
> const char *dev_id = dev_name(dev);
> struct reset_control *rstc = NULL;
>
> - mutex_lock(&reset_lookup_mutex);
> + guard(mutex)(&reset_lookup_mutex);
>
> list_for_each_entry(lookup, &reset_lookup_list, list) {
> …
> break;
> }
> }
>
> - mutex_unlock(&reset_lookup_mutex);
> -
> if (!rstc)
> return optional ? NULL : ERR_PTR(-ENOENT);
> …
>
> Would you really like to increase the lock scope here?
I don't think this would have been a problem.
regards
Philipp
Hi Philipp,
kernel test robot noticed the following build errors:
[auto build test ERROR on 487b1b32e317b85c2948eb4013f3e089a0433d49]
url: https://github.com/intel-lab-lkp/linux/commits/Philipp-Zabel/reset-Further-simplify-locking-with-guard/20240927-220355
base: 487b1b32e317b85c2948eb4013f3e089a0433d49
patch link: https://lore.kernel.org/r/20240927-reset-guard-v1-1-293bf1302210%40pengutronix.de
patch subject: [PATCH] reset: Further simplify locking with guard()
config: x86_64-kexec (https://download.01.org/0day-ci/archive/20240929/202409291457.lc5Xgv3u-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240929/202409291457.lc5Xgv3u-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409291457.lc5Xgv3u-lkp@intel.com/
All errors (new ones prefixed by >>):
>> drivers/reset/core.c:1035:4: error: cannot jump from this goto statement to its label
1035 | goto out_put;
| ^
drivers/reset/core.c:1039:2: note: jump bypasses initialization of variable with __attribute__((cleanup))
1039 | guard(mutex)(&reset_list_mutex);
| ^
include/linux/cleanup.h:167:15: note: expanded from macro 'guard'
167 | CLASS(_name, __UNIQUE_ID(guard))
| ^
include/linux/compiler.h:189:29: note: expanded from macro '__UNIQUE_ID'
189 | #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
| ^
include/linux/compiler_types.h:84:22: note: expanded from macro '__PASTE'
84 | #define __PASTE(a,b) ___PASTE(a,b)
| ^
include/linux/compiler_types.h:83:23: note: expanded from macro '___PASTE'
83 | #define ___PASTE(a,b) a##b
| ^
<scratch space>:110:1: note: expanded from here
110 | __UNIQUE_ID_guard501
| ^
1 error generated.
vim +1035 drivers/reset/core.c
c721f189e89c0d Krzysztof Kozlowski 2024-01-29 989
1c5e05c23f4a64 Philipp Zabel 2021-03-04 990 struct reset_control *
1c5e05c23f4a64 Philipp Zabel 2021-03-04 991 __of_reset_control_get(struct device_node *node, const char *id, int index,
1c5e05c23f4a64 Philipp Zabel 2021-03-04 992 bool shared, bool optional, bool acquired)
61fc41317666be Philipp Zabel 2012-11-19 993 {
c721f189e89c0d Krzysztof Kozlowski 2024-01-29 994 bool gpio_fallback = false;
d056c9b8191867 Philipp Zabel 2015-12-08 995 struct reset_control *rstc;
c721f189e89c0d Krzysztof Kozlowski 2024-01-29 996 struct reset_controller_dev *rcdev;
61fc41317666be Philipp Zabel 2012-11-19 997 struct of_phandle_args args;
61fc41317666be Philipp Zabel 2012-11-19 998 int rstc_id;
61fc41317666be Philipp Zabel 2012-11-19 999 int ret;
61fc41317666be Philipp Zabel 2012-11-19 1000
6c96f05c8bb8bc Hans de Goede 2016-02-23 1001 if (!node)
6c96f05c8bb8bc Hans de Goede 2016-02-23 1002 return ERR_PTR(-EINVAL);
6c96f05c8bb8bc Hans de Goede 2016-02-23 1003
6c96f05c8bb8bc Hans de Goede 2016-02-23 1004 if (id) {
6c96f05c8bb8bc Hans de Goede 2016-02-23 1005 index = of_property_match_string(node,
6c96f05c8bb8bc Hans de Goede 2016-02-23 1006 "reset-names", id);
bb475230b8e59a Ramiro Oliveira 2017-01-13 1007 if (index == -EILSEQ)
bb475230b8e59a Ramiro Oliveira 2017-01-13 1008 return ERR_PTR(index);
6c96f05c8bb8bc Hans de Goede 2016-02-23 1009 if (index < 0)
bb475230b8e59a Ramiro Oliveira 2017-01-13 1010 return optional ? NULL : ERR_PTR(-ENOENT);
6c96f05c8bb8bc Hans de Goede 2016-02-23 1011 }
6c96f05c8bb8bc Hans de Goede 2016-02-23 1012
fc0a5921561c71 Maxime Ripard 2013-12-20 1013 ret = of_parse_phandle_with_args(node, "resets", "#reset-cells",
61fc41317666be Philipp Zabel 2012-11-19 1014 index, &args);
bb475230b8e59a Ramiro Oliveira 2017-01-13 1015 if (ret == -EINVAL)
61fc41317666be Philipp Zabel 2012-11-19 1016 return ERR_PTR(ret);
c721f189e89c0d Krzysztof Kozlowski 2024-01-29 1017 if (ret) {
c721f189e89c0d Krzysztof Kozlowski 2024-01-29 1018 if (!IS_ENABLED(CONFIG_RESET_GPIO))
c721f189e89c0d Krzysztof Kozlowski 2024-01-29 1019 return optional ? NULL : ERR_PTR(ret);
c721f189e89c0d Krzysztof Kozlowski 2024-01-29 1020
c721f189e89c0d Krzysztof Kozlowski 2024-01-29 1021 /*
c721f189e89c0d Krzysztof Kozlowski 2024-01-29 1022 * There can be only one reset-gpio for regular devices, so
c721f189e89c0d Krzysztof Kozlowski 2024-01-29 1023 * don't bother with the "reset-gpios" phandle index.
c721f189e89c0d Krzysztof Kozlowski 2024-01-29 1024 */
c721f189e89c0d Krzysztof Kozlowski 2024-01-29 1025 ret = of_parse_phandle_with_args(node, "reset-gpios", "#gpio-cells",
c721f189e89c0d Krzysztof Kozlowski 2024-01-29 1026 0, &args);
bb475230b8e59a Ramiro Oliveira 2017-01-13 1027 if (ret)
bb475230b8e59a Ramiro Oliveira 2017-01-13 1028 return optional ? NULL : ERR_PTR(ret);
61fc41317666be Philipp Zabel 2012-11-19 1029
c721f189e89c0d Krzysztof Kozlowski 2024-01-29 1030 gpio_fallback = true;
c721f189e89c0d Krzysztof Kozlowski 2024-01-29 1031
c721f189e89c0d Krzysztof Kozlowski 2024-01-29 1032 ret = __reset_add_reset_gpio_device(&args);
c721f189e89c0d Krzysztof Kozlowski 2024-01-29 1033 if (ret) {
c721f189e89c0d Krzysztof Kozlowski 2024-01-29 1034 rstc = ERR_PTR(ret);
c721f189e89c0d Krzysztof Kozlowski 2024-01-29 @1035 goto out_put;
61fc41317666be Philipp Zabel 2012-11-19 1036 }
61fc41317666be Philipp Zabel 2012-11-19 1037 }
61fc41317666be Philipp Zabel 2012-11-19 1038
784c4fbce820c0 Philipp Zabel 2024-09-27 1039 guard(mutex)(&reset_list_mutex);
c721f189e89c0d Krzysztof Kozlowski 2024-01-29 1040 rcdev = __reset_find_rcdev(&args, gpio_fallback);
61fc41317666be Philipp Zabel 2012-11-19 1041 if (!rcdev) {
b790c8ea5593d6 Geert Uytterhoeven 2018-10-08 1042 rstc = ERR_PTR(-EPROBE_DEFER);
784c4fbce820c0 Philipp Zabel 2024-09-27 1043 goto out_put;
61fc41317666be Philipp Zabel 2012-11-19 1044 }
61fc41317666be Philipp Zabel 2012-11-19 1045
e677774f502635 Maxime Ripard 2016-01-14 1046 if (WARN_ON(args.args_count != rcdev->of_reset_n_cells)) {
b790c8ea5593d6 Geert Uytterhoeven 2018-10-08 1047 rstc = ERR_PTR(-EINVAL);
784c4fbce820c0 Philipp Zabel 2024-09-27 1048 goto out_put;
e677774f502635 Maxime Ripard 2016-01-14 1049 }
e677774f502635 Maxime Ripard 2016-01-14 1050
61fc41317666be Philipp Zabel 2012-11-19 1051 rstc_id = rcdev->of_xlate(rcdev, &args);
61fc41317666be Philipp Zabel 2012-11-19 1052 if (rstc_id < 0) {
b790c8ea5593d6 Geert Uytterhoeven 2018-10-08 1053 rstc = ERR_PTR(rstc_id);
784c4fbce820c0 Philipp Zabel 2024-09-27 1054 goto out_put;
61fc41317666be Philipp Zabel 2012-11-19 1055 }
61fc41317666be Philipp Zabel 2012-11-19 1056
c15ddec2ca0607 Hans de Goede 2016-02-23 1057 /* reset_list_mutex also protects the rcdev's reset_control list */
c84b0326d5e4fe Philipp Zabel 2019-02-21 1058 rstc = __reset_control_get_internal(rcdev, rstc_id, shared, acquired);
61fc41317666be Philipp Zabel 2012-11-19 1059
c721f189e89c0d Krzysztof Kozlowski 2024-01-29 1060 out_put:
b790c8ea5593d6 Geert Uytterhoeven 2018-10-08 1061 of_node_put(args.np);
61fc41317666be Philipp Zabel 2012-11-19 1062
61fc41317666be Philipp Zabel 2012-11-19 1063 return rstc;
61fc41317666be Philipp Zabel 2012-11-19 1064 }
6c96f05c8bb8bc Hans de Goede 2016-02-23 1065 EXPORT_SYMBOL_GPL(__of_reset_control_get);
61fc41317666be Philipp Zabel 2012-11-19 1066
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Fri, Sep 27, 2024 at 04:02:32PM +0200, Philipp Zabel wrote:
> Use guard(mutex) to automatically unlock mutexes when going out of
> scope. Simplify error paths by removing a goto and manual mutex
> unlocking in multiple places.
And that, folks, is a live example of the reasons why guard() is an
attractive nuisance. We really need a very loud warning on
cleanup.h stuff - otherwise such patches from well-meaning folks
will keep coming.
> @@ -1041,29 +1036,27 @@ __of_reset_control_get(struct device_node *node, const char *id, int index,
> }
> }
>
> - mutex_lock(&reset_list_mutex);
> + guard(mutex)(&reset_list_mutex);
> rcdev = __reset_find_rcdev(&args, gpio_fallback);
> if (!rcdev) {
> rstc = ERR_PTR(-EPROBE_DEFER);
> - goto out_unlock;
> + goto out_put;
> }
>
> if (WARN_ON(args.args_count != rcdev->of_reset_n_cells)) {
> rstc = ERR_PTR(-EINVAL);
> - goto out_unlock;
> + goto out_put;
> }
>
> rstc_id = rcdev->of_xlate(rcdev, &args);
> if (rstc_id < 0) {
> rstc = ERR_PTR(rstc_id);
> - goto out_unlock;
> + goto out_put;
> }
>
> /* reset_list_mutex also protects the rcdev's reset_control list */
> rstc = __reset_control_get_internal(rcdev, rstc_id, shared, acquired);
>
> -out_unlock:
> - mutex_unlock(&reset_list_mutex);
> out_put:
> of_node_put(args.np);
Guess what happens if you take goto out_put prior to the entire thing,
in
ret = __reset_add_reset_gpio_device(&args);
if (ret) {
rstc = ERR_PTR(ret);
goto out_put;
}
That patch adds implicit mutex_unlock() at the points where we leave
the scope. Which extends to the end of function. In other words, there is
one downstream of out_put, turning any goto out_put upstream of guard() into
a bug.
What's worse, that bug is not caught by gcc - it quietly generates bogus code
that will get unnoticed until we get an error from __reset_add_reset_gpio_device()
call. At that point we'll look at the contents of uninitialized variable and,
if we are unlucky, call mutex_unlock() (with hell knows what pointer passed to it -
not that mutex_unlock(&reset_list_mutex) would do us any good at that point, since
we hadn't locked it in the first place).
Folks, that kind of cleanup patches is bloody dangerous; even something that
currently avoids that crap can easily grow that kind of quiet breakage later.
On 29/09/2024 00:27, Al Viro wrote:
> On Fri, Sep 27, 2024 at 04:02:32PM +0200, Philipp Zabel wrote:
>> Use guard(mutex) to automatically unlock mutexes when going out of
>> scope. Simplify error paths by removing a goto and manual mutex
>> unlocking in multiple places.
>
> And that, folks, is a live example of the reasons why guard() is an
> attractive nuisance. We really need a very loud warning on
> cleanup.h stuff - otherwise such patches from well-meaning folks
> will keep coming.
>
>> @@ -1041,29 +1036,27 @@ __of_reset_control_get(struct device_node *node, const char *id, int index,
>> }
>> }
>>
>> - mutex_lock(&reset_list_mutex);
>> + guard(mutex)(&reset_list_mutex);
>> rcdev = __reset_find_rcdev(&args, gpio_fallback);
>> if (!rcdev) {
>> rstc = ERR_PTR(-EPROBE_DEFER);
>> - goto out_unlock;
>> + goto out_put;
>> }
>>
>> if (WARN_ON(args.args_count != rcdev->of_reset_n_cells)) {
>> rstc = ERR_PTR(-EINVAL);
>> - goto out_unlock;
>> + goto out_put;
>> }
>>
>> rstc_id = rcdev->of_xlate(rcdev, &args);
>> if (rstc_id < 0) {
>> rstc = ERR_PTR(rstc_id);
>> - goto out_unlock;
>> + goto out_put;
>> }
>>
>> /* reset_list_mutex also protects the rcdev's reset_control list */
>> rstc = __reset_control_get_internal(rcdev, rstc_id, shared, acquired);
>>
>> -out_unlock:
>> - mutex_unlock(&reset_list_mutex);
>> out_put:
>> of_node_put(args.np);
>
> Guess what happens if you take goto out_put prior to the entire thing,
> in
> ret = __reset_add_reset_gpio_device(&args);
> if (ret) {
> rstc = ERR_PTR(ret);
> goto out_put;
> }
> That patch adds implicit mutex_unlock() at the points where we leave
> the scope. Which extends to the end of function. In other words, there is
> one downstream of out_put, turning any goto out_put upstream of guard() into
> a bug.
>
cleanup.h also mentions that one should do not mix cleanup with existing
goto, because of possibility of above issue.
But except careful review, this patch should have been simply compile
tested which would point to the issue above. Any guard/scope works must
be checked with clang W=1, which reports jumps over init.
Best regards,
Krzysztof
On So, 2024-09-29 at 20:48 +0200, Krzysztof Kozlowski wrote:
> On 29/09/2024 00:27, Al Viro wrote:
> > On Fri, Sep 27, 2024 at 04:02:32PM +0200, Philipp Zabel wrote:
> > > Use guard(mutex) to automatically unlock mutexes when going out of
> > > scope. Simplify error paths by removing a goto and manual mutex
> > > unlocking in multiple places.
> >
> > And that, folks, is a live example of the reasons why guard() is an
> > attractive nuisance. We really need a very loud warning on
> > cleanup.h stuff - otherwise such patches from well-meaning folks
> > will keep coming.
Thank you for the analysis. It think I'll drop this entirely.
[...]
> >
> > Guess what happens if you take goto out_put prior to the entire thing,
> > in
> > ret = __reset_add_reset_gpio_device(&args);
> > if (ret) {
> > rstc = ERR_PTR(ret);
> > goto out_put;
> > }
> > That patch adds implicit mutex_unlock() at the points where we leave
> > the scope. Which extends to the end of function. In other words, there is
> > one downstream of out_put, turning any goto out_put upstream of guard() into
> > a bug.
> >
>
> cleanup.h also mentions that one should do not mix cleanup with existing
> goto, because of possibility of above issue.
Yes, d5934e76316e ("cleanup: Add usage and style documentation"), last
paragraph.
> But except careful review, this patch should have been simply compile
> tested which would point to the issue above. Any guard/scope works must
> be checked with clang W=1, which reports jumps over init.
Thank you, I was missing a CC=clang W=1 build in my pre-flight checks.
That's fixed now.
regards
Philipp
© 2016 - 2024 Red Hat, Inc.