Messages in this thread | | | From | Doug Anderson <> | Date | Mon, 21 Aug 2023 09:50:32 -0700 | Subject | Re: [PATCH 3/6] regulator: core: simplify nested locking |
| |
Hi,
On Sat, Aug 19, 2023 at 3:46 PM Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote: > > Simplify regulator locking by removing locking around locking. > rdev->ref check when unlocking is moved inside the critical section. > > This patch depends on 12235da8c80a ("kernel/locking: Add context to > ww_mutex_trylock()").
nit: when I run checkpatch, it always wants me to put the word "commit" before the git hash when I refer to a commit. ;-)
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> > --- > drivers/regulator/core.c | 19 +++++-------------- > 1 file changed, 5 insertions(+), 14 deletions(-) > > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c > index 921c7039baa3..87e54b776a0f 100644 > --- a/drivers/regulator/core.c > +++ b/drivers/regulator/core.c > @@ -34,7 +34,6 @@ > #include "internal.h" > > static DEFINE_WW_CLASS(regulator_ww_class); > -static DEFINE_MUTEX(regulator_nesting_mutex); > static DEFINE_MUTEX(regulator_list_mutex); > static LIST_HEAD(regulator_map_list); > static LIST_HEAD(regulator_ena_gpio_list); > @@ -143,23 +142,18 @@ static inline int regulator_lock_nested(struct regulator_dev *rdev, > { > int ret = 0;
nit: remove initialization of "ret" to 0 since changing "return ret" to "return 0" below. Those changes belong in one of the previous patch, too.
> - mutex_lock(®ulator_nesting_mutex); > - > if (!ww_mutex_trylock(&rdev->mutex, ww_ctx) && > - rdev->mutex_owner != current) { > - mutex_unlock(®ulator_nesting_mutex); > + READ_ONCE(rdev->mutex_owner) != current) { > ret = ww_mutex_lock(&rdev->mutex, ww_ctx); > + > if (ret == -EDEADLK) > return ret; > - mutex_lock(®ulator_nesting_mutex); > } > > rdev->ref_cnt++; > rdev->mutex_owner = current; > > - mutex_unlock(®ulator_nesting_mutex); > - > - return ret; > + return 0; > } > > /** > @@ -186,16 +180,13 @@ static void regulator_lock(struct regulator_dev *rdev) > */ > static void regulator_unlock(struct regulator_dev *rdev) > { > - mutex_lock(®ulator_nesting_mutex); > + if (WARN_ON_ONCE(rdev->ref_cnt <= 0)) > + return; > > if (--rdev->ref_cnt == 0) { > rdev->mutex_owner = NULL; > ww_mutex_unlock(&rdev->mutex); > } > - > - WARN_ON_ONCE(rdev->ref_cnt < 0); > - > - mutex_unlock(®ulator_nesting_mutex);
I guess the "fix" you talked about in the cover letter is moving the WARN_ON up? That could be done in patch #1 and marked as a "Fix", right?
I'm not 100% sure why we needed the "regulator_nesting_mutex" to begin with. I'm also not 100% sure why we depend on commit 12235da8c80a ("kernel/locking: Add context to ww_mutex_trylock()"). Could you add something to the commit message to make this more obvious so I don't need to work so hard to figure it out? ;-)
| |