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