[PATCH] ASoC: soc-core: Support debugfs entries larger than PAGE_SIZE bytes

[PATCH] ASoC: soc-core: Support debugfs entries larger than PAGE_SIZE bytes

[Dimitris Papastamos]

For some codecs with large register maps, it was not possible to dump
all registers via the codec_reg file but only up to PAGE_SIZE bytes.
This patch fixes this problem.

Signed-off-by: Dimitris Papastamos <dp@opensource.wolfsonmicro.com>
---
 sound/soc/soc-core.c |  110 +++++++++++++++++++++++++++++++++-----------------
 1 files changed, 73 insertions(+), 37 deletions(-)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 205cbd7..83626f9 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -87,15 +87,56 @@ static int min_bytes_needed(unsigned long val)
 	return c;
 }
 
+/* fill buf which is 'len' bytes with a formatted
+ * string of the form 'reg: value\n' */
+static int format_register_str(struct snd_soc_codec *codec,
+			       unsigned int reg, char *buf, size_t len)
+{
+	int wordsize = codec->driver->reg_word_size * 2;
+	int regsize = min_bytes_needed(codec->driver->reg_cache_size) * 2;
+	int ret;
+	char tmpbuf[len + 1];
+	char regbuf[regsize + 1];
+
+	/* since tmpbuf is allocated on the stack, warn the callers if they
+	 * try to abuse this function */
+	WARN_ON(len > 63);
+
+	/* +2 for ': ' and + 1 for '\n' */
+	if (wordsize + regsize + 2 + 1 != len)
+		return -EINVAL;
+
+	ret = snd_soc_read(codec , reg);
+	if (ret < 0) {
+		memset(regbuf, 'X', regsize);
+		regbuf[regsize] = '\0';
+	} else {
+		snprintf(regbuf, regsize + 1, "%.*x", regsize, ret);
+	}
+
+	/* prepare the buffer */
+	snprintf(tmpbuf, len + 1, "%.*x: %s\n", wordsize, reg, regbuf);
+	/* copy it back to the caller without the '\0' */
+	memcpy(buf, tmpbuf, len);
+
+	return 0;
+}
+
 /* codec register dump */
-static ssize_t soc_codec_reg_show(struct snd_soc_codec *codec, char *buf)
+static ssize_t soc_codec_reg_show(struct snd_soc_codec *codec, char *buf,
+				  size_t count, loff_t pos)
 {
-	int ret, i, step = 1, count = 0;
+	int i, step = 1;
 	int wordsize, regsize;
+	int len;
+	size_t total = 0;
+	loff_t p = 0;
 
 	wordsize = codec->driver->reg_word_size * 2;
 	regsize = min_bytes_needed(codec->driver->reg_cache_size) * 2;
 
+	len = wordsize + regsize + 2 + 1;
+
 	if (!codec->driver->reg_cache_size)
 		return 0;
 
@@ -105,51 +146,34 @@ static ssize_t soc_codec_reg_show(struct snd_soc_codec *codec, char *buf)
 	for (i = 0; i < codec->driver->reg_cache_size; i += step) {
 		if (codec->readable_register && !codec->readable_register(codec, i))
 			continue;
-
-		count += sprintf(buf + count, "%.*x: ", regsize, i);
-		if (count >= PAGE_SIZE - 1)
-			break;
-
 		if (codec->driver->display_register) {
 			count += codec->driver->display_register(codec, buf + count,
 							 PAGE_SIZE - count, i);
 		} else {
-			/* If the read fails it's almost certainly due to
-			 * the register being volatile and the device being
-			 * powered off.
-			 */
-			ret = snd_soc_read(codec, i);
-			if (ret >= 0)
-				count += snprintf(buf + count,
-						  PAGE_SIZE - count,
-						  "%.*x", wordsize, ret);
-			else
-				count += snprintf(buf + count,
-						  PAGE_SIZE - count,
-						  "<no data: %d>", ret);
+			/* only support larger than PAGE_SIZE bytes debugfs
+			 * entries for the default case */
+			if (p >= pos) {
+				if (total + len >= count - 1)
+					break;
+				format_register_str(codec, i, buf + total, len);
+				total += len;
+			}
+			p += len;
 		}
-
-		if (count >= PAGE_SIZE - 1)
-			break;
-
-		count += snprintf(buf + count, PAGE_SIZE - count, "\n");
-		if (count >= PAGE_SIZE - 1)
-			break;
 	}
 
-	/* Truncate count; min() would cause a warning */
-	if (count >= PAGE_SIZE)
-		count = PAGE_SIZE - 1;
+	total = min(total, count - 1);
 
-	return count;
+	return total;
 }
