Re: [RFC 1/3] clockevents: add ->set_dev_mode() to struct clock_event_device

Viresh Kumar viresh.kumar@linaro.org writes:

There is a requirement to add another mode: CLOCK_EVT_MODE_ONESHOT_STOPPED (lkml.org/lkml/2014/5/9/508) to clockevent devices and clockevent-drivers may or maynot support it. Drivers that don't support it can return failure codes on a call to ->set_mode(), which has a return type of 'void' currently.

Following steps are suggested (by tglx) to get this fixed:

1. Add another callback ->set_dev_mode(), with return type 'int'.
2. Covert clockevent drivers to use ->set_dev_mode() instead of ->set_mode().
3. Once all are converted, remove support for ->set_mode().

This patch implements 'part 1' of this approach. It also marks ->set_mode() deprecated as ->set_dev_mode() should be used instead.

In order to propagate error codes to callers of clockevents_set_mode(), change its return type to 'int'. For now just add a WARN_ON() inside clockevents_set_mode() and don't explicitly handle any error codes at call sites, as the current modes are not expected/allowed to fail.

Reviewed-by: Preeti U Murthy preeti@linux.vnet.ibm.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org

[...]

@@ -441,15 +452,23 @@ EXPORT_SYMBOL_GPL(clockevents_config_and_register); int __clockevents_update_freq(struct clock_event_device *dev, u32 freq) {

• int ret = 0;
• clockevents_config(dev, freq);

if (dev->mode == CLOCK_EVT_MODE_ONESHOT) return clockevents_program_event(dev, dev->next_event, false);

• if (dev->mode == CLOCK_EVT_MODE_PERIODIC)

• dev->set_mode(CLOCK_EVT_MODE_PERIODIC, dev);
  

• if (dev->mode == CLOCK_EVT_MODE_PERIODIC) {

• if (dev->set_dev_mode) {
  

• 	ret = dev->set_dev_mode(CLOCK_EVT_MODE_PERIODIC, dev);
  

• 	WARN_ON(ret);
  

• } else {
  

• 	dev->set_mode(CLOCK_EVT_MODE_PERIODIC, dev);
  

• }
  

• }

• return 0;

• return ret;

}

First, this change isn't mentioned in the changelog. But more importantly (as Daniel has mentioned), returning an error here has consequences.

Specifically, why would the caller of this function care that the specific clkevt mode was not implmented? As Thomas mentioned in his reviews, the clockevent code should handle the failures itself, not just pass them up and expect other layers to handle them.

Also, if PERIODIC mode can't fail today, why would we expect it to fail in the changed version?

Kevin