* [PATCH] ASoC: tegra: Use flat regcache.
@ 2014-03-18 3:58 Dylan Reid
[not found] ` <1395115139-22243-1-git-send-email-dgreid-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
0 siblings, 1 reply; 8+ messages in thread
From: Dylan Reid @ 2014-03-18 3:58 UTC (permalink / raw)
To: alsa-devel; +Cc: linux-tegra, abrestic, Dylan Reid, broonie, swarren
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 = {
--
1.8.1.3.605.g02339dd
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] ASoC: tegra: Use flat regcache.
[not found] ` <1395115139-22243-1-git-send-email-dgreid-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
@ 2014-03-18 4:07 ` Andrew Bresticker
2014-03-18 5:04 ` Dylan Reid
2014-03-18 6:46 ` [alsa-devel] " Takashi Iwai
1 sibling, 1 reply; 8+ messages in thread
From: Andrew Bresticker @ 2014-03-18 4:07 UTC (permalink / raw)
To: Dylan Reid
Cc: alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
broonie-DgEjT+Ai2ygdnm+yROfE0A, Stephen Warren
> 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-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ASoC: tegra: Use flat regcache.
2014-03-18 4:07 ` Andrew Bresticker
@ 2014-03-18 5:04 ` Dylan Reid
0 siblings, 0 replies; 8+ messages in thread
From: Dylan Reid @ 2014-03-18 5:04 UTC (permalink / raw)
To: Andrew Bresticker; +Cc: linux-tegra, alsa-devel, broonie, Stephen Warren
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
>>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [alsa-devel] [PATCH] ASoC: tegra: Use flat regcache.
[not found] ` <1395115139-22243-1-git-send-email-dgreid-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
2014-03-18 4:07 ` Andrew Bresticker
@ 2014-03-18 6:46 ` Takashi Iwai
[not found] ` <s5hha6wdmxa.wl%tiwai-l3A5Bk7waGM@public.gmane.org>
1 sibling, 1 reply; 8+ messages in thread
From: Takashi Iwai @ 2014-03-18 6:46 UTC (permalink / raw)
To: Dylan Reid
Cc: alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
abrestic-F7+t8E8rja9g9hUCZPvPmw, broonie-DgEjT+Ai2ygdnm+yROfE0A,
swarren-3lzwWm7+Weoh9ZMKESR00Q
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-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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);
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [alsa-devel] [PATCH] ASoC: tegra: Use flat regcache.
[not found] ` <s5hha6wdmxa.wl%tiwai-l3A5Bk7waGM@public.gmane.org>
@ 2014-03-18 10:28 ` Mark Brown
[not found] ` <20140318102858.GE11706-GFdadSzt00ze9xe1eoZjHA@public.gmane.org>
0 siblings, 1 reply; 8+ messages in thread
From: Mark Brown @ 2014-03-18 10:28 UTC (permalink / raw)
To: Takashi Iwai
Cc: Dylan Reid, alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
abrestic-F7+t8E8rja9g9hUCZPvPmw, swarren-3lzwWm7+Weoh9ZMKESR00Q
[-- Attachment #1: Type: text/plain, Size: 552 bytes --]
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.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [alsa-devel] [PATCH] ASoC: tegra: Use flat regcache.
[not found] ` <20140318102858.GE11706-GFdadSzt00ze9xe1eoZjHA@public.gmane.org>
@ 2014-03-18 10:33 ` Takashi Iwai
2014-03-18 10:39 ` Lars-Peter Clausen
0 siblings, 1 reply; 8+ messages in thread
From: Takashi Iwai @ 2014-03-18 10:33 UTC (permalink / raw)
To: Mark Brown
Cc: Dylan Reid, alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
abrestic-F7+t8E8rja9g9hUCZPvPmw, swarren-3lzwWm7+Weoh9ZMKESR00Q
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ASoC: tegra: Use flat regcache.
2014-03-18 10:33 ` Takashi Iwai
@ 2014-03-18 10:39 ` Lars-Peter Clausen
[not found] ` <53282262.2030300-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>
0 siblings, 1 reply; 8+ messages in thread
From: Lars-Peter Clausen @ 2014-03-18 10:39 UTC (permalink / raw)
To: Takashi Iwai
Cc: alsa-devel, swarren, abrestic, Mark Brown, linux-tegra, Dylan Reid
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [alsa-devel] [PATCH] ASoC: tegra: Use flat regcache.
[not found] ` <53282262.2030300-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>
@ 2014-03-18 10:44 ` Mark Brown
0 siblings, 0 replies; 8+ messages in thread
From: Mark Brown @ 2014-03-18 10:44 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: Takashi Iwai, linux-tegra-u79uwXL29TY76Z2rM5mHXA,
abrestic-F7+t8E8rja9g9hUCZPvPmw, Dylan Reid,
alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw,
swarren-3lzwWm7+Weoh9ZMKESR00Q
[-- Attachment #1: Type: text/plain, Size: 627 bytes --]
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.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-03-18 10:44 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-03-18 3:58 [PATCH] ASoC: tegra: Use flat regcache Dylan Reid
[not found] ` <1395115139-22243-1-git-send-email-dgreid-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
2014-03-18 4:07 ` Andrew Bresticker
2014-03-18 5:04 ` Dylan Reid
2014-03-18 6:46 ` [alsa-devel] " Takashi Iwai
[not found] ` <s5hha6wdmxa.wl%tiwai-l3A5Bk7waGM@public.gmane.org>
2014-03-18 10:28 ` Mark Brown
[not found] ` <20140318102858.GE11706-GFdadSzt00ze9xe1eoZjHA@public.gmane.org>
2014-03-18 10:33 ` Takashi Iwai
2014-03-18 10:39 ` Lars-Peter Clausen
[not found] ` <53282262.2030300-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>
2014-03-18 10:44 ` [alsa-devel] " Mark Brown
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).