[PATCH v3) posix-timers: make it configurable

Message ID alpine.LFD.2.20.1609151523510.14769@knanqh.ubzr
State New
Headers show

Commit Message

Nicolas Pitre Sept. 15, 2016, 7:31 p.m.
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?




> 

> thanks

> -john

>

Comments

John Stultz Sept. 15, 2016, 7:58 p.m. | #1
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
Nicolas Pitre Sept. 15, 2016, 9:35 p.m. | #2
On Thu, 15 Sep 2016, Josh Triplett wrote:

> On Thu, Sep 15, 2016 at 11:07:24PM +0200, Richard Cochran wrote:

> > On Thu, Sep 15, 2016 at 12:58:22PM -0700, John Stultz wrote:

> > > This doesn't look too bad.

> > 

> > I disagree.  It looks ugly.  If tinification means sprinkling more and

> > more of these conditionals all over the place, then it is going to be

> > a tough sell.

> 

> Looking at this particular patch, it does seem a bit much for the

> ability to have PTP without timers.  That doesn't seem like a very

> likely combination.  Handling that in Kconfig seems fine, unless there's

> a concrete use case for that combination.


I doubt there is.  This is more for randconfig purposes or the like.

I suspect there is more of a case for having net drivers _without_ ptp 
support.  This could be implemented with a ptp_clock_register() stub 
returning NULL when ptp is not configured.  I didn't look at most 
drivers but at least broadcom/tg3.c seems to be fine with such an 
approach.

Alternatively, all those ethernet drivers currently selecting 
PTP_1588_CLOCK could be banned from the kernel config when POSIX_TIMERS 
is not selected.

What do people prefer?


Nicolas

Patch hide | download patch | download mbox

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);