2023-07-13 00:56:23

by Mark Brown

[permalink] [raw]
Subject: [PATCH 08/11] mfd: wm5110: Update to use maple tree register cache

The maple tree register cache is based on a much more modern data structure
than the rbtree cache and makes optimisation choices which are probably
more appropriate for modern systems than those made by the rbtree cache. In
v6.5 it has also acquired the ability to generate multi-register writes in
sync operations, bringing performance up to parity with the rbtree cache
there.

Update the wm5110 driver to use the more modern data structure.

Signed-off-by: Mark Brown <[email protected]>
---
drivers/mfd/wm5110-tables.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mfd/wm5110-tables.c b/drivers/mfd/wm5110-tables.c
index 65b9b1d6daec..eba324875afd 100644
--- a/drivers/mfd/wm5110-tables.c
+++ b/drivers/mfd/wm5110-tables.c
@@ -3218,7 +3218,7 @@ const struct regmap_config wm5110_i2c_regmap = {
.readable_reg = wm5110_readable_register,
.volatile_reg = wm5110_volatile_register,

- .cache_type = REGCACHE_RBTREE,
+ .cache_type = REGCACHE_MAPLE,
.reg_defaults = wm5110_reg_default,
.num_reg_defaults = ARRAY_SIZE(wm5110_reg_default),
};

--
2.39.2



2023-07-18 16:00:04

by Charles Keepax

[permalink] [raw]
Subject: Re: [PATCH 08/11] mfd: wm5110: Update to use maple tree register cache

On Thu, Jul 13, 2023 at 01:49:26AM +0100, Mark Brown wrote:
> The maple tree register cache is based on a much more modern data structure
> than the rbtree cache and makes optimisation choices which are probably
> more appropriate for modern systems than those made by the rbtree cache. In
> v6.5 it has also acquired the ability to generate multi-register writes in
> sync operations, bringing performance up to parity with the rbtree cache
> there.
>
> Update the wm5110 driver to use the more modern data structure.
>
> Signed-off-by: Mark Brown <[email protected]>
> ---
> drivers/mfd/wm5110-tables.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mfd/wm5110-tables.c b/drivers/mfd/wm5110-tables.c
> index 65b9b1d6daec..eba324875afd 100644
> --- a/drivers/mfd/wm5110-tables.c
> +++ b/drivers/mfd/wm5110-tables.c
> @@ -3218,7 +3218,7 @@ const struct regmap_config wm5110_i2c_regmap = {
> .readable_reg = wm5110_readable_register,
> .volatile_reg = wm5110_volatile_register,
>
> - .cache_type = REGCACHE_RBTREE,
> + .cache_type = REGCACHE_MAPLE,
> .reg_defaults = wm5110_reg_default,
> .num_reg_defaults = ARRAY_SIZE(wm5110_reg_default),
> };
>
> --
> 2.39.2
>

This one appears to cause me some issues, seems to get the IRQs
into a weird state when doing compressed stream stuff. The
issue seems to also require commit bfa0b38c1483 ("regmap:
maple: Implement block sync for the maple tree cache") to be
present. So it definitely seems to relate to the cache sync,
but not sure if it is something todo with the device itself,
or the maple tree stuff yet.

Thanks,
Charles

2023-07-18 17:43:41

by Charles Keepax

[permalink] [raw]
Subject: Re: [PATCH 08/11] mfd: wm5110: Update to use maple tree register cache

On Tue, Jul 18, 2023 at 03:42:00PM +0000, Charles Keepax wrote:
> On Thu, Jul 13, 2023 at 01:49:26AM +0100, Mark Brown wrote:
> > The maple tree register cache is based on a much more modern data structure
> > than the rbtree cache and makes optimisation choices which are probably
> > more appropriate for modern systems than those made by the rbtree cache. In
> > v6.5 it has also acquired the ability to generate multi-register writes in
> > sync operations, bringing performance up to parity with the rbtree cache
> > there.
> >
> > Update the wm5110 driver to use the more modern data structure.
> >
> > Signed-off-by: Mark Brown <[email protected]>
> > ---
> > drivers/mfd/wm5110-tables.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/mfd/wm5110-tables.c b/drivers/mfd/wm5110-tables.c
> > index 65b9b1d6daec..eba324875afd 100644
> > --- a/drivers/mfd/wm5110-tables.c
> > +++ b/drivers/mfd/wm5110-tables.c
> > @@ -3218,7 +3218,7 @@ const struct regmap_config wm5110_i2c_regmap = {
> > .readable_reg = wm5110_readable_register,
> > .volatile_reg = wm5110_volatile_register,
> >
> > - .cache_type = REGCACHE_RBTREE,
> > + .cache_type = REGCACHE_MAPLE,
> > .reg_defaults = wm5110_reg_default,
> > .num_reg_defaults = ARRAY_SIZE(wm5110_reg_default),
> > };
> >
> > --
> > 2.39.2
> >
>
> This one appears to cause me some issues, seems to get the IRQs
> into a weird state when doing compressed stream stuff. The
> issue seems to also require commit bfa0b38c1483 ("regmap:
> maple: Implement block sync for the maple tree cache") to be
> present. So it definitely seems to relate to the cache sync,
> but not sure if it is something todo with the device itself,
> or the maple tree stuff yet.
>

Ah... I think I see the regcache_sync sets async=true, but then
the maple tree code immediately deletes the buffer after calling
_regmap_raw_write. So its a racy use after free.

How would we feel about having the maple tree code, clear async
again?

Thanks,
Charles

2023-07-18 18:12:41

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH 08/11] mfd: wm5110: Update to use maple tree register cache

On Tue, Jul 18, 2023 at 05:00:35PM +0000, Charles Keepax wrote:
> On Tue, Jul 18, 2023 at 03:42:00PM +0000, Charles Keepax wrote:

> > This one appears to cause me some issues, seems to get the IRQs
> > into a weird state when doing compressed stream stuff. The
> > issue seems to also require commit bfa0b38c1483 ("regmap:
> > maple: Implement block sync for the maple tree cache") to be
> > present. So it definitely seems to relate to the cache sync,
> > but not sure if it is something todo with the device itself,
> > or the maple tree stuff yet.

> Ah... I think I see the regcache_sync sets async=true, but then
> the maple tree code immediately deletes the buffer after calling
> _regmap_raw_write. So its a racy use after free.

> How would we feel about having the maple tree code, clear async
> again?

I was going to say, it must be a maple tree issue. I think we should
push that async down into the rbtree code, that's probably also broken
for other cache types if used in conjunction with slow buses...


Attachments:
(No filename) (1.02 kB)
signature.asc (499.00 B)
Download all attachments