+
 static ssize_t codec_reg_show(struct device *dev,
 	struct device_attribute *attr, char *buf)
 {
 	struct snd_soc_pcm_runtime *rtd =
 			container_of(dev, struct snd_soc_pcm_runtime, dev);
 
-	return soc_codec_reg_show(rtd->codec, buf);
+	return soc_codec_reg_show(rtd->codec, buf, PAGE_SIZE, 0);
 }
 
 static DEVICE_ATTR(codec_reg, 0444, codec_reg_show, NULL);
@@ -188,16 +212,28 @@ static int codec_reg_open_file(struct inode *inode, struct file *file)
 }
 
 static ssize_t codec_reg_read_file(struct file *file, char __user *user_buf,
-			       size_t count, loff_t *ppos)
+				   size_t count, loff_t *ppos)
 {
 	ssize_t ret;
 	struct snd_soc_codec *codec = file->private_data;
-	char *buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
+	char *buf;
+
+	if (*ppos < 0 || !count)
+		return -EINVAL;
+
+	buf = kmalloc(count, GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
-	ret = soc_codec_reg_show(codec, buf);
-	if (ret >= 0)
-		ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
+
+	ret = soc_codec_reg_show(codec, buf, count, *ppos);
+	if (ret >= 0) {
+		if (copy_to_user(user_buf, buf, ret)) {
+			kfree(buf);
+			return -EFAULT;
+		}
+		*ppos += ret;
+	}
+
 	kfree(buf);
 	return ret;
 }
-- 
1.7.3.5

Re: [PATCH] ASoC: soc-core: Support debugfs entries larger than PAGE_SIZE bytes

[Liam Girdwood]

On Wed, 2011-02-02 at 13:58 +0000, Dimitris Papastamos wrote:
> For some codecs with large register maps, it was not possible to dump
> all registers via the codec_reg file but only up to PAGE_SIZE bytes.
> This patch fixes this problem.
> 
> Signed-off-by: Dimitris Papastamos <dp@opensource.wolfsonmicro.com>

Acked-by: Liam Girdwood <lrg@slimlogic.co.uk>
-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

Re: [PATCH] ASoC: soc-core: Support debugfs entries larger than PAGE_SIZE bytes

[Mark Brown]

On Wed, Feb 02, 2011 at 01:58:58PM +0000, Dimitris Papastamos wrote:
> For some codecs with large register maps, it was not possible to dump
> all registers via the codec_reg file but only up to PAGE_SIZE bytes.
> This patch fixes this problem.

Applied, thanks.

[PATCH] ASoC: soc-core: Support debugfs entries larger than PAGE_SIZE bytes

[Dimitris Papastamos]

For some codecs with large register maps, it was not possible to dump
all registers via the codec_reg file but only up to PAGE_SIZE bytes.
This patch fixes this problem.

Signed-off-by: Dimitris Papastamos <dp@opensource.wolfsonmicro.com>
---
 sound/soc/soc-core.c |  114 +++++++++++++++++++++++++++++++++----------------
 1 files changed, 77 insertions(+), 37 deletions(-)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 205cbd7..a7710d2 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -87,15 +87,55 @@ static int min_bytes_needed(unsigned long val)
 	return c;
 }
 
