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