| Message ID | alpine.LFD.2.20.1609151523510.14769@knanqh.ubzr |
|---|---|
| State | New |
| Headers | show |
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
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
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);