+/* fill buf which is 'len' bytes with a formatted
+ * string of the form 'reg: value\n' */
+static int format_register_str(struct snd_soc_codec *codec,
+			       unsigned int regidx, char *buf, size_t len)
+{
+	int wordsize = codec->driver->reg_word_size * 2;
+	int regsize = min_bytes_needed(codec->driver->reg_cache_size) * 2;
+	int ret;
+	char tmpbuf[len + 1];
+
+	/* since tmpbuf is allocated on the stack, warn the callers if they
+	 * try to abuse this function */
+	WARN_ON(len > 63);
+
+	/* +2 for ': ' and + 1 for '\n' */
+	if (wordsize + regsize + 2 + 1 != len)
+		return -EINVAL;
+
+	/* we don't currently handle failed reads */
+	ret = snd_soc_read(codec , regidx);
+	if (ret < 0) {
+		dev_err(codec->dev, "Failed to read from %#x\n", regidx);
+		return ret;
+	}
+
+	/* prepare the buffer */
+	snprintf(tmpbuf, len + 1, "%.*x: %.*x\n", wordsize,
+		 regidx, regsize, ret);
+	/* copy it back to the caller without the '\0' */
+	memcpy(buf, tmpbuf, len);
+
+	return 0;
+}
+
 /* codec register dump */
-static ssize_t soc_codec_reg_show(struct snd_soc_codec *codec, char *buf)
+static ssize_t soc_codec_reg_show(struct snd_soc_codec *codec, char *buf,
+				  size_t count, loff_t pos)
 {
-	int ret, i, step = 1, count = 0;
+	int i, step = 1;
 	int wordsize, regsize;
+	int len;
+	size_t total = 0;
+	loff_t p = 0;
 
 	wordsize = codec->driver->reg_word_size * 2;
 	regsize = min_bytes_needed(codec->driver->reg_cache_size) * 2;
 
+	len = wordsize + regsize + 2 + 1;
+
 	if (!codec->driver->reg_cache_size)
 		return 0;
 
@@ -105,51 +145,34 @@ static ssize_t soc_codec_reg_show(struct snd_soc_codec *codec, char *buf)
 	for (i = 0; i < codec->driver->reg_cache_size; i += step) {
 		if (codec->readable_register && !codec->readable_register(codec, i))
 			continue;
-
-		count += sprintf(buf + count, "%.*x: ", regsize, i);
-		if (count >= PAGE_SIZE - 1)
-			break;
-
 		if (codec->driver->display_register) {
 			count += codec->driver->display_register(codec, buf + count,
 							 PAGE_SIZE - count, i);
 		} else {
-			/* If the read fails it's almost certainly due to
-			 * the register being volatile and the device being
-			 * powered off.
-			 */
-			ret = snd_soc_read(codec, i);
-			if (ret >= 0)
-				count += snprintf(buf + count,
-						  PAGE_SIZE - count,
-						  "%.*x", wordsize, ret);
-			else
-				count += snprintf(buf + count,
-						  PAGE_SIZE - count,
-						  "<no data: %d>", ret);
+			/* only support larger than PAGE_SIZE bytes debugfs
+			 * entries for the default case */
+			if (p >= pos) {
+				if (total + len >= count - 1)
+					break;
+				format_register_str(codec, i, buf + total, len);
+				total += len;
+			}
+			p += len;
 		}
-
-		if (count >= PAGE_SIZE - 1)
-			break;
-
-		count += snprintf(buf + count, PAGE_SIZE - count, "\n");
-		if (count >= PAGE_SIZE - 1)
-			break;
 	}
 
-	/* Truncate count; min() would cause a warning */
-	if (count >= PAGE_SIZE)
-		count = PAGE_SIZE - 1;
+	total = min(total, count - 1);
 
-	return count;
+	return total;
 }
+
 static ssize_t codec_reg_show(struct device *dev,
 	struct device_attribute *attr, char *buf)
 {
 	struct snd_soc_pcm_runtime *rtd =
 			container_of(dev, struct snd_soc_pcm_runtime, dev);
 
-	return soc_codec_reg_show(rtd->codec, buf);
+	return soc_codec_reg_show(rtd->codec, buf, PAGE_SIZE, 0);
 }
 
 static DEVICE_ATTR(codec_reg, 0444, codec_reg_show, NULL);
