some patches for the ptp framework

some patches for the ptp framework

[Torben Hohn]

patches 1 and 3 apply on top of tip:core/timer

patch 2 also requires Richards ptp v12 1/4 for the ptp class.
modulo a small conflict in drivers/ptp/Makefile and Kconfig.
because i had added his ixp driver, which the software clock is based on.

Richards Patchset didnt apply to tip:core/timer as is.
which branch is it based on ?

[PATCH 1/3] remove __cpuinit from mwait_usable()

[Torben Hohn]

mwait_usable() is used by native_play_dead()
which is not __cpuinit.

Signed-off-by: Torben Hohn <torbenh@gmx.de>
Cc: Richard Cochran <richard.cochran@omicron.at>
Cc: John Stultz <johnstul@us.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/include/asm/cpu.h |    2 +-
 arch/x86/kernel/process.c  |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h
index 6e6e755..4564c8e 100644
--- a/arch/x86/include/asm/cpu.h
+++ b/arch/x86/include/asm/cpu.h
@@ -32,6 +32,6 @@ extern void arch_unregister_cpu(int);
 
 DECLARE_PER_CPU(int, cpu_state);
 
-int __cpuinit mwait_usable(const struct cpuinfo_x86 *);
+int mwait_usable(const struct cpuinfo_x86 *);
 
 #endif /* _ASM_X86_CPU_H */
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index e764fc0..3c189e9 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -506,7 +506,7 @@ static void poll_idle(void)
 #define MWAIT_ECX_EXTENDED_INFO		0x01
 #define MWAIT_EDX_C1			0xf0
 
