On 05/26/2014 12:08 PM, Viresh Kumar wrote:
On 26 May 2014 11:57, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
Ok in that case you must be returning an error in clockevent_set_mode() right? You currently just return. clockevent_set_mode() should be made to return an integer type to signify that setting clock modes will fail. Handling of these errors at the call site can be done in the next patchset when ONESHOT_STOPPED mode is introduced.
I had that earlier in my patches earlier and it looked irrelevant there and I decided to move that to the ONESHOT_STOPPED patchset. As returning an error wouldn't be useful with this patchset alone (Callers aren't handling it yet)...
In this case, just have a WARN_ON() and let the code fall through for now. The reason I am pushing on this front is when we look at the code, it should make sense one way or the other.
Either return the error code signifying that the call sites have to take care of this or don't return at this point. The early 'return' there is not of any significance. You may say that you will eventually replace that 'return' with 'return error'. But the code is not conveying this.
The same goes for clockevents_update_freq(). On warning, we need to return an error code. This will ensure that this patchset is complete in itself. As of now since none of the modes are expected to fail, the call sites needn't handle the return value of clockevents_set_mode() and clockevents_update_freq().
In update_freq() we must never fail as all we are doing is setting a valid mode. So, we don't need to return an error even (to make it complex for
Then why have a WARN_ON() here at all?
Regards Preeti U Murthy
call sites..)
I wasn't sure if WARN_ON() is the right thing to do here or we should rather have a BUG_ON() :) ..