Message-ID: <CAES_P+-Y1eocM043HDtU1T6Aj=UaX80pj0z37SmYhVyhY1YZsw@mail.gmail.com>
Date:   Sat, 1 Apr 2017 02:13:41 -0700
From:   noman pouigt <variksla@...il.com>
To:     "broonie@...nel.org" <broonie@...nel.org>,
        linux-kernel@...r.kernel.org
Cc:     gregkh@...uxfoundation.org
Subject: bug fix for registers debugfs file implementation [RFC]

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 <variksla@...il.com>
---
 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, &regmap_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, &regmap_map_fops);
+                    map, &regmap_debugfs_fops);
         debugfs_create_file("access", 0400, map->debugfs,
                     map, &regmap_access_fops);
     }
-- 
2.7.4

Powered by blists - more mailing lists