Re: [PATCH v3) posix-timers: make it configurable

[John Stultz <john.stultz@linaro.org>]

On Thu, Sep 15, 2016 at 12:31 PM, Nicolas Pitre
<nicolas.pitre@linaro.org> wrote:
> On Thu, 15 Sep 2016, John Stultz wrote:
>
>> On Thu, Sep 15, 2016 at 11:37 AM, Nicolas Pitre
>> <nicolas.pitre@linaro.org> wrote:
>> > On Thu, 15 Sep 2016, John Stultz wrote:
>> >
>> >> On Thu, Sep 15, 2016 at 11:28 AM, Nicolas Pitre
>> >> <nicolas.pitre@linaro.org> wrote:
>> >> > On Thu, 15 Sep 2016, John Stultz wrote:
>> >> >
>> >> >> > diff --git a/kernel/time/Kconfig b/kernel/time/Kconfig
>> >> >> > index 62824f2fe4..62504a2c9f 100644
>> >> >> > --- a/kernel/time/Kconfig
>> >> >> > +++ b/kernel/time/Kconfig
>> >> >> > @@ -195,3 +195,21 @@ config HIGH_RES_TIMERS
>> >> >> >
>> >> >> >  endmenu
>> >> >> >  endif
>> >> >> > +
>> >> >> > +config POSIX_TIMERS
>> >> >> > +       bool "Posix Clocks & timers" if EMBEDDED
>> >> >> > +       default y
>> >> >> > +       help
>> >> >> > +         This includes native support for POSIX timers to the kernel.
>> >> >> > +         Most embedded systems may have no use for them and therefore they
>> >> >> > +         can be configured out to reduce the size of the kernel image.
>> >> >> > +
>> >> >> > +         When this option is disabled, the following syscalls won't be
>> >> >> > +         available: timer_create, timer_gettime: timer_getoverrun,
>> >> >> > +         timer_settime, timer_delete, clock_adjtime. Furthermore, the
>> >> >> > +         clock_settime, clock_gettime, clock_getres and clock_nanosleep
>> >> >> > +         syscalls will be limited to CLOCK_REALTIME and CLOCK_MONOTONIC
>> >> >> > +         only.
>> >> >> > +
>> >> >> > +         If unsure say y.
>> >> >> >
>> >> >>
>> >> >> One thought.. Should this go under:
>> >> >>     Configure standard kernel features (expert users)
>> >> >> rather then a top level item under  General Setup ?
>> >> >
>> >> > Hmmm... probably yes.
>> >> >
>> >> > Do you need that I repost the patch?
>> >>
>> >> I can see about moving it..
>> >
>> > OK.
>> >
>> > Making it conditional on EXPERT rather than EMBEDDED would also be more
>> > inline with the other similar options there.
>>
>> Ack.
>>
>> Although I'm also seeing some Kconfig noise when I disable it:
>>
>> warning: (SFC && TILE_NET && AMD_XGBE && BFIN_MAC_USE_HWSTAMP &&
>> TIGON3 && BNX2X && LIQUIDIO && FEC && E1000E && IGB && IXGBE && I40E
>> && FM10K && MLX4_EN && MLX5_CORE_EN && RAVB && SXGBE_ETH && STMMAC_ETH
>> && TI_CPTS && PTP_1588_CLOCK_GIANFAR && PTP_1588_CLOCK_IXP46X &&
>> DP83640_PHY && PTP_1588_CLOCK_PCH) selects PTP_1588_CLOCK which has
>> unmet direct dependencies (NET && POSIX_TIMERS)
>> warning: (SFC && TILE_NET && AMD_XGBE && BFIN_MAC_USE_HWSTAMP &&
>> TIGON3 && BNX2X && LIQUIDIO && FEC && E1000E && IGB && IXGBE && I40E
>> && FM10K && MLX4_EN && MLX5_CORE_EN && RAVB && SXGBE_ETH && STMMAC_ETH
>> && TI_CPTS && PTP_1588_CLOCK_GIANFAR && PTP_1588_CLOCK_IXP46X &&
>> DP83640_PHY && PTP_1588_CLOCK_PCH) selects PTP_1588_CLOCK which has
>> unmet direct dependencies (NET && POSIX_TIMERS)
>>
>> Not sure if this is just expected given we can't do reverse dependency
>> checking on select...
>>
>> Maybe PTP_1588_CLOCK needs to select POSIX_TIMERS instead of just depend on it?
>
> That would forcefully pull in a whole bunch of code that one wanted out
> by explicitly not enabling CONFIG_POSIX_TIMERS.  And the current depends
> statement forces out a bunch of ethernet drivers.
>
> Maybe there ought to be fewer of those hard dependencies such as net
> drivers with ptp, and ptp with POSIX timers.
>
> What about the following patch that would take care of the later?
>
> diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
> index 00e6098e9a..ee3de3421f 100644
> --- a/drivers/ptp/Kconfig
> +++ b/drivers/ptp/Kconfig
> @@ -6,7 +6,7 @@ menu "PTP clock support"
>
>  config PTP_1588_CLOCK
>         tristate "PTP clock support"
> -       depends on NET && POSIX_TIMERS
> +       depends on NET
>         select PPS
>         select NET_PTP_CLASSIFY
>         help
> diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
> index 2e481b9e8e..873addff63 100644
> --- a/drivers/ptp/ptp_clock.c
> +++ b/drivers/ptp/ptp_clock.c
> @@ -208,7 +208,8 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
>                 goto no_slot;
>         }
>
> -       ptp->clock.ops = ptp_clock_ops;
> +       if (IS_ENABLED(CONFIG_POSIX_TIMERS))
> +               ptp->clock.ops = ptp_clock_ops;
>         ptp->clock.release = delete_ptp_clock;
>         ptp->info = info;
>         ptp->devid = MKDEV(major, index);
> @@ -245,10 +246,12 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
>         }
>
>         /* Create a posix clock. */
> -       err = posix_clock_register(&ptp->clock, ptp->devid);
> -       if (err) {
> -               pr_err("failed to create posix clock\n");
> -               goto no_clock;
> +       if (IS_ENABLED(CONFIG_POSIX_TIMERS)) {
> +               err = posix_clock_register(&ptp->clock, ptp->devid);
> +               if (err) {
> +                       pr_err("failed to create posix clock\n");
> +                       goto no_clock;
> +               }
>         }
>
>         return ptp;
> @@ -281,7 +284,8 @@ int ptp_clock_unregister(struct ptp_clock *ptp)
>         ptp_cleanup_sysfs(ptp);
>         device_destroy(ptp_class, ptp->devid);
>
> -       posix_clock_unregister(&ptp->clock);
> +       if (IS_ENABLED(CONFIG_POSIX_TIMERS))
> +               posix_clock_unregister(&ptp->clock);
>         return 0;
>  }
>  EXPORT_SYMBOL(ptp_clock_unregister);

This doesn't look too bad.

Richard: Your thoughts?

thanks
-john