-int __cpuinit mwait_usable(const struct cpuinfo_x86 *c)
+int mwait_usable(const struct cpuinfo_x86 *c)
 {
 	u32 eax, ebx, ecx, edx;
 
-- 
1.7.2.3

[PATCH 2/3] ptp: add a software clock based on clock_monotonic_raw

[Torben Hohn]

First version of a software clock. Not very useful yet,
as it doesnt generate events, but at least it allows
for testing the ptp framework without special hardware.

Signed-off-by: Torben Hohn <torbenh@gmx.de>
Cc: Richard Cochran <richard.cochran@omicron.at>
Cc: John Stultz <johnstul@us.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/ptp/Kconfig        |    6 +
 drivers/ptp/Makefile       |    1 +
 drivers/ptp/ptp_software.c |  221 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 228 insertions(+), 0 deletions(-)
 create mode 100644 drivers/ptp/ptp_software.c

diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
index d99e6f9..0427361 100644
--- a/drivers/ptp/Kconfig
+++ b/drivers/ptp/Kconfig
@@ -40,4 +40,10 @@ config PTP_1588_CLOCK_IXP46X
 	  To compile this driver as a module, choose M here: the module
 	  will be called ptp_ixp46x.
 
+config PTP_1588_CLOCK_SOFTWARE
+	tristate "software PTP clock"
+	depends on PTP_1588_CLOCK
+	help
+	  This driver adds a software PTP clock. 
+
 endmenu
diff --git a/drivers/ptp/Makefile b/drivers/ptp/Makefile
index f6933e8..8f18a55 100644
--- a/drivers/ptp/Makefile
+++ b/drivers/ptp/Makefile
@@ -5,3 +5,4 @@
 ptp-y					:= ptp_clock.o ptp_chardev.o ptp_sysfs.o
 obj-$(CONFIG_PTP_1588_CLOCK)		+= ptp.o
 obj-$(CONFIG_PTP_1588_CLOCK_IXP46X)	+= ptp_ixp46x.o
+obj-$(CONFIG_PTP_1588_CLOCK_SOFTWARE)	+= ptp_software.o
diff --git a/drivers/ptp/ptp_software.c b/drivers/ptp/ptp_software.c
new file mode 100644
index 0000000..1182426
--- /dev/null
+++ b/drivers/ptp/ptp_software.c
@@ -0,0 +1,221 @@
+/*
+ * Software PTP Clock
+ *
+ * Copyright (C) 2010 OMICRON electronics GmbH
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+#include <linux/ptp_clock_kernel.h>
+
+#define DRIVER		"ptp_sofware"
+#define N_EXT_TS	2
+
+struct ixp_clock {
+	struct ptp_clock *ptp_clock;
+	struct ptp_clock_info caps;
+	s64 offset_ns;
+	struct timespec last_timestamp;
+	s32 freq;
+};
+
+DEFINE_SEQLOCK(clock_lock);
+
+
+/*
+ * Register access functions
+ */
+
+static struct timespec timespec_mul_freq(struct timespec val, u32 ppb)
+{
+	struct timespec retval;
+	u32 remain_sec;
+	u64 muled_sec  = (u64) val.tv_sec * ppb;
+	u64 muled_nsec = (u64) val.tv_nsec * ppb;
+
+	muled_sec  = div_u64_rem(muled_sec, NSEC_PER_SEC, &remain_sec);
+	muled_nsec = div_u64(muled_nsec, NSEC_PER_SEC);
+
+	set_normalized_timespec(&retval, muled_sec, muled_nsec+remain_sec); 
+	return retval;
+}
+
+static struct timespec __transformed_time(struct timespec val, struct ixp_clock *clock)
+{
+	struct timespec diff, muled, retval;
+	int neg_adj = 0;
+	s32 ppb;
+
+	if (clock->freq < 0) {
+		neg_adj = 1;
+		ppb = -clock->freq;
+	} else {
+		ppb = clock->freq;
+	}
+
+	diff = timespec_sub(val, clock->last_timestamp);
+	muled = timespec_mul_freq(diff, (u32) ppb);
+
+	if (neg_adj) {
+		retval = timespec_sub(val, muled);
+	} else {
+		retval = timespec_add(val, muled);
+	}
+
+	if (clock->offset_ns < 0)
+		retval = timespec_sub(retval, ns_to_timespec(-clock->offset_ns));
+	else
+		retval = timespec_add(retval, ns_to_timespec(clock->offset_ns));
+
+	return retval;
+}
+
+/*
+ * PTP clock operations
+ */
+
+static int ptp_ixp_adjfreq(struct ptp_clock_info *ptp, s32 ppb)
+{
+	struct timespec now_kernel, now_this;
+	struct ixp_clock *ixp_clock = container_of(ptp, struct ixp_clock, caps);
+
+	getrawmonotonic(&now_kernel);
+
+	write_seqlock(&clock_lock);
+
+	now_this = __transformed_time(now_kernel, ixp_clock);
+
+	ixp_clock->offset_ns = timespec_to_ns(&now_this) - timespec_to_ns(&now_kernel);
+	ixp_clock->last_timestamp = now_kernel;
+	ixp_clock->freq = ppb;
+
+	write_sequnlock(&clock_lock);
+
+	return 0;
+}
+
+static int ptp_ixp_adjtime(struct ptp_clock_info *ptp, s64 delta)
+{
+	struct ixp_clock *ixp_clock = container_of(ptp, struct ixp_clock, caps);
+
+	write_seqlock(&clock_lock);
+	ixp_clock->offset_ns += delta;
+	write_sequnlock(&clock_lock);
+
+	return 0;
+}
+
+static int ptp_ixp_gettime(struct ptp_clock_info *ptp, struct timespec *ts)
+{
+	unsigned long seq;
+	struct timespec now_kernel;
+	struct ixp_clock *ixp_clock = container_of(ptp, struct ixp_clock, caps);
+
+	getrawmonotonic(&now_kernel);
+	
+	do {
+		seq = read_seqbegin(&clock_lock);
+
+		*ts = __transformed_time(now_kernel, ixp_clock);
+	} while (read_seqretry(&clock_lock, seq));
+
+	return 0;
+}
+
+static int ptp_ixp_settime(struct ptp_clock_info *ptp,
+			   const struct timespec *ts)
+{
+	struct ixp_clock *ixp_clock = container_of(ptp, struct ixp_clock, caps);
+
+	struct timespec now_kernel;
+
+	getrawmonotonic(&now_kernel);
+
+	write_seqlock(&clock_lock);
+	ixp_clock->last_timestamp = now_kernel;
+	ixp_clock->offset_ns = timespec_to_ns(ts) - timespec_to_ns(&now_kernel);
+	write_sequnlock(&clock_lock);
+
+	return 0;
+}
+
+static int ptp_ixp_enable(struct ptp_clock_info *ptp,
+			  struct ptp_clock_request *rq, int on)
+{
+	//struct ixp_clock *ixp_clock = container_of(ptp, struct ixp_clock, caps);
+
+	switch (rq->type) {
+	case PTP_CLK_REQ_EXTTS:
+		switch (rq->extts.index) {
+		default:
+			return -EINVAL;
+		}
+		return 0;
+	default:
+		break;
+	}
+
+	return -EOPNOTSUPP;
+}
+
+static struct ptp_clock_info ptp_ixp_caps = {
+	.owner		= THIS_MODULE,
+	.name		= "software timer",
+	.max_adj	= 66666655,
+	.n_ext_ts	= 0,
+	.pps		= 0,
+	.adjfreq	= ptp_ixp_adjfreq,
+	.adjtime	= ptp_ixp_adjtime,
+	.gettime	= ptp_ixp_gettime,
+	.settime	= ptp_ixp_settime,
+	.enable		= ptp_ixp_enable,
+};
+
+/* module operations */
+
+static struct ixp_clock ixp_clock;
+
+static void __exit ptp_ixp_exit(void)
+{
+	ptp_clock_unregister(ixp_clock.ptp_clock);
+}
+
+static int __init ptp_ixp_init(void)
+{
+	ixp_clock.caps = ptp_ixp_caps;
+
+	ixp_clock.ptp_clock = ptp_clock_register(&ixp_clock.caps);
+
+	if (IS_ERR(ixp_clock.ptp_clock))
+		return PTR_ERR(ixp_clock.ptp_clock);
+
+	return 0;
+}
+
+module_init(ptp_ixp_init);
+module_exit(ptp_ixp_exit);
+
+MODULE_AUTHOR("Richard Cochran <richard.cochran@omicron.at>");
+MODULE_DESCRIPTION("PTP clock using the IXP46X timer");
+MODULE_LICENSE("GPL");
-- 
1.7.2.3

Re: [PATCH 2/3] ptp: add a software clock based on clock_monotonic_raw

[john stultz]

On Thu, 2011-03-03 at 18:26 +0100, Torben Hohn wrote:
> First version of a software clock. Not very useful yet,
> as it doesnt generate events, but at least it allows
> for testing the ptp framework without special hardware.

So in the past, I pushed back on Richard for having something similar,
as I'm not psyched about duplicating interfaces. However, your
implementation is CLOCK_MONOTONIC_RAW instead of CLOCK_REALTIME and it
keeps the adjustments internal, I'm not as opposed.

So I think having such test driver is probably reasonable, but please
make it more explicit that it is just a testing driver, and not
something someone should try to use.

> +
> +struct ixp_clock {
> +	struct ptp_clock *ptp_clock;
> +	struct ptp_clock_info caps;
> +	s64 offset_ns;
> +	struct timespec last_timestamp;
> +	s32 freq;
> +};

Probably should rename things from ixp_*


thanks
-john

Re: [PATCH 2/3] ptp: add a software clock based on clock_monotonic_raw

[Richard Cochran]

On Thu, Mar 03, 2011 at 11:34:04AM -0800, john stultz wrote:
> On Thu, 2011-03-03 at 18:26 +0100, Torben Hohn wrote:
> > First version of a software clock. Not very useful yet,
> > as it doesnt generate events, but at least it allows
> > for testing the ptp framework without special hardware.
> 
> So in the past, I pushed back on Richard for having something similar,
> as I'm not psyched about duplicating interfaces. However, your
> implementation is CLOCK_MONOTONIC_RAW instead of CLOCK_REALTIME and it
> keeps the adjustments internal, I'm not as opposed.

(Aha! So it is okay after all ;)

I do think a software only PHC will make it easier for people to get
started with the new interface. I have been carrying along such a
patch privately for my own testing.

Thanks,

Richard

Re: [PATCH 2/3] ptp: add a software clock based on clock_monotonic_raw

[torbenh]

On Fri, Mar 04, 2011 at 07:46:39AM +0100, Richard Cochran wrote:
> On Thu, Mar 03, 2011 at 11:34:04AM -0800, john stultz wrote:
> > On Thu, 2011-03-03 at 18:26 +0100, Torben Hohn wrote:
> > > First version of a software clock. Not very useful yet,
> > > as it doesnt generate events, but at least it allows
> > > for testing the ptp framework without special hardware.
> > 
> > So in the past, I pushed back on Richard for having something similar,
> > as I'm not psyched about duplicating interfaces. However, your
> > implementation is CLOCK_MONOTONIC_RAW instead of CLOCK_REALTIME and it
> > keeps the adjustments internal, I'm not as opposed.
> 
> (Aha! So it is okay after all ;)
> 
> I do think a software only PHC will make it easier for people to get
> started with the new interface. I have been carrying along such a
> patch privately for my own testing.

can you post your patch then ?
if john likes somthing better in my implementation, we should merge
them.

i will be away for the next week, so its probably up to you.

i am still trying to find out, whether 802.1AS will spec accurate ptp
clocks. i have the feeling that they wont.
the main concern is synchronicity of the clock inside all devices of the
timing domain. that this clock i running at the same frequency as the
wallclock is not important.

here is a pretty confused mail about this:
http://article.gmane.org/gmane.comp.audio.jackit/23606

i hope this thread will yield more useful information as it develops.

i am currently acting under the assumption, that a device like this:
http://www.xmos.com/products/reference-designs/avbl2
would need to be the ptp master when the computer doesnt have h/w
timestamping in its NIC.
i am also assuming that the clock in that device would be crappy.

so we wouldnt want to servo CLOCK_REALTIME after that crappy clock.
but we want an instance of that clock as accurate as possible inside 
the computer. so that we can bind streaming apps to this clock.

-- 
torben Hohn

Re: [PATCH 2/3] ptp: add a software clock based on clock_monotonic_raw

[john stultz]

On Fri, 2011-03-04 at 07:46 +0100, Richard Cochran wrote:
> On Thu, Mar 03, 2011 at 11:34:04AM -0800, john stultz wrote:
> > On Thu, 2011-03-03 at 18:26 +0100, Torben Hohn wrote:
> > > First version of a software clock. Not very useful yet,
> > > as it doesnt generate events, but at least it allows
> > > for testing the ptp framework without special hardware.
> > 
> > So in the past, I pushed back on Richard for having something similar,
> > as I'm not psyched about duplicating interfaces. However, your
> > implementation is CLOCK_MONOTONIC_RAW instead of CLOCK_REALTIME and it
> > keeps the adjustments internal, I'm not as opposed.
> 
> (Aha! So it is okay after all ;)
> 
> I do think a software only PHC will make it easier for people to get
> started with the new interface. I have been carrying along such a
> patch privately for my own testing.

Consistency is but a hobgoblin... :)

