Messages in this thread Patch in this message | | | From | Stephen Boyd <> | Date | Tue, 29 Aug 2023 17:57:31 -0500 | Subject | Re: [PATCH 6/6] regulator: core: simplify lock_two() |
| |
Quoting Michał Mirosław (2023-08-29 14:25:46) > On Tue, Aug 29, 2023 at 03:52:19PM -0500, Stephen Boyd wrote: > > Quoting Michał Mirosław (2023-08-28 13:26:54) > > > Indeed they are quite similar. I did remove a bit more code than that, > > > though: in this case there is no early success return before the loop. > > > > > > Instead of saying: > > > > > > lock A > > > lock B > > > if ok return > > > if that failed, loop: > > > unlock A > > > lock B harder > > > lock A > > > if ok return > > > swap A <-> B > > > lock B > > > > > > Now it's: > > > > > > lock A > > > loop forever: > > > lock B > > > if ok, return > > > unlock A > > > swap them > > > lock A harder > > > > > > With the same condition 'A held' at the start of an iteration. > > > > > > > Removing duplicate code is great! I'm primarily concerned with > > readability. The terms 'A' and 'B' doesn't make it easy for me. Can you > > maintain the 'held' and 'contended' names for the variables? > > > > That would be > > > > 1. lock 'held' > > 2. loop forever: > > 3. lock 'contended' > > 4. if ok, return > > 5. unlock 'held' > > 6. swap them > > 7. lock 'held' harder > > Doesn't this make it more confusing? The lock is 'held' only in lines > 2-5 and looses this trait (but not the name) on the other lines. > 'contended' is more problematic: the contended lock is called 'held' > before locking it at line 7. > > The algorithm is basically: Take the locks in sequence. If that failed, > swap the order and try again. > > Would a comment like the sentence above help with readability? > > Or we could wrap the final lines of the iteration in a > 'regulator_lock_contended()' to make it self-documenting? >
Squash this in?
---8<--- diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index 9736507b62ff..39205cf00fb7 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -201,6 +201,7 @@ static int regulator_lock_two(struct regulator_dev *rdev1, struct regulator_dev *rdev2, struct ww_acquire_ctx *ww_ctx) { + struct regulator_dev *held, *contended; int ret;
ww_acquire_init(ww_ctx, ®ulator_ww_class); @@ -208,22 +209,24 @@ static int regulator_lock_two(struct regulator_dev *rdev1, ret = regulator_lock_nested(rdev1, ww_ctx); if (WARN_ON(ret)) goto exit; + held = rdev1; + contended = rdev2;
while (true) { - ret = regulator_lock_nested(rdev2, ww_ctx); + ret = regulator_lock_nested(contended, ww_ctx); if (!ret) - return 0; + break;
- regulator_unlock(rdev1); + regulator_unlock(held);
if (WARN_ON(ret != -EDEADLOCK)) break;
- swap(rdev1, rdev2); + ww_mutex_lock_slow(&contended->mutex, ww_ctx); + contended->ref_cnt++; + contended->mutex_owner = current;
- ww_mutex_lock_slow(&rdev1->mutex, ww_ctx); - rdev1->ref_cnt++; - rdev1->mutex_owner = current; + swap(held, contended); }
exit:
| |