| Message ID | 1414202637-18929-1-git-send-email-nicoleotsuka@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Oct 24, 2014 at 07:03:57PM -0700, Nicolin Chen wrote: > Kernel dump (WARN_ON) ocurred during system boot-up inside regmap_write(): > > ------------[ cut here ]------------ > WARNING: CPU: 0 PID: 47 at kernel/locking/lockdep.c:2744 lockdep_trace_alloc+0xe8/0x108() Applied, thanks. Please edit down or elide backtraces - they take up a lot of space in the changelog compared to the amount of information they contain. > By looking at 2744 line, we can get that it's because regcache_rbtree_write() > would call kmalloc() with GFP flag if it couldn't find an existing block to > insert nodes while this kmalloc() call is inside a spin_lock_irq_save pair, > i.e. IRQs disabled. > Even though this may be a bug that should be fixed, I still try to send this > patch as a quick fix (work around) since it does no harm to assign default > values of every registers when using regcache. It's not a bug, it's not reasonable to default allocations to atomic and we can't really tell what context we're in. Anything used inside a heavily locked path should either have a default provided or arrange for a prior write to set up the cache.
On Tue, Oct 28, 2014 at 12:19:04AM +0000, Mark Brown wrote: > On Fri, Oct 24, 2014 at 07:03:57PM -0700, Nicolin Chen wrote: > > Kernel dump (WARN_ON) ocurred during system boot-up inside regmap_write(): > > > > ------------[ cut here ]------------ > > WARNING: CPU: 0 PID: 47 at kernel/locking/lockdep.c:2744 lockdep_trace_alloc+0xe8/0x108() > > Even though this may be a bug that should be fixed, I still try to send this > > patch as a quick fix (work around) since it does no harm to assign default > > values of every registers when using regcache. > > It's not a bug, it's not reasonable to default allocations to atomic and > we can't really tell what context we're in. Anything used inside a > heavily locked path should either have a default provided or arrange for > a prior write to set up the cache. I've a little trouble to understand the prior write over here. Inside my probe() there's a register_init() call which has a set of regmap_write(). And then the first regmap_write() results the dump. Does that mean this regmap_write() isn't prior write? If so, how should I do if not setting default values here -- Some IPs may have default value 0 for registers. And this would make reg_defaults tedious since there's nothing special to assign. And actually I remember I haven't seen any dump when I sent the initial patch for the whole ASRC driver but it manifested last month. I thought it might be a partially-merging issue and it would be fixed after merge window's done..... Thank you Nicolin
On Mon, Oct 27, 2014 at 09:26:26PM -0700, Nicolin Chen wrote: > On Tue, Oct 28, 2014 at 12:19:04AM +0000, Mark Brown wrote: > > It's not a bug, it's not reasonable to default allocations to atomic and > > we can't really tell what context we're in. Anything used inside a > > heavily locked path should either have a default provided or arrange for > > a prior write to set up the cache. > I've a little trouble to understand the prior write over here. Inside my > probe() there's a register_init() call which has a set of regmap_write(). > And then the first regmap_write() results the dump. Does that mean this > regmap_write() isn't prior write? If so, how should I do if not setting Oh, bother. We fixed things so that we're now always running with the spinlock held... never mind. > default values here -- Some IPs may have default value 0 for registers. > And this would make reg_defaults tedious since there's nothing special > to assign. Write a small script then, or a little bit of code to create the defaults dynamically. > And actually I remember I haven't seen any dump when I sent the initial > patch for the whole ASRC driver but it manifested last month. I thought > it might be a partially-merging issue and it would be fixed after merge > window's done..... Nothing has changed here for a while as far as I remember.
On Tue, Oct 28, 2014 at 10:47:42AM +0000, Mark Brown wrote: > On Mon, Oct 27, 2014 at 09:26:26PM -0700, Nicolin Chen wrote: > > On Tue, Oct 28, 2014 at 12:19:04AM +0000, Mark Brown wrote: > > > > It's not a bug, it's not reasonable to default allocations to atomic and > > > we can't really tell what context we're in. Anything used inside a > > > heavily locked path should either have a default provided or arrange for > > > a prior write to set up the cache. > > > I've a little trouble to understand the prior write over here. Inside my > > probe() there's a register_init() call which has a set of regmap_write(). > > And then the first regmap_write() results the dump. Does that mean this > > regmap_write() isn't prior write? If so, how should I do if not setting > > Oh, bother. We fixed things so that we're now always running with the > spinlock held... never mind. Okay...so only one choice left. > > default values here -- Some IPs may have default value 0 for registers. > > And this would make reg_defaults tedious since there's nothing special > > to assign. > > Write a small script then, or a little bit of code to create the > defaults dynamically. It actually doesn't bother me at all. I just thought there might be a simpler way. :) Thank you Nicolin
============================================================================= 2741 /* 2742 * Oi! Can't be having __GFP_FS allocations with IRQs disabled. 2743 */ 2744 if (DEBUG_LOCKS_WARN_ON(irqs_disabled_flags(flags))) 2745 return; ============================================================================= By looking at 2744 line, we can get that it's because regcache_rbtree_write() would call kmalloc() with GFP flag if it couldn't find an existing block to insert nodes while this kmalloc() call is inside a spin_lock_irq_save pair, i.e. IRQs disabled. Even though this may be a bug that should be fixed, I still try to send this patch as a quick fix (work around) since it does no harm to assign default values of every registers when using regcache. Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com> --- sound/soc/fsl/fsl_asrc.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c index f2d8652..817d304 100644 --- a/sound/soc/fsl/fsl_asrc.c +++ b/sound/soc/fsl/fsl_asrc.c @@ -684,12 +684,38 @@ static bool fsl_asrc_writeable_reg(struct device *dev, unsigned int reg) } } +static struct reg_default fsl_asrc_reg[] = { + { REG_ASRCTR, 0x0000 }, { REG_ASRIER, 0x0000 }, + { REG_ASRCNCR, 0x0000 }, { REG_ASRCFG, 0x0000 }, + { REG_ASRCSR, 0x0000 }, { REG_ASRCDR1, 0x0000 }, + { REG_ASRCDR2, 0x0000 }, { REG_ASRSTR, 0x0000 }, + { REG_ASRRA, 0x0000 }, { REG_ASRRB, 0x0000 }, + { REG_ASRRC, 0x0000 }, { REG_ASRPM1, 0x0000 }, + { REG_ASRPM2, 0x0000 }, { REG_ASRPM3, 0x0000 }, + { REG_ASRPM4, 0x0000 }, { REG_ASRPM5, 0x0000 }, + { REG_ASRTFR1, 0x0000 }, { REG_ASRCCR, 0x0000 }, + { REG_ASRDIA, 0x0000 }, { REG_ASRDOA, 0x0000 }, + { REG_ASRDIB, 0x0000 }, { REG_ASRDOB, 0x0000 }, + { REG_ASRDIC, 0x0000 }, { REG_ASRDOC, 0x0000 }, + { REG_ASRIDRHA, 0x0000 }, { REG_ASRIDRLA, 0x0000 }, + { REG_ASRIDRHB, 0x0000 }, { REG_ASRIDRLB, 0x0000 }, + { REG_ASRIDRHC, 0x0000 }, { REG_ASRIDRLC, 0x0000 }, + { REG_ASR76K, 0x0A47 }, { REG_ASR56K, 0x0DF3 }, + { REG_ASRMCRA, 0x0000 }, { REG_ASRFSTA, 0x0000 }, + { REG_ASRMCRB, 0x0000 }, { REG_ASRFSTB, 0x0000 }, + { REG_ASRMCRC, 0x0000 }, { REG_ASRFSTC, 0x0000 }, + { REG_ASRMCR1A, 0x0000 }, { REG_ASRMCR1B, 0x0000 }, + { REG_ASRMCR1C, 0x0000 }, +}; + static const struct regmap_config fsl_asrc_regmap_config = { .reg_bits = 32, .reg_stride = 4, .val_bits = 32, .max_register = REG_ASRMCR1C, + .reg_defaults = fsl_asrc_reg, + .num_reg_defaults = ARRAY_SIZE(fsl_asrc_reg), .readable_reg = fsl_asrc_readable_reg, .volatile_reg = fsl_asrc_volatile_reg, .writeable_reg = fsl_asrc_writeable_reg,