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;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