Re: [PATCH 1/2] clockevents: introduce ->set_dev_mode() which can return error

On Tue, 9 Dec 2014, Kevin Hilman wrote:

diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c index 55449909f114..f7614041240e 100644 --- a/kernel/time/clockevents.c +++ b/kernel/time/clockevents.c @@ -105,7 +105,16 @@ void clockevents_set_mode(struct clock_event_device *dev, enum clock_event_mode mode) { if (dev->mode != mode) {

• dev->set_mode(mode, dev);
  

• if (dev->set_dev_mode) {
  

• 	int ret = dev->set_dev_mode(mode, dev);
  

• 	/* Currently available modes shouldn't fail */
  

• 	WARN_ONCE(ret, "Requested mode: %d, error: %d\n",
  

• 		  mode, ret);
  

• } else {
  

• 	dev->set_mode(mode, dev);
  

• }
  

• dev->mode = mode;

/* @@ -448,8 +457,14 @@ int __clockevents_update_freq(struct clock_event_device *dev, u32 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) {

• /* Shouldn't fail while switching to PERIODIC MODE */
  

• if (dev->set_dev_mode)
  

• 	WARN_ON_ONCE(dev->set_dev_mode(CLOCK_EVT_MODE_PERIODIC,
  

• 				       dev));
  

• else
  

• 	dev->set_mode(CLOCK_EVT_MODE_PERIODIC, dev);
  

Please split that out into

static int __clockevents_set_mode(dev, mode) { int ret = 0;

if (dev->set_dev_mode) { ret = set_dev_mode(mode, dev); WARN_ON_ONCE(ret); } else { dev->set_mode(CLOCK_EVT_MODE_PERIODIC, dev); } return ret; }

Now looking at the conversion patch, it looks like we add lots of

case X: case Y: return 0; to avoid the

default: return -ENOSYS;

After staring at that I'm less convinced about this approach. So lets think about something different:

static int __clockevents_set_mode(dev, mode) { /* Transition mode */ if (dev->set_mode) { if (mode > CLOCK_EVT_MODE_RESUME) return -ENOSYS; dev->set_mode(mode, dev); return 0; }

if (dev->features & CLOCK_EVT_FEAT_DUMMY) return 0;

switch (mode) { /* * This is an internal state, which is guaranteed to * go from SHUTDOWN to UNUSED. No driver interaction * required. */ case CLOCK_EVT_MODE_UNUSED: return 0;

case CLOCK_EVT_MODE_SHUTDOWN: return dev->shutdown(dev);

case CLOCK_EVT_MODE_PERIODIC: /* Core internal bug */ if (!(dev->features & CLOCK_EVT_FEAT_PERIODIC)) return -ENOSYS; return dev->setup_periodic(dev);

case CLOCK_EVT_MODE_ONESHOT: /* Core internal bug */ if (!(dev->features & CLOCK_EVT_FEAT_ONESHOT)) return -ENOSYS; return dev->setup_oneshot(dev);

case CLOCK_EVT_MODE_RESUME: return dev->setup_resume(dev);

default: return -ENOSYS; }

And do sanity checking on registration time:

static int ce_check(dev) { if (dev->set_mode) return 0;

if (dev->features & CLOCK_EVT_FEAT_DUMMY) return 0;

if (!dev->shutdown) return -EMORON;

if ((dev->features & CLOCK_EVT_FEAT_PERIODIC) && !dev->setup_periodic) return -EMORON;

if ((dev->features & CLOCK_EVT_FEAT_ONESHOT) && !dev->setup_oneshot) return -EMORON;

/* Optional callbacks */ if (!dev->setup_resume) dev->setup_resume = setup_noop();

return 0; }

That gives us a clear distinction of required and optional callbacks, which will make error handling and recovery simpler as well.

Thanks,

tglx