Current registers debugfs file implementation doesn't
handle if the size exceeds 4k. It just dumps 4k of registers.
Fix this by using the seq_file which already handles
the file offset logic instead of reinventing the wheel.
I am wondering if there is any issue is doing below which
I am not aware of?
Signed-off-by: variksla <[email protected]>
---
drivers/base/regmap/regmap-debugfs.c | 217 ++++++++++-------------------------
1 file changed, 61 insertions(+), 156 deletions(-)
diff --git a/drivers/base/regmap/regmap-debugfs.c
b/drivers/base/regmap/regmap-debugfs.c
index 36ce351..0bec69c 100644
--- a/drivers/base/regmap/regmap-debugfs.c
+++ b/drivers/base/regmap/regmap-debugfs.c
@@ -88,98 +88,7 @@ static bool regmap_printable(struct regmap *map,
unsigned int reg)
return true;
}
-/*
- * Work out where the start offset maps into register numbers, bearing
- * in mind that we suppress hidden registers.
- */
-static unsigned int regmap_debugfs_get_dump_start(struct regmap *map,
- unsigned int base,
- loff_t from,
- loff_t *pos)
-{
- struct regmap_debugfs_off_cache *c = NULL;
- loff_t p = 0;
- unsigned int i, ret;
- unsigned int fpos_offset;
- unsigned int reg_offset;
-
- /* Suppress the cache if we're using a subrange */
- if (base)
- return base;
-
- /*
- * If we don't have a cache build one so we don't have to do a
- * linear scan each time.
- */
- mutex_lock(&map->cache_lock);
- i = base;
- if (list_empty(&map->debugfs_off_cache)) {
- for (; i <= map->max_register; i += map->reg_stride) {
- /* Skip unprinted registers, closing off cache entry */
- if (!regmap_printable(map, i)) {
- if (c) {
- c->max = p - 1;
- c->max_reg = i - map->reg_stride;
- list_add_tail(&c->list,
- &map->debugfs_off_cache);
- c = NULL;
- }
-
- continue;
- }
-
- /* No cache entry? Start a new one */
- if (!c) {
- c = kzalloc(sizeof(*c), GFP_KERNEL);
- if (!c) {
- regmap_debugfs_free_dump_cache(map);
- mutex_unlock(&map->cache_lock);
- return base;
- }
- c->min = p;
- c->base_reg = i;
- }
-
- p += map->debugfs_tot_len;
- }
- }
-
- /* Close the last entry off if we didn't scan beyond it */
- if (c) {
- c->max = p - 1;
- c->max_reg = i - map->reg_stride;
- list_add_tail(&c->list,
- &map->debugfs_off_cache);
- }
-
- /*
- * This should never happen; we return above if we fail to
- * allocate and we should never be in this code if there are
- * no registers at all.
- */
- WARN_ON(list_empty(&map->debugfs_off_cache));
- ret = base;
-
- /* Find the relevant block:offset */
- list_for_each_entry(c, &map->debugfs_off_cache, list) {
- if (from >= c->min && from <= c->max) {
- fpos_offset = from - c->min;
- reg_offset = fpos_offset / map->debugfs_tot_len;
- *pos = c->min + (reg_offset * map->debugfs_tot_len);
- mutex_unlock(&map->cache_lock);
- return c->base_reg + (reg_offset * map->reg_stride);
- }
-
- *pos = c->max;
- ret = c->max_reg;
- }
- mutex_unlock(&map->cache_lock);
-
- return ret;
-}
-
-static inline void regmap_calc_tot_len(struct regmap *map,
- void *buf, size_t count)
+static inline void regmap_calc_tot_len(struct regmap *map)
{
/* Calculate the length of a fixed format */
if (!map->debugfs_tot_len) {
@@ -190,84 +99,80 @@ static inline void regmap_calc_tot_len(struct regmap *map,
}
}
-static ssize_t regmap_read_debugfs(struct regmap *map, unsigned int from,
- unsigned int to, char __user *user_buf,
- size_t count, loff_t *ppos)
+static void *regmap_debugfs_start(struct seq_file *s, loff_t *pos)
{
- size_t buf_pos = 0;
- loff_t p = *ppos;
- ssize_t ret;
- int i;
- char *buf;
- unsigned int val, start_reg;
-
- if (*ppos < 0 || !count)
- return -EINVAL;
+ struct regmap *map = s->private;
+ regmap_calc_tot_len(map);
+ return pos;
+}
- buf = kmalloc(count, GFP_KERNEL);
- if (!buf)
- return -ENOMEM;
+static void *regmap_debugfs_next(struct seq_file *s, void *v, loff_t *pos)
+{
+ struct regmap *map = s->private;
- regmap_calc_tot_len(map, buf, count);
+ (*pos)++;
+ if (*pos >= map->max_register) {
+ return NULL;
+ }
+ return pos;
+}
- /* Work out which register we're starting at */
- start_reg = regmap_debugfs_get_dump_start(map, from, *ppos, &p);
+static void regmap_debugfs_stop(struct seq_file *s, void *v)
+{
+}
- for (i = start_reg; i <= to; i += map->reg_stride) {
- if (!regmap_readable(map, i) && !regmap_cached(map, i))
- continue;
+static int regmap_debugfs_show(struct seq_file *s, void *v)
+{
+ struct regmap *map = s->private;
+ unsigned int val, current_reg = *(loff_t *)v;
+ int ret;
- if (regmap_precious(map, i))
- continue;
+ if (!regmap_printable(map, current_reg))
+ return 0;
- /* If we're in the region the user is trying to read */
- if (p >= *ppos) {
- /* ...but not beyond it */
- if (buf_pos + map->debugfs_tot_len > count)
- break;
+ /* Format the register */
+ seq_printf(s, "%.*x: ", map->debugfs_reg_len, current_reg);
- /* Format the register */
- snprintf(buf + buf_pos, count - buf_pos, "%.*x: ",
- map->debugfs_reg_len, i - from);
- buf_pos += map->debugfs_reg_len + 2;
-
- /* Format the value, write all X if we can't read */
- ret = regmap_read(map, i, &val);
- if (ret == 0)
- snprintf(buf + buf_pos, count - buf_pos,
- "%.*x", map->debugfs_val_len, val);
- else
- memset(buf + buf_pos, 'X',
- map->debugfs_val_len);
- buf_pos += 2 * map->format.val_bytes;
-
- buf[buf_pos++] = '\n';
- }
- p += map->debugfs_tot_len;
+ /* Format the value, write all X if we can't read */
+ ret = regmap_read(map, current_reg, &val);
+ if (ret == 0) {
+ seq_printf(s, "%.*x\n", map->debugfs_val_len, val);
+ } else {
+ char buf[20];
+ memset(buf, 'X', map->debugfs_val_len);
+ seq_printf(s, "%.*s\n", map->debugfs_val_len, buf);
}
+ return 0;
+}
- ret = buf_pos;
+static struct seq_operations regmap_debugfs_seq_ops = {
+ .start = regmap_debugfs_start,
+ .next = regmap_debugfs_next,
+ .stop = regmap_debugfs_stop,
+ .show = regmap_debugfs_show
+};
- if (copy_to_user(user_buf, buf, buf_pos)) {
- ret = -EFAULT;
- goto out;
+static int debugfs_open(struct inode *inode, struct file *file)
+{
+ int ret;
+ ret = seq_open(file, ®map_debugfs_seq_ops);
+ if (!ret) {
+ struct seq_file *seq;
+ /* seq_open will have modified file->private_data */
+ seq = file->private_data;
+ seq->private = inode->i_private;
}
- *ppos += buf_pos;
-
-out:
- kfree(buf);
return ret;
-}
-
-static ssize_t regmap_map_read_file(struct file *file, char __user *user_buf,
- size_t count, loff_t *ppos)
-{
- struct regmap *map = file->private_data;
+};
- return regmap_read_debugfs(map, 0, map->max_register, user_buf,
- count, ppos);
-}
+static struct file_operations regmap_debugfs_fops = {
+ .owner = THIS_MODULE,
+ .open = debugfs_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = seq_release
+};
#undef REGMAP_ALLOW_WRITE_DEBUGFS
#ifdef REGMAP_ALLOW_WRITE_DEBUGFS
@@ -579,7 +484,7 @@ void regmap_debugfs_init(struct regmap *map, const
char *name)
#endif
debugfs_create_file("registers", registers_mode, map->debugfs,
- map, ®map_map_fops);
+ map, ®map_debugfs_fops);
debugfs_create_file("access", 0400, map->debugfs,
map, ®map_access_fops);
}
--
2.7.4
On Sat, Apr 01, 2017 at 02:13:41AM -0700, noman pouigt wrote:
> Current registers debugfs file implementation doesn't
> handle if the size exceeds 4k. It just dumps 4k of registers.
> Fix this by using the seq_file which already handles
> the file offset logic instead of reinventing the wheel.
>
> I am wondering if there is any issue is doing below which
> I am not aware of?
If I remember correctly this is done the way it is because seq_file has
to iterate through the entire file to get to the point being read by the
application. This is a *very* big overhead for some applications (like
monitoring some registers to see what they're doing) on bigger devices,
especially if there's lots of uncached stuff and the reads have to go to
the hardware. With the current code you can open the file, seek to the
registers you're interested in and read only them. You'll notice that
the other debug files have been converted over to seq_file as they're
pure software and there's less reason to repeatedly read them.
I'm also very surprised that this is failing for you as I know this code
has been fairly heavily exercised with devices with very large register
maps much larger than 4k and my own testing is mainly doing cats which
seemed to work last time I tried and should be iterating over the 4k
boundary... what's the actual failure mode? I'm guessing it's
something if we end a register exactly on a page boundary or something?
Also your patch is completely tab/space mangled so it's not appliable.
> +static void regmap_debugfs_stop(struct seq_file *s, void *v)
> +{
> +}
The need for empty functions is worrying...
> On Apr 21, 2017, at 10:27 AM, Mark Brown <[email protected]> wrote:
Thanks for the response.
>
>> On Sat, Apr 01, 2017 at 02:13:41AM -0700, noman pouigt wrote:
>> Current registers debugfs file implementation doesn't
>> handle if the size exceeds 4k. It just dumps 4k of registers.
>> Fix this by using the seq_file which already handles
>> the file offset logic instead of reinventing the wheel.
>>
>> I am wondering if there is any issue is doing below which
>> I am not aware of?
>
> If I remember correctly this is done the way it is because seq_file has
> to iterate through the entire file to get to the point being read by the
> application. This is a *very* big overhead for some applications (like
> monitoring some registers to see what they're doing) on bigger devices,
Wondering why would the user space application be monitoring the registers?
> especially if there's lots of uncached stuff and the reads have to go to
> the hardware. With the current code you can open the file, seek to the
> registers you're interested in and read only them. You'll notice that
> the other debug files have been converted over to seq_file as they're
> pure software and there's less reason to repeatedly read them.
Yes. I noticed that and that is why I realized that I am doing something wrong.
>
> I'm also very surprised that this is failing for you as I know this code
> has been fairly heavily exercised with devices with very large register
> maps much larger than 4k and my own testing is mainly doing cats which
> seemed to work last time I tried and should be iterating over the 4k
> boundary... what's the actual failure mode? I'm guessing it's
I have integrated Florida codec(wm8281) from cirrus and it has more than 4K registers. I could not dump more than 4K with the existing interface.
Charles, are you able to dump all the registers?
> something if we end a register exactly on a page boundary or something?
>
> Also your patch is completely tab/space mangled so it's not appliable.
>
>> +static void regmap_debugfs_stop(struct seq_file *s, void *v)
>> +{
>> +}
>
> The need for empty functions is worrying...
In several other places in kernel code the usage is similar.
On Sat, Apr 22, 2017 at 01:28:15PM -0700, Variksla wrote:
>
>
> > On Apr 21, 2017, at 10:27 AM, Mark Brown <[email protected]> wrote:
>
> Thanks for the response.
> >
> >> On Sat, Apr 01, 2017 at 02:13:41AM -0700, noman pouigt wrote:
> >> Current registers debugfs file implementation doesn't
> >> handle if the size exceeds 4k. It just dumps 4k of registers.
> >> Fix this by using the seq_file which already handles
> >> the file offset logic instead of reinventing the wheel.
> >>
> >> I am wondering if there is any issue is doing below which
> >> I am not aware of?
> >
> > If I remember correctly this is done the way it is because seq_file has
> > to iterate through the entire file to get to the point being read by the
> > application. This is a *very* big overhead for some applications (like
> > monitoring some registers to see what they're doing) on bigger devices,
>
> Wondering why would the user space application be monitoring the registers?
We do have tooling that accesses and occasionally monitors
registers using the debugfs files. It is often used by hardware
types while testing/debugging issues. And as Mark points out
speed is quite a priority for us on this front so we would be
very keen to avoid anything that slows down the debugfs interface
to the regmap and the ability to seek and acccess just part of
the file is absolutely vital.
> > especially if there's lots of uncached stuff and the reads have to go to
> > the hardware. With the current code you can open the file, seek to the
> > registers you're interested in and read only them. You'll notice that
> > the other debug files have been converted over to seq_file as they're
> > pure software and there's less reason to repeatedly read them.
>
> Yes. I noticed that and that is why I realized that I am doing something wrong.
> >
> > I'm also very surprised that this is failing for you as I know this code
> > has been fairly heavily exercised with devices with very large register
> > maps much larger than 4k and my own testing is mainly doing cats which
> > seemed to work last time I tried and should be iterating over the 4k
> > boundary... what's the actual failure mode? I'm guessing it's
>
> I have integrated Florida codec(wm8281) from cirrus and it has more than 4K registers. I could not dump more than 4K with the existing interface.
>
> Charles, are you able to dump all the registers?
>
I seem to be able to dump all the registers just fine here (all
500k of them). You say it only dumps 4k worth of registers then
just stops? What version of the kernel are you testing this
on? The test I just ran was on Linus's tree, is this an older
kernel or for-next?
If its an older kernel are you perhaps missing one of these
fixes:
commit 26ee47411ae22caa07d3f3b63ca6d097cba6681b
regmap: debugfs: Fix continued read from registers file
commit 213fa5d9685b985e0c61a8db1883a3abf94b18d7
regmap: debugfs: Fix return from regmap_debugfs_get_dump_start
Thanks,
Charles
On Mon, Apr 24, 2017 at 10:02:31AM +0100, Charles Keepax wrote:
> On Sat, Apr 22, 2017 at 01:28:15PM -0700, Variksla wrote:
> > > On Apr 21, 2017, at 10:27 AM, Mark Brown <[email protected]> wrote:
> > > If I remember correctly this is done the way it is because seq_file has
> > > to iterate through the entire file to get to the point being read by the
> > > application. This is a *very* big overhead for some applications (like
> > > monitoring some registers to see what they're doing) on bigger devices,
> > Wondering why would the user space application be monitoring the registers?
> We do have tooling that accesses and occasionally monitors
> registers using the debugfs files. It is often used by hardware
> types while testing/debugging issues. And as Mark points out
Even without an actual program explicitly written for it interactive
monitoring of the register map can be done by a person as well which is
just as much of an application (and the major point of the file).
On Mon, Apr 24, 2017 at 2:02 AM, Charles Keepax
<[email protected]> wrote:
Thanks for replying.
> On Sat, Apr 22, 2017 at 01:28:15PM -0700, Variksla wrote:
>>
>>
>> > On Apr 21, 2017, at 10:27 AM, Mark Brown <[email protected]> wrote:
>>
>> Thanks for the response.
>> >
>> >> On Sat, Apr 01, 2017 at 02:13:41AM -0700, noman pouigt wrote:
>> >> Current registers debugfs file implementation doesn't
>> >> handle if the size exceeds 4k. It just dumps 4k of registers.
I am able to dump till 0x00058a address.
>> >> Fix this by using the seq_file which already handles
>> >> the file offset logic instead of reinventing the wheel.
>> >>
>> >> I am wondering if there is any issue is doing below which
>> >> I am not aware of?
>> >
>> > If I remember correctly this is done the way it is because seq_file has
>> > to iterate through the entire file to get to the point being read by the
>> > application. This is a *very* big overhead for some applications (like
>> > monitoring some registers to see what they're doing) on bigger devices,
>>
>> Wondering why would the user space application be monitoring the registers?
>
> We do have tooling that accesses and occasionally monitors
> registers using the debugfs files. It is often used by hardware
> types while testing/debugging issues. And as Mark points out
> speed is quite a priority for us on this front so we would be
> very keen to avoid anything that slows down the debugfs interface
> to the regmap and the ability to seek and acccess just part of
> the file is absolutely vital.
>
>> > especially if there's lots of uncached stuff and the reads have to go to
>> > the hardware. With the current code you can open the file, seek to the
>> > registers you're interested in and read only them. You'll notice that
>> > the other debug files have been converted over to seq_file as they're
>> > pure software and there's less reason to repeatedly read them.
>>
>> Yes. I noticed that and that is why I realized that I am doing something wrong.
>> >
>> > I'm also very surprised that this is failing for you as I know this code
>> > has been fairly heavily exercised with devices with very large register
>> > maps much larger than 4k and my own testing is mainly doing cats which
>> > seemed to work last time I tried and should be iterating over the 4k
>> > boundary... what's the actual failure mode? I'm guessing it's
>>
>> I have integrated Florida codec(wm8281) from cirrus and it has more than 4K registers. I could not dump more than 4K with the existing interface.
>>
>> Charles, are you able to dump all the registers?
>>
>
> I seem to be able to dump all the registers just fine here (all
> 500k of them). You say it only dumps 4k worth of registers then
> just stops? What version of the kernel are you testing this
> on? The test I just ran was on Linus's tree, is this an older
> kernel or for-next?
>
> If its an older kernel are you perhaps missing one of these
Using
https://github.com/CirrusLogic/linux-drivers/tree/v3.18-arizona/drivers
> fixes:
I have both the patches mentioned here in my tree.
>
> commit 26ee47411ae22caa07d3f3b63ca6d097cba6681b
> regmap: debugfs: Fix continued read from registers file
>
> commit 213fa5d9685b985e0c61a8db1883a3abf94b18d7
> regmap: debugfs: Fix return from regmap_debugfs_get_dump_start
>
> Thanks,
> Charles
On Wed, Jun 07, 2017 at 04:31:15PM -0700, noman pouigt wrote:
> On Mon, Apr 24, 2017 at 2:02 AM, Charles Keepax
> <[email protected]> wrote:
> >> > I'm also very surprised that this is failing for you as I know this code
> >> > has been fairly heavily exercised with devices with very large register
> >> > maps much larger than 4k and my own testing is mainly doing cats which
> >> > seemed to work last time I tried and should be iterating over the 4k
> >> > boundary... what's the actual failure mode? I'm guessing it's
> >>
> >> I have integrated Florida codec(wm8281) from cirrus and it has more than 4K registers. I could not dump more than 4K with the existing interface.
> >>
> >> Charles, are you able to dump all the registers?
> >>
> >
> > I seem to be able to dump all the registers just fine here (all
> > 500k of them). You say it only dumps 4k worth of registers then
> > just stops? What version of the kernel are you testing this
> > on? The test I just ran was on Linus's tree, is this an older
> > kernel or for-next?
> >
> > If its an older kernel are you perhaps missing one of these
>
> Using
> https://github.com/CirrusLogic/linux-drivers/tree/v3.18-arizona/drivers
>
I am able to dump all the registers if I merge in that branch
into my stock 3.18 setup here. My guess would be there must be
some vendor kernel excitement or something going on here. If you
can ping through a copy of your
drivers/base/regmap/regmap-debugfs.c I can have a look at that
see how it compares to mine.
But I suspect you might need to work through and work out where
it is bombing out in regmap_read_debugfs.
Thanks,
Charles