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

[Viresh Kumar]

Hi Thomas,

On 23 January 2015 at 17:27, Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> 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);

Because setting to all these existing modes isn't allowed
to fail currently, shouldn't we make return type of all the new
callbacks as 'void' and return 0 from this routine ? That way, these
callbacks would stay consistent with the set_mode() callback..

>
>         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.

Now that we are looking forward to providing individual callbacks
per feature, I wanted to propose few more solutions to it, though I am
quite sure you would have already considered and rejected them.

I remember from your earlier mail that you didn't wanted to add
ONESHOT_STOPPED as a feature (as it isn't a feature really)
and that restricts us from doing this:

static int __clockevents_set_mode(dev, mode)
{
        if ((mode == CLOCK_EVT_MODE_ONESHOT_STOPPED) &&
            !(dev->features & CLOCK_EVT_FEAT_ONESHOT_STOPPED))
                return -ENOSYS;

        dev->set_mode(mode, dev);
        return 0;
}

But what about providing a separate callback only for ONESHOT_STOPPED
mode and use set_mode() for everything else ?

static int __clockevents_set_mode(dev, mode)
{
        if (mode == CLOCK_EVT_MODE_ONESHOT_STOPPED) {
                if (dev->stop_onshot)
                        return dev->stop_onshot(dev);
                else
                        return -ENOSYS;
        }

        dev->set_mode(mode, dev);
        return 0;
}

Thanks for looking into this thread again..

--
viresh
--
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/