* Re: [PATCH 3/7] drivers/mtd/nand/mpc5121_nfc.c: Add of_node_put to avoid memory leak
From: Wolfram Sang @ 2010-08-29 15:47 UTC (permalink / raw)
To: Julia Lawall
Cc: David Woodhouse, devicetree-discuss, kernel-janitors,
linux-kernel, linux-mtd, linuxppc-dev
In-Reply-To: <1283075566-27441-4-git-send-email-julia@diku.dk>
[-- Attachment #1: Type: text/plain, Size: 2434 bytes --]
On Sun, Aug 29, 2010 at 11:52:42AM +0200, Julia Lawall wrote:
> Add a call to of_node_put in the error handling code following a call to
> of_find_compatible_node.
>
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @r exists@
> local idexpression x;
> expression E,E1;
> statement S;
> @@
>
> *x =
> (of_find_node_by_path
> |of_find_node_by_name
> |of_find_node_by_phandle
> |of_get_parent
> |of_get_next_parent
> |of_get_next_child
> |of_find_compatible_node
> |of_match_node
> )(...);
> ...
> if (x == NULL) S
> <... when != x = E
> *if (...) {
> ... when != of_node_put(x)
> when != if (...) { ... of_node_put(x); ... }
> (
> return <+...x...+>;
> |
> * return ...;
> )
> }
> ...>
> of_node_put(x);
> // </smpl>
>
> Signed-off-by: Julia Lawall <julia@diku.dk>
Acked-by: Wolfram Sang <w.sang@pengutronix.de>
adding ppc-list to cc.
>
> ---
> drivers/mtd/nand/mpc5121_nfc.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mtd/nand/mpc5121_nfc.c b/drivers/mtd/nand/mpc5121_nfc.c
> index df0c1da..f4610bc 100644
> --- a/drivers/mtd/nand/mpc5121_nfc.c
> +++ b/drivers/mtd/nand/mpc5121_nfc.c
> @@ -568,6 +568,7 @@ static int mpc5121_nfc_read_hw_config(struct mtd_info *mtd)
> uint rcw_width;
> uint rcwh;
> uint romloc, ps;
> + int ret = 0;
>
> rmnode = of_find_compatible_node(NULL, NULL, "fsl,mpc5121-reset");
> if (!rmnode) {
> @@ -579,7 +580,8 @@ static int mpc5121_nfc_read_hw_config(struct mtd_info *mtd)
> rm = of_iomap(rmnode, 0);
> if (!rm) {
> dev_err(prv->dev, "Error mapping reset module node!\n");
> - return -EBUSY;
> + ret = -EBUSY;
> + goto out;
> }
>
> rcwh = in_be32(&rm->rcwhr);
> @@ -628,8 +630,9 @@ static int mpc5121_nfc_read_hw_config(struct mtd_info *mtd)
> rcw_width * 8, rcw_pagesize,
> rcw_sparesize);
> iounmap(rm);
> +out:
> of_node_put(rmnode);
> - return 0;
> + return ret;
> }
>
> /* Free driver resources */
>
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Re: [PATCH 2/7] drivers/serial/mpc52xx_uart.c: Add of_node_put to avoid memory leak
From: Wolfram Sang @ 2010-08-29 15:42 UTC (permalink / raw)
To: Julia Lawall; +Cc: linuxppc-dev, kernel-janitors, linux-kernel
In-Reply-To: <1283075566-27441-3-git-send-email-julia@diku.dk>
[-- Attachment #1: Type: text/plain, Size: 1894 bytes --]
On Sun, Aug 29, 2010 at 11:52:41AM +0200, Julia Lawall wrote:
> Add a call to of_node_put in the error handling code following a call to
> of_find_compatible_node.
>
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @r exists@
> local idexpression x;
> expression E,E1;
> statement S;
> @@
>
> *x =
> (of_find_node_by_path
> |of_find_node_by_name
> |of_find_node_by_phandle
> |of_get_parent
> |of_get_next_parent
> |of_get_next_child
> |of_find_compatible_node
> |of_match_node
> )(...);
> ...
> if (x == NULL) S
> <... when != x = E
> *if (...) {
> ... when != of_node_put(x)
> when != if (...) { ... of_node_put(x); ... }
> (
> return <+...x...+>;
> |
> * return ...;
> )
> }
> ...>
> of_node_put(x);
> // </smpl>
>
> Signed-off-by: Julia Lawall <julia@diku.dk>
Acked-by: Wolfram Sang <w.sang@pengutronix.de>
adding ppc-list to CC.
>
> ---
> drivers/serial/mpc52xx_uart.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/serial/mpc52xx_uart.c b/drivers/serial/mpc52xx_uart.c
> index 8dedb26..c4399e2 100644
> --- a/drivers/serial/mpc52xx_uart.c
> +++ b/drivers/serial/mpc52xx_uart.c
> @@ -500,6 +500,7 @@ static int __init mpc512x_psc_fifoc_init(void)
> psc_fifoc = of_iomap(np, 0);
> if (!psc_fifoc) {
> pr_err("%s: Can't map FIFOC\n", __func__);
> + of_node_put(np);
> return -ENODEV;
> }
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Re: [PATCH 1/5] ptp: Added a brand new class driver for ptp clocks.
From: Christian Riesch @ 2010-08-29 13:32 UTC (permalink / raw)
To: Alan Cox
Cc: Rodolfo Giometti, Arnd Bergmann, john stultz, Richard Cochran,
linux-kernel, Christian Riesch, netdev, linuxppc-dev,
devicetree-discuss, linux-arm-kernel, Krzysztof Halasa
In-Reply-To: <20100827134154.50eef56c@lxorguk.ukuu.org.uk>
Alan Cox wrote:
>> The master node in a PTP network probably takes its time from a
>> precise external time source, like GPS. The GPS provides a 1 PPS
>> directly to the PTP clock hardware, which latches the PTP hardware
>> clock time on the PPS edge. This provides one sample as input to a
>> clock servo (in the PTPd) that, in turn, regulates the PTP clock
>> hardware.
>
> A PTP clock is TAI, Unix time is UTC.
Not necessarily. AFAIK, the time distributed by IEEE1588v2 can either be
based on the "PTP epoch" (timePropertiesDS.ptpTimescale=TRUE) and thus
represent TAI or be based on an implementation specific arbitrary epoch
(timePropertiesDS.ptpTimescale=FALSE) and represent time on some
arbitrary time scale.
Christian
^ permalink raw reply
* RE: [PATCH 1/3][MTD] P4080/eLBC: Make Freescale elbc interrupt common to elbc devices
From: Artem Bityutskiy @ 2010-08-29 11:06 UTC (permalink / raw)
To: Zang Roy-R61911
Cc: Lan Chunhe-B25806, linuxppc-dev, akpm, linux-mtd,
Gala Kumar-B11780
In-Reply-To: <3850A844E6A3854C827AC5C0BEC7B60A03E044@zch01exm23.fsl.freescale.net>
On Mon, 2010-08-09 at 15:33 +0800, Zang Roy-R61911 wrote:
> Any comment about this serial patches?
> If none, I'd ask Andrew to merge to his mm tree.
Could you please separate out MTD stuff, to the extent it is possible to
do?
--
Best Regards,
Artem Bityutskiy (Битюцкий Артём)
^ permalink raw reply
* [PATCH 4/7] arch/powerpc/platforms/powermac/pfunc_core.c: Add of_node_put to avoid memory leak
From: Julia Lawall @ 2010-08-29 9:52 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: devicetree-discuss, kernel-janitors, linux-kernel, Paul Mackerras,
linuxppc-dev
In-Reply-To: <1283075566-27441-1-git-send-email-julia@diku.dk>
Add a call to of_node_put in the error handling code following a call to
of_find_node_by_phandle.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@r exists@
local idexpression x;
expression E,E1;
statement S;
@@
*x =
(of_find_node_by_path
|of_find_node_by_name
|of_find_node_by_phandle
|of_get_parent
|of_get_next_parent
|of_get_next_child
|of_find_compatible_node
|of_match_node
)(...);
...
if (x == NULL) S
<... when != x = E
*if (...) {
... when != of_node_put(x)
when != if (...) { ... of_node_put(x); ... }
(
return <+...x...+>;
|
* return ...;
)
}
...>
of_node_put(x);
// </smpl>
Signed-off-by: Julia Lawall <julia@diku.dk>
---
arch/powerpc/platforms/powermac/pfunc_core.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/platforms/powermac/pfunc_core.c b/arch/powerpc/platforms/powermac/pfunc_core.c
index cec6359..b0c3777 100644
--- a/arch/powerpc/platforms/powermac/pfunc_core.c
+++ b/arch/powerpc/platforms/powermac/pfunc_core.c
@@ -837,8 +837,10 @@ struct pmf_function *__pmf_find_function(struct device_node *target,
return NULL;
find_it:
dev = pmf_find_device(actor);
- if (dev == NULL)
- return NULL;
+ if (dev == NULL) {
+ result = NULL;
+ goto out;
+ }
list_for_each_entry(func, &dev->functions, link) {
if (name && strcmp(name, func->name))
@@ -850,8 +852,9 @@ struct pmf_function *__pmf_find_function(struct device_node *target,
result = func;
break;
}
- of_node_put(actor);
pmf_put_device(dev);
+out:
+ of_node_put(actor);
return result;
}
^ permalink raw reply related
* [PATCH 5/7] arch/powerpc/sysdev/qe_lib/qe.c: Add of_node_put to avoid memory leak
From: Julia Lawall @ 2010-08-29 9:52 UTC (permalink / raw)
To: Timur Tabi
Cc: devicetree-discuss, kernel-janitors, linux-kernel, Paul Mackerras,
linuxppc-dev
In-Reply-To: <1283075566-27441-1-git-send-email-julia@diku.dk>
Add a call to of_node_put in the error handling code following a call to
of_find_compatible_node.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@r exists@
local idexpression x;
expression E,E1;
statement S;
@@
*x =
(of_find_node_by_path
|of_find_node_by_name
|of_find_node_by_phandle
|of_get_parent
|of_get_next_parent
|of_get_next_child
|of_find_compatible_node
|of_match_node
)(...);
...
if (x == NULL) S
<... when != x = E
*if (...) {
... when != of_node_put(x)
when != if (...) { ... of_node_put(x); ... }
(
return <+...x...+>;
|
* return ...;
)
}
...>
of_node_put(x);
// </smpl>
Signed-off-by: Julia Lawall <julia@diku.dk>
---
arch/powerpc/sysdev/qe_lib/qe.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/sysdev/qe_lib/qe.c b/arch/powerpc/sysdev/qe_lib/qe.c
index 3da8014..90020de 100644
--- a/arch/powerpc/sysdev/qe_lib/qe.c
+++ b/arch/powerpc/sysdev/qe_lib/qe.c
@@ -640,6 +640,7 @@ unsigned int qe_get_num_of_snums(void)
if ((num_of_snums < 28) || (num_of_snums > QE_NUM_OF_SNUM)) {
/* No QE ever has fewer than 28 SNUMs */
pr_err("QE: number of snum is invalid\n");
+ of_node_put(qe);
return -EINVAL;
}
}
^ permalink raw reply related
* [PATCH 6/7] arch/powerpc/platforms/maple/setup.c: Add of_node_put to avoid memory leak
From: Julia Lawall @ 2010-08-29 9:52 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: devicetree-discuss, kernel-janitors, linux-kernel, Paul Mackerras,
linuxppc-dev
In-Reply-To: <1283075566-27441-1-git-send-email-julia@diku.dk>
Add a call to of_node_put in the error handling code following a call to
of_find_node_by_path.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@r exists@
local idexpression x;
expression E,E1;
statement S;
@@
*x =
(of_find_node_by_path
|of_find_node_by_name
|of_find_node_by_phandle
|of_get_parent
|of_get_next_parent
|of_get_next_child
|of_find_compatible_node
|of_match_node
)(...);
...
if (x == NULL) S
<... when != x = E
*if (...) {
... when != of_node_put(x)
when != if (...) { ... of_node_put(x); ... }
(
return <+...x...+>;
|
* return ...;
)
}
...>
of_node_put(x);
// </smpl>
Signed-off-by: Julia Lawall <julia@diku.dk>
---
arch/powerpc/platforms/maple/setup.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/platforms/maple/setup.c b/arch/powerpc/platforms/maple/setup.c
index 3fff8d9..fe34c3d 100644
--- a/arch/powerpc/platforms/maple/setup.c
+++ b/arch/powerpc/platforms/maple/setup.c
@@ -358,6 +358,7 @@ static int __init maple_cpc925_edac_setup(void)
model = (const unsigned char *)of_get_property(np, "model", NULL);
if (!model) {
printk(KERN_ERR "%s: Unabel to get model info\n", __func__);
+ of_node_put(np);
return -ENODEV;
}
^ permalink raw reply related
* [PATCH 1/7] drivers/macintosh/via-pmu-led.c: Add of_node_put to avoid memory leak
From: Julia Lawall @ 2010-08-29 9:52 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: linuxppc-dev, devicetree-discuss, kernel-janitors, linux-kernel
In-Reply-To: <1283075566-27441-1-git-send-email-julia@diku.dk>
Add a call to of_node_put in the error handling code following a call to
of_find_node_by_path.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@r exists@
local idexpression x;
expression E,E1;
statement S;
@@
*x =
(of_find_node_by_path
|of_find_node_by_name
|of_find_node_by_phandle
|of_get_parent
|of_get_next_parent
|of_get_next_child
|of_find_compatible_node
|of_match_node
)(...);
...
if (x == NULL) S
<... when != x = E
*if (...) {
... when != of_node_put(x)
when != if (...) { ... of_node_put(x); ... }
(
return <+...x...+>;
|
* return ...;
)
}
...>
of_node_put(x);
// </smpl>
Signed-off-by: Julia Lawall <julia@diku.dk>
---
drivers/macintosh/via-pmu-led.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/macintosh/via-pmu-led.c b/drivers/macintosh/via-pmu-led.c
index d242976..19c3718 100644
--- a/drivers/macintosh/via-pmu-led.c
+++ b/drivers/macintosh/via-pmu-led.c
@@ -92,8 +92,10 @@ static int __init via_pmu_led_init(void)
if (dt == NULL)
return -ENODEV;
model = of_get_property(dt, "model", NULL);
- if (model == NULL)
+ if (model == NULL) {
+ of_node_put(dt);
return -ENODEV;
+ }
if (strncmp(model, "PowerBook", strlen("PowerBook")) != 0 &&
strncmp(model, "iBook", strlen("iBook")) != 0 &&
strcmp(model, "PowerMac7,2") != 0 &&
^ permalink raw reply related
* [PATCH 7/7] arch/powerpc/platforms/cell: Add of_node_put to avoid memory leak
From: Julia Lawall @ 2010-08-29 9:52 UTC (permalink / raw)
To: Arnd Bergmann
Cc: cbe-oss-dev, devicetree-discuss, kernel-janitors, linux-kernel,
Paul Mackerras, linuxppc-dev
In-Reply-To: <1283075566-27441-1-git-send-email-julia@diku.dk>
Add calls to of_node_put in the error handling code following calls to
of_find_node_by_path and of_find_node_by_phandle.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@r exists@
local idexpression x;
expression E,E1;
statement S;
@@
*x =
(of_find_node_by_path
|of_find_node_by_name
|of_find_node_by_phandle
|of_get_parent
|of_get_next_parent
|of_get_next_child
|of_find_compatible_node
|of_match_node
)(...);
...
if (x == NULL) S
<... when != x = E
*if (...) {
... when != of_node_put(x)
when != if (...) { ... of_node_put(x); ... }
(
return <+...x...+>;
|
* return ...;
)
}
...>
of_node_put(x);
// </smpl>
Signed-off-by: Julia Lawall <julia@diku.dk>
---
arch/powerpc/platforms/cell/ras.c | 4 +++-
arch/powerpc/platforms/cell/spider-pic.c | 4 +++-
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/platforms/cell/ras.c b/arch/powerpc/platforms/cell/ras.c
index 1d3c4ef..5ec1e47 100644
--- a/arch/powerpc/platforms/cell/ras.c
+++ b/arch/powerpc/platforms/cell/ras.c
@@ -173,8 +173,10 @@ static int __init cbe_ptcal_enable(void)
return -ENODEV;
size = of_get_property(np, "ibm,cbe-ptcal-size", NULL);
- if (!size)
+ if (!size) {
+ of_node_put(np);
return -ENODEV;
+ }
pr_debug("%s: enabling PTCAL, size = 0x%x\n", __func__, *size);
order = get_order(*size);
diff --git a/arch/powerpc/platforms/cell/spider-pic.c b/arch/powerpc/platforms/cell/spider-pic.c
index 5876e88..3f2e557 100644
--- a/arch/powerpc/platforms/cell/spider-pic.c
+++ b/arch/powerpc/platforms/cell/spider-pic.c
@@ -258,8 +258,10 @@ static unsigned int __init spider_find_cascade_and_node(struct spider_pic *pic)
return NO_IRQ;
imap += intsize + 1;
tmp = of_get_property(iic, "#interrupt-cells", NULL);
- if (tmp == NULL)
+ if (tmp == NULL) {
+ of_node_put(iic);
return NO_IRQ;
+ }
intsize = *tmp;
/* Assume unit is last entry of interrupt specifier */
unit = imap[intsize - 1];
^ permalink raw reply related
* Re: [PATCH] powerpc/fsl_soc: Search all global-utilities nodes for rstccr
From: Timur Tabi @ 2010-08-28 22:34 UTC (permalink / raw)
To: Matthew McClintock; +Cc: kumar.gala, linuxppc-dev
In-Reply-To: <1282946147-28164-1-git-send-email-msm@freescale.com>
> <msm@freescale.com> wrote:
> +
> + =A0 =A0 =A0 for_each_node_by_name(np, "global-utilities") {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if ((of_get_property(np, "fsl,has-rstcr", N=
ULL))) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 rstcr =3D of_iomap(np, 0) +=
0xb0;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!rstcr)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 printk (KER=
N_EMERG "Error: reset control "
I'm not sure KERN_EMERG is warranted for this kind of error.
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 =A0 =A0 =A0 =A0 "register not mapped!\n");
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
So if a node has an fsl,rstcr property, but the of_iomap() fails, we
jump to the next global-utilities node? Perhaps you need a 'break'
after the printk()?
> + =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 if (!rstcr && ppc_md.restart =3D=3D fsl_rstcr_restart)
Wouldn't it make more sense to assign fsl_rstcr_restart to
ppc_md.restart only if we find a valid fsl,has-rstcr property?
--=20
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply
* Re: [PATCH] powerpc/kvm/e500_tlb: Fix a minor copy-paste tracing bug
From: Alexander Graf @ 2010-08-28 10:51 UTC (permalink / raw)
To: Kyle Moffett; +Cc: kvm-ppc, linuxppc-dev, Liu Yu, linux-kernel, Kyle Moffett
In-Reply-To: <AANLkTimwBL42bAxa5okPeJ-y_oSCfp05_POxxRasZGFn@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 828 bytes --]
On 28.08.2010, at 07:24, Kyle Moffett wrote:
> On Fri, Aug 27, 2010 at 20:58, Alexander Graf <agraf@suse.de> wrote:
>> On 27.08.2010, at 21:06, Kyle Moffett wrote:
>>> The kvmppc_e500_stlbe_invalidate() function was trying to pass too many
>>> parameters to trace_kvm_stlb_inval(). This appears to be a bad
>>> copy-paste from a call to trace_kvm_stlb_write().
>>
>> Which kernel is this against? That trace point is already commented out in my tree.
>
> Oh, hm, I guess I haven't rebased this patch since 2.6.35-ish. The
> trace point seems to work correctly with the fixed arguments; if
> you'll tell me which tree I should base it on I can easily resubmit.
My tree is at git://github.com/agraf/linux-2.6.git kvm-ppc-next. If you like I'll gladly take a patch in to enable the point again :).
Alex
[-- Attachment #2: Type: text/html, Size: 1552 bytes --]
^ permalink raw reply
* Re: [PATCH] powerpc/kvm/e500_tlb: Fix a minor copy-paste tracing bug
From: Kyle Moffett @ 2010-08-28 5:24 UTC (permalink / raw)
To: Alexander Graf; +Cc: kvm-ppc, linuxppc-dev, Liu Yu, linux-kernel, Kyle Moffett
In-Reply-To: <B3286B71-599C-405E-847F-06B843A46449@suse.de>
On Fri, Aug 27, 2010 at 20:58, Alexander Graf <agraf@suse.de> wrote:
> On 27.08.2010, at 21:06, Kyle Moffett wrote:
>> The kvmppc_e500_stlbe_invalidate() function was trying to pass too many
>> parameters to trace_kvm_stlb_inval(). =C2=A0This appears to be a bad
>> copy-paste from a call to trace_kvm_stlb_write().
>
> Which kernel is this against? That trace point is already commented out i=
n my tree.
Oh, hm, I guess I haven't rebased this patch since 2.6.35-ish. The
trace point seems to work correctly with the fixed arguments; if
you'll tell me which tree I should base it on I can easily resubmit.
Cheers,
Kyle Moffett
^ permalink raw reply
* Re: [PATCH] powerpc/kvm/e500_tlb: Fix a minor copy-paste tracing bug
From: Alexander Graf @ 2010-08-28 0:58 UTC (permalink / raw)
To: Kyle Moffett; +Cc: linuxppc-dev, Kyle Moffett, Liu Yu, linux-kernel, kvm-ppc
In-Reply-To: <1282936005-17078-1-git-send-email-Kyle.D.Moffett@boeing.com>
On 27.08.2010, at 21:06, Kyle Moffett wrote:
> The kvmppc_e500_stlbe_invalidate() function was trying to pass too =
many
> parameters to trace_kvm_stlb_inval(). This appears to be a bad
> copy-paste from a call to trace_kvm_stlb_write().
Which kernel is this against? That trace point is already commented out =
in my tree.
Alex
^ permalink raw reply
* Re: [PATCH 1/5] ptp: Added a brand new class driver for ptp clocks.
From: John Stultz @ 2010-08-27 22:30 UTC (permalink / raw)
To: Richard Cochran
Cc: Rodolfo Giometti, Arnd Bergmann, netdev, devicetree-discuss,
linux-kernel, linuxppc-dev, linux-arm-kernel, Krzysztof Halasa
In-Reply-To: <20100827123849.GC11657@riccoc20.at.omicron.at>
On Fri, 2010-08-27 at 14:38 +0200, Richard Cochran wrote:
> On Mon, Aug 23, 2010 at 01:08:45PM -0700, john stultz wrote:
> > On Thu, 2010-08-19 at 07:55 +0200, Richard Cochran wrote:
> > > The clockid_t CLOCK_PTP will be arch-neutral.
> >
> > Sure, but are they conceptually neutral? There are other clock
> > synchronization algorithms out there. Will they need their own
> > similar-but-different clock_ids?
> >
> > Look at the other clock ids and what the represent:
>
> IMHO, the presently offered clock ids are a mixed bag...
>
> > CLOCK_REALTIME : Wall time (possibly freq/offset corrected)
> > CLOCK_MONOTONIC: Monotonic time (possibly freq corrected).
> > CLOCK_PROCESS_CPUTIME_ID: Process cpu time.
> > CLOCK_THREAD_CPUTIME_ID: Thread cpu time.
>
> The amount of time a thread has been granted by the kernel is really
> not connected to the real passage of time, at least not in a direct
> way.
>
> > CLOCK_MONOTONIC_RAW: Non freq corrected monotonic time.
>
> This one comes from commit 2d42244ae71d6c7b0884b5664cf2eda30fb2ae68
> and is surely a special case, unrelated to the other clock ids. The
> commit message mentions that this was added to help the btime.sf.net
> project. That project does not seem to have had any activity since
> 2007. If we can justify adding a clock id in this case, surely we can
> add one for PTP as well!
I'm not opposed to adding a clock id, I just want it to be generic
enough to make sense.
So while MONOTONIC_RAW it was spurred into being from btime, it isn't a
CLOCK_BTIME interface.
I've also been pushing the RADClock folks to use it since it avoids the
ugly raw hardware access they want to get access to a constant freq
counter. In addition, I've used it to monitor freq adjustments done by
NTP.
> > CLOCK_REALTIME_COARSE: Tick granular wall time (filesystem timestamp)
> > CLOCK_MONOTONIC_COARSE: Tick granular monotonic time.
>
> These were added in commit da15cfdae03351c689736f8d142618592e3cebc3
> in order to fulfill needs of special applications.
Right, but what they represent and how its different from CLOCK_REALTIME
is easy to describe in a generic way.
> > CLOCK_PTP that you're proposing doesn't seem to be at the same level of
> > abstraction. I'm not saying that this isn't the right place for it, but
> > can we take a step back from PTP and consider what your exposing in more
> > generic terms. In other words, could someone use the same
> > packet-timestamping hardware to implement a different non-PTP time
> > synchronization algorithm?
>
> Yes, of course. There is nothing at all in the patch set about the PTP
> protocol itself. It just lets you access the hardware. In short:
>
> 1. SO_TIMESTAMPING delivers timestamped packets
> 2. the PTP API lets you tune the clock.
>
> "That's all, folks."
Right. So CLOCK_SO_TIMESTAMP is maybe a better description? And isn't it
just an adjustment interface, not a PTP API to tune the clock?
> > Further, if we're using PTP to synchoronize the system time, then there
> > shouldn't be any measurable difference between CLOCK_PTP and
> > CLOCK_REALTIME, no?
>
> When using software timestamping, then the clocks are one in the same.
So this is the duplication I don't like. If the API represents CLOCK_PTP
or whatever as something different from CLOCK_REALTIME, there shouldn't
be a mode where they're the same. That would just cause confusion.
Instead CLOCK_PTP calls just return -EINVAL and the PTPd application
should use CLOCK_REALTIME.
> When using PTP, with the PPS hook to synchoronize the Linux system
> time, the clocks will be a close as the servo algorithm provides. I
> have not measured this yet, but it cannot be much different than using
> any other PPS source.
>
> > > SYSCALL_DEFINE3(clock_adjtime, const clockid_t, clkid,
> > > int, ppb, struct timespec __user *, ts)
> > >
> > > ppb - desired frequency adjustment in parts per billion
> > > ts - desired time step (or jump) in <sec,nsec> to correct
> > > a measured offset
> > >
> > > Arguably, this syscall might be useful for other clocks, too.
> >
> > So yea, obviously the syscall should not be CLOCK_PTP specific, so we
> > would want it to be usable against CLOCK_REALTIME.
> >
> > That said, the clock_adjtime your proposing does not seem to be
> > sufficient for usage by NTPd. So this suggests that it is not generic
> > enough.
>
> I don't think we need to support ntpd. It already has adjtimex, and it
> won't get any better by using another interface.
I strongly disagree. If the new interface isn't generic enough that NTPd
couldn't use it to adjust CLOCK_REALTIME, then its too specific to only
your needs.
There will be other clock sync algorithms down the road, so if we have
to expose this sort of timestamping hardware to userland, we need to
allow those other sync methods to use the API you're providing, so if
you're API isn't a superset of the existing adjustment needs, then its
already missing something.
I'm not saying we have to implement the kernel PLL adjustment and every
other adjustment mode for every CLOCK_ID right now, but we should be
able to extend things in the future.
> > > In contrast, sysfs attributes will fit the need nicely:
> > >
> > > 1. enable or disable pps
> > > 2. enable or disable external timestamps
> > > 3. read out external timestamp
> > > 4. configure period for periodic output
> >
> > Things to consider here:
> > Do having these options really make sense?
>
> Yes, since they represent the PTP clock's hardware features. As I
> explained previously, if you don't have any hardware interfaces, then
> having your clocks synchoronized to under 100 nanoseconds does not
> help you more than having them to within 1 microsecond.
>
> > Why would we want pps disabled?
>
> If you are a master clock, then you want to take your PPS from an
> external time source, like GPS. If you leave the PTP PPS events on,
> then they will occur close in time to the GPS PPS events and may add
> unwanted latency to the interrupt handler.
>
> > And if that does make sense, would it
> > be better to do so via the existing pps interface instead of adding a
> > new ptp specific one?
>
> We have not introduced new PPS interface. We use existing PPS subsystem.
Doesn't the pps subsystem have its own way to control the pps signal
interrupt? I'm not totally sure here, but given your point above that
having multiple pps events it seems like they should be selectable. It
seems something that we'd want to control via the global pps interface,
rather then having a pps-enable flag on every random bit of hardware
that can support it.
> > Same for the timestamps and periodic output (ie: and how do they differ
> > from reading or setting a timer on CLOCK_PTP?)
>
> The posix timer calls won't work:
>
> I have a PTP hardware clocks with multiple external timestamp
> channels. Using timer_gettime, how can I specify (or decode) the
> channel of interest to me?
I guess I'm not following you here. Again, I'm not super familiar with
the hardware involved. Could you clarify a bit more? What do you mean by
external timestamp? Is this what is used on the packet timestamping?
> > What the kernel needs to provide are ways to address #1 and #2 above,
> > but what the kernel needs to expose to userland should be minimal and
> > generic.
>
> My point was this:
>
> The application requires that the soundcard DA clocks (*not* the CPU
> clocks) be synchronized. Currently the Linux kernel offers no way at
> all to do this at all.
So yea.. I think this gets to the main point of contention.
On one side, there is the view that the kernel should abstract and hide
the hardware details to increase app portability. And the other is that
the raw hardware details should be exposed, so the OS stays out of the
way.
Neither of these views are always right. Ideally we can come up with a
abstracted way to provide the hardware data needed that's generic enough
that applications don't have to be changed when moving between hardware
or configurations.
The posix clock id interface is frustrating because the flat static
enumeration is really limiting.
I wonder if a dynamic enumeration for posix clocks would be possibly a
way to go?
In other words, a driver registers a clock with the system, and the
system reserves a clock_id for it from the designated available pool and
assigns it back. Then userland could query the driver via something like
sysfs to get the clockid for the hardware.
Would that maybe work?
> > 2) You've mentioned multiple PTP hardware clocks are possible, but maybe
> > not practically useful. How does PTPd enumerate the existing clocks, and
> > know which devices to listen to for Sync/Delay_Resp messages?
> >
> > The issue I'm trying to address here is the interface inconsistency
> > between the message timestamping interface (ie: likely from a packet,
> > possibly multiple sources) and the proposed CLOCK_PTP interface (with
> > only a single clock being exposed at a time, and that being controlled
> > by a sysfs interface).
>
> The sysfs will include one class device for each PTP clock. Each clock
> has a sysfs attribute with the corresponding clock id.
Do you mean the clock_id # in the posix clocks namespace?
> For the network timestamps, they need to be enabled using the
> SIOCSHWTSTAMP ioctl. We can easily extend that ioctl to include the
> desired clock id.
>
> > My concerns:
> > 1) Again, I'm not totally comfortable exposing the PTP hardware via the
> > posix-clocks/timers interface. I'm not dead set against it, but it just
> > doesn't seem right as a top-level abstraction.
>
> I would also be happy with the character device idea already
> posted. Just pick one of the two, and I'll resubmit the patch set...
Personally, with regard to exposing the ptp clock to userland I'm more
comfortable via the chardev. However, the posix clocks/timer api is
really close to what you need, so I'm not totally set against it. And
maybe the dynamic enumeration would resolve my main problems with it?
That said, with the chardev method, I don't like the idea of duplicating
the existing time apis via a systemtime device. Dropping that from your
earlier patch would ease the majority of my objections.
> > I'm curious if its possible to do the PTP hardware offset/adjustment
> > calculation in a module internally to the kernel? That would allow the
> > PPS interface to still be used to sync the system time, and not expose
> > additional interfaces.
>
> It would be possible, but not too nice, IMHO. In contrast to NTP,
> there is no real need to place the servo in the kernel. Having the
> protocol code and servo in user space makes life much easier.
For you, yes, but from an API maintenance view its just adding to the
list of interfaces we have to preserve forever. :)
> > 2) If this is a top-level interface, I'd prefer the inconsistency
> > between how the timestamped messages are received and the proposed
> > posix_clocks/timer interface be clarified.
> >
> > For example: does the networking stack need to have the source clock_id
> > to use for SO_TIMESTAMPing be specified?
>
> We could do it that way. I never liked the <SW,SYS,RAW> tuple in the
> SO_TIMESTAMPING control message in the first place. Sending three
> fields with two blank seems wasteful. Instead, <clockid,timestamp>
> would be sufficient. Maybe that it is too late to change this.
>
> Even without altering the timestamp, we can simply augment
> SIOCSHWTSTAMP with a clock id.
>
> At this point I would just like to go forward with one of the two
> proposed APIs. I had modelled the character device on the posix clock
> calls in order to make it immediately familar, and I think it is a
> viable approach. After the lkml discussion, I think it is even cleaner
> and nicer to just offer a new clock id.
>
> Thanks for all the feedback and comments,
Thanks for the clarifications and putting up with my questions!
-john
^ permalink raw reply
* [PATCH] powerpc/kernel: Adds correct calling convention for kexec purgatory
From: Matthew McClintock @ 2010-08-27 21:58 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Matthew McClintock, kumar.gala
Call kexec purgatory code correctly. We were getting lucky before.
If you examine the powerpc 32bit kexec "purgatory" code you will
see it expects the following:
>From kexec-tools: purgatory/arch/ppc/v2wrap_32.S
-> calling convention:
-> r3 = physical number of this cpu (all cpus)
-> r4 = address of this chunk (master only)
As such, we need to set r3 to the current core, r4 happens to be
unused by purgatory at the moment but we go ahead and set it
here as well
Signed-off-by: Matthew McClintock <msm@freescale.com>
---
arch/powerpc/kernel/misc_32.S | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/arch/powerpc/kernel/misc_32.S b/arch/powerpc/kernel/misc_32.S
index 99bc652..6d1151f 100644
--- a/arch/powerpc/kernel/misc_32.S
+++ b/arch/powerpc/kernel/misc_32.S
@@ -807,6 +807,9 @@ relocate_new_kernel:
isync
sync
+ mfspr r3, SPRN_PIR /* current core we are running on */
+ mr r4, r5 /* load physical address of chunk called */
+
/* jump to the entry point, usually the setup routine */
mtlr r5
blrl
--
1.6.6.1
^ permalink raw reply related
* [PATCH] powerpc/fsl_soc: Search all global-utilities nodes for rstccr
From: Matthew McClintock @ 2010-08-27 21:55 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Matthew McClintock, kumar.gala
The first global-utilities node might not contain the rstcr
property, so we should search all the nodes
Signed-off-by: Matthew McClintock <msm@freescale.com>
---
arch/powerpc/sysdev/fsl_soc.c | 20 +++++++++++---------
1 files changed, 11 insertions(+), 9 deletions(-)
diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index b91f7ac..e2c8e47 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -378,17 +378,19 @@ static __be32 __iomem *rstcr;
static int __init setup_rstcr(void)
{
struct device_node *np;
- np = of_find_node_by_name(NULL, "global-utilities");
- if ((np && of_get_property(np, "fsl,has-rstcr", NULL))) {
- rstcr = of_iomap(np, 0) + 0xb0;
- if (!rstcr)
- printk (KERN_EMERG "Error: reset control register "
- "not mapped!\n");
- } else if (ppc_md.restart == fsl_rstcr_restart)
+
+ for_each_node_by_name(np, "global-utilities") {
+ if ((of_get_property(np, "fsl,has-rstcr", NULL))) {
+ rstcr = of_iomap(np, 0) + 0xb0;
+ if (!rstcr)
+ printk (KERN_EMERG "Error: reset control "
+ "register not mapped!\n");
+ }
+ }
+
+ if (!rstcr && ppc_md.restart == fsl_rstcr_restart)
printk(KERN_ERR "No RSTCR register, warm reboot won't work\n");
- if (np)
- of_node_put(np);
return 0;
}
--
1.6.6.1
^ permalink raw reply related
* Re: [PATCH 1/5] ptp: Added a brand new class driver for ptp clocks.
From: John Stultz @ 2010-08-27 20:56 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Rodolfo Giometti, netdev, devicetree-discuss, linux-kernel,
linuxppc-dev, Richard Cochran, linux-arm-kernel, Krzysztof Halasa
In-Reply-To: <201008271403.41949.arnd@arndb.de>
On Fri, 2010-08-27 at 14:03 +0200, Arnd Bergmann wrote:
> On Friday 27 August 2010, Richard Cochran wrote:
> > On Mon, Aug 23, 2010 at 01:21:39PM -0700, john stultz wrote:
> > > On Thu, 2010-08-19 at 17:38 +0200, Richard Cochran wrote:
> > > > On Thu, Aug 19, 2010 at 02:28:04PM +0200, Arnd Bergmann wrote:
> > > > > Have you considered passing a struct timex instead of ppb and ts?
> > > >
> > > > Yes, but the timex is not suitable, IMHO.
> > >
> > > Could you expand on this?
> >
> > We need to able to specify that the call is for a PTP clock. We could
> > add that to the modes flag, like this:
> >
> > /*timex.h*/
> > #define ADJ_PTP_0 0x10000
> > #define ADJ_PTP_1 0x20000
> > #define ADJ_PTP_2 0x30000
> > #define ADJ_PTP_3 0x40000
> >
> > I can live with this, if everyone else can, too.
>
> My suggestion was actually to have a new syscall with the existing
> structure, and pass a clockid_t value to it, similar to your
> sys_clock_adjtime(), not change the actual sys_adjtime syscall.
>
> > > Could we not add a adjustment mode ADJ_SETOFFSET or something that would
> > > provide the instantaneous offset correction?
> >
> > Yes, but we would also need to add a struct timespec to the struct
> > timex, in order to get nanosecond resolution. I think it would be
> > possible to do in the padding at the end?
>
> Yes, that's exactly what the padding is for. Instead of timespec, you can
> probably have a extra values for replacing the existing ppm values with
> ppb values.
Right, although the ppm/ppb issue shouldn't be a problem as the timex
allows for much finer then ppb resolution changes.
The only adjustment to the adjtimex/timex interface that may be needed
is the ability to set the time by an offset (ie: ADJ_SETOFFSET), rather
then slewing the offset in (ADJ_OFFSET, or ADJ_OFFSET_SINGLESHOT).
This avoids the calc offset, gettime(&now), settime(now+offset) method
where any latency between the gettime and settime adds to the error.
thanks
-john
thanks
-john
^ permalink raw reply
* Re: [PATCH 1/5] ptp: Added a brand new class driver for ptp clocks.
From: John Stultz @ 2010-08-27 20:14 UTC (permalink / raw)
To: Richard Cochran
Cc: Rodolfo Giometti, Arnd Bergmann, netdev, devicetree-discuss,
linux-kernel, linuxppc-dev, linux-arm-kernel, Krzysztof Halasa
In-Reply-To: <20100827110855.GA11657@riccoc20.at.omicron.at>
On Fri, 2010-08-27 at 13:08 +0200, Richard Cochran wrote:
> On Mon, Aug 23, 2010 at 01:21:39PM -0700, john stultz wrote:
> > On Thu, 2010-08-19 at 17:38 +0200, Richard Cochran wrote:
> > > On Thu, Aug 19, 2010 at 02:28:04PM +0200, Arnd Bergmann wrote:
> > > > My point was that a syscall is better than an ioctl based interface here,
> > > > which I definitely still think. Given that John knows much more about
> > > > clocks than I do, we still need to get agreement on the question that
> > > > he raised, which is whether we actually need to expose this clock to the
> > > > user or not.
> > > >
> > > > If we can find a way to sync system time accurate enough with PTP and
> > > > PPS, user applications may not need to see two separate clocks at all.
> > >
> > > At the very least, one user application (the PTPd) needs to see the
> > > PTP clock.
> > >
> > > > > SYSCALL_DEFINE3(clock_adjtime, const clockid_t, clkid,
> > > > > int, ppb, struct timespec __user *, ts)
> > > > >
> > > > > ppb - desired frequency adjustment in parts per billion
> > > > > ts - desired time step (or jump) in <sec,nsec> to correct
> > > > > a measured offset
> > > > >
> > > > > Arguably, this syscall might be useful for other clocks, too.
> > > >
> > > > This is a mix of adjtime and adjtimex with the addition of
> > > > the clkid parameter, right?
> > >
> > > Sort of, but not really. ADJTIME(3) takes an offset and slowly
> > > corrects the clock using a servo in the kernel, over hours.
> > >
> > > For this function, the offset passed in the 'ts' parameter will be
> > > immediately corrected, by jumping to the new time. This reflects the
> > > way that PTP works. After the first few samples, the PTPd has an
> > > estimate of the offset to the master and the rate difference. The PTPd
> > > can proceed in one of two ways.
> > >
> > > 1. If resetting the clock is not desired, then the clock is set to the
> > > maximum adjustment (in the right direction) until the clock time is
> > > close to the master's time.
> > >
> > > 2. The estimated offset is added to the current time, resulting in a
> > > jump in time.
> > >
> > > We need clock_adjtime(id, 0, ts) for the second case.
> > >
> > > > Have you considered passing a struct timex instead of ppb and ts?
> > >
> > > Yes, but the timex is not suitable, IMHO.
> >
> > Could you expand on this?
>
> We need to able to specify that the call is for a PTP clock. We could
> add that to the modes flag, like this:
>
> /*timex.h*/
> #define ADJ_PTP_0 0x10000
> #define ADJ_PTP_1 0x20000
> #define ADJ_PTP_2 0x30000
> #define ADJ_PTP_3 0x40000
>
> I can live with this, if everyone else can, too.
I wasn't suggesting adding the clock multiplexing to the timex, just
using the timex to specify the adjustments in the clock_adjtime call.
So I was asking why a timex was not suitable instead of using just the
ppb and timespec.
> > Could we not add a adjustment mode ADJ_SETOFFSET or something that would
> > provide the instantaneous offset correction?
>
> Yes, but we would also need to add a struct timespec to the struct
> timex, in order to get nanosecond resolution. I think it would be
> possible to do in the padding at the end?
The existing struct timeval in the timex can be also used as a timespec.
NTPv4 uses it that way specifying the ADJ_NANO flag.
> > You're right that the timex is a little crufty. But its legacy that we
> > will support indefinitely. So following the established interface helps
> > maintainability.
>
> We can use it for PTP, with the modifications suggested above. Or we
> can just introduce the clock_adjtime method, instead.
Again, I think you misunderstood my suggestion. I was suggesting
something like clock_adjtime(clock_id, struct timex*).
> > So if the clock_adjtime interface is needed, it would seem best for it
> > to be generic enough to support not only PTP, but also the NTP kernel
> > PLL.
>
> For the proposed clock_adjime, what else is needed to support clock
> adjustment in general?
>
> I don't mind making the interface generic enough to support any
> (realistic) conceivable clock adjustment scheme, but beyond the
> present PTP hardware clocks, I don't know what else might be needed.
I think using the timex struct covers most of the existing knowledge of
what is needed.
thanks
-john
^ permalink raw reply
* [PATCH] powerpc/kvm/e500_tlb: Fix a minor copy-paste tracing bug
From: Kyle Moffett @ 2010-08-27 19:06 UTC (permalink / raw)
To: linux-kernel; +Cc: linuxppc-dev, Kyle Moffett, Liu Yu, Kyle Moffett, kvm-ppc
The kvmppc_e500_stlbe_invalidate() function was trying to pass too many
parameters to trace_kvm_stlb_inval(). This appears to be a bad
copy-paste from a call to trace_kvm_stlb_write().
Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
---
arch/powerpc/kvm/e500_tlb.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kvm/e500_tlb.c b/arch/powerpc/kvm/e500_tlb.c
index 21011e1..1261a21 100644
--- a/arch/powerpc/kvm/e500_tlb.c
+++ b/arch/powerpc/kvm/e500_tlb.c
@@ -226,8 +226,7 @@ static void kvmppc_e500_stlbe_invalidate(struct kvmppc_vcpu_e500 *vcpu_e500,
kvmppc_e500_shadow_release(vcpu_e500, tlbsel, esel);
stlbe->mas1 = 0;
- trace_kvm_stlb_inval(index_of(tlbsel, esel), stlbe->mas1, stlbe->mas2,
- stlbe->mas3, stlbe->mas7);
+ trace_kvm_stlb_inval(index_of(tlbsel, esel));
}
static void kvmppc_e500_tlb1_invalidate(struct kvmppc_vcpu_e500 *vcpu_e500,
--
1.7.1
^ permalink raw reply related
* Re: [PATCH 1/5] ptp: Added a brand new class driver for ptp clocks.
From: Jacob Keller @ 2010-08-27 16:17 UTC (permalink / raw)
To: Patrick Loschmidt
Cc: Rodolfo Giometti, Arnd Bergmann, john stultz, Richard Cochran,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, netdev@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org,
devicetree-discuss@lists.ozlabs.org, Alan Cox, Krzysztof Halasa
In-Reply-To: <4C77D804.9060500@gmx.net>
[-- Attachment #1: Type: text/plain, Size: 809 bytes --]
On Fri, Aug 27, 2010 at 8:06 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote
> > > system time bimble track a source makes sense just as with NTP but
> making
> > > it a new clock seems the wrong model extending a non-too-bright API
> when
> > > you can just put the time sources in a file tree.
> >
> > Don't get your meaning here, what did you mean by "file tree?"
>
> Something like
>
> /sys/class/timesource/<name>/...
>
> at which point we don't have to enumerate them all, add special system
> calls and then fret about the fact you can't access them from things like
> shell scripts.
>
I agree that this interface is the best way to be future proof. This appears
to allow for multiple "clocks" or nodes, and doesn't get messed up when we
add more than one NIC with separate clocks to a machine.
[-- Attachment #2: Type: text/html, Size: 1171 bytes --]
^ permalink raw reply
* Re: [PATCH 1/5] ptp: Added a brand new class driver for ptp clocks.
From: M. Warner Losh @ 2010-08-27 15:35 UTC (permalink / raw)
To: richardcochran
Cc: giometti, johnstul, devicetree-discuss, linuxppc-dev,
linux-kernel, linux-arm-kernel, netdev, christian.riesch, alan,
khc
In-Reply-To: <20100827140205.GA3293@riccoc20.at.omicron.at>
In message: <20100827140205.GA3293@riccoc20.at.omicron.at>
Richard Cochran <richardcochran@gmail.com> writes:
: On Fri, Aug 27, 2010 at 01:41:54PM +0100, Alan Cox wrote:
: > > The master node in a PTP network probably takes its time from a
: > > precise external time source, like GPS. The GPS provides a 1 PPS
: > > directly to the PTP clock hardware, which latches the PTP hardware
: > > clock time on the PPS edge. This provides one sample as input to a
: > > clock servo (in the PTPd) that, in turn, regulates the PTP clock
: > > hardware.
: >
: > A PTP clock is TAI, Unix time is UTC.
:
: But TAI and UTC progress at the same rate, and UTC differs from TAI by
: a constant offset. In fact, the needed conversion is provided by the
: protocol, so it is not hard to take a 1 PPS from GPS and set the PTP
: clock to TAI.
Except for leap seconds, this is true. However, Unix time isn't UTC
either. Unix time is UTC that pretends leap seconds just don't
exist. POSIX enshrined this long ago, and nobody is going to change
that any time soon.
I don't believe IEEEv2 propagates leap seconds, does it?
Warner
^ permalink raw reply
* Re: [PATCH 1/5] ptp: Added a brand new class driver for ptp clocks.
From: Patrick Loschmidt @ 2010-08-27 15:21 UTC (permalink / raw)
To: Richard Cochran
Cc: Rodolfo Giometti, Arnd Bergmann, john stultz,
devicetree-discuss@lists.ozlabs.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, netdev@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, Alan Cox, Krzysztof Halasa
In-Reply-To: <20100827160624.433b3374@lxorguk.ukuu.org.uk>
Hi!
I'd like to add my two cents about the discussion. Just to shortly
introduce myself: I'm working with PTP since version 2002 (now 2008 or
PTPv2) and I'm developing matching network cards, drivers, and also
sometimes a bit of the stack.
I always had the problem of different HW implementations (even my own)
and how to access the clocks there. So reading this thread, I strongly
support the idea to provide a driver class, which allows the userspace
to run certain standard operations (settime, gettime, adjtime, ...) as
proposed and leave the detailed implementation to a specific PTP clock
driver. I always made my own char device, but I don't want to open this
discussion again as for me it doesn't matter.
Concerning the long discussion about PTP clock, system time, etc. I'd
like to say, that from a point of view of PTP every node/host has only
one clock. That means, that if you have a NIC with integrated clock
(required for HW timestamping) it is counted as a node. As soon as you
want to use multiple NICs this is actually outside the PTP protocol
definition, unless you have only one clock available to both network
interfaces (which is hard to achieve).
So the solution to treat a PTP enabled NIC as a time source for the
system time is in general sufficient, unless for applications that
require precise timestamps in applications. I know of use cases where
code gets instrumented to measure time delays, processing time, and
sequence in SW, e.g. distributed databases for trading systems at the
stock exchange.
Summarizing I think it would be a huge step forward, if all the
different HW implementations could be controlled via a standardised
interface through the kernel. I need timestamps from my network card
with ps resolution and to steer the onboard clock from user space. Then
I would be happy already. :-)
Thanks,
Patrick
^ permalink raw reply
* Re: [PATCH 1/5] ptp: Added a brand new class driver for ptp clocks.
From: Alan Cox @ 2010-08-27 15:06 UTC (permalink / raw)
To: Richard Cochran
Cc: Rodolfo Giometti, Arnd Bergmann, john stultz, devicetree-discuss,
linux-kernel, netdev, linuxppc-dev, linux-arm-kernel,
Krzysztof Halasa
In-Reply-To: <20100827143437.GB3293@riccoc20.at.omicron.at>
> Sorry for causing confusion, but please understand "a hardware clock
> with timestamping capabilities than can be used for PTP support"
> whenever I wrote "PTP" or "PTP clock."
Ok that makes sense.
>
> Well, what I just said is not entirely true.
> most are bound to the PTP protocol. That may change in the future...
Which seems fine to me too - its an implementation detail of that time
source.
> > Specialist applications will presumably need to know which time source or
> > sources they are tracking and synchronizing too out of multiple potential
> > PTP sources
>
> Yes, the PTPd will have some special knowledge.
Not only that but consumers of different time synchronizations will need
to be able to describe which time source they want to talk about from a
selection of PTP or similar things.
> > system time bimble track a source makes sense just as with NTP but making
> > it a new clock seems the wrong model extending a non-too-bright API when
> > you can just put the time sources in a file tree.
>
> Don't get your meaning here, what did you mean by "file tree?"
Something like
/sys/class/timesource/<name>/...
at which point we don't have to enumerate them all, add special system
calls and then fret about the fact you can't access them from things like
shell scripts.
The fact SYS5.4 Unix and SuS got obsessed with numbering things
rather than using names unlike Unix doesn't mean it's the right model to
number them - usually the reverse is true, a heirarchy of file names is
rather more future proof.
Alan
^ permalink raw reply
* Re: [PATCH 1/5] ptp: Added a brand new class driver for ptp clocks.
From: Alan Cox @ 2010-08-27 14:50 UTC (permalink / raw)
To: Richard Cochran
Cc: Rodolfo Giometti, Arnd Bergmann, john stultz, devicetree-discuss,
linux-kernel, Christian Riesch, netdev, linuxppc-dev,
linux-arm-kernel, Krzysztof Halasa
In-Reply-To: <20100827140205.GA3293@riccoc20.at.omicron.at>
> To tell the truth, my original motivation for the patch set was to
> support PTP clocks and applications. I don't think that is such a bad
ptp *clocks*
> idea. After all, the adjtimex interface was added just to support NTP.
>
> At the same time, I can understand the desire to have a generic
> hardware clock adjustment API. Let me see if I can understand and
> summarize what people are asking for:
>
> clock_adjtime(clockid_t id, struct timex *t);
>
> and struct timex gets some new fields at the end.
For a new syscall you could equally make it
(clockid_t id, void *args)
> Using the call, NTPd can call clock_adjtime(CLOCK_REALTIME) and PTPd
> can call clock_realtime(CLOCK_PTP) and everyone is happy, no?
If you only have one clock that you are calling 'the PTP clock' - but is
that a good assumption ?
I agree with your fundamental arguments as I understand them
- That it's another clock or clocks possibly not synchronized with the
system clock
- That there should be a sensible API for doing slews and steps on other
clocks but the systen clock.
I'm concerned about the assumption that there is a single magic PTP
clock, and calling it a PTP clock for two reasons
- There can be more than one
- PTP is just a protocol, in five years time it might be TICTOC or
something newer and more wonderous, in some environments it'll be a
synchronous distributed clock generation not PTP etc. Wiring PTP or
IEE1588v2 into the clock name doesn't make sense.
I'd be happier with a model which says we have some arbitary number of
synchronization sources which may or may not have a connection to system
time, and may be using all sorts of synchronization methods. Clock in
fact seems almost a misnomer.
Alan
^ permalink raw reply
* Re: [PATCH 1/5] ptp: Added a brand new class driver for ptp clocks.
From: Richard Cochran @ 2010-08-27 14:34 UTC (permalink / raw)
To: Alan Cox
Cc: Rodolfo Giometti, Arnd Bergmann, john stultz, devicetree-discuss,
linux-kernel, netdev, linuxppc-dev, linux-arm-kernel,
Krzysztof Halasa
In-Reply-To: <20100827143844.646eccf6@lxorguk.ukuu.org.uk>
On Fri, Aug 27, 2010 at 02:38:44PM +0100, Alan Cox wrote:
> > 2007. If we can justify adding a clock id in this case, surely we can
> > add one for PTP as well!
>
> But PTP isn't really a clock - its a time sync protocol. You can (and may
> need to) have multiple clocks of this form on the same host because it's
> master based and you may have to deal with multiple masters who disagree.
Okay, I really meant "for PTP hardware clocks". In general the
discussion is about supporting a kind of hardware and not about the
PTP network protocol. In fact, the hardware clocks and clock servo
loops are not at all part of the IEEE 1588 standard.
Sorry for causing confusion, but please understand "a hardware clock
with timestamping capabilities than can be used for PTP support"
whenever I wrote "PTP" or "PTP clock."
Well, what I just said is not entirely true.
In fact, most of the current crop of PTP hardware clocks operate by
recognizing PTP network frames and timestamping them. One clock I know
of can timestamp every frame, but that seems to be the exception
rather than the rule. So, in theory they are just clocks, but actually
most are bound to the PTP protocol. That may change in the future...
> > viable approach. After the lkml discussion, I think it is even cleaner
> > and nicer to just offer a new clock id.
>
> PTP is not a clock, it's many clocks so a clock id doesn't really work.
> You could assume a single time domain and add a CLOCK_TAI plus then use
> PTP to track it I guess ?
>
> The question then is who would consume it and how ?
>
> Generic applications want POSIX time, which is managed by NTP but could
> in userspace also be slewed via the existing API to track a PTP source if
> someone wanted and if there is a GPS source around they can compute UTC
> from it.
Yes, and even without a GPS, the PTP protocol (this time I really do
mean the protocol!) does provide the UTC offset whenever it is known.
> Specialist applications will presumably need to know which time source or
> sources they are tracking and synchronizing too out of multiple potential
> PTP sources
Yes, the PTPd will have some special knowledge.
> Kernel stuff is more of a problem.
>
> I'm not sure shoehorning a source of many clocks and time sync bases into
> a jump up and down and make it fit single time assumption is wise. Making
> system time bimble track a source makes sense just as with NTP but making
> it a new clock seems the wrong model extending a non-too-bright API when
> you can just put the time sources in a file tree.
Don't get your meaning here, what did you mean by "file tree?"
Thanks,
Richard
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox