Add sysfs attribute to track regulator events received from regulator
notifier block handler.

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

diff --git a/drivers/regulator/userspace-consumer.c b/drivers/regulator/userspace-consumer.c
index 97f075ed68c9..a0b980022993 100644
--- a/drivers/regulator/userspace-consumer.c
+++ b/drivers/regulator/userspace-consumer.c
@@ -29,6 +29,10 @@ struct userspace_consumer_data {
 
 	int num_supplies;
 	struct regulator_bulk_data *supplies;
+
+	struct kobject *kobj;
+	struct notifier_block nb;
+	unsigned long events;
 };
 
 static ssize_t name_show(struct device *dev,
@@ -89,12 +93,30 @@ static ssize_t state_store(struct device *dev, struct device_attribute *attr,
 	return count;
 }
 
+static DEFINE_MUTEX(events_lock);
+
+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);
+
+	return sprintf(buf, "0x%lx\n", e);
+}
+
 static DEVICE_ATTR_RO(name);
 static DEVICE_ATTR_RW(state);
+static DEVICE_ATTR_RO(events);
 
 static struct attribute *attributes[] = {
 	&dev_attr_name.attr,
 	&dev_attr_state.attr,
+	&dev_attr_events.attr,
 	NULL,
 };
 
@@ -115,12 +137,28 @@ static const struct attribute_group attr_group = {
 	.is_visible =  attr_visible,
 };
 
