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

[Takashi Iwai]

At Wed, 6 Nov 2013 23:48:06 +0000,
Mark Brown wrote:
> 
> On Wed, Nov 06, 2013 at 06:21:36PM +0100, Takashi Iwai wrote:
> 
> > 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.
> 
> Sure, it's a bit nicer in some situations.  My point is that you're
> greatly overstating the problems with asserting in practical usage.
> The environment and productisation process used with this stuff is
> different to that for PCs.

I do understand that BUG*() can be used in the development phase.
But my intention in the whole silly patchsets is to eliminate BUG*()
in the production code.  What we have in our git trees are supposed to
be all in the production level.  At such a stage, we shouldn't leave
mines.  As already mentioned in another post, it'd be end-users who
suffer from the explosion, and the developer would get much less
benefit by that.  You'll see only a dead body after explosion.

So, just kill these BUG*() in your code.  It's irresponsible to leave
such things in the final code.


> > BTW, another substantial problem is that this dapm code doesn't handle
> > any errors...
> 
> Feel free to send patches.  I've never considered it a high priority to
> go through and add stuff there since the unwinding code is way more
> effort to write than it's worth and would never get tested - generally
> where constructive error recovery is possible the natural user reaction
> of stopping and restarting things is about as constructive as it gets
> anyway.  There's always something more pressing to work on.
> 
> > > 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.
> 
> I don't understand what you mean by this?

The insertion of the very same check is to avoid applying the POST
events for the items that haven't been processed for PRE events.

> > >  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.
> 
> But then we're still left with bombing out if the first half of the
> function ever changes.  The check is there for the benefit of the
> register handling, the fact that it's affecting events is just a side
> effect rather than the intention.

Actually the check reg == w->reg must be always true for all pending
items, no matter at which point.  Otherwise BUG() would have hit.

With the patch, the behavior is changed to that the item that doesn't
satisfy reg == w->reg will be skipped in the first loop.  At the
second loop, these items need to be skipped as well.  They are broken
items, and the PRE_ events weren't applied, thus you must not apply
POST_ events to them --- it's the very reason of the second loop
check.

That being said, maybe a cleaner way would be to run the check over
the loop once, and quit the function if anything inconsistent found.

> Remember also that BUG() can be disabled so there is a faint chance that
> we'd not trigger, and of course the issue might be introduced between
> the two events.

If the issue were to be introduced between two loops, it's rather
nicer if you can catch there? :)

Well, I really don't mind how to implement.  Again, my wish is just to
kill crappy BUG*() calls from my tree.


thanks,

Takashi