linux-kernel - Re: [PATCH 1/2] clockevents: introduce ->set_dev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 25 Jan 2015 19:27:57 +0530
From:	Viresh Kumar <viresh.kumar@...aro.org>
To:	Thomas Gleixner <tglx@...utronix.de>
Cc:	Kevin Hilman <khilman@...nel.org>,
	Linaro Kernel Mailman List <linaro-kernel@...ts.linaro.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Preeti U Murthy <preeti@...ux.vnet.ibm.com>
Subject: Re: [PATCH 1/2] clockevents: introduce ->set_dev_mode() which can
 return error

Hi Thomas,

On 23 January 2015 at 17:27, Thomas Gleixner <tglx@...utronix.de> 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/