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;BUG_ON(!mem); region_name = "DM"; reg = wm_adsp_region_to_reg(mem, offset); break;BUG_ON(!mem); region_name = "XM"; reg = wm_adsp_region_to_reg(mem, offset); break;BUG_ON(!mem); region_name = "YM"; reg = wm_adsp_region_to_reg(mem, offset); break;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