@@ -188,16 +211,33 @@ static int codec_reg_open_file(struct inode *inode, struct file *file)
 }
 
 static ssize_t codec_reg_read_file(struct file *file, char __user *user_buf,
-			       size_t count, loff_t *ppos)
+				   size_t count, loff_t *ppos)
 {
 	ssize_t ret;
 	struct snd_soc_codec *codec = file->private_data;
-	char *buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
+	char *buf;
+
+	if (*ppos < 0 || !count)
+		return -EINVAL;
+
+	buf = kmalloc(count, GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
-	ret = soc_codec_reg_show(codec, buf);
-	if (ret >= 0)
-		ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
+
+	ret = soc_codec_reg_show(codec, buf, count, *ppos);
+	if (ret >= 0) {
+		if (!access_ok(VERIFY_WRITE, user_buf, ret)) {
+			ret = -EFAULT;
+			goto out;
+		}
+		if (copy_to_user(user_buf, buf, ret)) {
+			ret = -EFAULT;
+			goto out;
+		}
+		*ppos += ret;
+	}
+
+out:
 	kfree(buf);
 	return ret;
 }
-- 
1.7.3.5

Re: [PATCH] ASoC: soc-core: Support debugfs entries larger than PAGE_SIZE bytes

[Mark Brown]

On Wed, Feb 02, 2011 at 11:29:05AM +0000, Dimitris Papastamos wrote:

> +/* fill buf which is 'len' bytes with a formatted
> + * string of the form 'reg: value\n' */
> +static int format_register_str(struct snd_soc_codec *codec,
> +			       unsigned int regidx, char *buf, size_t len)

The naming of regidx is really inconsistent with the rest of the code
which pretty much always calls registers reg.

> +	/* we don't currently handle failed reads */
> +	ret = snd_soc_read(codec , regidx);
> +	if (ret < 0) {
> +		dev_err(codec->dev, "Failed to read from %#x\n", regidx);
> +		return ret;
> +	}

I'd suggest filling the field with some obvious out of bound character
such as X.

> +	if (ret >= 0) {
> +		if (!access_ok(VERIFY_WRITE, user_buf, ret)) {
> +			ret = -EFAULT;
> +			goto out;
> +		}
> +		if (copy_to_user(user_buf, buf, ret)) {
> +			ret = -EFAULT;
> +			goto out;
> +		}

Why do we need the access_ok() here?  I'd really expect copy_to_user()
to do the right thing here and simple_read_from_buffer() doesn't do
this.

Re: [PATCH] ASoC: soc-core: Support debugfs entries larger than PAGE_SIZE bytes

[Dimitris Papastamos]

On Wed, 2011-02-02 at 13:07 +0000, Mark Brown wrote:
> On Wed, Feb 02, 2011 at 11:29:05AM +0000, Dimitris Papastamos wrote:
> > +	if (ret >= 0) {
> > +		if (!access_ok(VERIFY_WRITE, user_buf, ret)) {
> > +			ret = -EFAULT;
> > +			goto out;
> > +		}
> > +		if (copy_to_user(user_buf, buf, ret)) {
> > +			ret = -EFAULT;
> > +			goto out;
> > +		}
> 
> Why do we need the access_ok() here?  I'd really expect copy_to_user()
> to do the right thing here and simple_read_from_buffer() doesn't do
> this.

I thought it'd be a problem if userspace provides a pointer that points
in kernelspace.  The call to access_ok() ensures that the pointer lies
indeed in userspace.  I noticed that simple_read_from_buffer() doesn't
do this, but I did not see how this could harm things.

Thanks,
Dimitris

Re: [PATCH] ASoC: soc-core: Support debugfs entries larger than PAGE_SIZE bytes

[Mark Brown]

On Wed, Feb 02, 2011 at 01:28:16PM +0000, Dimitris Papastamos wrote:
> On Wed, 2011-02-02 at 13:07 +0000, Mark Brown wrote:

> > Why do we need the access_ok() here?  I'd really expect copy_to_user()
> > to do the right thing here and simple_read_from_buffer() doesn't do
> > this.

> I thought it'd be a problem if userspace provides a pointer that points
> in kernelspace.  The call to access_ok() ensures that the pointer lies
> indeed in userspace.  I noticed that simple_read_from_buffer() doesn't
> do this, but I did not see how this could harm things.

My expectation is that access_ok() would only be used if we were parsing
userspace passed values directly, having to do the check before doing a
copy_to_user() reads like we're doing something wrong.