[PATCH] regulator: debugfs: Adding debugfs functions into regulator framework

[PATCH] regulator: debugfs: Adding debugfs functions into regulator framework

[Brandon Leong]

Allow the user to read and edit regulator information in user space
through the debugfs file system.

Signed-off-by: Brandon Leong <bleong@codeaurora.org>
---
 drivers/regulator/core.c |  335 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 335 insertions(+), 0 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index e63366f..2004e17 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -23,6 +23,8 @@
 #include <linux/mutex.h>
 #include <linux/suspend.h>
 #include <linux/delay.h>
+#include <linux/debugfs.h>
+#include <linux/uaccess.h>
 #include <linux/regulator/consumer.h>
 #include <linux/regulator/driver.h>
 #include <linux/regulator/machine.h>
@@ -2270,6 +2272,336 @@ static int add_regulator_attributes(struct regulator_dev *rdev)
 	return status;
 }
 
+#ifdef CONFIG_DEBUG_FS
+
+#define MAX_DEBUG_BUF_LEN 50
+
+static DEFINE_MUTEX(debug_buf_mutex);
+static char debug_buf[MAX_DEBUG_BUF_LEN];
+
+static int reg_debug_enable_set(void *data, u64 val)
+{
+	int err_info;
+	if (IS_ERR(data) || data == NULL) {
+		pr_err("Function Input Error %ld\n", PTR_ERR(data));
+		return -ENOMEM;
+	}
+
+	if (val)
+		err_info = regulator_enable(data);
+	else
+		err_info = regulator_disable(data);
+
+	return err_info;
+}
+
+static int reg_debug_enable_get(void *data, u64 *val)
+{
+	if (IS_ERR(data) || data == NULL) {
+		pr_err("Function Input Error %ld\n", PTR_ERR(data));
+		return -ENOMEM;
+	}
+
+	*val = regulator_is_enabled(data);
+	return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(reg_enable_fops, reg_debug_enable_get,
+			reg_debug_enable_set, "%llu\n");
+
+static int reg_debug_fdisable_set(void *data, u64 val)
+{
+	int err_info;
+	if (IS_ERR(data) || data == NULL) {
+		pr_err("Function Input Error %ld\n", PTR_ERR(data));
+		return -ENOMEM;
+	}
+
+	if (val > 0)
+		err_info = regulator_force_disable(data);
+	else
+		err_info = 0;
+
+	return err_info;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(reg_fdisable_fops, reg_debug_enable_get,
+			reg_debug_fdisable_set, "%llu\n");
+
+static ssize_t reg_debug_volt_set(struct file *file, const char __user *buf,
+					size_t count, loff_t *ppos)
+{
+	int err_info, filled;
+	int min, max = -1;
+	if (IS_ERR(file) || file == NULL) {
+		pr_err("Function Input Error %ld\n", PTR_ERR(file));
+		return -ENOMEM;
+	}
+
+	if (count < MAX_DEBUG_BUF_LEN) {
+		mutex_lock(&debug_buf_mutex);
+
+		if (copy_from_user(debug_buf, (void __user *) buf, count))
+			return -EFAULT;
+
+		debug_buf[count] = '\0';
+		filled = sscanf(debug_buf, "%d %d", &min, &max);
+
+		mutex_unlock(&debug_buf_mutex);
+		/* check that user entered two numbers */
+		if (filled < 2 || min < 0 || max < min) {
+			pr_info("Error, correct format: 'echo \"min max\""
+				" > voltage");
+			return -ENOMEM;
+		} else {
+			err_info = regulator_set_voltage(file->private_data,
+							min, max);
+		}
+	} else {
+		pr_err("Error-Input voltage pair"
+				"string exceeds maximum buffer length");
+
+		return -ENOMEM;
+	}
+
+	return count;
+}
+
+static ssize_t reg_debug_volt_get(struct file *file, char __user *buf,
+					size_t count, loff_t *ppos)
+{
+	int voltage, output, rc;
+	if (IS_ERR(file) || file == NULL) {
+		pr_err("Function Input Error %ld\n", PTR_ERR(file));
+		return -ENOMEM;
+	}
+
+	voltage = regulator_get_voltage(file->private_data);
+	mutex_lock(&debug_buf_mutex);
+
+	output = snprintf(debug_buf, MAX_DEBUG_BUF_LEN-1, "%d\n", voltage);
+	rc = simple_read_from_buffer((void __user *) buf, output, ppos,
+					(void *) debug_buf, output);
+
+	mutex_unlock(&debug_buf_mutex);
+
+	return rc;
+}
+
+static int reg_debug_volt_open(struct inode *inode, struct file *file)
+{
+	if (IS_ERR(file) || file == NULL) {
+		pr_err("Function Input Error %ld\n", PTR_ERR(file));
+		return -ENOMEM;
+	}
+
+	file->private_data = inode->i_private;
+	return 0;
+}
+
+static const struct file_operations reg_volt_fops = {
+	.write	= reg_debug_volt_set,
+	.open   = reg_debug_volt_open,
+	.read	= reg_debug_volt_get,
+};
+
+static int reg_debug_mode_set(void *data, u64 val)
+{
+	int err_info;
+	if (IS_ERR(data) || data == NULL) {
+		pr_err("Function Input Error %ld\n", PTR_ERR(data));
+		return -ENOMEM;
+	}
+
+	err_info = regulator_set_mode(data, (unsigned int)val);
+
+	return err_info;
+}
+
+static int reg_debug_mode_get(void *data, u64 *val)
+{
+	int err_info;
+	if (IS_ERR(data) || data == NULL) {
+		pr_err("Function Input Error %ld\n", PTR_ERR(data));
+		return -ENOMEM;
+	}
+
+	err_info = regulator_get_mode(data);
+
+	if (err_info < 0) {
+		pr_err("regulator_get_mode returned an error!\n");
+		return -ENOMEM;
+	} else {
+		*val = err_info;
+		return 0;
+	}
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(reg_mode_fops, reg_debug_mode_get,
+			reg_debug_mode_set, "%llu\n");
+
+static int reg_debug_optimum_mode_set(void *data, u64 val)
+{
+	int err_info;
+	if (IS_ERR(data) || data == NULL) {
+		pr_err("Function Input Error %ld\n", PTR_ERR(data));
+		return -ENOMEM;
+	}
+
+	err_info = regulator_set_optimum_mode(data, (unsigned int)val);
+
+	if (err_info < 0) {
+		pr_err("regulator_set_optimum_mode returned an error!\n");
+		return err_info;
+	}
+
+	return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(reg_optimum_mode_fops, reg_debug_mode_get,
+			reg_debug_optimum_mode_set, "%llu\n");
+
+static struct dentry *debugfs_base;
+
+static int reg_debug_init(void)
+{
+	debugfs_base = debugfs_create_dir("regulator", NULL);
+	if (IS_ERR(debugfs_base) || debugfs_base == NULL) {
+		pr_err("debugfs_create_dir returned error"
+			" %ld\n", PTR_ERR(debugfs_base));
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+/**
+ * regulator_debug_create_directory - Creates debugfs directories and files
+ *					for each regulator
+ *
+ * @param regulator_dev Voltage/Current regulator class device.
+ *
+ * Description: This function is run everytime "regulator_register" is run.
+ *		It is called for every regulator and initilaizes both the
+ *		regulator directory and the data files inside (ex. enable,
+ *		get/set voltage etc).
+ */
+static int regulator_debug_create_directory(struct regulator_dev *regulator_dev)
+{
+	struct dentry *reg_subdir;
+	struct dentry *err_ptr;
+	struct regulator *reg;
+	struct regulator_ops *reg_ops;
+	mode_t mode;
+	if (IS_ERR(regulator_dev) || regulator_dev == NULL ||
+		IS_ERR(debugfs_base) || debugfs_base == NULL) {
+		pr_err("Error-Bad Input\n");
+		goto error;
+	}
+
+	reg_subdir = debugfs_create_dir(regulator_dev->desc->name,
+					debugfs_base);
+
+	reg = regulator_get(NULL, regulator_dev->desc->name);
+	if (IS_ERR(reg) || reg == NULL) {
+		pr_err("Error-Bad Input\n");
+		goto error;
+	}
+
+	reg_ops = regulator_dev->desc->ops;
+	mode = 0;
+	/* Enabled File */
+	if (reg_ops->is_enabled)
+		mode |= S_IRUGO;
+	if (reg_ops->enable || reg_ops->disable)
+		mode |= S_IWUSR;
+	if (mode)
+		err_ptr = debugfs_create_file("enable", mode, reg_subdir,
+						reg, &reg_enable_fops);
+	if (IS_ERR(err_ptr)) {
+		pr_err("Error-Could not create enable file\n");
+		debugfs_remove_recursive(reg_subdir);
+		goto error;
+	}
+
+	mode = 0;
+	/* Force-Disable File */
+	if (reg_ops->is_enabled)
+		mode |= S_IRUGO;
+	if (reg_ops->enable || reg_ops->disable)
+		mode |= S_IWUSR;
+	if (mode)
+		err_ptr = debugfs_create_file("force_disable", mode,
+					reg_subdir, reg, &reg_fdisable_fops);
+	if (IS_ERR(err_ptr)) {
+		pr_err("Error-Could not create force_disable file\n");
+		debugfs_remove_recursive(reg_subdir);
+		goto error;
+	}
+
+	mode = 0;
+	/* Voltage File */
+	if (reg_ops->get_voltage)
+		mode |= S_IRUGO;
+	if (reg_ops->set_voltage)
+		mode |= S_IWUSR;
+	if (mode)
+		err_ptr = debugfs_create_file("voltage", mode, reg_subdir,
+						reg, &reg_volt_fops);
+	if (IS_ERR(err_ptr)) {
+		pr_err("Error-Could not create voltage file\n");
+		debugfs_remove_recursive(reg_subdir);
+		goto error;
+	}
+
+	mode = 0;
+	/* Mode File */
+	if (reg_ops->get_mode)
+		mode |= S_IRUGO;
+	if (reg_ops->set_mode)
+		mode |= S_IWUSR;
+	if (mode)
+		err_ptr = debugfs_create_file("mode", mode, reg_subdir,
+						reg, &reg_mode_fops);
+	if (IS_ERR(err_ptr)) {
+		pr_err("Error-Could not create mode file\n");
+		debugfs_remove_recursive(reg_subdir);
+		goto error;
+	}
+
+	mode = 0;
+	/* Optimum Mode File */
+	if (reg_ops->get_mode)
+		mode |= S_IRUGO;
+	if (reg_ops->set_mode)
+		mode |= S_IWUSR;
+	if (mode)
+		err_ptr = debugfs_create_file("optimum_mode", mode,
+				reg_subdir, reg, &reg_optimum_mode_fops);
+	if (IS_ERR(err_ptr)) {
+		pr_err("Error-Could not create optimum_mode file\n");
+		debugfs_remove_recursive(reg_subdir);
+		goto error;
+	}
+
+	return 0;
+
+error:
+	return -ENOMEM;
+}
+#else
+static inline void regulator_debug_create_directory(struct regulator_dev
+						*regulator_dev)
+{
+	return;
+}
+
+static inline void reg_debug_init(void)
+{
+	return;
+}
+#endif
+
 /**
  * regulator_register - register regulator
  * @regulator_desc: regulator to register
@@ -2400,6 +2732,7 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
 	list_add(&rdev->list, &regulator_list);
 out:
 	mutex_unlock(&regulator_list_mutex);
+	regulator_debug_create_directory(rdev);
 	return rdev;
 
 unset_supplies:
@@ -2575,6 +2908,8 @@ static int __init regulator_init(void)
 
 	regulator_dummy_init();
 
+	reg_debug_init();
+
 	return ret;
 }
 
-- 
1.7.3.1

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] regulator: debugfs: Adding debugfs functions into regulator framework

[Mark Brown]

On Mon, Dec 06, 2010 at 12:52:43PM -0800, Brandon Leong wrote:

> Allow the user to read and edit regulator information in user space
> through the debugfs file system.

Basically what you're doing here is providing a default consumer via
debugfs along with the consumers that have been defined by the board.
On the one hand it's good that this means that we won't let the user
change anything unless it has been explicitly enabled, on the other hand
it means that we've not really added anything that you can't do already
with the virtual consumer except made it slightly more convenient to set
up and give it a different interface.

This makes me a bit nervous, especially given that the regulator API is
mostly used in embedded systems where things like debugfs not being a
production API don't entirely count for much.  It feels like it
encourages people to write random userspace code which prods and pokes
at the regulators directly without any form of mediation and without
suggesting that perhaps they ought to be doing something a bit nicer.

I can see some useful debug facilities we could add using debugfs.  For
example, it'd be good to expose more of the dynamic structures the API
has like the consumers, their reference counts and the voltages they
have requested.  Showing the voltage lists could also be nice.  This is
all peering inside the API to provide additional insight to the user,
though, rather than something layered on top of the API as this is.

If we do want to go this way it'd be good to have more discussion in the
changelog about what we're adding here, what it gives us, how it should
be used and how it compares with the existing approaches.  Given the
similarities with the virtual consumer I am inclined to wonder why we'd
want to reimplement the actual user visible API bit.

Some more specific comments on the code:

> +#ifdef CONFIG_DEBUG_FS
> +
> +#define MAX_DEBUG_BUF_LEN 50

Seems low...

> +static DEFINE_MUTEX(debug_buf_mutex);
> +static char debug_buf[MAX_DEBUG_BUF_LEN];

May as well just allocate buffers dynamically?  Saves locking and means
less worry about allocating a larger buffer.

> +static int reg_debug_enable_set(void *data, u64 val)
> +{
> +	int err_info;
> +	if (IS_ERR(data) || data == NULL) {
> +		pr_err("Function Input Error %ld\n", PTR_ERR(data));

Please Try To Make Your Log Messages A Bit More Descriptive And
Typographically Correct - I'd not expect a user to have a hope of
figuring out what's gone wrong here.  That said, I suspect you're
looking for BUG_ON() here...

> +	if (val)
> +		err_info = regulator_enable(data);
> +	else
> +		err_info = regulator_disable(data);

This isn't going to do what people expect - the refcounting really is
going to surprise people, especially as you read back the physical
enable/disable state through the same file.  Abuse of this file is
likely to confuse any actual consumers we have too.

> +static ssize_t reg_debug_volt_set(struct file *file, const char __user *buf,
> +					size_t count, loff_t *ppos)

Don't think we ever use volt as an abberviation anywhere else in the
API?

> +		mutex_unlock(&debug_buf_mutex);
> +		/* check that user entered two numbers */

Blank line between these two.

> +		pr_err("Error-Input voltage pair"
> +				"string exceeds maximum buffer length");
> +

Don't split text like this, it just makes the source hard to read and
grep (and note that you're missing a space in there the absence of which
is hidden by the line break).

> +	voltage = regulator_get_voltage(file->private_data);
> +	mutex_lock(&debug_buf_mutex);

Blank line here too.

> +	if (err_info < 0) {
> +		pr_err("regulator_get_mode returned an error!\n");
> +		return -ENOMEM;

What was the error, and why not pass it back as the return value as well
as logging it?

> +	if (IS_ERR(err_ptr)) {
> +		pr_err("Error-Could not create voltage file\n");

Throughout the patch you're using this sort of non-idomatic format for
log messages and not logging error codes - I'd expect to see something
like:

	pr_err("Failed to create voltage file: %d\n", PTR_ERR(err_ptr));

though since this is per-regulator you should be able to say dev_err()
instead.

> +	mode = 0;
> +	/* Mode File */
> +	if (reg_ops->get_mode)

Odd formatting.

> +static inline void regulator_debug_create_directory(struct regulator_dev
> +						*regulator_dev)
> +{
> +	return;
> +}

No need for return statement here.

> @@ -2400,6 +2732,7 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
>  	list_add(&rdev->list, &regulator_list);
>  out:
>  	mutex_unlock(&regulator_list_mutex);
> +	regulator_debug_create_directory(rdev);
>  	return rdev;
>  
>  unset_supplies:

We never clean up if the regulator is unregistered.

Re: [PATCH] regulator: debugfs: Adding debugfs functions into regulator framework

[Daniel Walker]

On Mon, 2010-12-06 at 22:29 +0000, Mark Brown wrote:

> > +static int reg_debug_enable_set(void *data, u64 val)
> > +{
> > +	int err_info;
> > +	if (IS_ERR(data) || data == NULL) {
> > +		pr_err("Function Input Error %ld\n", PTR_ERR(data));
> 
> Please Try To Make Your Log Messages A Bit More Descriptive And
> Typographically Correct - I'd not expect a user to have a hope of
> figuring out what's gone wrong here.  That said, I suspect you're
> looking for BUG_ON() here...

Could we do WARN_ON() here? Unless this is a really serious problem.

Daniel

-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.

Re: [PATCH] regulator: debugfs: Adding debugfs functions into regulator framework

[Mark Brown]

On Mon, Dec 06, 2010 at 06:52:00PM -0800, Daniel Walker wrote:
> On Mon, 2010-12-06 at 22:29 +0000, Mark Brown wrote:

> > > +static int reg_debug_enable_set(void *data, u64 val)
> > > +{
> > > +	int err_info;
> > > +	if (IS_ERR(data) || data == NULL) {
> > > +		pr_err("Function Input Error %ld\n", PTR_ERR(data));

> > Please Try To Make Your Log Messages A Bit More Descriptive And
> > Typographically Correct - I'd not expect a user to have a hope of
> > figuring out what's gone wrong here.  That said, I suspect you're
> > looking for BUG_ON() here...

> Could we do WARN_ON() here? Unless this is a really serious problem.

It means we've managed to let the user open a file without setting up
the private data for the file correctly which is a pretty bad bug which
we'd never expect to see at runtime unless there was a clear kernel bug.

Re: [PATCH] regulator: debugfs: Adding debugfs functions into regulator framework

[Brandon Leong]

So is it decided that we should use BUG_ON() now?

Also, regarding this issue:

> +        if (val)
> +                err_info = regulator_enable(data);
> +        else
> +                err_info = regulator_disable(data);

This isn't going to do what people expect - the refcounting really is
going to surprise people, especially as you read back the physical
enable/disable state through the same file.  Abuse of this file is
likely to confuse any actual consumers we have too.

----

Could you clarify the issue with this? All I am doing here is if the user
enters a "1", then enable, if the user enters a "0" then disable.

Thanks,
-Brandon

> On Mon, Dec 06, 2010 at 06:52:00PM -0800, Daniel Walker wrote:
>> On Mon, 2010-12-06 at 22:29 +0000, Mark Brown wrote:
>
>> > > +static int reg_debug_enable_set(void *data, u64 val)
>> > > +{
>> > > +	int err_info;
>> > > +	if (IS_ERR(data) || data == NULL) {
>> > > +		pr_err("Function Input Error %ld\n", PTR_ERR(data));
>
>> > Please Try To Make Your Log Messages A Bit More Descriptive And
>> > Typographically Correct - I'd not expect a user to have a hope of
>> > figuring out what's gone wrong here.  That said, I suspect you're
>> > looking for BUG_ON() here...
>
>> Could we do WARN_ON() here? Unless this is a really serious problem.
>
> It means we've managed to let the user open a file without setting up
> the private data for the file correctly which is a pretty bad bug which
> we'd never expect to see at runtime unless there was a clear kernel bug.
>


-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] regulator: debugfs: Adding debugfs functions into regulator framework

[Mark Brown]

On Tue, Dec 07, 2010 at 02:04:02PM -0800, Brandon Leong wrote:

[Please don't top post, and please quote properly - I've added a layer
of quotation to my text.]

> So is it decided that we should use BUG_ON() now?

I'd prefer it, other people's mileage may vary.  But please do also
engage with the big picture stuff I was talking about.

> Also, regarding this issue:

> > > +        if (val)
> > > +                err_info = regulator_enable(data);
> > > +        else
> > > +                err_info = regulator_disable(data);

> > This isn't going to do what people expect - the refcounting really is
> > going to surprise people, especially as you read back the physical
> > enable/disable state through the same file.  Abuse of this file is
> > likely to confuse any actual consumers we have too.

> ----

> Could you clarify the issue with this? All I am doing here is if the user
> enters a "1", then enable, if the user enters a "0" then disable.

Right, but remember that the regulator API does refcounting.  Writing
to the file won't always have an immediate effect, it'll update the
refcount which may or may not do what's expected to the actual hardware.

One other thing: your code checks for operations before it creates files
but the operations may not be permitted by the constraints.