[PATCH 1/3] regulator: Add uapi header

[PATCH 1/3] regulator: Add uapi header

[Naresh Solanki]

Add UAPI header for regulator.
Include regulator events in the header.

Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
---
 include/linux/regulator/consumer.h | 47 +--------------------------
 include/uapi/linux/regulator.h     | 52 ++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 46 deletions(-)
 create mode 100644 include/uapi/linux/regulator.h

diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index 39b666b40ea6..fb65cf35beca 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -33,6 +33,7 @@
 
 #include <linux/err.h>
 #include <linux/suspend.h>
+#include <uapi/linux/regulator.h>
 
 struct device;
 struct notifier_block;
@@ -84,52 +85,6 @@ struct regulator_dev;
 #define REGULATOR_MODE_IDLE			0x4
 #define REGULATOR_MODE_STANDBY			0x8
 
-/*
- * Regulator notifier events.
- *
- * UNDER_VOLTAGE  Regulator output is under voltage.
- * OVER_CURRENT   Regulator output current is too high.
- * REGULATION_OUT Regulator output is out of regulation.
- * FAIL           Regulator output has failed.
- * OVER_TEMP      Regulator over temp.
- * FORCE_DISABLE  Regulator forcibly shut down by software.
- * VOLTAGE_CHANGE Regulator voltage changed.
- *                Data passed is old voltage cast to (void *).
- * DISABLE        Regulator was disabled.
- * PRE_VOLTAGE_CHANGE   Regulator is about to have voltage changed.
- *                      Data passed is "struct pre_voltage_change_data"
- * ABORT_VOLTAGE_CHANGE Regulator voltage change failed for some reason.
- *                      Data passed is old voltage cast to (void *).
- * PRE_DISABLE    Regulator is about to be disabled
- * ABORT_DISABLE  Regulator disable failed for some reason
- *
- * NOTE: These events can be OR'ed together when passed into handler.
- */
-
-#define REGULATOR_EVENT_UNDER_VOLTAGE		0x01
-#define REGULATOR_EVENT_OVER_CURRENT		0x02
-#define REGULATOR_EVENT_REGULATION_OUT		0x04
-#define REGULATOR_EVENT_FAIL			0x08
-#define REGULATOR_EVENT_OVER_TEMP		0x10
-#define REGULATOR_EVENT_FORCE_DISABLE		0x20
-#define REGULATOR_EVENT_VOLTAGE_CHANGE		0x40
-#define REGULATOR_EVENT_DISABLE			0x80
-#define REGULATOR_EVENT_PRE_VOLTAGE_CHANGE	0x100
-#define REGULATOR_EVENT_ABORT_VOLTAGE_CHANGE	0x200
-#define REGULATOR_EVENT_PRE_DISABLE		0x400
-#define REGULATOR_EVENT_ABORT_DISABLE		0x800
-#define REGULATOR_EVENT_ENABLE			0x1000
-/*
- * Following notifications should be emitted only if detected condition
- * is such that the HW is likely to still be working but consumers should
- * take a recovery action to prevent problems esacalating into errors.
- */
-#define REGULATOR_EVENT_UNDER_VOLTAGE_WARN	0x2000
-#define REGULATOR_EVENT_OVER_CURRENT_WARN	0x4000
-#define REGULATOR_EVENT_OVER_VOLTAGE_WARN	0x8000
-#define REGULATOR_EVENT_OVER_TEMP_WARN		0x10000
-#define REGULATOR_EVENT_WARN_MASK		0x1E000
-
 /*
  * Regulator errors that can be queried using regulator_get_error_flags
  *
diff --git a/include/uapi/linux/regulator.h b/include/uapi/linux/regulator.h
new file mode 100644
index 000000000000..d5f94a301841
--- /dev/null
+++ b/include/uapi/linux/regulator.h
@@ -0,0 +1,52 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __UAPI_LINUX_REGULATOR_CONSUMER_H_
+#define __UAPI_LINUX_REGULATOR_CONSUMER_H_
+
+/*
+ * Regulator notifier events.
+ *
+ * UNDER_VOLTAGE  Regulator output is under voltage.
+ * OVER_CURRENT   Regulator output current is too high.
+ * REGULATION_OUT Regulator output is out of regulation.
+ * FAIL           Regulator output has failed.
+ * OVER_TEMP      Regulator over temp.
+ * FORCE_DISABLE  Regulator forcibly shut down by software.
+ * VOLTAGE_CHANGE Regulator voltage changed.
+ *                Data passed is old voltage cast to (void *).
+ * DISABLE        Regulator was disabled.
+ * PRE_VOLTAGE_CHANGE   Regulator is about to have voltage changed.
+ *                      Data passed is "struct pre_voltage_change_data"
+ * ABORT_VOLTAGE_CHANGE Regulator voltage change failed for some reason.
+ *                      Data passed is old voltage cast to (void *).
+ * PRE_DISABLE    Regulator is about to be disabled
+ * ABORT_DISABLE  Regulator disable failed for some reason
+ *
+ * NOTE: These events can be OR'ed together when passed into handler.
+ */
+
+#define REGULATOR_EVENT_UNDER_VOLTAGE		0x01
+#define REGULATOR_EVENT_OVER_CURRENT		0x02
+#define REGULATOR_EVENT_REGULATION_OUT		0x04
+#define REGULATOR_EVENT_FAIL			0x08
+#define REGULATOR_EVENT_OVER_TEMP		0x10
+#define REGULATOR_EVENT_FORCE_DISABLE		0x20
+#define REGULATOR_EVENT_VOLTAGE_CHANGE		0x40
+#define REGULATOR_EVENT_DISABLE			0x80
+#define REGULATOR_EVENT_PRE_VOLTAGE_CHANGE	0x100
+#define REGULATOR_EVENT_ABORT_VOLTAGE_CHANGE	0x200
+#define REGULATOR_EVENT_PRE_DISABLE		0x400
+#define REGULATOR_EVENT_ABORT_DISABLE		0x800
+#define REGULATOR_EVENT_ENABLE			0x1000
+/*
+ * Following notifications should be emitted only if detected condition
+ * is such that the HW is likely to still be working but consumers should
+ * take a recovery action to prevent problems esacalating into errors.
+ */
+#define REGULATOR_EVENT_UNDER_VOLTAGE_WARN	0x2000
+#define REGULATOR_EVENT_OVER_CURRENT_WARN	0x4000
+#define REGULATOR_EVENT_OVER_VOLTAGE_WARN	0x8000
+#define REGULATOR_EVENT_OVER_TEMP_WARN		0x10000
+#define REGULATOR_EVENT_WARN_MASK		0x1E000
+
+#endif

