[alsa-devel] [PATCH 6/9] ASoC: max98095: Add missing negative channel checks

[Takashi Iwai]

At Wed, 30 Oct 2013 14:31:21 -0700,
Mark Brown wrote:
> 
> On Wed, Oct 30, 2013 at 06:36:21PM +0100, Takashi Iwai wrote:
> > Mark Brown wrote:
> > > On Wed, Oct 30, 2013 at 08:35:04AM +0100, Takashi Iwai wrote:
> 
> > > > +	if (channel < 0)
> > > > +		return channel;
> > > >  	BUG_ON(channel > 1);
> 
> > > This doesn't seem to fit in well with the adjacent code - the handling
> > > of out of bounds data isn't consistent though it looks like they're both
> > > doing essentially the same thing here.
> 
> > Actually BUG_ON(channel > 1) is a pretty stupid line and should be
> > removed.  You'll see that this never happens if you read the code.
> 
> Yeah, but then nor does a return of less than zero...

Well, max98095_get_eq_channel() returns 0, 1, or a negative error.
It's the interface definition.  So, as long as the function is written
in that way, checking a negative value is logical there, while the
condition "channel > 1" never happens, thus a pure garbage.

Of course, if you are paranoid enough and cannot trust the return
value from any function, you'd like to check it in allover places.
However, in that case, use of BUG_ON() for channel < 0 is still wrong,
because a negative value is a valid return value from the function.
If you handle it a fatal error (as BUG_ON() does), it means that the
design of the function itself is wrong.

> > Overall, this driver has way too many BUG_ON() without
> > considerations.  Such BUT_ON()'s are worse than nothing, IMO.
> 
> Yeah, I'm ambivalent.  I think they're not too bad if it's about a
> fairly small part of the driver agreeing with itself which is the case
> here.

The biggest problem of BUG_ON() is it immediately triggers panic().
BUG_ON() is useful only in the situation where the system can't do
anything better than stopping the whole operation at this point.  And
the only postmortem would be kdump, which is practically never
available for embedded devices.

In other words, BUG_ON() is the worst choice in almost all sound/*
codes.  If a sanity check is needed, use WARN_ON() and handle the
error.  (Or use snd_BUG_ON() that invokes WARN_ON() when
CONFIG_SND_DEBUG is set.  Why it's called snd_BUG_ON(), not
snd_WARN_ON()?  Don't ask me :)

And, if you still insist to use BUG_ON(), use it in the right place;
at the cause, not at the result.  In this channel check case, it'd be
in max98095_get_eq_channel(), put BUG_ON() instead of returning an
error.  Then all other BUG_ON()'s wrt channel can be omitted.


OK, go back from bikeshedding...


Takashi