[alsa-devel] [PATCH 21/31] ASoC: dapm: Use WARN_ON() instead of BUG_ON()

[Takashi Iwai]

At Wed, 6 Nov 2013 11:48:36 +0000,
Mark Brown wrote:
> 
> [1  <text/plain; us-ascii (quoted-printable)>]
> On Wed, Nov 06, 2013 at 12:33:14PM +0100, Takashi Iwai wrote:
> > Mark Brown wrote:
> 
> > > Please fix the comment about BUG_ON() being useless; it does exactly
> > > what it's supposed to do.
> 
> > Well,  I can correct the word "useless" with "stupid" in the comment,
> > as BUG_ON() is the most stupid idea to do at such a point.  soc-dapm.c
> > is the common code that can be executed from any others. How you can
> > know that the machine *must* be killed suddenly at this point?  If you
> > didn't attach the console beforehand, you can't get even any debug
> > logs.
> 
> The point is that it's not appropriate in this situation but not
> useless; if it were useless you should be sending a patch to remove it
> from the kernel entirely.  

OK, feel free to run s/useless/inappropriate/ in the patch.
I don't mind wording but only codes.

> > Using BUG() and BUG_ON() in buggy points without any really serious
> > damage (like heavy memory corruption or data corruption) has to be
> > avoided.  Linus stated this in the past, too.  (I have no pointer
> > now, unfortunately.)
> 
> The ones in DAPM are mostly about memory corruption; if this stuff is
> getting confused then there's a good chance that the data structures
> that it's looking at aren't what they're supposed to be.

This isn't so easy to guess.  Such a bug is often a memory corruption
indeed, but it's not necessarily so.  The data can be modified by the
device drivers that are calling dapm, so everything is possible.
Driver writers can do anything unbelievable you can never imagine...

Practically seen, using BUG_ON() makes such debugging even more
difficult.

> > > >  	list_for_each_entry(w, pending, power_list) {
> > > > +		if (WARN_ON(reg != w->reg))
> > > > +			continue;
> > > >  		dapm_seq_check_event(card, w, SND_SOC_DAPM_POST_PMU);
> > > >  		dapm_seq_check_event(card, w, SND_SOC_DAPM_POST_PMD);
> > > >  	}
> 
> > > We're not using reg here...
> 
> > Because you must not issue *_POST_* events without *_PRE_* events.
> > *_PRE_* are skipped for these entries in the previous WARN_ON()
> > checks.
> 
> No, that's not the case at all - it's very rare for devices to have both
> events in the first place.

Hrm, are you saying that *_POST events can be issued without the
corresponding *_PRE events, as an official dpam design?  I didn't
expect such a bad consistency in API design level...


Takashi