+static int regulator_userspace_notify(struct notifier_block *nb,
+				      unsigned long event,
+				      void *ignored)
+{
+	struct userspace_consumer_data *data =
+		container_of(nb, struct userspace_consumer_data, nb);
+
+	mutex_lock(&events_lock);
+	data->events |= event;
+	mutex_unlock(&events_lock);
+
+	sysfs_notify(data->kobj, NULL, dev_attr_events.attr.name);
+
+	return NOTIFY_OK;
+}
+
 static int regulator_userspace_consumer_probe(struct platform_device *pdev)
 {
 	struct regulator_userspace_consumer_data tmpdata;
 	struct regulator_userspace_consumer_data *pdata;
 	struct userspace_consumer_data *drvdata;
-	int ret;
+	int i, ret;
 
 	pdata = dev_get_platdata(&pdev->dev);
 	if (!pdata) {
@@ -153,6 +191,7 @@ static int regulator_userspace_consumer_probe(struct platform_device *pdev)
 	drvdata->num_supplies = pdata->num_supplies;
 	drvdata->supplies = pdata->supplies;
 	drvdata->no_autoswitch = pdata->no_autoswitch;
+	drvdata->kobj = &pdev->dev.kobj;
 
 	mutex_init(&drvdata->lock);
 
@@ -186,6 +225,13 @@ static int regulator_userspace_consumer_probe(struct platform_device *pdev)
 	}
 	drvdata->enabled = !!ret;
 
+	drvdata->nb.notifier_call = regulator_userspace_notify;
+	for (i = 0; i < drvdata->num_supplies; i++) {
+		ret = devm_regulator_register_notifier(drvdata->supplies[i].consumer, &drvdata->nb);
+		if (ret)
+			goto err_enable;
+	}
+
 	return 0;
 
 err_enable:
@@ -197,6 +243,10 @@ static int regulator_userspace_consumer_probe(struct platform_device *pdev)
 static int regulator_userspace_consumer_remove(struct platform_device *pdev)
 {
 	struct userspace_consumer_data *data = platform_get_drvdata(pdev);
+	int i;
+
+	for (i = 0; i < data->num_supplies; i++)
+		devm_regulator_unregister_notifier(data->supplies[i].consumer, &data->nb);
 
 	sysfs_remove_group(&pdev->dev.kobj, &attr_group);
 

base-commit: 4fb53b2377c364e3753d6e293913b57dad68e98b
-- 
2.41.0

On Thu, Aug 03, 2023 at 04:12:25AM PDT, Naresh Solanki wrote:
>Add sysfs attribute to track regulator events received from regulator
>notifier block handler.
>

Hi Naresh,

Could you provide a bit more detail on how this is intended to be used?  
Some of the details (more below) seem a bit odd to me...

>Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
>---
> drivers/regulator/userspace-consumer.c | 52 +++++++++++++++++++++++++-
> 1 file changed, 51 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/regulator/userspace-consumer.c b/drivers/regulator/userspace-consumer.c
>index 97f075ed68c9..a0b980022993 100644
>--- a/drivers/regulator/userspace-consumer.c
>+++ b/drivers/regulator/userspace-consumer.c
>@@ -29,6 +29,10 @@ struct userspace_consumer_data {
>
> 	int num_supplies;
> 	struct regulator_bulk_data *supplies;
>+
>+	struct kobject *kobj;
>+	struct notifier_block nb;
>+	unsigned long events;
> };
>
> static ssize_t name_show(struct device *dev,
>@@ -89,12 +93,30 @@ static ssize_t state_store(struct device *dev, struct device_attribute *attr,
> 	return count;
> }
>
>+static DEFINE_MUTEX(events_lock);
>+
>+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;

...particularly this bit -- a read operation on a read-only file (and 
especially one with 0644 permissions) having side-effects (clearing the 
value it accesses) seems on the face of it like fairly surprising 
behavior.  Is this a pattern that's used elsewhere in any other sysfs 
files?

>+	mutex_unlock(&events_lock);
>+
>+	return sprintf(buf, "0x%lx\n", e);
>+}
>+
> static DEVICE_ATTR_RO(name);
> static DEVICE_ATTR_RW(state);
>+static DEVICE_ATTR_RO(events);

New sysfs attributes should be documented in Documentation/ABI, which 
this appears to be missing.

However, it looks like this would expose the values of all the 
REGULATOR_EVENT_* constants as a userspace-visible ABI -- is that 
something we really want to do?

>
> static struct attribute *attributes[] = {
> 	&dev_attr_name.attr,
> 	&dev_attr_state.attr,
>+	&dev_attr_events.attr,
> 	NULL,
> };
>
>@@ -115,12 +137,28 @@ static const struct attribute_group attr_group = {
> 	.is_visible =  attr_visible,
> };
>
>+static int regulator_userspace_notify(struct notifier_block *nb,
>+				      unsigned long event,
>+				      void *ignored)
>+{
>+	struct userspace_consumer_data *data =
>+		container_of(nb, struct userspace_consumer_data, nb);
>+
>+	mutex_lock(&events_lock);
>+	data->events |= event;
>+	mutex_unlock(&events_lock);
>+

Using a single global mutex (events_lock) to protect a single member of 
a per-device struct looks weird.  Unless there's something subtle going 
on that I'm not seeing, it seems like the lock should be a member of the 
data struct instead of global, and since no blocking operations happen 
under it could it just be a spinlock?  Or since it's just some simple 
updates to a single variable, why not just use an atomic_t and skip the 
lock entirely?

>+	sysfs_notify(data->kobj, NULL, dev_attr_events.attr.name);
>+
>+	return NOTIFY_OK;
>+}
>+
> static int regulator_userspace_consumer_probe(struct platform_device *pdev)
> {
> 	struct regulator_userspace_consumer_data tmpdata;
> 	struct regulator_userspace_consumer_data *pdata;
> 	struct userspace_consumer_data *drvdata;
>-	int ret;
>+	int i, ret;
>
> 	pdata = dev_get_platdata(&pdev->dev);
> 	if (!pdata) {
>@@ -153,6 +191,7 @@ static int regulator_userspace_consumer_probe(struct platform_device *pdev)
> 	drvdata->num_supplies = pdata->num_supplies;
> 	drvdata->supplies = pdata->supplies;
> 	drvdata->no_autoswitch = pdata->no_autoswitch;
>+	drvdata->kobj = &pdev->dev.kobj;
>
> 	mutex_init(&drvdata->lock);
>
>@@ -186,6 +225,13 @@ static int regulator_userspace_consumer_probe(struct platform_device *pdev)
> 	}
> 	drvdata->enabled = !!ret;
>
>+	drvdata->nb.notifier_call = regulator_userspace_notify;
>+	for (i = 0; i < drvdata->num_supplies; i++) {
>+		ret = devm_regulator_register_notifier(drvdata->supplies[i].consumer, &drvdata->nb);
>+		if (ret)
>+			goto err_enable;
>+	}
>+
> 	return 0;
>
> err_enable:
>@@ -197,6 +243,10 @@ static int regulator_userspace_consumer_probe(struct platform_device *pdev)
> static int regulator_userspace_consumer_remove(struct platform_device *pdev)
> {
> 	struct userspace_consumer_data *data = platform_get_drvdata(pdev);
>+	int i;
>+
>+	for (i = 0; i < data->num_supplies; i++)
>+		devm_regulator_unregister_notifier(data->supplies[i].consumer, &data->nb);
>
> 	sysfs_remove_group(&pdev->dev.kobj, &attr_group);
>
>
>base-commit: 4fb53b2377c364e3753d6e293913b57dad68e98b
>-- 
>2.41.0
>

Hi Zev,


On Fri, 4 Aug 2023 at 02:15, Zev Weiss <zev@bewilderbeest.net> wrote:
>
> On Thu, Aug 03, 2023 at 04:12:25AM PDT, Naresh Solanki wrote:
> >Add sysfs attribute to track regulator events received from regulator
> >notifier block handler.
> >
>
> Hi Naresh,
>
> Could you provide a bit more detail on how this is intended to be used?
> Some of the details (more below) seem a bit odd to me...
My application registers a event callback on the 'events' to track regulator
events
Reference:
https://github.com/9elements/pwrseqd/blob/main/src/VoltageRegulatorSysfs.cpp#L258
>
> >Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> >---
> > drivers/regulator/userspace-consumer.c | 52 +++++++++++++++++++++++++-
> > 1 file changed, 51 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/regulator/userspace-consumer.c b/drivers/regulator/userspace-consumer.c
> >index 97f075ed68c9..a0b980022993 100644
> >--- a/drivers/regulator/userspace-consumer.c
> >+++ b/drivers/regulator/userspace-consumer.c
> >@@ -29,6 +29,10 @@ struct userspace_consumer_data {
> >
> >       int num_supplies;
> >       struct regulator_bulk_data *supplies;
> >+
> >+      struct kobject *kobj;
> >+      struct notifier_block nb;
> >+      unsigned long events;
> > };
> >
> > static ssize_t name_show(struct device *dev,
> >@@ -89,12 +93,30 @@ static ssize_t state_store(struct device *dev, struct device_attribute *attr,
> >       return count;
> > }
> >
> >+static DEFINE_MUTEX(events_lock);
> >+
> >+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;
>
> ...particularly this bit -- a read operation on a read-only file (and
> especially one with 0644 permissions) having side-effects (clearing the
> value it accesses) seems on the face of it like fairly surprising
> behavior.  Is this a pattern that's used elsewhere in any other sysfs
> files?
These are regulator events & are valid when it occurs.
Userspace application is intended to consume them as soon as the
event is notified by kernel sysfs_notify.

>
> >+      mutex_unlock(&events_lock);
> >+
> >+      return sprintf(buf, "0x%lx\n", e);
> >+}
> >+
> > static DEVICE_ATTR_RO(name);
> > static DEVICE_ATTR_RW(state);
> >+static DEVICE_ATTR_RO(events);
>
> New sysfs attributes should be documented in Documentation/ABI, which
> this appears to be missing.
Sure I can check.
>
> However, it looks like this would expose the values of all the
> REGULATOR_EVENT_* constants as a userspace-visible ABI -- is that
> something we really want to do?
Yes.
>
> >
> > static struct attribute *attributes[] = {
> >       &dev_attr_name.attr,
> >       &dev_attr_state.attr,
> >+      &dev_attr_events.attr,
> >       NULL,
> > };
> >
> >@@ -115,12 +137,28 @@ static const struct attribute_group attr_group = {
> >       .is_visible =  attr_visible,
> > };
> >
> >+static int regulator_userspace_notify(struct notifier_block *nb,
> >+                                    unsigned long event,
> >+                                    void *ignored)
> >+{
> >+      struct userspace_consumer_data *data =
> >+              container_of(nb, struct userspace_consumer_data, nb);
> >+
> >+      mutex_lock(&events_lock);
> >+      data->events |= event;
> >+      mutex_unlock(&events_lock);
> >+
>
> Using a single global mutex (events_lock) to protect a single member of
> a per-device struct looks weird.  Unless there's something subtle going
> on that I'm not seeing, it seems like the lock should be a member of the
> data struct instead of global, and since no blocking operations happen
> under it could it just be a spinlock?  Or since it's just some simple
> updates to a single variable, why not just use an atomic_t and skip the
> lock entirely?
Intent is that only one thread at a time is to be allowed to access/modify
the data->events variable to prevent potential data corruption and
race conditions. Sure can change it to spinlock or atomic_t.

>
> >+      sysfs_notify(data->kobj, NULL, dev_attr_events.attr.name);
> >+
> >+      return NOTIFY_OK;
> >+}
> >+
> > static int regulator_userspace_consumer_probe(struct platform_device *pdev)
> > {
> >       struct regulator_userspace_consumer_data tmpdata;
> >       struct regulator_userspace_consumer_data *pdata;
> >       struct userspace_consumer_data *drvdata;
> >-      int ret;
> >+      int i, ret;
> >
> >       pdata = dev_get_platdata(&pdev->dev);
> >       if (!pdata) {
> >@@ -153,6 +191,7 @@ static int regulator_userspace_consumer_probe(struct platform_device *pdev)
> >       drvdata->num_supplies = pdata->num_supplies;
> >       drvdata->supplies = pdata->supplies;
> >       drvdata->no_autoswitch = pdata->no_autoswitch;
> >+      drvdata->kobj = &pdev->dev.kobj;
> >
> >       mutex_init(&drvdata->lock);
> >
> >@@ -186,6 +225,13 @@ static int regulator_userspace_consumer_probe(struct platform_device *pdev)
> >       }
> >       drvdata->enabled = !!ret;
> >
> >+      drvdata->nb.notifier_call = regulator_userspace_notify;
> >+      for (i = 0; i < drvdata->num_supplies; i++) {
> >+              ret = devm_regulator_register_notifier(drvdata->supplies[i].consumer, &drvdata->nb);
> >+              if (ret)
> >+                      goto err_enable;
> >+      }
> >+
> >       return 0;
> >
> > err_enable:
> >@@ -197,6 +243,10 @@ static int regulator_userspace_consumer_probe(struct platform_device *pdev)
> > static int regulator_userspace_consumer_remove(struct platform_device *pdev)
> > {
> >       struct userspace_consumer_data *data = platform_get_drvdata(pdev);
> >+      int i;
> >+
> >+      for (i = 0; i < data->num_supplies; i++)
> >+              devm_regulator_unregister_notifier(data->supplies[i].consumer, &data->nb);
> >
> >       sysfs_remove_group(&pdev->dev.kobj, &attr_group);
> >
> >
> >base-commit: 4fb53b2377c364e3753d6e293913b57dad68e98b
> >--
> >2.41.0
> >

On Fri, Aug 04, 2023 at 01:59:44AM PDT, Naresh Solanki wrote:
>Hi Zev,
>
>
>On Fri, 4 Aug 2023 at 02:15, Zev Weiss <zev@bewilderbeest.net> wrote:
>>
>> On Thu, Aug 03, 2023 at 04:12:25AM PDT, Naresh Solanki wrote:
>> >Add sysfs attribute to track regulator events received from regulator
>> >notifier block handler.
>> >
>>
>> Hi Naresh,
>>
>> Could you provide a bit more detail on how this is intended to be used?
>> Some of the details (more below) seem a bit odd to me...
>My application registers a event callback on the 'events' to track regulator
>events
>Reference:
>https://github.com/9elements/pwrseqd/blob/main/src/VoltageRegulatorSysfs.cpp#L258
>>
>> >Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
>> >---
>> > drivers/regulator/userspace-consumer.c | 52 +++++++++++++++++++++++++-
>> > 1 file changed, 51 insertions(+), 1 deletion(-)
>> >
>> >diff --git a/drivers/regulator/userspace-consumer.c b/drivers/regulator/userspace-consumer.c
>> >index 97f075ed68c9..a0b980022993 100644
>> >--- a/drivers/regulator/userspace-consumer.c
>> >+++ b/drivers/regulator/userspace-consumer.c
>> >@@ -29,6 +29,10 @@ struct userspace_consumer_data {
>> >
>> >       int num_supplies;
>> >       struct regulator_bulk_data *supplies;
>> >+
>> >+      struct kobject *kobj;
>> >+      struct notifier_block nb;
>> >+      unsigned long events;
>> > };
>> >
>> > static ssize_t name_show(struct device *dev,
>> >@@ -89,12 +93,30 @@ static ssize_t state_store(struct device *dev, struct device_attribute *attr,
>> >       return count;
>> > }
>> >
>> >+static DEFINE_MUTEX(events_lock);
>> >+
>> >+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;
>>
>> ...particularly this bit -- a read operation on a read-only file (and
>> especially one with 0644 permissions) having side-effects (clearing the
>> value it accesses) seems on the face of it like fairly surprising
>> behavior.  Is this a pattern that's used elsewhere in any other sysfs
>> files?
>These are regulator events & are valid when it occurs.
>Userspace application is intended to consume them as soon as the
>event is notified by kernel sysfs_notify.
>

Sure, but that doesn't really address what I was concerned about -- as 
written this is a read operation on a read-only file (0444, not 0644 as 
I mistakenly wrote above) that nevertheless alters the state of an 
internal kernel data structure.  Can you point to any other sysfs 
attributes that behave like that?  I can't think of one offhand, and I'd 
be reluctant to establish the precedent.

Would a uevent-based mechanism maybe be a better fit for the problem 
you're trying to solve?

>>
>> >+      mutex_unlock(&events_lock);
>> >+
>> >+      return sprintf(buf, "0x%lx\n", e);
>> >+}
>> >+
>> > static DEVICE_ATTR_RO(name);
>> > static DEVICE_ATTR_RW(state);
>> >+static DEVICE_ATTR_RO(events);
>>
>> New sysfs attributes should be documented in Documentation/ABI, which
>> this appears to be missing.
>Sure I can check.
>>
>> However, it looks like this would expose the values of all the
>> REGULATOR_EVENT_* constants as a userspace-visible ABI -- is that
>> something we really want to do?
>Yes.

Given that the REGULATOR_EVENT_* constants are defined in headers under 
include/linux and not include/uapi, it doesn't seem like they were 
intended to be used as part of a userspace-visible interface.  If 
they're going to be, I think they should be moved to the uapi directory 
so that applications can use the proper definitions from the kernel 
instead of manually replicating it on their own (but I suspect we should 
probably find a different approach instead).

>>
>> >
>> > static struct attribute *attributes[] = {
>> >       &dev_attr_name.attr,
>> >       &dev_attr_state.attr,
>> >+      &dev_attr_events.attr,
>> >       NULL,
>> > };
>> >
>> >@@ -115,12 +137,28 @@ static const struct attribute_group attr_group = {
>> >       .is_visible =  attr_visible,
>> > };
>> >
>> >+static int regulator_userspace_notify(struct notifier_block *nb,
>> >+                                    unsigned long event,
>> >+                                    void *ignored)
>> >+{
>> >+      struct userspace_consumer_data *data =
>> >+              container_of(nb, struct userspace_consumer_data, nb);
>> >+
>> >+      mutex_lock(&events_lock);
>> >+      data->events |= event;
>> >+      mutex_unlock(&events_lock);
>> >+
>>
>> Using a single global mutex (events_lock) to protect a single member of
>> a per-device struct looks weird.  Unless there's something subtle going
>> on that I'm not seeing, it seems like the lock should be a member of the
>> data struct instead of global, and since no blocking operations happen
>> under it could it just be a spinlock?  Or since it's just some simple
>> updates to a single variable, why not just use an atomic_t and skip the
>> lock entirely?
>Intent is that only one thread at a time is to be allowed to access/modify
>the data->events variable to prevent potential data corruption and
>race conditions. Sure can change it to spinlock or atomic_t.
>
>>
>> >+      sysfs_notify(data->kobj, NULL, dev_attr_events.attr.name);
>> >+
>> >+      return NOTIFY_OK;
>> >+}
>> >+
>> > static int regulator_userspace_consumer_probe(struct platform_device *pdev)
>> > {
>> >       struct regulator_userspace_consumer_data tmpdata;
>> >       struct regulator_userspace_consumer_data *pdata;
>> >       struct userspace_consumer_data *drvdata;
>> >-      int ret;
>> >+      int i, ret;
>> >
>> >       pdata = dev_get_platdata(&pdev->dev);
>> >       if (!pdata) {
>> >@@ -153,6 +191,7 @@ static int regulator_userspace_consumer_probe(struct platform_device *pdev)
>> >       drvdata->num_supplies = pdata->num_supplies;
>> >       drvdata->supplies = pdata->supplies;
>> >       drvdata->no_autoswitch = pdata->no_autoswitch;
>> >+      drvdata->kobj = &pdev->dev.kobj;
>> >
>> >       mutex_init(&drvdata->lock);
>> >
>> >@@ -186,6 +225,13 @@ static int regulator_userspace_consumer_probe(struct platform_device *pdev)
>> >       }
>> >       drvdata->enabled = !!ret;
>> >
>> >+      drvdata->nb.notifier_call = regulator_userspace_notify;
>> >+      for (i = 0; i < drvdata->num_supplies; i++) {
>> >+              ret = devm_regulator_register_notifier(drvdata->supplies[i].consumer, &drvdata->nb);
>> >+              if (ret)
>> >+                      goto err_enable;
>> >+      }
>> >+
>> >       return 0;
>> >
>> > err_enable:
>> >@@ -197,6 +243,10 @@ static int regulator_userspace_consumer_probe(struct platform_device *pdev)
>> > static int regulator_userspace_consumer_remove(struct platform_device *pdev)
>> > {
>> >       struct userspace_consumer_data *data = platform_get_drvdata(pdev);
>> >+      int i;
>> >+
>> >+      for (i = 0; i < data->num_supplies; i++)
>> >+              devm_regulator_unregister_notifier(data->supplies[i].consumer, &data->nb);
>> >
>> >       sysfs_remove_group(&pdev->dev.kobj, &attr_group);
>> >
>> >
>> >base-commit: 4fb53b2377c364e3753d6e293913b57dad68e98b
>> >--
>> >2.41.0
>> >

On Fri, Aug 04, 2023 at 05:02:02AM -0700, Zev Weiss wrote:
> On Fri, Aug 04, 2023 at 01:59:44AM PDT, Naresh Solanki wrote:
> > On Fri, 4 Aug 2023 at 02:15, Zev Weiss <zev@bewilderbeest.net> wrote:
> > > On Thu, Aug 03, 2023 at 04:12:25AM PDT, Naresh Solanki wrote:

> > > >Add sysfs attribute to track regulator events received from regulator
> > > >notifier block handler.

> > > >+      mutex_lock(&events_lock);
> > > >+      e = data->events;
> > > >+      data->events = 0;

> > > ...particularly this bit -- a read operation on a read-only file (and
> > > especially one with 0644 permissions) having side-effects (clearing the
> > > value it accesses) seems on the face of it like fairly surprising
> > > behavior.  Is this a pattern that's used elsewhere in any other sysfs
> > > files?

> > These are regulator events & are valid when it occurs.
> > Userspace application is intended to consume them as soon as the
> > event is notified by kernel sysfs_notify.

> Sure, but that doesn't really address what I was concerned about -- as
> written this is a read operation on a read-only file (0444, not 0644 as I
> mistakenly wrote above) that nevertheless alters the state of an internal
> kernel data structure.  Can you point to any other sysfs attributes that
> behave like that?  I can't think of one offhand, and I'd be reluctant to
> establish the precedent.

The whole userspace consumer interface is a kludge so I'm not super
concerned about what's effectively clear on read interrupts, ideally
it'd be a file reporting the current status but we don't have a way to
read the current status of everything...

> Would a uevent-based mechanism maybe be a better fit for the problem you're
> trying to solve?

uevents would definitely be good to have, and much better than polling
for apps that can use them, but they don't preclude a read interface.

> > > However, it looks like this would expose the values of all the
> > > REGULATOR_EVENT_* constants as a userspace-visible ABI -- is that
> > > something we really want to do?

> > Yes.

> Given that the REGULATOR_EVENT_* constants are defined in headers under
> include/linux and not include/uapi, it doesn't seem like they were intended
> to be used as part of a userspace-visible interface.  If they're going to
> be, I think they should be moved to the uapi directory so that applications
> can use the proper definitions from the kernel instead of manually
> replicating it on their own (but I suspect we should probably find a
> different approach instead).

This is a concern.  We should probably indirect via strings at least,
but that probably implies a file per event at least.  Due to that I'll
drop this patch for this release.  Sorrt for doing that this late, it's
not ideal - like I said in the other thread I lost this thread under a
bunch of others in my inbox.

Hi Zev,

On Fri, 4 Aug 2023 at 17:32, Zev Weiss <zev@bewilderbeest.net> wrote:
>
> On Fri, Aug 04, 2023 at 01:59:44AM PDT, Naresh Solanki wrote:
> >Hi Zev,
> >
> >
> >On Fri, 4 Aug 2023 at 02:15, Zev Weiss <zev@bewilderbeest.net> wrote:
> >>
> >> On Thu, Aug 03, 2023 at 04:12:25AM PDT, Naresh Solanki wrote:
> >> >Add sysfs attribute to track regulator events received from regulator
> >> >notifier block handler.
> >> >
> >>
> >> Hi Naresh,
> >>
> >> Could you provide a bit more detail on how this is intended to be used?
> >> Some of the details (more below) seem a bit odd to me...
> >My application registers a event callback on the 'events' to track regulator
> >events
> >Reference:
> >https://github.com/9elements/pwrseqd/blob/main/src/VoltageRegulatorSysfs.cpp#L258
> >>
> >> >Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> >> >---
> >> > drivers/regulator/userspace-consumer.c | 52 +++++++++++++++++++++++++-
> >> > 1 file changed, 51 insertions(+), 1 deletion(-)
> >> >
> >> >diff --git a/drivers/regulator/userspace-consumer.c b/drivers/regulator/userspace-consumer.c
> >> >index 97f075ed68c9..a0b980022993 100644
> >> >--- a/drivers/regulator/userspace-consumer.c
> >> >+++ b/drivers/regulator/userspace-consumer.c
> >> >@@ -29,6 +29,10 @@ struct userspace_consumer_data {
> >> >
> >> >       int num_supplies;
> >> >       struct regulator_bulk_data *supplies;
> >> >+
> >> >+      struct kobject *kobj;
> >> >+      struct notifier_block nb;
> >> >+      unsigned long events;
> >> > };
> >> >
> >> > static ssize_t name_show(struct device *dev,
> >> >@@ -89,12 +93,30 @@ static ssize_t state_store(struct device *dev, struct device_attribute *attr,
> >> >       return count;
> >> > }
> >> >
> >> >+static DEFINE_MUTEX(events_lock);
> >> >+
> >> >+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;
> >>
> >> ...particularly this bit -- a read operation on a read-only file (and
> >> especially one with 0644 permissions) having side-effects (clearing the
> >> value it accesses) seems on the face of it like fairly surprising
> >> behavior.  Is this a pattern that's used elsewhere in any other sysfs
> >> files?
> >These are regulator events & are valid when it occurs.
> >Userspace application is intended to consume them as soon as the
> >event is notified by kernel sysfs_notify.
> >
>
> Sure, but that doesn't really address what I was concerned about -- as
> written this is a read operation on a read-only file (0444, not 0644 as
> I mistakenly wrote above) that nevertheless alters the state of an
> internal kernel data structure.  Can you point to any other sysfs
> attributes that behave like that?  I can't think of one offhand, and I'd
> be reluctant to establish the precedent.
I guess many hwmon properties on input are readonly & its possible to
send sysfs_notify on the properties.
Like in
https://github.com/torvalds/linux/blob/master/drivers/hwmon/hwmon.c#L668

>
> Would a uevent-based mechanism maybe be a better fit for the problem
> you're trying to solve?
If the application also needs uevent then that can be added as done in hwmon.
>
> >>
> >> >+      mutex_unlock(&events_lock);
> >> >+
> >> >+      return sprintf(buf, "0x%lx\n", e);
> >> >+}
> >> >+
> >> > static DEVICE_ATTR_RO(name);
> >> > static DEVICE_ATTR_RW(state);
> >> >+static DEVICE_ATTR_RO(events);
> >>
> >> New sysfs attributes should be documented in Documentation/ABI, which
> >> this appears to be missing.
> >Sure I can check.
For Documentation/ABI, 'sysfs-driver-regulator-output' below. let me know
if this looks ok.
What:           /sys/bus/platform/drivers/regulator-output/*/events
Date:           August 2023
Contact:        Naresh Solanki <naresh.solanki@9elements.com>
Description:    Provided regulator events.

                Read provides various events the regulator associated with the
                driver has encountered. All REGULATOR_EVENT_* are
                defined in include/linux/regulator/consumer.h

                e.g.
                cat /sys/bus/platform/drivers/regulator-output/ssb_rssd32/events
                0x0
> >>
> >> However, it looks like this would expose the values of all the
> >> REGULATOR_EVENT_* constants as a userspace-visible ABI -- is that
> >> something we really want to do?
> >Yes.
>
> Given that the REGULATOR_EVENT_* constants are defined in headers under
> include/linux and not include/uapi, it doesn't seem like they were
> intended to be used as part of a userspace-visible interface.  If
> they're going to be, I think they should be moved to the uapi directory
> so that applications can use the proper definitions from the kernel
> instead of manually replicating it on their own (but I suspect we should
> probably find a different approach instead).
Yes they have to be moved to include/uapi in that case.
>
> >>
> >> >
> >> > static struct attribute *attributes[] = {
> >> >       &dev_attr_name.attr,
> >> >       &dev_attr_state.attr,
> >> >+      &dev_attr_events.attr,
> >> >       NULL,
> >> > };
> >> >
> >> >@@ -115,12 +137,28 @@ static const struct attribute_group attr_group = {
> >> >       .is_visible =  attr_visible,
> >> > };
> >> >
> >> >+static int regulator_userspace_notify(struct notifier_block *nb,
> >> >+                                    unsigned long event,
> >> >+                                    void *ignored)
> >> >+{
> >> >+      struct userspace_consumer_data *data =
> >> >+              container_of(nb, struct userspace_consumer_data, nb);
> >> >+
> >> >+      mutex_lock(&events_lock);
> >> >+      data->events |= event;
> >> >+      mutex_unlock(&events_lock);
> >> >+
> >>
> >> Using a single global mutex (events_lock) to protect a single member of
> >> a per-device struct looks weird.  Unless there's something subtle going
> >> on that I'm not seeing, it seems like the lock should be a member of the
> >> data struct instead of global, and since no blocking operations happen
> >> under it could it just be a spinlock?  Or since it's just some simple
> >> updates to a single variable, why not just use an atomic_t and skip the
> >> lock entirely?
> >Intent is that only one thread at a time is to be allowed to access/modify
> >the data->events variable to prevent potential data corruption and
> >race conditions. Sure can change it to spinlock or atomic_t.
> >
> >>
> >> >+      sysfs_notify(data->kobj, NULL, dev_attr_events.attr.name);
> >> >+
> >> >+      return NOTIFY_OK;
> >> >+}
> >> >+
> >> > static int regulator_userspace_consumer_probe(struct platform_device *pdev)
> >> > {
> >> >       struct regulator_userspace_consumer_data tmpdata;
> >> >       struct regulator_userspace_consumer_data *pdata;
> >> >       struct userspace_consumer_data *drvdata;
> >> >-      int ret;
> >> >+      int i, ret;
> >> >
> >> >       pdata = dev_get_platdata(&pdev->dev);
> >> >       if (!pdata) {
> >> >@@ -153,6 +191,7 @@ static int regulator_userspace_consumer_probe(struct platform_device *pdev)
> >> >       drvdata->num_supplies = pdata->num_supplies;
> >> >       drvdata->supplies = pdata->supplies;
> >> >       drvdata->no_autoswitch = pdata->no_autoswitch;
> >> >+      drvdata->kobj = &pdev->dev.kobj;
> >> >
> >> >       mutex_init(&drvdata->lock);
> >> >
> >> >@@ -186,6 +225,13 @@ static int regulator_userspace_consumer_probe(struct platform_device *pdev)
> >> >       }
> >> >       drvdata->enabled = !!ret;
> >> >
> >> >+      drvdata->nb.notifier_call = regulator_userspace_notify;
> >> >+      for (i = 0; i < drvdata->num_supplies; i++) {
> >> >+              ret = devm_regulator_register_notifier(drvdata->supplies[i].consumer, &drvdata->nb);
> >> >+              if (ret)
> >> >+                      goto err_enable;
> >> >+      }
> >> >+
> >> >       return 0;
> >> >
> >> > err_enable:
> >> >@@ -197,6 +243,10 @@ static int regulator_userspace_consumer_probe(struct platform_device *pdev)
> >> > static int regulator_userspace_consumer_remove(struct platform_device *pdev)
> >> > {
> >> >       struct userspace_consumer_data *data = platform_get_drvdata(pdev);
> >> >+      int i;
> >> >+
> >> >+      for (i = 0; i < data->num_supplies; i++)
> >> >+              devm_regulator_unregister_notifier(data->supplies[i].consumer, &data->nb);
> >> >
> >> >       sysfs_remove_group(&pdev->dev.kobj, &attr_group);
> >> >
> >> >
> >> >base-commit: 4fb53b2377c364e3753d6e293913b57dad68e98b
> >> >--
> >> >2.41.0
> >> >

On Thu, 03 Aug 2023 13:12:25 +0200, Naresh Solanki wrote:
> Add sysfs attribute to track regulator events received from regulator
> notifier block handler.
> 
> 

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next

Thanks!

[1/1] regulator: userspace-consumer: Add regulator event support
      commit: 22475bcc2083196544fa55b861d76e0e7ee9da11

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark