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

[Takashi Iwai]

At Wed, 6 Nov 2013 16:03:04 +0000,
Mark Brown wrote:
> 
> On Wed, Nov 06, 2013 at 01:03:41PM +0100, Takashi Iwai wrote:
> > Mark Brown wrote:
> 
> > > 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...
> 
> If the driver is modifying the DAPM data then it looses anyway.

Yes, but you cannot know in DAPM level.  That's the point.
In soc-dapm.c, you know only something is wrong.  But you can never
know how severely wrong it is.

> > Practically seen, using BUG_ON() makes such debugging even more
> > difficult.
> 
> That's not been my experience at all with these ones, it's much more
> common for people to ignore soft failures and then wonder why they get
> breakage further down the line.
> 
> This is also partly a function of the usage model for these drivers - if
> someone is triggering an issue they will almost certainly be doing
> development on the system.  There aren't the problems with random system
> configurations or unknown hardware that you see with specs like HDA.

My concern here is that dapm isn't in the position to decide whether
you press the self destruction switch at all.  DAPM, or in general
ASoC core, should serve at best it can, should return an error if
something looks weird.  As repeatedly mentioned, in the layer of DAPM,
you cannot know what could be wrong, have too little to judge whether
it's an emergency exit or not.

So, basically what such lazy people would need is a knob to make
WARN*() triggering panic().  But this shouldn't be done
unconditionally for all DAPM usages, or better in all sound driver
usages.

BTW, another substantial problem is that this dapm code doesn't handle
any errors...

> > > > 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...
> 
> They shouldn't be generated unmatched but it's extremely rare for them
> both to actually be used by one widget.

So, the unprocessed list item must be filtered out.

>  To the extent that they're used
> matched it's with the PRE_PMU matched with POST_PMD and vice versa,
> though using just one is quite common for inserting delays.  It's going
> to cause more harm than good to actively suppress events.

It won't suppress any events that have been working.
Remember that there's been BUG_ON() with the very same condition
before the patch.  Thus any good-working cases must not hit with this
conversion.


Takashi