At Thu, 7 Nov 2013 11:31:45 +0000, Mark Brown wrote:
On Thu, Nov 07, 2013 at 08:31:59AM +0100, Takashi Iwai wrote:
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.
Right, but that's not actually what the check is there for. The loop is both doing an event check and building up the register value to write, the check is to tell us if we're confused about the register value since we'll end up writing to the wrong register during coalescing.
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.
Or do two passes, one for the registers and one for the events. You could also reorder the loop to pull the events to the top and then do the check and skip between them and the value update, or simply remove the BUG().
Or just don't worry about it and leave the second loop alone since none of this is ideal - possibly the most helpful thing would be to insert a non-coalesced write for the out of place widget, though depending on why things are going wrong perhaps that'll just make things worse and we should be trying to unwind the entire transaction instead. Unwinding the entire thing is going to be the generally safest but least helpful option.
My main concern here is not to confuse the code by having the check for the register value protecting an event call, that'll be confusing later on. If you just want to fix the BUG()s do that alone, change them to WARN()s or whatever.
OK, back to square, I simplified the patch without extra error handling, just replacing with WARN_ON() at that point. The revised patch is below.
Takashi
--- From: Takashi Iwai tiwai@suse.de Subject: [PATCH] ASoC: dapm: Use WARN_ON() instead of BUG_ON()
Leaving BUG_ON() in a core layer like dapm is rather inappropriate as it leads to panic(), even though sanity checks might be still useful for debugging. Instead, Use WARN_ON(), and handle the error cases accordingly.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/soc/soc-dapm.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index cc36caaf6443..36f4553a2973 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -1460,7 +1460,7 @@ static void dapm_seq_run_coalesced(struct snd_soc_card *card, power_list)->reg;
list_for_each_entry(w, pending, power_list) { - BUG_ON(reg != w->reg); + WARN_ON(reg != w->reg); w->power = w->new_power;
mask |= w->mask << w->shift; @@ -3359,8 +3359,9 @@ static int snd_soc_dai_link_event(struct snd_soc_dapm_widget *w, u64 fmt; int ret;
- BUG_ON(!config); - BUG_ON(list_empty(&w->sources) || list_empty(&w->sinks)); + if (WARN_ON(!config) || + WARN_ON(list_empty(&w->sources) || list_empty(&w->sinks))) + return -EINVAL;
/* We only support a single source and sink, pick the first */ source_p = list_first_entry(&w->sources, struct snd_soc_dapm_path, @@ -3368,9 +3369,10 @@ static int snd_soc_dai_link_event(struct snd_soc_dapm_widget *w, sink_p = list_first_entry(&w->sinks, struct snd_soc_dapm_path, list_source);
- BUG_ON(!source_p || !sink_p); - BUG_ON(!sink_p->source || !source_p->sink); - BUG_ON(!source_p->source || !sink_p->sink); + if (WARN_ON(!source_p || !sink_p) || + WARN_ON(!sink_p->source || !source_p->sink) || + WARN_ON(!source_p->source || !sink_p->sink)) + return -EINVAL;
source = source_p->source->priv; sink = sink_p->sink->priv;