* [PATCH net-next 0/3] Two new PTP Hardware Clock features
@ 2012-09-21 17:00 Richard Cochran
2012-09-21 17:00 ` [PATCH net-next 1/3] ptp: remember the adjusted frequency Richard Cochran
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Richard Cochran @ 2012-09-21 17:00 UTC (permalink / raw)
To: netdev
Cc: Ben Hutchings, David Miller, Jacob Keller, Jeff Kirsher,
John Stultz, Matthew Vick
This patch series adds two new features to the PHC code.
The first two patches let a user program find out the previously
dialed frequency adjustment. This is primarily useful when restarting
a PTP service, since without this information, the presumably correct
adjustment will bias the new frequency estimation.
The third patch makes the sysfs clock name come from the 'struct
device'. This idea was recently discussed on netdev as a way of taking
the guesswork out of the naming scheme.
Thanks,
Richard
Richard Cochran (3):
ptp: remember the adjusted frequency
ptp: provide the clock's adjusted frequency
ptp: derive the device name from the parent device
drivers/net/ethernet/freescale/gianfar_ptp.c | 3 +--
drivers/net/ethernet/intel/igb/igb_ptp.c | 7 ++-----
drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c | 5 ++---
drivers/net/phy/dp83640.c | 3 +--
drivers/ptp/ptp_clock.c | 13 +++++++++++--
drivers/ptp/ptp_ixp46x.c | 3 +--
drivers/ptp/ptp_pch.c | 3 +--
drivers/ptp/ptp_private.h | 2 ++
drivers/ptp/ptp_sysfs.c | 2 +-
include/linux/ptp_clock_kernel.h | 8 +++++---
10 files changed, 27 insertions(+), 22 deletions(-)
--
1.7.2.5
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH net-next 1/3] ptp: remember the adjusted frequency 2012-09-21 17:00 [PATCH net-next 0/3] Two new PTP Hardware Clock features Richard Cochran @ 2012-09-21 17:00 ` Richard Cochran 2012-09-21 17:00 ` [PATCH net-next 2/3] ptp: provide the clock's " Richard Cochran 2012-09-21 17:00 ` [PATCH net-next 3/3] ptp: derive the device name from the parent device Richard Cochran 2 siblings, 0 replies; 15+ messages in thread From: Richard Cochran @ 2012-09-21 17:00 UTC (permalink / raw) To: netdev Cc: Ben Hutchings, David Miller, Jacob Keller, Jeff Kirsher, John Stultz, Matthew Vick This patch adds a field to the representation of a PTP hardware clock in order to remember the frequency adjustment value dialed by the user. Adding this field will let us answer queries in the manner of adjtimex in a follow on patch. Signed-off-by: Richard Cochran <richardcochran@gmail.com> --- drivers/ptp/ptp_clock.c | 1 + drivers/ptp/ptp_private.h | 1 + 2 files changed, 2 insertions(+), 0 deletions(-) diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c index 1e528b5..fbd7104 100644 --- a/drivers/ptp/ptp_clock.c +++ b/drivers/ptp/ptp_clock.c @@ -147,6 +147,7 @@ static int ptp_clock_adjtime(struct posix_clock *pc, struct timex *tx) } else if (tx->modes & ADJ_FREQUENCY) { err = ops->adjfreq(ops, scaled_ppm_to_ppb(tx->freq)); + ptp->dialed_frequency = tx->freq; } return err; diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h index 4d5b508..69d3207 100644 --- a/drivers/ptp/ptp_private.h +++ b/drivers/ptp/ptp_private.h @@ -45,6 +45,7 @@ struct ptp_clock { dev_t devid; int index; /* index into clocks.map */ struct pps_device *pps_source; + long dialed_frequency; /* remembers the frequency adjustment */ struct timestamp_event_queue tsevq; /* simple fifo for time stamps */ struct mutex tsevq_mux; /* one process at a time reading the fifo */ wait_queue_head_t tsev_wq; -- 1.7.2.5 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH net-next 2/3] ptp: provide the clock's adjusted frequency 2012-09-21 17:00 [PATCH net-next 0/3] Two new PTP Hardware Clock features Richard Cochran 2012-09-21 17:00 ` [PATCH net-next 1/3] ptp: remember the adjusted frequency Richard Cochran @ 2012-09-21 17:00 ` Richard Cochran 2012-09-21 17:00 ` [PATCH net-next 3/3] ptp: derive the device name from the parent device Richard Cochran 2 siblings, 0 replies; 15+ messages in thread From: Richard Cochran @ 2012-09-21 17:00 UTC (permalink / raw) To: netdev Cc: Ben Hutchings, David Miller, Jacob Keller, Jeff Kirsher, John Stultz, Matthew Vick If the timex.mode field indicates a query, then we provide the value of the current frequency adjustment. Signed-off-by: Richard Cochran <richardcochran@gmail.com> --- drivers/ptp/ptp_clock.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-) diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c index fbd7104..6e47450 100644 --- a/drivers/ptp/ptp_clock.c +++ b/drivers/ptp/ptp_clock.c @@ -148,6 +148,11 @@ static int ptp_clock_adjtime(struct posix_clock *pc, struct timex *tx) err = ops->adjfreq(ops, scaled_ppm_to_ppb(tx->freq)); ptp->dialed_frequency = tx->freq; + + } else if (tx->modes == 0) { + + tx->freq = ptp->dialed_frequency; + err = 0; } return err; -- 1.7.2.5 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH net-next 3/3] ptp: derive the device name from the parent device 2012-09-21 17:00 [PATCH net-next 0/3] Two new PTP Hardware Clock features Richard Cochran 2012-09-21 17:00 ` [PATCH net-next 1/3] ptp: remember the adjusted frequency Richard Cochran 2012-09-21 17:00 ` [PATCH net-next 2/3] ptp: provide the clock's " Richard Cochran @ 2012-09-21 17:00 ` Richard Cochran 2012-09-21 17:19 ` Ben Hutchings 2 siblings, 1 reply; 15+ messages in thread From: Richard Cochran @ 2012-09-21 17:00 UTC (permalink / raw) To: netdev Cc: Ben Hutchings, David Miller, Jacob Keller, Jeff Kirsher, John Stultz, Matthew Vick PTP Hardware Clock device have a name that appears under sysfs that should identify the underlying device. Instead of leaving it up to the driver to invent a name, this patch changes the registration to automatically use the name from the parent device. Signed-off-by: Richard Cochran <richardcochran@gmail.com> --- drivers/net/ethernet/freescale/gianfar_ptp.c | 3 +-- drivers/net/ethernet/intel/igb/igb_ptp.c | 7 ++----- drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c | 5 ++--- drivers/net/phy/dp83640.c | 3 +-- drivers/ptp/ptp_clock.c | 7 +++++-- drivers/ptp/ptp_ixp46x.c | 3 +-- drivers/ptp/ptp_pch.c | 3 +-- drivers/ptp/ptp_private.h | 1 + drivers/ptp/ptp_sysfs.c | 2 +- include/linux/ptp_clock_kernel.h | 8 +++++--- 10 files changed, 20 insertions(+), 22 deletions(-) diff --git a/drivers/net/ethernet/freescale/gianfar_ptp.c b/drivers/net/ethernet/freescale/gianfar_ptp.c index c08e5d4..a0eaafd 100644 --- a/drivers/net/ethernet/freescale/gianfar_ptp.c +++ b/drivers/net/ethernet/freescale/gianfar_ptp.c @@ -408,7 +408,6 @@ static int ptp_gianfar_enable(struct ptp_clock_info *ptp, static struct ptp_clock_info ptp_gianfar_caps = { .owner = THIS_MODULE, - .name = "gianfar clock", .max_adj = 512000, .n_alarm = N_ALARM, .n_ext_ts = N_EXT_TS, @@ -510,7 +509,7 @@ static int gianfar_ptp_probe(struct platform_device *dev) spin_unlock_irqrestore(&etsects->lock, flags); - etsects->clock = ptp_clock_register(&etsects->caps); + etsects->clock = ptp_clock_register(&etsects->caps, &dev->dev); if (IS_ERR(etsects->clock)) { err = PTR_ERR(etsects->clock); goto no_clock; diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c index e13ba1d..546e9e2 100644 --- a/drivers/net/ethernet/intel/igb/igb_ptp.c +++ b/drivers/net/ethernet/intel/igb/igb_ptp.c @@ -665,11 +665,9 @@ int igb_ptp_hwtstamp_ioctl(struct net_device *netdev, void igb_ptp_init(struct igb_adapter *adapter) { struct e1000_hw *hw = &adapter->hw; - struct net_device *netdev = adapter->netdev; switch (hw->mac.type) { case e1000_82576: - snprintf(adapter->ptp_caps.name, 16, "%pm", netdev->dev_addr); adapter->ptp_caps.owner = THIS_MODULE; adapter->ptp_caps.max_adj = 1000000000; adapter->ptp_caps.n_ext_ts = 0; @@ -688,7 +686,6 @@ void igb_ptp_init(struct igb_adapter *adapter) break; case e1000_82580: case e1000_i350: - snprintf(adapter->ptp_caps.name, 16, "%pm", netdev->dev_addr); adapter->ptp_caps.owner = THIS_MODULE; adapter->ptp_caps.max_adj = 62499999; adapter->ptp_caps.n_ext_ts = 0; @@ -707,7 +704,6 @@ void igb_ptp_init(struct igb_adapter *adapter) break; case e1000_i210: case e1000_i211: - snprintf(adapter->ptp_caps.name, 16, "%pm", netdev->dev_addr); adapter->ptp_caps.owner = THIS_MODULE; adapter->ptp_caps.max_adj = 62499999; adapter->ptp_caps.n_ext_ts = 0; @@ -752,7 +748,8 @@ void igb_ptp_init(struct igb_adapter *adapter) wr32(E1000_IMS, E1000_IMS_TS); } - adapter->ptp_clock = ptp_clock_register(&adapter->ptp_caps); + adapter->ptp_clock = ptp_clock_register(&adapter->ptp_caps, + &adapter->pdev->dev); if (IS_ERR(adapter->ptp_clock)) { adapter->ptp_clock = NULL; dev_err(&adapter->pdev->dev, "ptp_clock_register failed\n"); diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c index 3456d56..6219c91 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c @@ -917,7 +917,6 @@ void ixgbe_ptp_init(struct ixgbe_adapter *adapter) switch (adapter->hw.mac.type) { case ixgbe_mac_X540: - snprintf(adapter->ptp_caps.name, 16, "%pm", netdev->dev_addr); adapter->ptp_caps.owner = THIS_MODULE; adapter->ptp_caps.max_adj = 250000000; adapter->ptp_caps.n_alarm = 0; @@ -931,7 +930,6 @@ void ixgbe_ptp_init(struct ixgbe_adapter *adapter) adapter->ptp_caps.enable = ixgbe_ptp_enable; break; case ixgbe_mac_82599EB: - snprintf(adapter->ptp_caps.name, 16, "%pm", netdev->dev_addr); adapter->ptp_caps.owner = THIS_MODULE; adapter->ptp_caps.max_adj = 250000000; adapter->ptp_caps.n_alarm = 0; @@ -960,7 +958,8 @@ void ixgbe_ptp_init(struct ixgbe_adapter *adapter) /* (Re)start the overflow check */ adapter->flags2 |= IXGBE_FLAG2_OVERFLOW_CHECK_ENABLED; - adapter->ptp_clock = ptp_clock_register(&adapter->ptp_caps); + adapter->ptp_clock = ptp_clock_register(&adapter->ptp_caps, + &adapter->pdev->dev); if (IS_ERR(adapter->ptp_clock)) { adapter->ptp_clock = NULL; e_dev_err("ptp_clock_register failed\n"); diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c index b0da022..12a4c4c 100644 --- a/drivers/net/phy/dp83640.c +++ b/drivers/net/phy/dp83640.c @@ -871,7 +871,6 @@ static void dp83640_clock_init(struct dp83640_clock *clock, struct mii_bus *bus) mutex_init(&clock->clock_lock); INIT_LIST_HEAD(&clock->phylist); clock->caps.owner = THIS_MODULE; - sprintf(clock->caps.name, "dp83640 timer"); clock->caps.max_adj = 1953124; clock->caps.n_alarm = 0; clock->caps.n_ext_ts = N_EXT_TS; @@ -980,7 +979,7 @@ static int dp83640_probe(struct phy_device *phydev) if (choose_this_phy(clock, phydev)) { clock->chosen = dp83640; - clock->ptp_clock = ptp_clock_register(&clock->caps); + clock->ptp_clock = ptp_clock_register(&clock->caps, &phydev->dev); if (IS_ERR(clock->ptp_clock)) { err = PTR_ERR(clock->ptp_clock); goto no_register; diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c index 6e47450..60fa259 100644 --- a/drivers/ptp/ptp_clock.c +++ b/drivers/ptp/ptp_clock.c @@ -186,7 +186,8 @@ static void delete_ptp_clock(struct posix_clock *pc) /* public interface */ -struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info) +struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info, + struct device *parent) { struct ptp_clock *ptp; int err = 0, index, major = MAJOR(ptp_devt); @@ -219,11 +220,13 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info) init_waitqueue_head(&ptp->tsev_wq); /* Create a new device in our class. */ - ptp->dev = device_create(ptp_class, NULL, ptp->devid, ptp, + ptp->dev = device_create(ptp_class, parent, ptp->devid, ptp, "ptp%d", ptp->index); if (IS_ERR(ptp->dev)) goto no_device; + ptp->name = parent ? dev_name(parent) : dev_name(ptp->dev); + dev_set_drvdata(ptp->dev, ptp); err = ptp_populate_sysfs(ptp); diff --git a/drivers/ptp/ptp_ixp46x.c b/drivers/ptp/ptp_ixp46x.c index e03c406..c0939b1 100644 --- a/drivers/ptp/ptp_ixp46x.c +++ b/drivers/ptp/ptp_ixp46x.c @@ -241,7 +241,6 @@ static int ptp_ixp_enable(struct ptp_clock_info *ptp, static struct ptp_clock_info ptp_ixp_caps = { .owner = THIS_MODULE, - .name = "IXP46X timer", .max_adj = 66666655, .n_ext_ts = N_EXT_TS, .pps = 0, @@ -298,7 +297,7 @@ static int __init ptp_ixp_init(void) ixp_clock.caps = ptp_ixp_caps; - ixp_clock.ptp_clock = ptp_clock_register(&ixp_clock.caps); + ixp_clock.ptp_clock = ptp_clock_register(&ixp_clock.caps, NULL); if (IS_ERR(ixp_clock.ptp_clock)) return PTR_ERR(ixp_clock.ptp_clock); diff --git a/drivers/ptp/ptp_pch.c b/drivers/ptp/ptp_pch.c index 3a9c17e..0ae0c27 100644 --- a/drivers/ptp/ptp_pch.c +++ b/drivers/ptp/ptp_pch.c @@ -511,7 +511,6 @@ static int ptp_pch_enable(struct ptp_clock_info *ptp, static struct ptp_clock_info ptp_pch_caps = { .owner = THIS_MODULE, - .name = "PCH timer", .max_adj = 50000000, .n_ext_ts = N_EXT_TS, .pps = 0, @@ -627,7 +626,7 @@ pch_probe(struct pci_dev *pdev, const struct pci_device_id *id) } chip->caps = ptp_pch_caps; - chip->ptp_clock = ptp_clock_register(&chip->caps); + chip->ptp_clock = ptp_clock_register(&chip->caps, &pdev->dev); if (IS_ERR(chip->ptp_clock)) return PTR_ERR(chip->ptp_clock); diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h index 69d3207..a3bf959 100644 --- a/drivers/ptp/ptp_private.h +++ b/drivers/ptp/ptp_private.h @@ -42,6 +42,7 @@ struct ptp_clock { struct posix_clock clock; struct device *dev; struct ptp_clock_info *info; + const char *name; dev_t devid; int index; /* index into clocks.map */ struct pps_device *pps_source; diff --git a/drivers/ptp/ptp_sysfs.c b/drivers/ptp/ptp_sysfs.c index 2f93926..f3c4519 100644 --- a/drivers/ptp/ptp_sysfs.c +++ b/drivers/ptp/ptp_sysfs.c @@ -25,7 +25,7 @@ static ssize_t clock_name_show(struct device *dev, struct device_attribute *attr, char *page) { struct ptp_clock *ptp = dev_get_drvdata(dev); - return snprintf(page, PAGE_SIZE-1, "%s\n", ptp->info->name); + return snprintf(page, PAGE_SIZE-1, "%s\n", ptp->name); } #define PTP_SHOW_INT(name) \ diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h index 945704c..e673de0 100644 --- a/include/linux/ptp_clock_kernel.h +++ b/include/linux/ptp_clock_kernel.h @@ -21,6 +21,7 @@ #ifndef _PTP_CLOCK_KERNEL_H_ #define _PTP_CLOCK_KERNEL_H_ +#include <linux/device.h> #include <linux/ptp_clock.h> @@ -73,7 +74,6 @@ struct ptp_clock_request { struct ptp_clock_info { struct module *owner; - char name[16]; s32 max_adj; int n_alarm; int n_ext_ts; @@ -92,10 +92,12 @@ struct ptp_clock; /** * ptp_clock_register() - register a PTP hardware clock driver * - * @info: Structure describing the new clock. + * @info: Structure describing the new clock. + * @parent: Pointer to the parent device of the new clock. */ -extern struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info); +extern struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info, + struct device *parent); /** * ptp_clock_unregister() - unregister a PTP hardware clock driver -- 1.7.2.5 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH net-next 3/3] ptp: derive the device name from the parent device 2012-09-21 17:00 ` [PATCH net-next 3/3] ptp: derive the device name from the parent device Richard Cochran @ 2012-09-21 17:19 ` Ben Hutchings 2012-09-21 17:37 ` Richard Cochran 0 siblings, 1 reply; 15+ messages in thread From: Ben Hutchings @ 2012-09-21 17:19 UTC (permalink / raw) To: Richard Cochran Cc: netdev, David Miller, Jacob Keller, Jeff Kirsher, John Stultz, Matthew Vick On Fri, 2012-09-21 at 19:00 +0200, Richard Cochran wrote: > PTP Hardware Clock device have a name that appears under sysfs that should > identify the underlying device. Instead of leaving it up to the driver to > invent a name, this patch changes the registration to automatically use > the name from the parent device. I thought you wanted the driver name and not the parent device name? [...] > diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c > index 6e47450..60fa259 100644 > --- a/drivers/ptp/ptp_clock.c > +++ b/drivers/ptp/ptp_clock.c [...] > @@ -219,11 +220,13 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info) > init_waitqueue_head(&ptp->tsev_wq); > > /* Create a new device in our class. */ > - ptp->dev = device_create(ptp_class, NULL, ptp->devid, ptp, > + ptp->dev = device_create(ptp_class, parent, ptp->devid, ptp, > "ptp%d", ptp->index); > if (IS_ERR(ptp->dev)) > goto no_device; > > + ptp->name = parent ? dev_name(parent) : dev_name(ptp->dev); > + The fallback of using dev_name(ptp->dev) is a bit sad, as that's the same name userland already has when it reads this attribute. [...] > diff --git a/drivers/ptp/ptp_ixp46x.c b/drivers/ptp/ptp_ixp46x.c > index e03c406..c0939b1 100644 > --- a/drivers/ptp/ptp_ixp46x.c > +++ b/drivers/ptp/ptp_ixp46x.c > @@ -241,7 +241,6 @@ static int ptp_ixp_enable(struct ptp_clock_info *ptp, > > static struct ptp_clock_info ptp_ixp_caps = { > .owner = THIS_MODULE, > - .name = "IXP46X timer", > .max_adj = 66666655, > .n_ext_ts = N_EXT_TS, > .pps = 0, > @@ -298,7 +297,7 @@ static int __init ptp_ixp_init(void) > > ixp_clock.caps = ptp_ixp_caps; > > - ixp_clock.ptp_clock = ptp_clock_register(&ixp_clock.caps); > + ixp_clock.ptp_clock = ptp_clock_register(&ixp_clock.caps, NULL); [...] I think this should really register a platform driver and a platform device to be the parent of the clock device. And then you don't need the fallback for parent == NULL. Since David has pulled the addition of PTP/PHC support to sfc, that will need to be adjusted as well. Ben. -- Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next 3/3] ptp: derive the device name from the parent device 2012-09-21 17:19 ` Ben Hutchings @ 2012-09-21 17:37 ` Richard Cochran 2012-09-21 17:39 ` Keller, Jacob E 0 siblings, 1 reply; 15+ messages in thread From: Richard Cochran @ 2012-09-21 17:37 UTC (permalink / raw) To: Ben Hutchings Cc: netdev, David Miller, Jacob Keller, Jeff Kirsher, John Stultz, Matthew Vick On Fri, Sep 21, 2012 at 06:19:25PM +0100, Ben Hutchings wrote: > On Fri, 2012-09-21 at 19:00 +0200, Richard Cochran wrote: > > PTP Hardware Clock device have a name that appears under sysfs that should > > identify the underlying device. Instead of leaving it up to the driver to > > invent a name, this patch changes the registration to automatically use > > the name from the parent device. > > I thought you wanted the driver name and not the parent device name? Yes, I originally wanted the driver name, because it gives the user information about the expected performance. But here I am bowing to Jacob's suggestion that we really need to identify the device. Intel cards with two ports have *two* clocks. Sad but true. > > + ptp->name = parent ? dev_name(parent) : dev_name(ptp->dev); > > + > > The fallback of using dev_name(ptp->dev) is a bit sad, as that's the > same name userland already has when it reads this attribute. This fallback only exists for the ixp46x, since that driver has no proper platform or device tree device. > > - ixp_clock.ptp_clock = ptp_clock_register(&ixp_clock.caps); > > + ixp_clock.ptp_clock = ptp_clock_register(&ixp_clock.caps, NULL); > [...] > > I think this should really register a platform driver and a platform > device to be the parent of the clock device. And then you don't need > the fallback for parent == NULL. That chip is long past EOL, and its PTP capabilities are pretty poor, and so it is not worth the trouble of fixing it up. All other (and future) drivers must provide the pointer. > Since David has pulled the addition of PTP/PHC support to sfc, that will > need to be adjusted as well. Okay, I will add that to the series. Thanks, Richard ^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH net-next 3/3] ptp: derive the device name from the parent device 2012-09-21 17:37 ` Richard Cochran @ 2012-09-21 17:39 ` Keller, Jacob E 2012-09-21 17:40 ` Ben Hutchings 0 siblings, 1 reply; 15+ messages in thread From: Keller, Jacob E @ 2012-09-21 17:39 UTC (permalink / raw) To: Richard Cochran, Ben Hutchings Cc: netdev@vger.kernel.org, David Miller, Kirsher, Jeffrey T, John Stultz, Vick, Matthew > -----Original Message----- > From: Richard Cochran [mailto:richardcochran@gmail.com] > Sent: Friday, September 21, 2012 10:37 AM > To: Ben Hutchings > Cc: netdev@vger.kernel.org; David Miller; Keller, Jacob E; Kirsher, > Jeffrey T; John Stultz; Vick, Matthew > Subject: Re: [PATCH net-next 3/3] ptp: derive the device name from the > parent device > > On Fri, Sep 21, 2012 at 06:19:25PM +0100, Ben Hutchings wrote: > > On Fri, 2012-09-21 at 19:00 +0200, Richard Cochran wrote: > > > PTP Hardware Clock device have a name that appears under sysfs that > > > should identify the underlying device. Instead of leaving it up to > > > the driver to invent a name, this patch changes the registration to > > > automatically use the name from the parent device. > > > > I thought you wanted the driver name and not the parent device name? > > Yes, I originally wanted the driver name, because it gives the user > information about the expected performance. But here I am bowing to > Jacob's suggestion that we really need to identify the device. Intel cards > with two ports have *two* clocks. Sad but true. > With the device, it is possible to lookup the driver. - Jake > Thanks, > Richard ^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH net-next 3/3] ptp: derive the device name from the parent device 2012-09-21 17:39 ` Keller, Jacob E @ 2012-09-21 17:40 ` Ben Hutchings 2012-09-21 18:11 ` Richard Cochran 0 siblings, 1 reply; 15+ messages in thread From: Ben Hutchings @ 2012-09-21 17:40 UTC (permalink / raw) To: Keller, Jacob E Cc: Richard Cochran, netdev@vger.kernel.org, David Miller, Kirsher, Jeffrey T, John Stultz, Vick, Matthew On Fri, 2012-09-21 at 17:39 +0000, Keller, Jacob E wrote: > > -----Original Message----- > > From: Richard Cochran [mailto:richardcochran@gmail.com] > > Sent: Friday, September 21, 2012 10:37 AM > > To: Ben Hutchings > > Cc: netdev@vger.kernel.org; David Miller; Keller, Jacob E; Kirsher, > > Jeffrey T; John Stultz; Vick, Matthew > > Subject: Re: [PATCH net-next 3/3] ptp: derive the device name from the > > parent device > > > > On Fri, Sep 21, 2012 at 06:19:25PM +0100, Ben Hutchings wrote: > > > On Fri, 2012-09-21 at 19:00 +0200, Richard Cochran wrote: > > > > PTP Hardware Clock device have a name that appears under sysfs that > > > > should identify the underlying device. Instead of leaving it up to > > > > the driver to invent a name, this patch changes the registration to > > > > automatically use the name from the parent device. > > > > > > I thought you wanted the driver name and not the parent device name? > > > > Yes, I originally wanted the driver name, because it gives the user > > information about the expected performance. But here I am bowing to > > Jacob's suggestion that we really need to identify the device. Intel cards > > with two ports have *two* clocks. Sad but true. > > > > With the device, it is possible to lookup the driver. For PCI devices, yes. But there is really no guarantee that device names (as in dev_name()) are globally unique either. Ben. -- Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next 3/3] ptp: derive the device name from the parent device 2012-09-21 17:40 ` Ben Hutchings @ 2012-09-21 18:11 ` Richard Cochran 2012-09-21 18:23 ` Ben Hutchings 0 siblings, 1 reply; 15+ messages in thread From: Richard Cochran @ 2012-09-21 18:11 UTC (permalink / raw) To: Ben Hutchings Cc: Keller, Jacob E, netdev@vger.kernel.org, David Miller, Kirsher, Jeffrey T, John Stultz, Vick, Matthew On Fri, Sep 21, 2012 at 06:40:58PM +0100, Ben Hutchings wrote: > On Fri, 2012-09-21 at 17:39 +0000, Keller, Jacob E wrote: > > With the device, it is possible to lookup the driver. > > For PCI devices, yes. But there is really no guarantee that device > names (as in dev_name()) are globally unique either. Ben, I tried to implement your suggestion from September 7th. Don't you like it, or did I misunderstand you? Thanks, Richard ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next 3/3] ptp: derive the device name from the parent device 2012-09-21 18:11 ` Richard Cochran @ 2012-09-21 18:23 ` Ben Hutchings 2012-09-21 18:40 ` Richard Cochran 0 siblings, 1 reply; 15+ messages in thread From: Ben Hutchings @ 2012-09-21 18:23 UTC (permalink / raw) To: Richard Cochran Cc: Keller, Jacob E, netdev@vger.kernel.org, David Miller, Kirsher, Jeffrey T, John Stultz, Vick, Matthew On Fri, 2012-09-21 at 20:11 +0200, Richard Cochran wrote: > On Fri, Sep 21, 2012 at 06:40:58PM +0100, Ben Hutchings wrote: > > On Fri, 2012-09-21 at 17:39 +0000, Keller, Jacob E wrote: > > > With the device, it is possible to lookup the driver. > > > > For PCI devices, yes. But there is really no guarantee that device > > names (as in dev_name()) are globally unique either. > > Ben, I tried to implement your suggestion from September 7th. > Don't you like it, or did I misunderstand you? For programmatic purposes, setting the parent is the really useful thing. I think what I'm still missing from you is some explanation of what the 'clock name' is meant to be used as - a type name, a unique identifier, a 'friendly name' for listing clocks in a user interface? Ben. -- Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next 3/3] ptp: derive the device name from the parent device 2012-09-21 18:23 ` Ben Hutchings @ 2012-09-21 18:40 ` Richard Cochran 2012-09-21 19:01 ` Ben Hutchings 0 siblings, 1 reply; 15+ messages in thread From: Richard Cochran @ 2012-09-21 18:40 UTC (permalink / raw) To: Ben Hutchings Cc: Keller, Jacob E, netdev@vger.kernel.org, David Miller, Kirsher, Jeffrey T, John Stultz, Vick, Matthew On Fri, Sep 21, 2012 at 07:23:03PM +0100, Ben Hutchings wrote: > > I think what I'm still missing from you is some explanation of what the > 'clock name' is meant to be used as - a type name, a unique identifier, > a 'friendly name' for listing clocks in a user interface? The original idea was a type/friendly kind of thing. Imagine you are the admin of some random box that should run PTP. You do 'cat /sys/class/ptp/ptp0/clock_name' and see "gianfar clock". Then you think to yourself, "Excellent, I know that one, it works great, and I can even get a PPS output." But if you saw "IXP46X timer" then you would think, "Forget it, this will never work." That was the idea. But before the ethtool thing came along, people started putting MAC addresses in that string, and it continued even after the ethtool method appeared. I wanted to correct the MAC abuses, but then I thought you were concurring with putting some kind of semi-unique device ID there. I really don't care too much about the clock_name anyhow, because I always know my own hardware. I don't mind changing it from type-string or friendly-name to device ID, if people think that is more useful. Thanks, Richard ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next 3/3] ptp: derive the device name from the parent device 2012-09-21 18:40 ` Richard Cochran @ 2012-09-21 19:01 ` Ben Hutchings 2012-09-22 5:43 ` Richard Cochran 0 siblings, 1 reply; 15+ messages in thread From: Ben Hutchings @ 2012-09-21 19:01 UTC (permalink / raw) To: Richard Cochran Cc: Keller, Jacob E, netdev@vger.kernel.org, David Miller, Kirsher, Jeffrey T, John Stultz, Vick, Matthew On Fri, 2012-09-21 at 20:40 +0200, Richard Cochran wrote: > On Fri, Sep 21, 2012 at 07:23:03PM +0100, Ben Hutchings wrote: > > > > I think what I'm still missing from you is some explanation of what the > > 'clock name' is meant to be used as - a type name, a unique identifier, > > a 'friendly name' for listing clocks in a user interface? > > The original idea was a type/friendly kind of thing. > > Imagine you are the admin of some random box that should run PTP. You > do 'cat /sys/class/ptp/ptp0/clock_name' and see "gianfar clock". Then > you think to yourself, "Excellent, I know that one, it works great, > and I can even get a PPS output." > > But if you saw "IXP46X timer" then you would think, "Forget it, this > will never work." > > That was the idea. > > But before the ethtool thing came along, people started putting MAC > addresses in that string, and it continued even after the ethtool > method appeared. I wanted to correct the MAC abuses, but then I > thought you were concurring with putting some kind of semi-unique > device ID there. I was confused about whether it was actually meant to be unique. The ethtool command is useful but setting the parent device may be even more useful, e.g. you will be able to write udev rules for PHC devices based on the parent device's identity. > I really don't care too much about the clock_name anyhow, because I > always know my own hardware. I don't mind changing it from type-string > or friendly-name to device ID, if people think that is more useful. I don't mind either, just so long as the rule is either (1) implemented in the PTP core code or (2) made very clear to driver writers. The lack of a parent for the IXP device makes (1) hard to do well. Ben. -- Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next 3/3] ptp: derive the device name from the parent device 2012-09-21 19:01 ` Ben Hutchings @ 2012-09-22 5:43 ` Richard Cochran 2012-09-22 5:46 ` Richard Cochran 2012-09-24 22:57 ` Keller, Jacob E 0 siblings, 2 replies; 15+ messages in thread From: Richard Cochran @ 2012-09-22 5:43 UTC (permalink / raw) To: Ben Hutchings Cc: Keller, Jacob E, netdev@vger.kernel.org, David Miller, Kirsher, Jeffrey T, John Stultz, Vick, Matthew On Fri, Sep 21, 2012 at 08:01:46PM +0100, Ben Hutchings wrote: > > The ethtool command is useful but setting the parent device may be even > more useful, e.g. you will be able to write udev rules for PHC devices > based on the parent device's identity. Thinking about this a bit more, it makes no sense to put the parent device name into clock_name, because that information is redundant. # ls -l /sys/class/ptp/ptp0/ -r--r--r-- 1 root root 8192 Jan 1 00:00 clock_name -r--r--r-- 1 root root 8192 Jan 1 00:00 dev lrwxrwxrwx 1 root root 0 Jan 1 00:00 device -> ../../../fec-1:01 I agree that the parent device is useful, and I will add it. However, I will leave the clock_name as it is, adding a bit more prose to the ABI description. The one case where clock_name becomes really important is when you have a PHY clock, like in the above example, since it provides a way to see that the clock is *not* related to the MAC. Thanks, Richard ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next 3/3] ptp: derive the device name from the parent device 2012-09-22 5:43 ` Richard Cochran @ 2012-09-22 5:46 ` Richard Cochran 2012-09-24 22:57 ` Keller, Jacob E 1 sibling, 0 replies; 15+ messages in thread From: Richard Cochran @ 2012-09-22 5:46 UTC (permalink / raw) To: Ben Hutchings Cc: Keller, Jacob E, netdev@vger.kernel.org, David Miller, Kirsher, Jeffrey T, John Stultz, Vick, Matthew On Sat, Sep 22, 2012 at 07:43:46AM +0200, Richard Cochran wrote: > > The one case where clock_name becomes really important is when you > have a PHY clock, like in the above example, since it provides a way > to see that the clock is *not* related to the MAC. I mean that a PHY clock is not a part of the MAC. Of course it is related to the MAC by virtue of being connected to it via a MDIO bus. Thanks, Richard ^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH net-next 3/3] ptp: derive the device name from the parent device 2012-09-22 5:43 ` Richard Cochran 2012-09-22 5:46 ` Richard Cochran @ 2012-09-24 22:57 ` Keller, Jacob E 1 sibling, 0 replies; 15+ messages in thread From: Keller, Jacob E @ 2012-09-24 22:57 UTC (permalink / raw) To: Richard Cochran, Ben Hutchings Cc: netdev@vger.kernel.org, David Miller, Kirsher, Jeffrey T, John Stultz, Vick, Matthew > -----Original Message----- > From: Richard Cochran [mailto:richardcochran@gmail.com] > Sent: Friday, September 21, 2012 10:44 PM > To: Ben Hutchings > Cc: Keller, Jacob E; netdev@vger.kernel.org; David Miller; Kirsher, > Jeffrey T; John Stultz; Vick, Matthew > Subject: Re: [PATCH net-next 3/3] ptp: derive the device name from the > parent device > > On Fri, Sep 21, 2012 at 08:01:46PM +0100, Ben Hutchings wrote: > > > > The ethtool command is useful but setting the parent device may be > > even more useful, e.g. you will be able to write udev rules for PHC > > devices based on the parent device's identity. > > Thinking about this a bit more, it makes no sense to put the parent device > name into clock_name, because that information is redundant. > Agreed. > # ls -l /sys/class/ptp/ptp0/ > -r--r--r-- 1 root root 8192 Jan 1 00:00 clock_name > -r--r--r-- 1 root root 8192 Jan 1 00:00 dev > lrwxrwxrwx 1 root root 0 Jan 1 00:00 device -> ../../../fec-1:01 > > I agree that the parent device is useful, and I will add it. However, I > will leave the clock_name as it is, adding a bit more prose to the ABI > description. > > The one case where clock_name becomes really important is when you have a > PHY clock, like in the above example, since it provides a way to see that > the clock is *not* related to the MAC. > Agreed. Now that I understand your intent, and with the new parent and ethtool options, I will go ahead and change the ixgbe clock name. This makes more sense to me once I understood the intent. - Jake > Thanks, > Richard ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2012-09-24 22:57 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-09-21 17:00 [PATCH net-next 0/3] Two new PTP Hardware Clock features Richard Cochran 2012-09-21 17:00 ` [PATCH net-next 1/3] ptp: remember the adjusted frequency Richard Cochran 2012-09-21 17:00 ` [PATCH net-next 2/3] ptp: provide the clock's " Richard Cochran 2012-09-21 17:00 ` [PATCH net-next 3/3] ptp: derive the device name from the parent device Richard Cochran 2012-09-21 17:19 ` Ben Hutchings 2012-09-21 17:37 ` Richard Cochran 2012-09-21 17:39 ` Keller, Jacob E 2012-09-21 17:40 ` Ben Hutchings 2012-09-21 18:11 ` Richard Cochran 2012-09-21 18:23 ` Ben Hutchings 2012-09-21 18:40 ` Richard Cochran 2012-09-21 19:01 ` Ben Hutchings 2012-09-22 5:43 ` Richard Cochran 2012-09-22 5:46 ` Richard Cochran 2012-09-24 22:57 ` Keller, Jacob E
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).