But seriously, do forgive me if I misunderstood your earlier patches
with this respect. I thought it was just exporting CLOCK_REALTIME
directly through a new interface/clockid (which would be duplicative).

thanks
-john

Re: [PATCH 2/3] ptp: add a software clock based on clock_monotonic_raw

[Richard Cochran]

On Sat, Mar 05, 2011 at 12:08:07PM -0800, john stultz wrote:
> On Fri, 2011-03-04 at 07:46 +0100, Richard Cochran wrote:
> > I do think a software only PHC will make it easier for people to get
> > started with the new interface. I have been carrying along such a
> > patch privately for my own testing.
> 
> Consistency is but a hobgoblin... :)
> 
> But seriously, do forgive me if I misunderstood your earlier patches
> with this respect. I thought it was just exporting CLOCK_REALTIME
> directly through a new interface/clockid (which would be duplicative).

You are right, I was doing that, but the purpose was for testing.

I am not sure where Torben is going with his idea. In order to figure
out meaningful clock adjustments, you need the clock's timestamps on
the PTP packet tx/rx events. Implementing an internal clock correction
with no connection to the network stack might still be interesting,
but I think having the timestamps is much more useful.

Let's wait and see what Torben says once he come back from travel.

Thanks,

Richard

Re: [PATCH 2/3] ptp: add a software clock based on clock_monotonic_raw

