[alsa-devel] [PATCH 13/31] ASoC: wm_adsp: Fix BUG_ON() and WARN_ON() usages

[Takashi Iwai]

At Wed, 6 Nov 2013 18:44:22 +0000,
Charles Keepax wrote:
> 
> On Tue, Nov 05, 2013 at 06:40:00PM +0100, Takashi Iwai wrote:
> > @@ -645,27 +647,22 @@ static int wm_adsp_load(struct wm_adsp *dsp)
> >  			reg = offset;
> >  			break;
> >  		case WMFW_ADSP1_PM:
> > -			BUG_ON(!mem);
> >  			region_name = "PM";
> >  			reg = wm_adsp_region_to_reg(mem, offset);
> >  			break;
> >  		case WMFW_ADSP1_DM:
> > -			BUG_ON(!mem);
> >  			region_name = "DM";
> >  			reg = wm_adsp_region_to_reg(mem, offset);
> >  			break;
> >  		case WMFW_ADSP2_XM:
> > -			BUG_ON(!mem);
> >  			region_name = "XM";
> >  			reg = wm_adsp_region_to_reg(mem, offset);
> >  			break;
> >  		case WMFW_ADSP2_YM:
> > -			BUG_ON(!mem);
> >  			region_name = "YM";
> >  			reg = wm_adsp_region_to_reg(mem, offset);
> >  			break;
> >  		case WMFW_ADSP1_ZM:
> > -			BUG_ON(!mem);
> >  			region_name = "ZM";
> >  			reg = wm_adsp_region_to_reg(mem, offset);
> >  			break;
> 
> These do make me a little nervous previously we would panic here,
> so everything would stop but now we may write random registers on
> the CODEC which could cause some odd behaviour. I think it would
> be better to update wm_adsp_region_to_reg to allow a failed
> return code and then error out of wm_adsp_load rather than just
> relying on the WARN_ON inside.

Yeah, handling the error properly would be definitely better.
Care to enhance the patch?

Actually, I decided to work on and send these patch series just
because I casually found that there are so many BUG*() macros in the
tree.  Fixing bugs aren't my intention.

Note that it's perfectly fine that BUG*() are used in the development
code during the development phase.  It's a matter of debugging
techniques.

However, It's definitely not OK if these BUG*() are left in the
production code.  Particularly, the common code (like codec drivers)
that can be called by others shouldn't trigger the death button like
that.

This problem can be seen from several perspectives.

Firstly, leaving BUG*() means essentially that the driver can be
buggy.  If such a problem might happen in reality, you should have
written the driver code more cleanly to handle the exceptions.

Secondly, more importantly, it's end-users who would suffer from the
sudden death problem, not the developers.  This is the production
level, so you can't justify the sudden death.  And, with panic() by
BUG*(), developers would receive only news that machines are killed
without dying messages.


Takashi