[PATCH] regulator: userspace-consumer: Use atomic operation

[PATCH] regulator: userspace-consumer: Use atomic operation

[Naresh Solanki]

Replace mutexes with atomic operations.

Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
---
 drivers/regulator/userspace-consumer.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/regulator/userspace-consumer.c b/drivers/regulator/userspace-consumer.c
index a0b980022993..a86714bd32fd 100644
--- a/drivers/regulator/userspace-consumer.c
+++ b/drivers/regulator/userspace-consumer.c
@@ -32,7 +32,7 @@ struct userspace_consumer_data {
 
 	struct kobject *kobj;
 	struct notifier_block nb;
-	unsigned long events;
+	atomic_long_t events;
 };
 
 static ssize_t name_show(struct device *dev,
@@ -99,12 +99,7 @@ static ssize_t events_show(struct device *dev,
 			   struct device_attribute *attr, char *buf)
 {
 	struct userspace_consumer_data *data = dev_get_drvdata(dev);
-	unsigned long e;
-
-	mutex_lock(&events_lock);
-	e = data->events;
-	data->events = 0;
-	mutex_unlock(&events_lock);
+	unsigned long e = atomic_long_xchg(&data->events, 0);
 
 	return sprintf(buf, "0x%lx\n", e);
 }
@@ -144,9 +139,7 @@ static int regulator_userspace_notify(struct notifier_block *nb,
 	struct userspace_consumer_data *data =
 		container_of(nb, struct userspace_consumer_data, nb);
 
-	mutex_lock(&events_lock);
-	data->events |= event;
-	mutex_unlock(&events_lock);
+	atomic_long_or(event, &data->events);
 
 	sysfs_notify(data->kobj, NULL, dev_attr_events.attr.name);
 

base-commit: 8950c4a350c188deb5f5b05df3244d4db82fe3d8
-- 
2.41.0

Re: [PATCH] regulator: userspace-consumer: Use atomic operation

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 190 bytes --]

On Wed, Aug 23, 2023 at 04:15:57PM +0200, Naresh Solanki wrote:
> Replace mutexes with atomic operations.

Why?  Generally atomics are more complicated and hard to understand and
get right.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regulator: userspace-consumer: Use atomic operation

[Naresh Solanki]

Hi,


On Wed, 23 Aug 2023 at 20:41, Mark Brown <broonie@kernel.org> wrote:
>
> On Wed, Aug 23, 2023 at 04:15:57PM +0200, Naresh Solanki wrote:
> > Replace mutexes with atomic operations.
>
> Why?  Generally atomics are more complicated and hard to understand and
> get right.
Since the operations involved here are simple & short & can be managed by
atomic operation.

Regards,
Naresh

Re: [PATCH] regulator: userspace-consumer: Use atomic operation

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 761 bytes --]

On Thu, Aug 24, 2023 at 04:34:05PM +0530, Naresh Solanki wrote:
> On Wed, 23 Aug 2023 at 20:41, Mark Brown <broonie@kernel.org> wrote:
> > On Wed, Aug 23, 2023 at 04:15:57PM +0200, Naresh Solanki wrote:

> > > Replace mutexes with atomic operations.

> > Why?  Generally atomics are more complicated and hard to understand and
> > get right.

> Since the operations involved here are simple & short & can be managed by
> atomic operation.

Unless there's a strong positive reason to specifically use atomics it
seems better to avoid them, like I say they're full of landmines with
unexpeted behaviours and therefore something that sets off alarm bells
about needing careful study, the mutex is going to be less preformant
but is also much more clearly correct.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regulator: userspace-consumer: Use atomic operation

[Zev Weiss]

On Thu, Aug 24, 2023 at 09:55:41AM PDT, Mark Brown wrote:
>On Thu, Aug 24, 2023 at 04:34:05PM +0530, Naresh Solanki wrote:
>> On Wed, 23 Aug 2023 at 20:41, Mark Brown <broonie@kernel.org> wrote:
>> > On Wed, Aug 23, 2023 at 04:15:57PM +0200, Naresh Solanki wrote:
>
>> > > Replace mutexes with atomic operations.
>
>> > Why?  Generally atomics are more complicated and hard to understand and
>> > get right.
>
>> Since the operations involved here are simple & short & can be managed by
>> atomic operation.
>
>Unless there's a strong positive reason to specifically use atomics it
>seems better to avoid them, like I say they're full of landmines with
>unexpeted behaviours and therefore something that sets off alarm bells
>about needing careful study, the mutex is going to be less preformant
>but is also much more clearly correct.

I assume this patch was posted as a result of a comment I made on the 
original patch [1], but in hindsight I probably shouldn't have suggested 
it as it's a relatively minor issue either way -- I think the other 
things brought up in that email are much more significant concerns.  
Honestly I don't think that patch should be applied in its present form, 
though I see it's still present in the regulator/for-next and 
regulator/for-6.6 branches -- Mark, do you intend to include it as-is in 
your pull request to Linus for the 6.6 merge window?

[1] https://lore.kernel.org/lkml/d3ea0fe2-00bb-493b-aca7-ba7a31bd3c78@hatter.bewilderbeest.net/


Zev

Re: [PATCH] regulator: userspace-consumer: Use atomic operation

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1860 bytes --]

On Thu, Aug 24, 2023 at 12:32:22PM -0700, Zev Weiss wrote:
> On Thu, Aug 24, 2023 at 09:55:41AM PDT, Mark Brown wrote:

> > Unless there's a strong positive reason to specifically use atomics it
> > seems better to avoid them, like I say they're full of landmines with
> > unexpeted behaviours and therefore something that sets off alarm bells
> > about needing careful study, the mutex is going to be less preformant
> > but is also much more clearly correct.

> I assume this patch was posted as a result of a comment I made on the
> original patch [1], but in hindsight I probably shouldn't have suggested it

I'm literally on a train right now with very intermittent connectivity:

Please include human readable descriptions of things like commits and
issues being discussed in e-mail in your mails, this makes them much
easier for humans to read especially when they have no internet access.
I do frequently catch up on my mail on flights or while otherwise
travelling so this is even more pressing for me than just being about
making things a bit easier to read.

> [1] https://lore.kernel.org/lkml/d3ea0fe2-00bb-493b-aca7-ba7a31bd3c78@hatter.bewilderbeest.net/

That's review of "regulator: userspace-consumer: Add regulator event
support".  Sorry, I had managed to reply on that thread but it keeps
getting buried under an avalanche of enormous serieses.

> as it's a relatively minor issue either way -- I think the other things
> brought up in that email are much more significant concerns.  Honestly I
> don't think that patch should be applied in its present form, though I see
> it's still present in the regulator/for-next and regulator/for-6.6 branches
> -- Mark, do you intend to include it as-is in your pull request to Linus for
> the 6.6 merge window?

If it's there I'm intending to include it, though I'm still thinking
about the numbers.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]