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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <25844d80-fadb-44c3-a0e6-334aa6e4afd7@hatter.bewilderbeest.net>
Date:   Wed, 20 Sep 2023 02:02:49 -0700
From:   Zev Weiss <zev@...ilderbeest.net>
To:     Mark Brown <broonie@...nel.org>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Naresh Solanki <naresh.solanki@...ements.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] Documentation: ABI: sysfs-driver-regulator-output

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;