base-commit: 35d0d2350d774fecf596cfb2fb050559fe5e1850
-- 
2.41.0

[PATCH 2/3] regulator: userspace-consumer: Add regulator event support

[Naresh Solanki]

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 | 54 +++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/userspace-consumer.c b/drivers/regulator/userspace-consumer.c
index 97f075ed68c9..a936661d99cd 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_SPINLOCK(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;
+
+	spin_lock(&events_lock);
+	e = data->events;
+	data->events = 0;
+	spin_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,30 @@ 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);
+	static const char * const *envp[] = { "NAME=events", NULL };
+
+	spin_lock(&events_lock);
+	data->events |= event;
+	spin_unlock(&events_lock);
+
+	sysfs_notify(data->kobj, NULL, dev_attr_events.attr.name);
+	kobject_uevent_env(data->kobj, KOBJ_CHANGE, envp);
+
+	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 +193,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 +227,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 +245,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);
 
-- 
2.41.0

[PATCH 3/3] Documentation: ABI: sysfs-driver-regulator-output

[Naresh Solanki]

Adds sysfs-driver-regulator-output

Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
---
 .../ABI/testing/sysfs-driver-regulator-output         | 11 +++++++++++
 1 file changed, 11 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-regulator-output

diff --git a/Documentation/ABI/testing/sysfs-driver-regulator-output b/Documentation/ABI/testing/sysfs-driver-regulator-output
new file mode 100644
index 000000000000..f9b0a8f810fa
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-regulator-output
@@ -0,0 +1,11 @@
+What:		/sys/bus/platform/drivers/regulator-output/*/events
+Date:		August 2023
+Description:	Provided regulator events.
+
+		Read provides various events the regulator associated with the
+		driver has encountered. All REGULATOR_EVENT_* are
+		defined in include/uapi/linux/regulator.h
+
+		e.g.
+		cat /sys/bus/platform/drivers/regulator-output/ssb_rssd32/events
+		0x0
-- 
2.41.0

Re: [PATCH 1/3] regulator: Add uapi header

[Mark Brown]

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

On Thu, Aug 31, 2023 at 02:14:08PM +0200, Naresh Solanki wrote:

> +#define REGULATOR_EVENT_UNDER_VOLTAGE		0x01
> +#define REGULATOR_EVENT_OVER_CURRENT		0x02
> +#define REGULATOR_EVENT_REGULATION_OUT		0x04
> +#define REGULATOR_EVENT_FAIL			0x08
> +#define REGULATOR_EVENT_OVER_TEMP		0x10
> +#define REGULATOR_EVENT_FORCE_DISABLE		0x20
> +#define REGULATOR_EVENT_VOLTAGE_CHANGE		0x40
> +#define REGULATOR_EVENT_DISABLE			0x80
> +#define REGULATOR_EVENT_PRE_VOLTAGE_CHANGE	0x100
> +#define REGULATOR_EVENT_ABORT_VOLTAGE_CHANGE	0x200
> +#define REGULATOR_EVENT_PRE_DISABLE		0x400
> +#define REGULATOR_EVENT_ABORT_DISABLE		0x800
> +#define REGULATOR_EVENT_ENABLE			0x1000

I'm not sure embedded the raw numbers in the UAPI like this for just one
driver is ideal - it might be better to do something string based,
though that would result in us ending up with a file per event which is
a bit noisy.

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

Re: [PATCH 1/3] regulator: Add uapi header

[kernel test robot]

Hi Naresh,

kernel test robot noticed the following build errors:

[auto build test ERROR on 35d0d2350d774fecf596cfb2fb050559fe5e1850]

url:    https://github.com/intel-lab-lkp/linux/commits/Naresh-Solanki/regulator-userspace-consumer-Add-regulator-event-support/20230831-201619
base:   35d0d2350d774fecf596cfb2fb050559fe5e1850
patch link:    https://lore.kernel.org/r/20230831121412.2359239-1-Naresh.Solanki%409elements.com
patch subject: [PATCH 1/3] regulator: Add uapi header
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230831/202308312120.dESwyZdm-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230831/202308312120.dESwyZdm-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/202308312120.dESwyZdm-lkp@intel.com/

All errors (new ones prefixed by >>):

   scripts/genksyms/parse.y: warning: 9 shift/reduce conflicts [-Wconflicts-sr]
   scripts/genksyms/parse.y: warning: 5 reduce/reduce conflicts [-Wconflicts-rr]
   scripts/genksyms/parse.y: note: rerun with option '-Wcounterexamples' to generate conflict counterexamples
>> error: include/uapi/linux/regulator.h: missing "WITH Linux-syscall-note" for SPDX-License-Identifier
   make[3]: *** [scripts/Makefile.headersinst:63: usr/include/linux/regulator.h] Error 1
   make[3]: Target '__headers' not remade because of errors.
   make[2]: *** [Makefile:1383: headers] Error 2
   make[2]: Target 'prepare' not remade because of errors.
   make[1]: *** [Makefile:234: __sub-make] Error 2
   make[1]: Target 'prepare' not remade because of errors.
   make: *** [Makefile:234: __sub-make] Error 2
   make: Target 'prepare' not remade because of errors.

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/3] regulator: Add uapi header

[Naresh Solanki]

Hi


On Thu, 31 Aug 2023 at 18:05, Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Aug 31, 2023 at 02:14:08PM +0200, Naresh Solanki wrote:
>
> > +#define REGULATOR_EVENT_UNDER_VOLTAGE                0x01
> > +#define REGULATOR_EVENT_OVER_CURRENT         0x02
> > +#define REGULATOR_EVENT_REGULATION_OUT               0x04
> > +#define REGULATOR_EVENT_FAIL                 0x08
> > +#define REGULATOR_EVENT_OVER_TEMP            0x10
> > +#define REGULATOR_EVENT_FORCE_DISABLE                0x20
> > +#define REGULATOR_EVENT_VOLTAGE_CHANGE               0x40
> > +#define REGULATOR_EVENT_DISABLE                      0x80
> > +#define REGULATOR_EVENT_PRE_VOLTAGE_CHANGE   0x100
> > +#define REGULATOR_EVENT_ABORT_VOLTAGE_CHANGE 0x200
> > +#define REGULATOR_EVENT_PRE_DISABLE          0x400
> > +#define REGULATOR_EVENT_ABORT_DISABLE                0x800
> > +#define REGULATOR_EVENT_ENABLE                       0x1000
>
> I'm not sure embedded the raw numbers in the UAPI like this for just one
> driver is ideal - it might be better to do something string based,
> though that would result in us ending up with a file per event which is
> a bit noisy.
I guess this is the standard approach & can be a starting point for
future use cases.

If you still insist on string based then can you please elaborate on it.
Is the events attribute is expected to print string of space separated
events like
for REGULATOR_EVENT_ENABLE -> ENABLE
And even multiple events like: 'UNDER_VOLTAGE REGULATION_OUT'

Let me know your thoughts.

Regards,
Naresh

Re: [PATCH 3/3] Documentation: ABI: sysfs-driver-regulator-output

[Zev Weiss]

Adding Greg re: sysfs ABI design...

On Thu, Aug 31, 2023 at 05:14:10AM PDT, Naresh Solanki wrote:
>Adds sysfs-driver-regulator-output
>
>Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
>---
> .../ABI/testing/sysfs-driver-regulator-output         | 11 +++++++++++
> 1 file changed, 11 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-driver-regulator-output
>
>diff --git a/Documentation/ABI/testing/sysfs-driver-regulator-output b/Documentation/ABI/testing/sysfs-driver-regulator-output
>new file mode 100644
>index 000000000000..f9b0a8f810fa
>--- /dev/null
>+++ b/Documentation/ABI/testing/sysfs-driver-regulator-output
>@@ -0,0 +1,11 @@
>+What:		/sys/bus/platform/drivers/regulator-output/*/events
>+Date:		August 2023
>+Description:	Provided regulator events.
>+
>+		Read provides various events the regulator associated with the
>+		driver has encountered. All REGULATOR_EVENT_* are
>+		defined in include/uapi/linux/regulator.h
>+
>+		e.g.
>+		cat /sys/bus/platform/drivers/regulator-output/ssb_rssd32/events
>+		0x0

