Re: [PATCH] clockevents: Introduce mode specific callbacks

On 10 February 2015 at 22:15, Peter Zijlstra peterz@infradead.org wrote:

On Wed, Feb 04, 2015 at 01:06:23PM +0530, Viresh Kumar wrote:

...

• /*
  

•  * Mode transition callback(s): Only one of the two groups should be
  

•  * defined:
  

•  * - set_mode(), only for modes <= CLOCK_EVT_MODE_RESUME.
  

•  * - set_mode_{shutdown|periodic|oneshot|resume}().
  

•  */
  void                    (*set_mode)(enum clock_event_mode mode,
                                      struct clock_event_device *);
  

• int                     (*set_mode_periodic)(struct clock_event_device *);
  

• int                     (*set_mode_oneshot)(struct clock_event_device *);
  

• int                     (*set_mode_shutdown)(struct clock_event_device *);
  

• int                     (*set_mode_resume)(struct clock_event_device *);
  

...

+/* Sanity check of mode transition callbacks */ +static int clockevents_sanity_check(struct clock_event_device *dev) +{

• /* Legacy set_mode() callback */
  

• if (dev->set_mode)
  

•         return 0;
  

• if (dev->features & CLOCK_EVT_FEAT_DUMMY)
  

•         return 0;
  

• /* New mode-specific callbacks */
  

• if (!dev->set_mode_shutdown)
  

•         return -EINVAL;
  

• if ((dev->features & CLOCK_EVT_FEAT_PERIODIC) &&
  

•     !dev->set_mode_periodic)
  

•         return -EINVAL;
  

• if ((dev->features & CLOCK_EVT_FEAT_ONESHOT) &&
  

•     !dev->set_mode_oneshot)
  

•         return -EINVAL;
  

• return 0;
  

+}

It appears to me you've not actually checked that condition outlined above, a driver could set both the legacy and the new callbacks.

Exactly for this reason I mentioned this in the logs:

...

If the legacy ->set_mode() callback is provided, all mode specific callbacks would be ignored.

So, either we can mention that in the code as well OR add code to check and WARN about that. Will do whatever looks better to you guys.

-- viresh