[alsa-devel] [PATCH] ASoC: tegra: Use flat regcache.
When using an rbtree cache, there can be allocations the first time a register is accessed. This can cause an attempt to schedule while atomic in the case that the regmap is using a spinlock. This could be fixed by either initializing all the registers or using a flat cache. The register maps for tegra30_ahub and tegra30_i2s are dense and don't save much from using a tree so convert them to flat.
Signed-off-by: Dylan Reid dgreid@chromium.org --- sound/soc/tegra/tegra30_ahub.c | 4 ++-- sound/soc/tegra/tegra30_i2s.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/sound/soc/tegra/tegra30_ahub.c b/sound/soc/tegra/tegra30_ahub.c index d6f4c99..0db68f4 100644 --- a/sound/soc/tegra/tegra30_ahub.c +++ b/sound/soc/tegra/tegra30_ahub.c @@ -471,7 +471,7 @@ static const struct regmap_config tegra30_ahub_apbif_regmap_config = { .readable_reg = tegra30_ahub_apbif_wr_rd_reg, .volatile_reg = tegra30_ahub_apbif_volatile_reg, .precious_reg = tegra30_ahub_apbif_precious_reg, - .cache_type = REGCACHE_RBTREE, + .cache_type = REGCACHE_FLAT, };
static bool tegra30_ahub_ahub_wr_rd_reg(struct device *dev, unsigned int reg) @@ -490,7 +490,7 @@ static const struct regmap_config tegra30_ahub_ahub_regmap_config = { .max_register = LAST_REG(AUDIO_RX), .writeable_reg = tegra30_ahub_ahub_wr_rd_reg, .readable_reg = tegra30_ahub_ahub_wr_rd_reg, - .cache_type = REGCACHE_RBTREE, + .cache_type = REGCACHE_FLAT, };
static struct tegra30_ahub_soc_data soc_data_tegra30 = { diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c index 49ad936..f146c41 100644 --- a/sound/soc/tegra/tegra30_i2s.c +++ b/sound/soc/tegra/tegra30_i2s.c @@ -357,7 +357,7 @@ static const struct regmap_config tegra30_i2s_regmap_config = { .writeable_reg = tegra30_i2s_wr_rd_reg, .readable_reg = tegra30_i2s_wr_rd_reg, .volatile_reg = tegra30_i2s_volatile_reg, - .cache_type = REGCACHE_RBTREE, + .cache_type = REGCACHE_FLAT, };
static const struct tegra30_i2s_soc_data tegra30_i2s_config = {
When using an rbtree cache, there can be allocations the first time a register is accessed. This can cause an attempt to schedule while atomic in the case that the regmap is using a spinlock. This could be fixed by either initializing all the registers or using a flat cache. The register maps for tegra30_ahub and tegra30_i2s are dense and don't save much from using a tree so convert them to flat.
Looks like the Tegra20 drivers have the same issue as well.
-Andrew
Signed-off-by: Dylan Reid dgreid@chromium.org
sound/soc/tegra/tegra30_ahub.c | 4 ++-- sound/soc/tegra/tegra30_i2s.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/sound/soc/tegra/tegra30_ahub.c b/sound/soc/tegra/tegra30_ahub.c index d6f4c99..0db68f4 100644 --- a/sound/soc/tegra/tegra30_ahub.c +++ b/sound/soc/tegra/tegra30_ahub.c @@ -471,7 +471,7 @@ static const struct regmap_config tegra30_ahub_apbif_regmap_config = { .readable_reg = tegra30_ahub_apbif_wr_rd_reg, .volatile_reg = tegra30_ahub_apbif_volatile_reg, .precious_reg = tegra30_ahub_apbif_precious_reg,
.cache_type = REGCACHE_RBTREE,
.cache_type = REGCACHE_FLAT,
};
static bool tegra30_ahub_ahub_wr_rd_reg(struct device *dev, unsigned int reg) @@ -490,7 +490,7 @@ static const struct regmap_config tegra30_ahub_ahub_regmap_config = { .max_register = LAST_REG(AUDIO_RX), .writeable_reg = tegra30_ahub_ahub_wr_rd_reg, .readable_reg = tegra30_ahub_ahub_wr_rd_reg,
.cache_type = REGCACHE_RBTREE,
.cache_type = REGCACHE_FLAT,
};
static struct tegra30_ahub_soc_data soc_data_tegra30 = { diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c index 49ad936..f146c41 100644 --- a/sound/soc/tegra/tegra30_i2s.c +++ b/sound/soc/tegra/tegra30_i2s.c @@ -357,7 +357,7 @@ static const struct regmap_config tegra30_i2s_regmap_config = { .writeable_reg = tegra30_i2s_wr_rd_reg, .readable_reg = tegra30_i2s_wr_rd_reg, .volatile_reg = tegra30_i2s_volatile_reg,
.cache_type = REGCACHE_RBTREE,
.cache_type = REGCACHE_FLAT,
};
static const struct tegra30_i2s_soc_data tegra30_i2s_config = {
1.8.1.3.605.g02339dd
On Mon, Mar 17, 2014 at 9:07 PM, Andrew Bresticker abrestic@chromium.org wrote:
When using an rbtree cache, there can be allocations the first time a register is accessed. This can cause an attempt to schedule while atomic in the case that the regmap is using a spinlock. This could be fixed by either initializing all the registers or using a flat cache. The register maps for tegra30_ahub and tegra30_i2s are dense and don't save much from using a tree so convert them to flat.
Looks like the Tegra20 drivers have the same issue as well.
Correct, I can tack those on too. I couldn't find a tegra20 board that boots to test on, so that part will only get compile tested.
-Andrew
Signed-off-by: Dylan Reid dgreid@chromium.org
sound/soc/tegra/tegra30_ahub.c | 4 ++-- sound/soc/tegra/tegra30_i2s.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/sound/soc/tegra/tegra30_ahub.c b/sound/soc/tegra/tegra30_ahub.c index d6f4c99..0db68f4 100644 --- a/sound/soc/tegra/tegra30_ahub.c +++ b/sound/soc/tegra/tegra30_ahub.c @@ -471,7 +471,7 @@ static const struct regmap_config tegra30_ahub_apbif_regmap_config = { .readable_reg = tegra30_ahub_apbif_wr_rd_reg, .volatile_reg = tegra30_ahub_apbif_volatile_reg, .precious_reg = tegra30_ahub_apbif_precious_reg,
.cache_type = REGCACHE_RBTREE,
.cache_type = REGCACHE_FLAT,
};
static bool tegra30_ahub_ahub_wr_rd_reg(struct device *dev, unsigned int reg) @@ -490,7 +490,7 @@ static const struct regmap_config tegra30_ahub_ahub_regmap_config = { .max_register = LAST_REG(AUDIO_RX), .writeable_reg = tegra30_ahub_ahub_wr_rd_reg, .readable_reg = tegra30_ahub_ahub_wr_rd_reg,
.cache_type = REGCACHE_RBTREE,
.cache_type = REGCACHE_FLAT,
};
static struct tegra30_ahub_soc_data soc_data_tegra30 = { diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c index 49ad936..f146c41 100644 --- a/sound/soc/tegra/tegra30_i2s.c +++ b/sound/soc/tegra/tegra30_i2s.c @@ -357,7 +357,7 @@ static const struct regmap_config tegra30_i2s_regmap_config = { .writeable_reg = tegra30_i2s_wr_rd_reg, .readable_reg = tegra30_i2s_wr_rd_reg, .volatile_reg = tegra30_i2s_volatile_reg,
.cache_type = REGCACHE_RBTREE,
.cache_type = REGCACHE_FLAT,
};
static const struct tegra30_i2s_soc_data tegra30_i2s_config = {
1.8.1.3.605.g02339dd
At Mon, 17 Mar 2014 20:58:59 -0700, Dylan Reid wrote:
When using an rbtree cache, there can be allocations the first time a register is accessed. This can cause an attempt to schedule while atomic in the case that the regmap is using a spinlock. This could be fixed by either initializing all the registers or using a flat cache. The register maps for tegra30_ahub and tegra30_i2s are dense and don't save much from using a tree so convert them to flat.
Signed-off-by: Dylan Reid dgreid@chromium.org
Looking through regmap code, the fast_io lock seems broken in more places, not only via rbtree cache access. regmap_bulk_write() calls kmemdup() with GFP_KERNEL in the lock context. Ditto in regmap_register_patch(), which calls krealloc() with GFP_KERNEL.
The former could be fixed by moving the lock like below. The fix for the latter depends on whether we need to protect map->patch_regs growth from races or not. If not, krealloc() can be moved out of the lock.
Takashi
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c index 4b2ed0c9e80d..2a1d43e23f1f 100644 --- a/drivers/base/regmap/regmap.c +++ b/drivers/base/regmap/regmap.c @@ -1520,12 +1520,12 @@ int regmap_bulk_write(struct regmap *map, unsigned int reg, const void *val, if (reg % map->reg_stride) return -EINVAL;
- map->lock(map->lock_arg); /* * Some devices don't support bulk write, for * them we have a series of single write operations. */ if (!map->bus || map->use_single_rw) { + map->lock(map->lock_arg); for (i = 0; i < val_count; i++) { unsigned int ival;
@@ -1554,24 +1554,25 @@ int regmap_bulk_write(struct regmap *map, unsigned int reg, const void *val, if (ret != 0) goto out; } + out: + map->unlock(map->lock_arg); } else { void *wval;
wval = kmemdup(val, val_count * val_bytes, GFP_KERNEL); if (!wval) { - ret = -ENOMEM; dev_err(map->dev, "Error in memory allocation\n"); - goto out; + return -ENOMEM; } + map->lock(map->lock_arg); for (i = 0; i < val_count * val_bytes; i += val_bytes) map->format.parse_inplace(wval + i);
ret = _regmap_raw_write(map, reg, wval, val_bytes * val_count);
+ map->unlock(map->lock_arg); kfree(wval); } -out: - map->unlock(map->lock_arg); return ret; } EXPORT_SYMBOL_GPL(regmap_bulk_write);
On Tue, Mar 18, 2014 at 07:46:09AM +0100, Takashi Iwai wrote:
kmemdup() with GFP_KERNEL in the lock context. Ditto in regmap_register_patch(), which calls krealloc() with GFP_KERNEL.
So send a patch...
The former could be fixed by moving the lock like below. The fix for the latter depends on whether we need to protect map->patch_regs growth from races or not. If not, krealloc() can be moved out of the lock.
It should only be happening on init so probably not. On the other hand doing it without any sort of locking isn't great.
At Tue, 18 Mar 2014 10:28:58 +0000, Mark Brown wrote:
On Tue, Mar 18, 2014 at 07:46:09AM +0100, Takashi Iwai wrote:
kmemdup() with GFP_KERNEL in the lock context. Ditto in regmap_register_patch(), which calls krealloc() with GFP_KERNEL.
So send a patch...
Yeah, yeah, don't rush :)
The former could be fixed by moving the lock like below. The fix for the latter depends on whether we need to protect map->patch_regs growth from races or not. If not, krealloc() can be moved out of the lock.
It should only be happening on init so probably not. On the other hand doing it without any sort of locking isn't great.
Right. OTOH, it's still better than papering over with GFP_ATOMIC, I think. We can just give a proper note in the function description, for example.
Takashi
On 03/18/2014 11:33 AM, Takashi Iwai wrote:
At Tue, 18 Mar 2014 10:28:58 +0000, Mark Brown wrote:
On Tue, Mar 18, 2014 at 07:46:09AM +0100, Takashi Iwai wrote:
kmemdup() with GFP_KERNEL in the lock context. Ditto in regmap_register_patch(), which calls krealloc() with GFP_KERNEL.
So send a patch...
Yeah, yeah, don't rush :)
The former could be fixed by moving the lock like below. The fix for the latter depends on whether we need to protect map->patch_regs growth from races or not. If not, krealloc() can be moved out of the lock.
It should only be happening on init so probably not. On the other hand doing it without any sort of locking isn't great.
Right. OTOH, it's still better than papering over with GFP_ATOMIC, I think. We can just give a proper note in the function description, for example.
We should still hold the log over the _regmap_write portion of regmap_register_patch(), but I think we should otherwise be fine if we make it a API requirement that the caller needs to make sure that regmap_register_patch() is not called concurrently to itself or to regcache_sync().
- Lars
On Tue, Mar 18, 2014 at 11:39:30AM +0100, Lars-Peter Clausen wrote:
On 03/18/2014 11:33 AM, Takashi Iwai wrote:
Right. OTOH, it's still better than papering over with GFP_ATOMIC, I think. We can just give a proper note in the function description, for example.
We should still hold the log over the _regmap_write portion of regmap_register_patch(), but I think we should otherwise be fine if we make it a API requirement that the caller needs to make sure that regmap_register_patch() is not called concurrently to itself or to regcache_sync().
Yes, it's just the (re)alloc I was talking about there.
participants (5)
-
Andrew Bresticker
-
Dylan Reid
-
Lars-Peter Clausen
-
Mark Brown
-
Takashi Iwai