Kevin Hilman had written, on 03/19/2010 03:49 PM, the following:
Nishanth Menon <nm@xxxxxx> writes:
Kevin Hilman had written, on 03/19/2010 01:42 PM, the following:
Felipe Balbi <me@xxxxxxxxxxxxxxx> writes:
On Fri, Mar 19, 2010 at 10:46:54AM -0700, Kevin Hilman wrote:
IMO, Using BUG* macros usually indicates improper or incomplete error
handling rather than a real catastrophic system failure.
on the other hand a kernel oops and system hang will always get
noted. Rather than a WARN() which simply sits in the log buffer.
Of course, but what I'm trying to avoid is making other people deal
with a BUG inserted by a developer when proper error checking and
recovery is what is really needed.
I respect your views. but a few moments of thoughts:
how would the recovery look like? I can think of 2 options here.. do
share your views:
Option 1:
if (opp_init_list(OPP_MPU, omap3_opp_def_list[0])) {
WARN("dsp OPP table registration failed");
return;
}
if (opp_init_list(OPP_L3, omap3_opp_def_list[1])) {
WARN("dsp OPP table registration failed");
return;
}
if (opp_init_list(OPP_DSP, omap3_opp_def_list[2])) {
WARN("dsp OPP table registration failed");
return;
}
Option 2:
if (opp_init_list(OPP_MPU, omap3_opp_def_list[0]))
return;
if (opp_init_list(OPP_L3, omap3_opp_def_list[1]))
goto mpu_disable;
if (opp_init_list(OPP_DSP, omap3_opp_def_list[2]))
goto l3_disable;
return;
l3_disable:
freq = 0;
while (!IS_ERR(opp = opp_find_freq_ceil(OPP_L3, &freq)) {
opp_disable(opp);
freq++;
}
mpu_disable:
freq = 0;
while (!IS_ERR(opp = opp_find_freq_ceil(OPP_MPU, &freq)) {
opp_disable(opp);
freq++;
}
WARN("Registration of OPP tables failed!!");
return;
Option 1 is a bad idea as it leaves the system in an invalid state
Option 2 is the better idea as we dont have a opp_delete option(not
required usually).
I'm OK with either actually, as either is better than BUG_ON()
instead of error checking.
With option 1, the system is not really in an invalid state, just and
untested state. What will happen is users of the OPP API will error
codes when trying to get OPPs and they should fail gracefully as well.
except if L3/DSP reg fails, you allow MPU freqs configured by no DSP or
L3 freq.. prefer the option 2 in that respect.. it is binary - you get
all the configurations done right OR you dont.
Once again, this is about proper error checking, and robustness, not
about causing a panic() when something (relatively) minor happens.
All that code for something that will almost never happen?
Yes. That's what error checking is all about.
Kevin
P.S. I can't find the ref atm, but I mentioned not liking the BUG_ON
early in the review of the OPP layer, and it would need to be
cleaned up before upstream merge.
alrite alrite.. i am black and blue with the beatings ;). I am going
with option 2 if there are no other objections.. will send out the patch
after collating other comments.
--
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
