On 05/26/2014 12:34 PM, Viresh Kumar wrote:
On 26 May 2014 12:21, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
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.
Hmmm... Continuing here doesn't make any sense, when we know the mode hasn't changed at all.
Maybe we can leave it as is and see what tglx thinks ?
I would suggest add a WARN_ON() on failure and fall through.
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?
Actually can be removed but is there just for protection that we aren't failing. It is very much similar to the WARN_ON in above routine. If we buy your argument, then we don't even need a WARN() in clocevents_set_mode(), we can just check for failure in some special case and return error. Why a WARN ? And so I added it here as well..
In the first case of clockevents_set_mode() we are unsure of the mode we are setting. There can be more modes added, going forward and we cannot special case each for a failed return. In the second case of update_freq() you *know* you are setting PERIODIC_MODE which is rest assured will not fail.
Regards Preeti U Murthy