[torbenh]

On Sun, Mar 06, 2011 at 02:28:36PM +0100, Richard Cochran wrote:
> On Sat, Mar 05, 2011 at 12:08:07PM -0800, john stultz wrote:
> > On Fri, 2011-03-04 at 07:46 +0100, Richard Cochran wrote:
> > > I do think a software only PHC will make it easier for people to get
> > > started with the new interface. I have been carrying along such a
> > > patch privately for my own testing.
> > 
> > Consistency is but a hobgoblin... :)
> > 
> > But seriously, do forgive me if I misunderstood your earlier patches
> > with this respect. I thought it was just exporting CLOCK_REALTIME
> > directly through a new interface/clockid (which would be duplicative).
> 
> You are right, I was doing that, but the purpose was for testing.
> 
> I am not sure where Torben is going with his idea. In order to figure
> out meaningful clock adjustments, you need the clock's timestamps on
> the PTP packet tx/rx events. Implementing an internal clock correction
> with no connection to the network stack might still be interesting,
> but I think having the timestamps is much more useful.

I am planning to enable the networking stack to timestamp using
arbitrary clock id. 
But that means adding an ioctl. So there is going to be a lot of
critical review, and questions asked. 

On the audio lists, we have a guy now claiming that software ptp would
confuse the whole h/w ptp infrastructure. His main agenda seems to be to
prevent a software only ptp/avb implementatation.
I cant imagine how a slave clock can confuse a master clock.
And emission of audio stream packets does not need to be accurate to the
nanosecond.

I am a bit confused now, because i initially thought, i could quote that
guy to prove that an avb master clock might not necessarily be suitable
to become CLOCK_REALTIME. 

I am still assuming this. But i dont really have proof.
This might block this.

> 
> Let's wait and see what Torben says once he come back from travel.
> 
> Thanks,
> 
> Richard

-- 
torben Hohn

RE: [PATCH 2/3] ptp: add a software clock based on clock_monotonic_raw

[Christian Riesch]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 482 bytes --]

Torben,

On Friday, March 11, 2011 12:13, torbenh wrote:
>On the audio lists, we have a guy now claiming that software ptp would
>confuse the whole h/w ptp infrastructure. His main agenda seems to be to
>prevent a software only ptp/avb implementatation.