If we really are going to proceed with a "read with side-effects" 
(clear-on-read) design, that should absolutely be loudly and clearly 
documented, since it's very different from how sysfs files typically 
work and hence a somewhat glaring principle-of-least-surprise violation.  
Also, since from the code it looks like it's intended to be used via 
poll(2), that should be described here as well.


Zev

Re: [PATCH 2/3] regulator: userspace-consumer: Add regulator event support

[Zev Weiss]

On Thu, Aug 31, 2023 at 05:14:09AM PDT, Naresh Solanki wrote:
>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 | 54 +++++++++++++++++++++++++-
> 1 file changed, 53 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/regulator/userspace-consumer.c b/drivers/regulator/userspace-consumer.c
>index 97f075ed68c9..a936661d99cd 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_SPINLOCK(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;
>+
>+	spin_lock(&events_lock);
>+	e = data->events;
>+	data->events = 0;

I still don't think this is a good solution for the problem.

I for one frequently examine things in sysfs using shell commands like 
'cat' and 'grep' and such, and I suspect I'm (very, very) far from alone 
in that.  With this design a user doing that could cause a monitoring 
daemon to miss events that it was expecting to receive via this file.  

I don't think we should be creating sysfs files that are secretly land 
mines that allow a curious user innocently peeking around in sysfs doing 
(they think) read-only operations to break things for other programs 
using those files.


Zev

Re: [PATCH 1/3] regulator: Add uapi header

[Mark Brown]

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

On Fri, Sep 01, 2023 at 12:04:13AM +0530, Naresh Solanki wrote:

> If you still insist on string based then can you please elaborate on it.
> Is the events attribute is expected to print string of space separated
> events like
> for REGULATOR_EVENT_ENABLE -> ENABLE
> And even multiple events like: 'UNDER_VOLTAGE REGULATION_OUT'

> Let me know your thoughts.

You'd need to have a file per event, the string would be the filename.

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

Re: [PATCH 2/3] regulator: userspace-consumer: Add regulator event support

[Mark Brown]

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

On Fri, Sep 01, 2023 at 02:27:28AM -0700, Zev Weiss wrote:

> I for one frequently examine things in sysfs using shell commands like 'cat'
> and 'grep' and such, and I suspect I'm (very, very) far from alone in that.
> With this design a user doing that could cause a monitoring daemon to miss
> events that it was expecting to receive via this file.

> I don't think we should be creating sysfs files that are secretly land mines
> that allow a curious user innocently peeking around in sysfs doing (they
> think) read-only operations to break things for other programs using those
> files.

It could be switched to clear on read.  Like I said on the prior version
the userspace consumer is a bit of a sharp edge at the best of times.

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

Re: [PATCH 3/3] Documentation: ABI: sysfs-driver-regulator-output

[Greg Kroah-Hartman]

On Fri, Sep 01, 2023 at 02:13:23AM -0700, Zev Weiss wrote:
> Adding Greg re: sysfs ABI design...
> 
> On Thu, Aug 31, 2023 at 05:14:10AM PDT, Naresh Solanki wrote:
> > Adds sysfs-driver-regulator-output
> > 
> > Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> > ---
> > .../ABI/testing/sysfs-driver-regulator-output         | 11 +++++++++++
> > 1 file changed, 11 insertions(+)
> > create mode 100644 Documentation/ABI/testing/sysfs-driver-regulator-output
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-driver-regulator-output b/Documentation/ABI/testing/sysfs-driver-regulator-output
> > new file mode 100644
> > index 000000000000..f9b0a8f810fa
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-driver-regulator-output
> > @@ -0,0 +1,11 @@
> > +What:		/sys/bus/platform/drivers/regulator-output/*/events
> > +Date:		August 2023
> > +Description:	Provided regulator events.
> > +
> > +		Read provides various events the regulator associated with the
> > +		driver has encountered. All REGULATOR_EVENT_* are
> > +		defined in include/uapi/linux/regulator.h
> > +
> > +		e.g.
> > +		cat /sys/bus/platform/drivers/regulator-output/ssb_rssd32/events
> > +		0x0
> 
> If we really are going to proceed with a "read with side-effects"
> (clear-on-read) design, that should absolutely be loudly and clearly
> documented, since it's very different from how sysfs files typically work
> and hence a somewhat glaring principle-of-least-surprise violation.  Also,
> since from the code it looks like it's intended to be used via poll(2), that
> should be described here as well.

Poll?  Ick, but that can happen.

But yes, reading a sysfs should almost never cause a side affect at all.

But what do you mean by "clear events?"  sysfs is only "one value per
file", so you are only returning a single event, right?  It can't be a
list of events, sorry, that's not ok, use a different api for that
please (like a char device node.)

thanks,

greg k-h

Re: [PATCH 3/3] Documentation: ABI: sysfs-driver-regulator-output

[Zev Weiss]

On Sun, Sep 03, 2023 at 06:04:23AM PDT, Greg Kroah-Hartman wrote:
>On Fri, Sep 01, 2023 at 02:13:23AM -0700, Zev Weiss wrote:
>> Adding Greg re: sysfs ABI design...
>>
>> On Thu, Aug 31, 2023 at 05:14:10AM PDT, Naresh Solanki wrote:
>> > Adds sysfs-driver-regulator-output
>> >
>> > Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
>> > ---
>> > .../ABI/testing/sysfs-driver-regulator-output         | 11 +++++++++++
>> > 1 file changed, 11 insertions(+)
>> > create mode 100644 Documentation/ABI/testing/sysfs-driver-regulator-output
>> >
>> > diff --git a/Documentation/ABI/testing/sysfs-driver-regulator-output b/Documentation/ABI/testing/sysfs-driver-regulator-output
>> > new file mode 100644
>> > index 000000000000..f9b0a8f810fa
>> > --- /dev/null
>> > +++ b/Documentation/ABI/testing/sysfs-driver-regulator-output
>> > @@ -0,0 +1,11 @@
>> > +What:		/sys/bus/platform/drivers/regulator-output/*/events
>> > +Date:		August 2023
>> > +Description:	Provided regulator events.
>> > +
>> > +		Read provides various events the regulator associated with the
>> > +		driver has encountered. All REGULATOR_EVENT_* are
>> > +		defined in include/uapi/linux/regulator.h
>> > +
>> > +		e.g.
>> > +		cat /sys/bus/platform/drivers/regulator-output/ssb_rssd32/events
>> > +		0x0
>>
>> If we really are going to proceed with a "read with side-effects"
>> (clear-on-read) design, that should absolutely be loudly and clearly
>> documented, since it's very different from how sysfs files typically work
>> and hence a somewhat glaring principle-of-least-surprise violation.  Also,
>> since from the code it looks like it's intended to be used via poll(2), that
>> should be described here as well.
>
>Poll?  Ick, but that can happen.
>
>But yes, reading a sysfs should almost never cause a side affect at all.
>
>But what do you mean by "clear events?"  

I mean that when the sysfs file is read, the state variable whose value 
it exposes is also cleared as a side-effect (so the read operation 
"consumes" the value it returns) -- see the implementation in patch 2 of 
this series (specifically the 'data->events = 0' assignment in 
events_show()):

https://lore.kernel.org/lkml/20230831121412.2359239-2-Naresh.Solanki@9elements.com/

(As indicated in my reply to that patch, this seems like a footgun to me 
and I'm hoping we can come up with a better approach.)


Zev

Re: [PATCH 3/3] Documentation: ABI: sysfs-driver-regulator-output

[Mark Brown]

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

On Sun, Sep 03, 2023 at 05:48:14PM -0700, Zev Weiss wrote:
> On Sun, Sep 03, 2023 at 06:04:23AM PDT, Greg Kroah-Hartman wrote:

> > But yes, reading a sysfs should almost never cause a side affect at all.

> > But what do you mean by "clear events?"

> I mean that when the sysfs file is read, the state variable whose value it
> exposes is also cleared as a side-effect (so the read operation "consumes"
> the value it returns) -- see the implementation in patch 2 of this series
> (specifically the 'data->events = 0' assignment in events_show()):

It's a clear on read interrupt.

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

Re: [PATCH 3/3] Documentation: ABI: sysfs-driver-regulator-output

[Zev Weiss]

On Mon, Sep 04, 2023 at 05:24:31AM PDT, Mark Brown wrote:
>On Sun, Sep 03, 2023 at 05:48:14PM -0700, Zev Weiss wrote:
>> On Sun, Sep 03, 2023 at 06:04:23AM PDT, Greg Kroah-Hartman wrote:
>
>> > But yes, reading a sysfs should almost never cause a side affect at all.
>
>> > But what do you mean by "clear events?"
>
>> I mean that when the sysfs file is read, the state variable whose value it
>> exposes is also cleared as a side-effect (so the read operation "consumes"
>> the value it returns) -- see the implementation in patch 2 of this series
>> (specifically the 'data->events = 0' assignment in events_show()):
>
>It's a clear on read interrupt.

Sure, analogous behavior in hardware is reasonably common, but that 
doesn't strike me as a very compelling reason to design the 
kernel<->userspace interface to mimic it -- providing nicer interfaces 
than the raw hardware is one of the main reasons for having an OS in the 
first place, after all.


Zev

Re: [PATCH 3/3] Documentation: ABI: sysfs-driver-regulator-output

[Mark Brown]

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

On Sun, Sep 10, 2023 at 01:50:37PM -0700, Zev Weiss wrote:
> On Mon, Sep 04, 2023 at 05:24:31AM PDT, Mark Brown wrote:

> > It's a clear on read interrupt.

> Sure, analogous behavior in hardware is reasonably common, but that doesn't
> strike me as a very compelling reason to design the kernel<->userspace
> interface to mimic it -- providing nicer interfaces than the raw hardware is
> one of the main reasons for having an OS in the first place, after all.

If it were something other than the userspace consumer I'd be more
worried, with this interface it's all sharp edges on a very embedded
system so it's a bit less concerning.  In any case the obvious fix here
is to use clear on read.

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

Re: [PATCH 3/3] Documentation: ABI: sysfs-driver-regulator-output

[Mark Brown]

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

On Sun, Sep 10, 2023 at 01:50:37PM -0700, Zev Weiss wrote:
> On Mon, Sep 04, 2023 at 05:24:31AM PDT, Mark Brown wrote:

> > It's a clear on read interrupt.

> Sure, analogous behavior in hardware is reasonably common, but that doesn't
> strike me as a very compelling reason to design the kernel<->userspace
> interface to mimic it -- providing nicer interfaces than the raw hardware is
> one of the main reasons for having an OS in the first place, after all.

If it were something other than the userspace consumer I'd be a bit more
concerned but that's all sharp edges and direct access in a very
controlled system.  In any case clear on write is the obvious
alternative approach.

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

Re: [PATCH 3/3] Documentation: ABI: sysfs-driver-regulator-output

[Zev Weiss]

On Tue, Sep 12, 2023 at 07:03:47AM PDT, Mark Brown wrote:
>On Sun, Sep 10, 2023 at 01:50:37PM -0700, Zev Weiss wrote:
>> On Mon, Sep 04, 2023 at 05:24:31AM PDT, Mark Brown wrote:
>
>> > It's a clear on read interrupt.
>
>> Sure, analogous behavior in hardware is reasonably common, but that doesn't
>> strike me as a very compelling reason to design the kernel<->userspace
>> interface to mimic it -- providing nicer interfaces than the raw hardware is
>> one of the main reasons for having an OS in the first place, after all.
>
>If it were something other than the userspace consumer I'd be a bit more
>concerned but that's all sharp edges and direct access in a very
>controlled system.  In any case clear on write is the obvious
>alternative approach.

I'm using this driver in production systems, and I think 
Naresh/9elements do or intend to as well (and in my case at least, 
they're systems human operators can and do log in to).  I, for one, 
would thus very much prefer it be treated as a first-class citizen and 
afforded considerations of robustness and such as with any other driver.  
(I'm not entirely sure what other sharp edges with it you're referring 
to.)

To make a slightly more concrete proposal (or perhaps just flesh out one 
I vaguely gestured at previously), how about something along the lines 
of the below, as a modification on top of Naresh's patch -- most of the 
code to do it via uevents is already there anyway.  With this code in 
place I can run 'udevadm monitor -p' and see the expected events 
delivered when I manually enable & disable the regulator via its 'state' 
sysfs attribute, which I think basically fulfills the requirements we're 
aiming for?  Naresh, could using netlink/uevents work for your needs?


Thanks,
Zev


diff --git a/drivers/regulator/userspace-consumer.c b/drivers/regulator/userspace-consumer.c
index 74247e526a42..df783ca02757 100644
--- a/drivers/regulator/userspace-consumer.c
+++ b/drivers/regulator/userspace-consumer.c
@@ -32,7 +32,6 @@ struct userspace_consumer_data {
  
  	struct kobject *kobj;
  	struct notifier_block nb;
-	unsigned long events;
  };
  
  static ssize_t name_show(struct device *dev,
@@ -93,30 +92,12 @@ static ssize_t state_store(struct device *dev, struct device_attribute *attr,
  	return count;
  }
  
-static DEFINE_SPINLOCK(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;
-
-	spin_lock(&events_lock);
-	e = data->events;
-	data->events = 0;
-	spin_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,
  };
  
@@ -137,19 +118,35 @@ static const struct attribute_group attr_group = {
  	.is_visible =  attr_visible,
  };
  
+/*
+ * This will of course need more of a real implementation (handling more than
+ * a single set event bit) and should probably live somewhere else, but for
+ * the sake of brevity...
+ */
+static const char *regulator_event_str(unsigned long event)
+{
+	switch (event) {
+	case REGULATOR_EVENT_PRE_DISABLE:
+		return "pre-disable";
+	case REGULATOR_EVENT_DISABLE:
+		return "disable";
+	case REGULATOR_EVENT_ENABLE:
+		return "enable";
+	default:
+		return "NYI";
+	}
+}
+
  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);
-	static const char * const *envp[] = { "NAME=events", NULL };
-
-	spin_lock(&events_lock);
-	data->events |= event;
-	spin_unlock(&events_lock);
+	char eventstr[128];
+	char *envp[] = { "NAME=event", eventstr, NULL };
  
-	sysfs_notify(data->kobj, NULL, dev_attr_events.attr.name);
+	scnprintf(eventstr, sizeof(eventstr), "EVENT=%s", regulator_event_str(event));
  	kobject_uevent_env(data->kobj, KOBJ_CHANGE, envp);
  
  	return NOTIFY_OK;

Re: [PATCH 3/3] Documentation: ABI: sysfs-driver-regulator-output

[Greg Kroah-Hartman]

On Wed, Sep 20, 2023 at 02:02:49AM -0700, Zev Weiss wrote:
>  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);
> -	static const char * const *envp[] = { "NAME=events", NULL };

You removed this user/kernel api value, what will break if you do that?

thanks,

greg k-h

Re: [PATCH 3/3] Documentation: ABI: sysfs-driver-regulator-output

[Zev Weiss]

On Wed, Sep 20, 2023 at 02:29:15AM PDT, Greg Kroah-Hartman wrote:
>On Wed, Sep 20, 2023 at 02:02:49AM -0700, Zev Weiss wrote:
>>  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);
>> -	static const char * const *envp[] = { "NAME=events", NULL };
>
>You removed this user/kernel api value, what will break if you do that?
>

Sorry, I don't follow -- what removal are you referring to?  The envp 
array still has a NAME entry -- I changed its value from "events" to 
"event", but I wrote that part before I realized that the 'event' 
parameter of the function was actually a bitmask that might convey 
multiple events and just forgot to change it back, so keeping it 
pluralized is probably more appropriate.

And FWIW, I didn't intend for the exact format of the EVENT parameter 
that I sketched there to be something that had to be kept; given that 
there might be multiple entries perhaps it'd be better to use separate 
parameters more like NUMEVENTS, EVENT0, EVENT1, etc?  (Or omit NUMEVENTS 
and just let the consumer count upward until it doesn't find a match.)


Zev

Re: [PATCH 3/3] Documentation: ABI: sysfs-driver-regulator-output

[Greg Kroah-Hartman]

On Wed, Sep 20, 2023 at 03:44:29AM -0700, Zev Weiss wrote:
> On Wed, Sep 20, 2023 at 02:29:15AM PDT, Greg Kroah-Hartman wrote:
> > On Wed, Sep 20, 2023 at 02:02:49AM -0700, Zev Weiss wrote:
> > >  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);
> > > -	static const char * const *envp[] = { "NAME=events", NULL };
> > 
> > You removed this user/kernel api value, what will break if you do that?
> > 
> 
> Sorry, I don't follow -- what removal are you referring to?  The envp array
> still has a NAME entry -- I changed its value from "events" to "event",

Yes, that value.

> but
> I wrote that part before I realized that the 'event' parameter of the
> function was actually a bitmask that might convey multiple events and just
> forgot to change it back, so keeping it pluralized is probably more
> appropriate.
> 
> And FWIW, I didn't intend for the exact format of the EVENT parameter that I
> sketched there to be something that had to be kept; given that there might
> be multiple entries perhaps it'd be better to use separate parameters more
> like NUMEVENTS, EVENT0, EVENT1, etc?  (Or omit NUMEVENTS and just let the
> consumer count upward until it doesn't find a match.)

I don't know, what does userspace do with this value today?  If you
change it, what will break?

thanks,

greg k-h

Re: [PATCH 3/3] Documentation: ABI: sysfs-driver-regulator-output

[Zev Weiss]

On Wed, Sep 20, 2023 at 03:48:00AM PDT, Greg Kroah-Hartman wrote:
>On Wed, Sep 20, 2023 at 03:44:29AM -0700, Zev Weiss wrote:
>> On Wed, Sep 20, 2023 at 02:29:15AM PDT, Greg Kroah-Hartman wrote:
>> > On Wed, Sep 20, 2023 at 02:02:49AM -0700, Zev Weiss wrote:
>> > >  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);
>> > > -	static const char * const *envp[] = { "NAME=events", NULL };
>> >
>> > You removed this user/kernel api value, what will break if you do that?
>> >
>>
>> Sorry, I don't follow -- what removal are you referring to?  The envp array
>> still has a NAME entry -- I changed its value from "events" to "event",
>
>Yes, that value.
>
>> but
>> I wrote that part before I realized that the 'event' parameter of the
>> function was actually a bitmask that might convey multiple events and just
>> forgot to change it back, so keeping it pluralized is probably more
>> appropriate.
>>
>> And FWIW, I didn't intend for the exact format of the EVENT parameter that I
>> sketched there to be something that had to be kept; given that there might
>> be multiple entries perhaps it'd be better to use separate parameters more
>> like NUMEVENTS, EVENT0, EVENT1, etc?  (Or omit NUMEVENTS and just let the
>> consumer count upward until it doesn't find a match.)
>
>I don't know, what does userspace do with this value today?  If you
>change it, what will break?
>

Unless there's something I'm not aware of, this is an entirely new 
interface being added, not a change to an existing one, so I don't think 
there's any existing userspace beyond draft/experimental code.  My patch 
was a relative one on top of Naresh's patch 2/3 in this series as a 
proposed amendment to it; the userspace-consumer driver as it presently 
exists in the mainline kernel doesn't have a uevent interface at all.


Zev