Messages in this thread | | | Date | Wed, 21 Aug 2013 16:21:43 +0200 | From | David Jander <> | Subject | Re: [PATCH] drivers: regmap: bugfix in regcache-rbtree.c |
| |
On Wed, 21 Aug 2013 14:32:00 +0100 Mark Brown <broonie@kernel.org> wrote:
> On Wed, Aug 21, 2013 at 03:02:35PM +0200, David Jander wrote: > > > rbnode register ranges can overlap, which is not a problem as long as > > They can? They aren't supposed to and I'd expect this to cause problems > with the cache sync code too. How does this happen?
Well, I am not an expert at rb-trees nor do I understand all of regmap, but I think I can explain how it can happen. The fact that it _does_ happen can be seen in my previous e-mail. Here's what I get from mainline SGTL5000 driver:
# cat /sys/kernel/debug/regmap/1-000a/rbtree 2-19 (24) 4-1b (24) 20-37 (24) 22-39 (24) 3c-53 (24) 100-117 (24) 104-11c (25) 11e-135 (24) 8 nodes, 193 registers, average 24 registers, used 626 bytes
Tracing all the calls to regcache-rbtree, I can see that the node "22-39 (24)" is created first, and later on, the driver tries to write to register 20 for the first time (the node 22-39 is still pointed to by rbtree_ctx->cached_rbnode). At that point the following code at line 358 is hit:
rbnode = regcache_rbtree_lookup(map, reg);
(rbnode will be NULL, since the register isn't mapped to the cache yet)
if (rbnode) { reg_tmp = (reg - rbnode->base_reg) / map->reg_stride; regcache_rbtree_set_register(map, rbnode, reg_tmp, value); } else { /* look for an adjacent register to the one we are about to add */
The following code will not find an adjacent register, becase map->reg_stride is 1 and not 2 as it should be. This is due to a different unrelated bug in sgtl5000.c which I will fix soon, but it doesn't matter for this case.
for (node = rb_first(&rbtree_ctx->root); node; node = rb_next(node)) { rbnode_tmp = rb_entry(node, struct regcache_rbtree_node, node); for (i = 0; i < rbnode_tmp->blklen; i++) { reg_tmp = rbnode_tmp->base_reg + (i * map->reg_stride); if (abs(reg_tmp - reg) != map->reg_stride) continue; /* decide where in the block to place our register */ if (reg_tmp + map->reg_stride == reg) pos = i + 1; else pos = i; ret = regcache_rbtree_insert_to_block(map, rbnode_tmp, pos, reg, value); if (ret) return ret; return 0; } }
So we didn't find an adjacent register, we will create a new node.
/* We did not manage to find a place to insert it in * an existing block so create a new rbnode. */ rbnode = regcache_rbtree_node_alloc(map, reg); if (!rbnode) return -ENOMEM; regcache_rbtree_set_register(map, rbnode, reg - rbnode->base_reg, value); regcache_rbtree_insert(map, &rbtree_ctx->root, rbnode); }
At this point the rbnode "20-37 (24)" is created. I don't (yet) fully understand the code in regcache_rbtree_node_alloc(), but it seems to ignore the fact that this new node will start at only slightly lower base register than another existing rbnode.
I hope you can explain to me how regcache_rbtree_node_alloc() is supposed to work, because I start to think that something in there is broken... Specially the code at line 323 strikes me:
if (!rbnode->blklen) { rbnode->blklen = sizeof(*rbnode); rbnode->base_reg = reg; }
Best regards,
-- David Jander Protonic Holland.
| |