could you please post a link to that discussion?
Thanks,
Christian
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 2/3] ptp: add a software clock based on clock_monotonic_raw

[torbenh]

On Fri, Mar 11, 2011 at 12:37:57PM +0100, Christian Riesch wrote:
> Torben,
> 
> On Friday, March 11, 2011 12:13, torbenh wrote:
> >On the audio lists, we have a guy now claiming that software ptp would
> >confuse the whole h/w ptp infrastructure. His main agenda seems to be to
> >prevent a software only ptp/avb implementatation.
> 
> could you please post a link to that discussion?

duncan started somewhat like this:
http://article.gmane.org/gmane.comp.audio.jackit/23606
http://article.gmane.org/gmane.comp.audio.jackit/23612

then it seems that he somehow turned his head:
http://www.mail-archive.com/linux-audio-dev@lists.linuxaudio.org/msg11765.html

maybe we have a communication problem, with the definition of "play"
i only want to be able to generate an RTP stream on my computer.

the actual conversion from packets to analog signal would be happening on a
networked soundcard. the goal is to achieve low latency in the range of
2-3ms ...

maybe he interprets playing the rtp stream, as converting the rtp
packets to analog audio.
this obviously wouldnt work with software ptp, because we would need
nanoseconds accuracy of the clock.

but to just create an rtp stream, where a 2ms buffer doesnt underflow,
just requires a clock accuracy of 500us.

all in all this discussion might be a bit confusing.


> Thanks,
> Christian

-- 
torben Hohn

Re: [PATCH 2/3] ptp: add a software clock based on clock_monotonic_raw

[Christian Riesch]

Torben,

On Fri, Mar 11, 2011 at 12:13 PM, torbenh <torbenh@gmx.de> wrote:
> On the audio lists, we have a guy now claiming that software ptp would
> confuse the whole h/w ptp infrastructure. His main agenda seems to be to
> prevent a software only ptp/avb implementatation.

could you please post a link to that discussion?
Thanks,
Christian

Re: [PATCH 2/3] ptp: add a software clock based on clock_monotonic_raw

[Richard Cochran]

On Thu, Mar 03, 2011 at 06:26:13PM +0100, Torben Hohn wrote:
> +config PTP_1588_CLOCK_SOFTWARE
> +	tristate "software PTP clock"
> +	depends on PTP_1588_CLOCK
> +	help
> +	  This driver adds a software PTP clock. 

As John said, this needs a longer description. You can look back at
my V6 as an example.

  https://lkml.org/lkml/2010/9/23/220

> +static int ptp_ixp_adjfreq(struct ptp_clock_info *ptp, s32 ppb)

Once again, please rename.

Better yet, why not just use the V6 example instead?  If we are going
to have a testing driver, then I would want the simulated ancillary
code as well.

