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

[Thomas Gleixner]

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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/