ASoC: soc-core: Support debugfs entries larger than PAGE_SIZE bytes
PATCH 1/1 ASoC: soc-core: Support debugfs entries larger than PAGE_SIZE bytes
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.
--- sound/soc/soc-core.c | 114 +++++++++++++++++++++++++++++++++---------------- 1 files changed, 77 insertions(+), 37 deletions(-)
sound/soc/soc-core.c +77 -37
@@ -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; }
Signature · 1 line
1.7.3.5
sound/soc/soc-core.c static int min_bytes_needed(unsigned long val) · line 93
Quoted 4 lines · Dimitris Papastamos
sound/soc/soc-core.c +4
-0+/* 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.
sound/soc/soc-core.c static int min_bytes_needed(unsigned long val) · line 113
Quoted 6 lines
sound/soc/soc-core.c +6
-0+/* 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.
sound/soc/soc-core.c static int codec_reg_open_file(struct inode *inode, struct file *file) · line 236
Quoted 9 lines
sound/soc/soc-core.c +9
-0+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.
Quoted 14 lines · Mark Brown
sound/soc/soc-core.c static int codec_reg_open_file(struct inode *inode, struct file *file) · line 236
Quoted 9 lines · Dimitris Papastamos
sound/soc/soc-core.c +9
-0+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 expectcopy_to_user()to do the right thing here andsimple_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
Quoted 10 lines · Dimitris Papastamos
Quoted 3 lines · Mark Brown
Why do we need the
access_ok()here? I'd really expectcopy_to_user()to do the right thing here andsimple_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 thatsimple_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.