> +
> +static struct ptp_clock_info ptp_ixp_caps = {
> +	.owner		= THIS_MODULE,
> +	.name		= "software timer",
> +	.max_adj	= 66666655,

You should calculate the actual limit of adjustment and use it here.

Thanks,

Richard

Re: [PATCH 2/3] ptp: add a software clock based on clock_monotonic_raw

[torbenh]

On Fri, Mar 04, 2011 at 07:42:51AM +0100, Richard Cochran wrote:
> On Thu, Mar 03, 2011 at 06:26:13PM +0100, Torben Hohn wrote:
> > +config PTP_1588_CLOCK_SOFTWARE
> > +	tristate "software PTP clock"
> > +	depends on PTP_1588_CLOCK
> > +	help
> > +	  This driver adds a software PTP clock. 
> 
> As John said, this needs a longer description. You can look back at
> my V6 as an example.
> 
>   https://lkml.org/lkml/2010/9/23/220
> 
> > +static int ptp_ixp_adjfreq(struct ptp_clock_info *ptp, s32 ppb)
> 
> Once again, please rename.

i was planning to rename it.
i just posted my patches early on because i had found the permission
problem, and thought the software clock might be useful to play around
with the whole framework.
iE without the software clock, i wouldnt have found the actual problem,
because i dont have the right h/w.


> 
> Better yet, why not just use the V6 example instead?  If we are going
> to have a testing driver, then I would want the simulated ancillary
> code as well.

well... your example code messes with clock_realtime.
i was planning to emit real timed events, not just some simulation.
but i dont completely understand this part of the code yet.


> 
> > +
> > +static struct ptp_clock_info ptp_ixp_caps = {
> > +	.owner		= THIS_MODULE,
> > +	.name		= "software timer",
> > +	.max_adj	= 66666655,
> 
> You should calculate the actual limit of adjustment and use it here.

yeah. i posted it too early...
gonna fix it in the next iteration.

-- 
torben Hohn

Re: [PATCH 2/3] ptp: add a software clock based on clock_monotonic_raw

[Richard Cochran]

On Fri, Mar 04, 2011 at 12:30:32PM +0100, torbenh wrote:
> On Fri, Mar 04, 2011 at 07:42:51AM +0100, Richard Cochran wrote:
> > 
> > Better yet, why not just use the V6 example instead?  If we are going
> > to have a testing driver, then I would want the simulated ancillary
> > code as well.
> 
> well... your example code messes with clock_realtime.

But could you take the file and replace the CLOCK_REALTIME code with
your SW implemented offset/frequency corrections?

And, just keep my code for simulating the ancillary stuff as is.

> i was planning to emit real timed events, not just some simulation.
> but i dont completely understand this part of the code yet.

??? (None of my PHC patches implement any user timers.)

I am not sure where you are going with the AVB clock idea, but it will
be fun to find out.

Thanks,

Richard

[PATCH 3/3] Check for write permission on FD based posix-clocks

[Torben Hohn]

pc_clock_settime() and pc_clock_adjtime() did not check
whether the fd was opened in write mode.

It was possible to set a clock, when we only had read
permissions.

for completeness, we would also need to check for Read permissions
on the read operations. but that would be a bit paranoid, probably.

Signed-off-by: Torben Hohn <torbenh@gmx.de>
Cc: Richard Cochran <richard.cochran@omicron.at>
Cc: John Stultz <johnstul@us.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/time/posix-clock.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/kernel/time/posix-clock.c b/kernel/time/posix-clock.c
index 04498cb..25028dd 100644
--- a/kernel/time/posix-clock.c
+++ b/kernel/time/posix-clock.c
@@ -287,11 +287,16 @@ static int pc_clock_adjtime(clockid_t id, struct timex *tx)
 	if (err)
 		return err;
 
+	if ((cd.fp->f_mode & FMODE_WRITE) == 0) {
+		err = -EACCES;
+		goto out;
+	}
+
 	if (cd.clk->ops.clock_adjtime)
 		err = cd.clk->ops.clock_adjtime(cd.clk, tx);
 	else
 		err = -EOPNOTSUPP;
-
+out:
 	put_clock_desc(&cd);
 
 	return err;
@@ -344,11 +349,16 @@ static int pc_clock_settime(clockid_t id, const struct timespec *ts)
 	if (err)
 		return err;
 
+	if ((cd.fp->f_mode & FMODE_WRITE) == 0) {
+		err = -EACCES;
+		goto out;
+	}
+
 	if (cd.clk->ops.clock_settime)
 		err = cd.clk->ops.clock_settime(cd.clk, ts);
 	else
 		err = -EOPNOTSUPP;
-
+out:
 	put_clock_desc(&cd);
 
 	return err;
-- 
1.7.2.3

Re: [PATCH 3/3] Check for write permission on FD based posix-clocks

[Richard Cochran]

On Thu, Mar 03, 2011 at 06:26:14PM +0100, Torben Hohn wrote:
> pc_clock_settime() and pc_clock_adjtime() did not check
> whether the fd was opened in write mode.
> 
> It was possible to set a clock, when we only had read
> permissions.
> 
> for completeness, we would also need to check for Read permissions
> on the read operations. but that would be a bit paranoid, probably.

I have no objection to this form of clock access control, but I would
like to get agreement about it from the list.

> diff --git a/kernel/time/posix-clock.c b/kernel/time/posix-clock.c
> index 04498cb..25028dd 100644
> --- a/kernel/time/posix-clock.c
> +++ b/kernel/time/posix-clock.c
> @@ -287,11 +287,16 @@ static int pc_clock_adjtime(clockid_t id, struct timex *tx)
>  	if (err)
>  		return err;
>  
> +	if ((cd.fp->f_mode & FMODE_WRITE) == 0) {
> +		err = -EACCES;

Looks like clock_settime and adjtimex are supposed to return EPERM in
this case.

Thanks,

Richard

Re: [PATCH 3/3] Check for write permission on FD based posix-clocks

[Arnd Bergmann]

On Friday 04 March 2011 08:22:39 Richard Cochran wrote:
> > diff --git a/kernel/time/posix-clock.c b/kernel/time/posix-clock.c
> > index 04498cb..25028dd 100644
> > --- a/kernel/time/posix-clock.c
> > +++ b/kernel/time/posix-clock.c
> > @@ -287,11 +287,16 @@ static int pc_clock_adjtime(clockid_t id, struct timex *tx)
> >       if (err)
> >               return err;
> >  
> > +     if ((cd.fp->f_mode & FMODE_WRITE) == 0) {
> > +             err = -EACCES;
> 
> Looks like clock_settime and adjtimex are supposed to return EPERM in
> this case.
> 

I think both choices are correct:

EACCESS often refers to the file permissions of an inode or the mode
of an open file. EPERM usually means that an operation can only be
performed by the owner of an object or by root. So clock_settime and
adjtimex should return EPERM and pc_clock_adjtime should return EACCESS.

	Arnd

Re: [PATCH 3/3] Check for write permission on FD based posix-clocks

[torbenh]

On Fri, Mar 04, 2011 at 08:22:39AM +0100, Richard Cochran wrote:
> On Thu, Mar 03, 2011 at 06:26:14PM +0100, Torben Hohn wrote:
> > pc_clock_settime() and pc_clock_adjtime() did not check
> > whether the fd was opened in write mode.
> > 
> > It was possible to set a clock, when we only had read
> > permissions.
> > 
> > for completeness, we would also need to check for Read permissions
> > on the read operations. but that would be a bit paranoid, probably.
> 
> I have no objection to this form of clock access control, but I would
> like to get agreement about it from the list.
> 
> > diff --git a/kernel/time/posix-clock.c b/kernel/time/posix-clock.c
> > index 04498cb..25028dd 100644
> > --- a/kernel/time/posix-clock.c
> > +++ b/kernel/time/posix-clock.c
> > @@ -287,11 +287,16 @@ static int pc_clock_adjtime(clockid_t id, struct timex *tx)
> >  	if (err)
> >  		return err;
> >  
> > +	if ((cd.fp->f_mode & FMODE_WRITE) == 0) {
> > +		err = -EACCES;
> 
> Looks like clock_settime and adjtimex are supposed to return EPERM in
> this case.

well... this is more similar to calling write(2) on an fd not opened
with FMODE_WRITE... 


ssize_t vfs_writev(struct file *file, const struct iovec __user *vec,
		   unsigned long vlen, loff_t *pos)
{
	if (!(file->f_mode & FMODE_WRITE))
		return -EBADF;
	if (!file->f_op || (!file->f_op->aio_write && !file->f_op->write))
		return -EINVAL;

	return do_readv_writev(WRITE, file, vec, vlen, pos);
}

so probably -EBADF is also a candidate :)
however, since the syscall is not really fd based, EPERM is probably
closer to the current man page.


-- 
torben Hohn

Re: [PATCH 3/3] Check for write permission on FD based posix-clocks

[Thomas Gleixner]

On Fri, 4 Mar 2011, Richard Cochran wrote:

> On Thu, Mar 03, 2011 at 06:26:14PM +0100, Torben Hohn wrote:
> > pc_clock_settime() and pc_clock_adjtime() did not check
> > whether the fd was opened in write mode.
> > 
> > It was possible to set a clock, when we only had read
> > permissions.
> > 
> > for completeness, we would also need to check for Read permissions
> > on the read operations. but that would be a bit paranoid, probably.
> 
> I have no objection to this form of clock access control, but I would
> like to get agreement about it from the list.

Acked-by-me
 
> > diff --git a/kernel/time/posix-clock.c b/kernel/time/posix-clock.c
> > index 04498cb..25028dd 100644
> > --- a/kernel/time/posix-clock.c
> > +++ b/kernel/time/posix-clock.c
> > @@ -287,11 +287,16 @@ static int pc_clock_adjtime(clockid_t id, struct timex *tx)
> >  	if (err)
> >  		return err;
> >  
> > +	if ((cd.fp->f_mode & FMODE_WRITE) == 0) {
> > +		err = -EACCES;
> 
> Looks like clock_settime and adjtimex are supposed to return EPERM in
> this case.

As Arnd already said -EACCES is sensible for file permission checks
while -EPERM means that you have no permission at all.

Thanks,

	tglx

Re: [PATCH 3/3] Check for write permission on FD based posix-clocks

[Richard Cochran]

On Fri, Mar 11, 2011 at 08:46:57PM +0100, Thomas Gleixner wrote:
> As Arnd already said -EACCES is sensible for file permission checks
> while -EPERM means that you have no permission at all.

Okay, so you will accept Torben's patch as is, into timers/core?

Thanks,

Richard

Re: [PATCH 3/3] Check for write permission on FD based posix-clocks

[Thomas Gleixner]

On Sat, 12 Mar 2011, Richard Cochran wrote:

> On Fri, Mar 11, 2011 at 08:46:57PM +0100, Thomas Gleixner wrote:
> > As Arnd already said -EACCES is sensible for file permission checks
> > while -EPERM means that you have no permission at all.
> 
> Okay, so you will accept Torben's patch as is, into timers/core?

If there are no further objections, I'll pick it up.

Thanks,

	tglx

[tip:timers/core] posix-clocks: Check write permissions in posix syscalls

[tip-bot for Torben Hohn]

Commit-ID:  6e6823d17b157f185be09f4c70181299f9273f0b
Gitweb:     http://git.kernel.org/tip/6e6823d17b157f185be09f4c70181299f9273f0b
Author:     Torben Hohn <torbenh@gmx.de>
AuthorDate: Thu, 3 Mar 2011 18:26:14 +0100
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Sat, 12 Mar 2011 21:27:07 +0100

posix-clocks: Check write permissions in posix syscalls

pc_clock_settime() and pc_clock_adjtime() do not check whether the fd
was opened in write mode, so a clock can be set with a read only fd.

[ tglx: We deliberately do not return -EPERM as we want this to be
  	distingushable from the capability based permission check ]

Signed-off-by: Torben Hohn <torbenh@gmx.de>
LKML-Reference: <1299173174-348-4-git-send-email-torbenh@gmx.de>
Cc: Richard Cochran <richard.cochran@omicron.at>
Cc: John Stultz <johnstul@us.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/time/posix-clock.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/kernel/time/posix-clock.c b/kernel/time/posix-clock.c
index 04498cb..25028dd 100644
--- a/kernel/time/posix-clock.c
+++ b/kernel/time/posix-clock.c
@@ -287,11 +287,16 @@ static int pc_clock_adjtime(clockid_t id, struct timex *tx)
 	if (err)
 		return err;
 
+	if ((cd.fp->f_mode & FMODE_WRITE) == 0) {
+		err = -EACCES;
+		goto out;
+	}
+
 	if (cd.clk->ops.clock_adjtime)
 		err = cd.clk->ops.clock_adjtime(cd.clk, tx);
 	else
 		err = -EOPNOTSUPP;
-
+out:
 	put_clock_desc(&cd);
 
 	return err;
@@ -344,11 +349,16 @@ static int pc_clock_settime(clockid_t id, const struct timespec *ts)
 	if (err)
 		return err;
 
+	if ((cd.fp->f_mode & FMODE_WRITE) == 0) {
+		err = -EACCES;
+		goto out;
+	}
+
 	if (cd.clk->ops.clock_settime)
 		err = cd.clk->ops.clock_settime(cd.clk, ts);
 	else
 		err = -EOPNOTSUPP;
-
+out:
 	put_clock_desc(&cd);
 
 	return err;

Re: some patches for the ptp framework

[Richard Cochran]

On Thu, Mar 03, 2011 at 06:26:11PM +0100, Torben Hohn wrote:
> patches 1 and 3 apply on top of tip:core/timer
> 
> patch 2 also requires Richards ptp v12 1/4 for the ptp class.
> modulo a small conflict in drivers/ptp/Makefile and Kconfig.
> because i had added his ixp driver, which the software clock is based on.
> 
> Richards Patchset didnt apply to tip:core/timer as is.
> which branch is it based on ?

I can apply them to

   db1c1cce4a653dcbe6949c72ae7b9f42cab1b929

near the head of timers/core in

  git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git

I guess the newer CLOCK_BOOTTIME commits cause a conflict. I will
rebase once again to fix that.

Thanks,
Richard

Re: some patches for the ptp framework

[Richard Cochran]

On Fri, Mar 04, 2011 at 07:21:20AM +0100, Richard Cochran wrote:
> On Thu, Mar 03, 2011 at 06:26:11PM +0100, Torben Hohn wrote:
> > Richards Patchset didnt apply to tip:core/timer as is.
> > which branch is it based on ?
> 
> I can apply them to
> 
>    db1c1cce4a653dcbe6949c72ae7b9f42cab1b929
> 
> near the head of timers/core in
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git
> 
> I guess the newer CLOCK_BOOTTIME commits cause a conflict. I will
> rebase once again to fix that.

Torben, I don't understand why you couldn't apply V12 to the
timers/core head. It works fine for me...

Richard


a0e5fef2 ptp: Added a clock driver for the National Semiconductor PHYTER.
a992fe1f ptp: Added a clock driver for the IXP46x.
d57a0b3e ptp: Added a clock that uses the eTSEC found on the MPC85xx.
3f341675 ptp: Added a brand new class driver for ptp clocks.
5cd10e79 hrtimer: Update base[CLOCK_BOOTTIME].offset correctly
7fdd7f89 timers: Export CLOCK_BOOTTIME via the posix timers interface
70a08cca timers: Add CLOCK_BOOTTIME hrtimer base
314ac371 time: Extend get_xtime_and_monotonic_offset() to also return sleep
abb3a4ea time: Introduce get_monotonic_boottime and ktime_get_boottime
e06383db hrtimers: extend hrtimer base code to handle more then 2 clockids
db1c1cce ntp: Remove redundant and incorrect parameter check
22b7fcda mn10300: Switch do_timer() to xtimer_update()
0606f422 posix clocks: Introduce dynamic clocks