>From e6a55411486de8a09b859e73140bf35c0ee36047 Mon Sep 17 00:00:00 2001
From: Dong Zhu <[email protected]>
Date: Sat, 11 May 2013 21:44:54 +0800
Subject: [PATCH] igb: add a method to get the nic hw time stamping policy
Currently kernel only support setting the hw time stamping policy
through ioctl,now add a method to check which packets(Outgoing and
Incoming) are time stamped by nic.
Signed-off-by: Dong Zhu <[email protected]>
---
drivers/net/ethernet/intel/igb/igb_ptp.c | 29 +++++++++++++++++++++++++++++
include/uapi/linux/net_tstamp.h | 4 +++-
2 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
index 7e8c477..8c06346 100644
--- a/drivers/net/ethernet/intel/igb/igb_ptp.c
+++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
@@ -577,6 +577,34 @@ int igb_ptp_hwtstamp_ioctl(struct net_device *netdev,
if (config.flags)
return -EINVAL;
+ if (config.rw == 0)
+ goto set_policy;
+ else if (config.rw == 1) {
+ if (config.tx_type || config.rx_filter)
+ return -EINVAL;
+
+ regval = rd32(E1000_TSYNCTXCTL);
+
+ if (regval & E1000_TSYNCTXCTL_ENABLED)
+ config.tx_type = HWTSTAMP_TX_ON;
+ else
+ config.tx_type = HWTSTAMP_TX_OFF;
+
+ regval = rd32(E1000_TSYNCRXCTL);
+
+ if (!(regval & E1000_TSYNCRXCTL_ENABLED))
+ config.rx_filter = HWTSTAMP_FILTER_NONE;
+ else if (E1000_TSYNCRXCTL_TYPE_ALL ==
+ (regval & E1000_TSYNCRXCTL_TYPE_MASK))
+ config.rx_filter = HWTSTAMP_FILTER_ALL;
+ else
+ return -ERANGE;
+
+ goto end;
+ } else
+ return -EINVAL;
+
+set_policy:
switch (config.tx_type) {
case HWTSTAMP_TX_OFF:
tsync_tx_ctl = 0;
@@ -707,6 +735,7 @@ int igb_ptp_hwtstamp_ioctl(struct net_device *netdev,
regval = rd32(E1000_RXSTMPL);
regval = rd32(E1000_RXSTMPH);
+end:
return copy_to_user(ifr->ifr_data, &config, sizeof(config)) ?
-EFAULT : 0;
}
diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h
index ae5df12..77147da 100644
--- a/include/uapi/linux/net_tstamp.h
+++ b/include/uapi/linux/net_tstamp.h
@@ -28,9 +28,10 @@ enum {
/**
* struct hwtstamp_config - %SIOCSHWTSTAMP parameter
*
+ * @rw: 0/1 represents set/get the hw time stamp policy
* @flags: no flags defined right now, must be zero
* @tx_type: one of HWTSTAMP_TX_*
- * @rx_type: one of one of HWTSTAMP_FILTER_*
+ * @rx_filter: one of one of HWTSTAMP_FILTER_*
*
* %SIOCSHWTSTAMP expects a &struct ifreq with a ifr_data pointer to
* this structure. dev_ifsioc() in the kernel takes care of the
@@ -39,6 +40,7 @@ enum {
* 32 and 64 bit systems, don't break this!
*/
struct hwtstamp_config {
+ int rw;
int flags;
int tx_type;
int rx_filter;
--
1.7.11.7
--
Best Regards,
Dong Zhu
On Sat, May 11, 2013 at 10:02:19PM +0800, Dong Zhu wrote:
>
> Currently kernel only support setting the hw time stamping policy
> through ioctl,now add a method to check which packets(Outgoing and
> Incoming) are time stamped by nic.
I don't really see a use case here. Applications needing time stamping
should just set the policy that they need.
> struct hwtstamp_config {
> + int rw;
Also, you can't just go adding fields to an ioctl.
> int flags;
> int tx_type;
> int rx_filter;
> --
Thanks,
Richard
Hello.
On 11-05-2013 18:02, Dong Zhu wrote:
> From e6a55411486de8a09b859e73140bf35c0ee36047 Mon Sep 17 00:00:00 2001
> From: Dong Zhu <[email protected]>
> Date: Sat, 11 May 2013 21:44:54 +0800
> Subject: [PATCH] igb: add a method to get the nic hw time stamping policy
Please, don't send this header with your patches.
> Currently kernel only support setting the hw time stamping policy
> through ioctl,now add a method to check which packets(Outgoing and
> Incoming) are time stamped by nic.
> Signed-off-by: Dong Zhu <[email protected]>
> ---
> drivers/net/ethernet/intel/igb/igb_ptp.c | 29 +++++++++++++++++++++++++++++
> include/uapi/linux/net_tstamp.h | 4 +++-
> 2 files changed, 32 insertions(+), 1 deletion(-)
> diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
> index 7e8c477..8c06346 100644
> --- a/drivers/net/ethernet/intel/igb/igb_ptp.c
> +++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
> @@ -577,6 +577,34 @@ int igb_ptp_hwtstamp_ioctl(struct net_device *netdev,
> if (config.flags)
> return -EINVAL;
>
> + if (config.rw == 0)
> + goto set_policy;
> + else if (config.rw == 1) {
Both arms of the *if* statement should have {} if one does,
according to Documentation/CodingStyle.
> + if (config.tx_type || config.rx_filter)
> + return -EINVAL;
> +
> + regval = rd32(E1000_TSYNCTXCTL);
> +
Empty line not needed here.
> + if (regval & E1000_TSYNCTXCTL_ENABLED)
> + config.tx_type = HWTSTAMP_TX_ON;
> + else
> + config.tx_type = HWTSTAMP_TX_OFF;
> +
> + regval = rd32(E1000_TSYNCRXCTL);
> +
... and here.
> + if (!(regval & E1000_TSYNCRXCTL_ENABLED))
> + config.rx_filter = HWTSTAMP_FILTER_NONE;
> + else if (E1000_TSYNCRXCTL_TYPE_ALL ==
> + (regval & E1000_TSYNCRXCTL_TYPE_MASK))
> + config.rx_filter = HWTSTAMP_FILTER_ALL;
> + else
> + return -ERANGE;
> +
> + goto end;
> + } else
> + return -EINVAL;
Same comment about missing {}. This *if*, however, asks to be
converted to *switch*...
> diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h
> index ae5df12..77147da 100644
> --- a/include/uapi/linux/net_tstamp.h
> +++ b/include/uapi/linux/net_tstamp.h
> @@ -28,9 +28,10 @@ enum {
> /**
> * struct hwtstamp_config - %SIOCSHWTSTAMP parameter
> *
> + * @rw: 0/1 represents set/get the hw time stamp policy
> * @flags: no flags defined right now, must be zero
> * @tx_type: one of HWTSTAMP_TX_*
> - * @rx_type: one of one of HWTSTAMP_FILTER_*
> + * @rx_filter: one of one of HWTSTAMP_FILTER_*
You don't mention this change in the changelog. Most probably it
should be in a separate patch.
> @@ -39,6 +40,7 @@ enum {
> * 32 and 64 bit systems, don't break this!
> */
> struct hwtstamp_config {
> + int rw;
Why not 'bool'?
WBR, Sergei
On Sat, May 11, 2013 at 05:31:43PM +0200, Richard Cochran wrote:
> On Sat, May 11, 2013 at 10:02:19PM +0800, Dong Zhu wrote:
> >
> > Currently kernel only support setting the hw time stamping policy
> > through ioctl,now add a method to check which packets(Outgoing and
> > Incoming) are time stamped by nic.
>
> I don't really see a use case here. Applications needing time stamping
> should just set the policy that they need.
I think it is necessary to check which type of packets are stamped
by nic.
For I350 (igb), it only support HWTSTAMP_FILTER_NONE and
HWTSTAMP_FILTER_ALL of outgoing packets.For 82599EB(ixgbe), we can
check more.
I add a new member of hwtstamp_config to judge the ioctl request is read
or write, we can specify the rw in the userspace using hwstamp_ctl
application to get the time stamped info.
--
Best Regards,
Dong Zhu
Thanks for your pointing out my mistakes of CodingStyle.
> > struct hwtstamp_config {
> >+ int rw;
My initial idea was that the type of rw should be enum like tx_type, but I am
not sure whther it is necessary to define a new enum, if this patch could
be accpeted I will ask someone about the rw. At that time I will change
the type of rw to bool or define a new enum, then convert the if to
switch if necessary.
Patch after modifying:
>From cf337e7863af66428554785c32a9840fafaa3492 Mon Sep 17 00:00:00 2001
From: Dong Zhu <[email protected]>
Date: Sun, 12 May 2013 21:57:57 +0800
Currently kernel only support setting the hw time stamping policy
through ioctl,now add a method to check which packets(Outgoing and
Incoming) are time stamped by nic.
Signed-off-by: Dong Zhu <[email protected]>
---
drivers/net/ethernet/intel/igb/igb_ptp.c | 28 ++++++++++++++++++++++++++++
include/uapi/linux/net_tstamp.h | 4 +++-
2 files changed, 31 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
index 7e8c477..4ea091d 100644
--- a/drivers/net/ethernet/intel/igb/igb_ptp.c
+++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
@@ -577,6 +577,33 @@ int igb_ptp_hwtstamp_ioctl(struct net_device *netdev,
if (config.flags)
return -EINVAL;
+ if (config.rw == 0) {
+ goto set_policy;
+ } else if (config.rw == 1) {
+ if (config.tx_type || config.rx_filter)
+ return -EINVAL;
+
+ regval = rd32(E1000_TSYNCTXCTL);
+ if (regval & E1000_TSYNCTXCTL_ENABLED)
+ config.tx_type = HWTSTAMP_TX_ON;
+ else
+ config.tx_type = HWTSTAMP_TX_OFF;
+
+ regval = rd32(E1000_TSYNCRXCTL);
+ if (!(regval & E1000_TSYNCRXCTL_ENABLED))
+ config.rx_filter = HWTSTAMP_FILTER_NONE;
+ else if (E1000_TSYNCRXCTL_TYPE_ALL ==
+ (regval & E1000_TSYNCRXCTL_TYPE_MASK))
+ config.rx_filter = HWTSTAMP_FILTER_ALL;
+ else
+ return -ERANGE;
+
+ goto end;
+ } else {
+ return -EINVAL;
+ }
+
+set_policy:
switch (config.tx_type) {
case HWTSTAMP_TX_OFF:
tsync_tx_ctl = 0;
@@ -707,6 +734,7 @@ int igb_ptp_hwtstamp_ioctl(struct net_device *netdev,
regval = rd32(E1000_RXSTMPL);
regval = rd32(E1000_RXSTMPH);
+end:
return copy_to_user(ifr->ifr_data, &config, sizeof(config)) ?
-EFAULT : 0;
}
diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h
index ae5df12..77147da 100644
--- a/include/uapi/linux/net_tstamp.h
+++ b/include/uapi/linux/net_tstamp.h
@@ -28,9 +28,10 @@ enum {
/**
* struct hwtstamp_config - %SIOCSHWTSTAMP parameter
*
+ * @rw: 0/1 represents set/get the hw time stamp policy
* @flags: no flags defined right now, must be zero
* @tx_type: one of HWTSTAMP_TX_*
- * @rx_type: one of one of HWTSTAMP_FILTER_*
+ * @rx_filter: one of one of HWTSTAMP_FILTER_*
*
* %SIOCSHWTSTAMP expects a &struct ifreq with a ifr_data pointer to
* this structure. dev_ifsioc() in the kernel takes care of the
@@ -39,6 +40,7 @@ enum {
* 32 and 64 bit systems, don't break this!
*/
struct hwtstamp_config {
+ int rw;
int flags;
int tx_type;
int rx_filter;
--
1.7.11.7
--
Best Regards,
Dong Zhu
On Sun, May 12, 2013 at 10:25:55PM +0800, Dong Zhu wrote:
> Thanks for your pointing out my mistakes of CodingStyle.
>
> > > struct hwtstamp_config {
> > >+ int rw;
>
> My initial idea was that the type of rw should be enum like tx_type, but I am
> not sure whther it is necessary to define a new enum, if this patch could
> be accpeted I will ask someone about the rw. At that time I will change
> the type of rw to bool or define a new enum, then convert the if to
> switch if necessary.
You cannot add any new field at all. That would break a userland ABI.
Thanks,
Richard
On Sun, May 12, 2013 at 07:24:46PM +0200, Richard Cochran wrote:
> On Sun, May 12, 2013 at 10:25:55PM +0800, Dong Zhu wrote:
> > Thanks for your pointing out my mistakes of CodingStyle.
> >
> > > > struct hwtstamp_config {
> > > >+ int rw;
> >
> > My initial idea was that the type of rw should be enum like tx_type, but I am
> > not sure whther it is necessary to define a new enum, if this patch could
> > be accpeted I will ask someone about the rw. At that time I will change
> > the type of rw to bool or define a new enum, then convert the if to
> > switch if necessary.
>
> You cannot add any new field at all. That would break a userland ABI.
>
Thanks for your info Richard.
Can I use the 'flags' which members of hwtstamp_config to judge the
ioctl request instead ? If not could you plz give me a new way to
resolve this issue ?
Thanks a lot !
--
Best Regards,
Dong Zhu
On Mon, May 13, 2013 at 10:12:36AM +0800, Dong Zhu wrote:
>
> Can I use the 'flags' which members of hwtstamp_config to judge the
> ioctl request instead ? If not could you plz give me a new way to
> resolve this issue ?
You could use the flags field, as it has no definition yet.
But you still need to explain why this new functionality is needed in
the first place:
- You can query an interface's time stamping capabilities with the
GET_TS_INFO ethtool command.
- You can choose a setting with SIOCSHWTSTAMP.
What more do you need?
Thanks,
Richard
> You could use the flags field, as it has no definition yet.
>
> But you still need to explain why this new functionality is needed in
> the first place:
>
> - You can query an interface's time stamping capabilities with the
> GET_TS_INFO ethtool command.
>
Hi,
I modified this patch and added the method to igb_get_ts_info function.
For 82576 nic, through this method we can easily check which type of packets
are time stamped now, such as (HWTSTAMP_FILTER_PTP_V1_L4_SYNC and
HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ), then change or disable it up to your requests.
I think it is convenient.The origial GET_TS_INFO method can only show the device’s
time stamping capabilities which nics supported.
I use the tx_reserved[0] and rx_reserved[0] to restore the hwtstamp_tx_types and
hwtstamp_rx_filters enumeration values.
Due to the limitation of 80 characters one line, I have to move the
switch out of else judegment.
I test it on I350 and 82576NS nics and it works as expect.
Could help reviewing it again ? Any comments would be appreciated.
>From 8a12932fd2a3bb5ca904bc72b20140247a5d81be Mon Sep 17 00:00:00 2001
From: Dong Zhu <[email protected]>
Date: Mon, 13 May 2013 17:27:59 +0800
Currently kernel only support setting the hw time stamping policy
through ioctl,now add a method to check which packets(Outgoing and
Incoming) are time stamped by nic.
Add this to igb_get_ts_info, we can query this by using the GET_TS_INFO
ethtool command. Testing on I350 and 82576NS it seems work well.
Signed-off-by: Dong Zhu <[email protected]>
---
drivers/net/ethernet/intel/igb/igb_ethtool.c | 78 +++++++++++++++++++++++++++-
include/uapi/linux/ethtool.h | 3 ++
2 files changed, 79 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index 7876240..49486b8 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
@@ -2327,6 +2327,8 @@ static int igb_get_ts_info(struct net_device *dev,
struct ethtool_ts_info *info)
{
struct igb_adapter *adapter = netdev_priv(dev);
+ struct e1000_hw *hw = &adapter->hw;
+ u32 regval;
switch (adapter->hw.mac.type) {
case e1000_82575:
@@ -2360,10 +2362,29 @@ static int igb_get_ts_info(struct net_device *dev,
info->rx_filters = 1 << HWTSTAMP_FILTER_NONE;
+ regval = rd32(E1000_TSYNCTXCTL);
+ if (regval & E1000_TSYNCTXCTL_ENABLED)
+ info->tx_reserved[0] = 1 << HWTSTAMP_TX_ON;
+ else
+ info->tx_reserved[0] = 1 << HWTSTAMP_TX_OFF;
+
+ regval = rd32(E1000_TSYNCRXCTL);
+
/* 82576 does not support timestamping all packets. */
- if (adapter->hw.mac.type >= e1000_82580)
+ if (adapter->hw.mac.type >= e1000_82580) {
info->rx_filters |= 1 << HWTSTAMP_FILTER_ALL;
- else
+
+ if (!(regval & E1000_TSYNCRXCTL_ENABLED))
+ info->rx_reserved[0] =
+ 1 << HWTSTAMP_FILTER_NONE;
+ else if (E1000_TSYNCRXCTL_TYPE_ALL ==
+ (regval & E1000_TSYNCRXCTL_TYPE_MASK))
+ info->rx_reserved[0] = 1 << HWTSTAMP_FILTER_ALL;
+ else
+ return -ERANGE;
+
+ return 0;
+ } else {
info->rx_filters |=
(1 << HWTSTAMP_FILTER_PTP_V1_L4_SYNC) |
(1 << HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ) |
@@ -2373,6 +2394,59 @@ static int igb_get_ts_info(struct net_device *dev,
(1 << HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ) |
(1 << HWTSTAMP_FILTER_PTP_V2_EVENT);
+ if (!(regval & E1000_TSYNCRXCTL_ENABLED)) {
+ info->rx_reserved[0] =
+ 1 << HWTSTAMP_FILTER_NONE;
+ return 0;
+ }
+ }
+
+ switch (regval & E1000_TSYNCRXCTL_TYPE_MASK) {
+ case E1000_TSYNCRXCTL_TYPE_L4_V1:
+ regval = rd32(E1000_TSYNCRXCFG);
+ if (E1000_TSYNCRXCFG_PTP_V1_SYNC_MESSAGE ==
+ (regval &
+ E1000_TSYNCRXCFG_PTP_V1_CTRLT_MASK))
+ info->rx_reserved[0] =
+ (1 << HWTSTAMP_FILTER_PTP_V1_L4_SYNC);
+ else if (E1000_TSYNCRXCFG_PTP_V1_DELAY_REQ_MESSAGE
+ == (regval &
+ E1000_TSYNCRXCFG_PTP_V1_CTRLT_MASK))
+ info->rx_reserved[0] =
+ (1 <<
+ HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ);
+ else
+ return -ERANGE;
+ break;
+ case E1000_TSYNCRXCTL_TYPE_L2_L4_V2:
+ regval = rd32(E1000_TSYNCRXCFG);
+ if (E1000_TSYNCRXCFG_PTP_V2_SYNC_MESSAGE ==
+ (regval &
+ E1000_TSYNCRXCFG_PTP_V2_MSGID_MASK))
+ info->rx_reserved[0] =
+ (1 << HWTSTAMP_FILTER_PTP_V2_L2_SYNC) |
+ (1 << HWTSTAMP_FILTER_PTP_V2_L4_SYNC);
+ else if (E1000_TSYNCRXCFG_PTP_V2_DELAY_REQ_MESSAGE ==
+ (regval &
+ E1000_TSYNCRXCFG_PTP_V2_MSGID_MASK))
+ info->rx_reserved[0] =
+ (1 <<
+ HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ) |
+ (1 <<
+ HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ);
+ else
+ return -ERANGE;
+ break;
+ case E1000_TSYNCRXCTL_TYPE_EVENT_V2:
+ info->rx_reserved[0] =
+ (1 << HWTSTAMP_FILTER_PTP_V2_EVENT) |
+ (1 << HWTSTAMP_FILTER_PTP_V2_SYNC) |
+ (1 << HWTSTAMP_FILTER_PTP_V2_DELAY_REQ);
+ break;
+ default:
+ return -ERANGE;
+ }
+
return 0;
default:
return -EOPNOTSUPP;
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 0c9b448..06cdbc0 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -772,7 +772,10 @@ struct ethtool_sfeatures {
* @so_timestamping: bit mask of the sum of the supported SO_TIMESTAMPING flags
* @phc_index: device index of the associated PHC, or -1 if there is none
* @tx_types: bit mask of the supported hwtstamp_tx_types enumeration values
+ * @tx_reserved[0]: bit mask of the in use hwtstamp_tx_types enumeration values
* @rx_filters: bit mask of the supported hwtstamp_rx_filters enumeration values
+ * @rx_reserved[0]: bit mask of the in use hwtstamp_rx_filters enumeration
+ * values
*
* The bits in the 'tx_types' and 'rx_filters' fields correspond to
* the 'hwtstamp_tx_types' and 'hwtstamp_rx_filters' enumeration values,
--
1.7.11.7
--
Best Regards,
Dong Zhu
On 5/13/13 3:07 AM, "Dong Zhu" <[email protected]> wrote:
>> You could use the flags field, as it has no definition yet.
>>
>> But you still need to explain why this new functionality is needed in
>> the first place:
>>
>> - You can query an interface's time stamping capabilities with the
>> GET_TS_INFO ethtool command.
>>
>
>Hi,
>
>I modified this patch and added the method to igb_get_ts_info function.
>For 82576 nic, through this method we can easily check which type of
>packets
>are time stamped now, such as (HWTSTAMP_FILTER_PTP_V1_L4_SYNC and
>HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ), then change or disable it up to
>your requests.
>I think it is convenient.The origial GET_TS_INFO method can only show the
>device?s
>time stamping capabilities which nics supported.
>
>I use the tx_reserved[0] and rx_reserved[0] to restore the
>hwtstamp_tx_types and
>hwtstamp_rx_filters enumeration values.
>
>Due to the limitation of 80 characters one line, I have to move the
>switch out of else judegment.
>
>I test it on I350 and 82576NS nics and it works as expect.
>
>Could help reviewing it again ? Any comments would be appreciated.
>
>
>From 8a12932fd2a3bb5ca904bc72b20140247a5d81be Mon Sep 17 00:00:00 2001
>From: Dong Zhu <[email protected]>
>Date: Mon, 13 May 2013 17:27:59 +0800
>
>Currently kernel only support setting the hw time stamping policy
>through ioctl,now add a method to check which packets(Outgoing and
>Incoming) are time stamped by nic.
>
>Add this to igb_get_ts_info, we can query this by using the GET_TS_INFO
>ethtool command. Testing on I350 and 82576NS it seems work well.
>
>Signed-off-by: Dong Zhu <[email protected]>
>---
> drivers/net/ethernet/intel/igb/igb_ethtool.c | 78
>+++++++++++++++++++++++++++-
> include/uapi/linux/ethtool.h | 3 ++
> 2 files changed, 79 insertions(+), 2 deletions(-)
[...]
>diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
>index 0c9b448..06cdbc0 100644
>--- a/include/uapi/linux/ethtool.h
>+++ b/include/uapi/linux/ethtool.h
>@@ -772,7 +772,10 @@ struct ethtool_sfeatures {
> * @so_timestamping: bit mask of the sum of the supported
>SO_TIMESTAMPING flags
> * @phc_index: device index of the associated PHC, or -1 if there is none
> * @tx_types: bit mask of the supported hwtstamp_tx_types enumeration
>values
>+ * @tx_reserved[0]: bit mask of the in use hwtstamp_tx_types enumeration
>values
> * @rx_filters: bit mask of the supported hwtstamp_rx_filters
>enumeration values
>+ * @rx_reserved[0]: bit mask of the in use hwtstamp_rx_filters
>enumeration
>+ * values
> *
> * The bits in the 'tx_types' and 'rx_filters' fields correspond to
> * the 'hwtstamp_tx_types' and 'hwtstamp_rx_filters' enumeration values,
>--
>1.7.11.7
>
>
>--
>Best Regards,
>Dong Zhu
CCing Ben Hutchings, since now your patch is affecting ethtool.
Dong,
The drivers should already be updating the config.tx_type and
config.rx_filter with what the driver is configuring the device for, so
any userspace application that needs to know what the device is actually
configured for can check those values after the ioctl call. If the
userspace app needs to know for certain that something is going to be
handled, it should just call the ioctl (as Richard suggested).
I'm not sure what you mean by "checking more" with the 82599EB in ixgbe.
It is permissible to timestamp additional packets, as long as the user
gets the packets they requested timestamped. This in mind, it's much more
efficient for the I350 to timestamp all receive packets and prevents other
headaches (for example, failure to timestamp if the device receives two
packets that should be timestamped at once). Because of this, the 82599EB
may have more individual filters, but the driver will be upscaling any
user input request to HWTSTAMP_FILTER_ALL on I350.
Cheers,
Matthew
On Mon, 2013-05-13 at 15:48 +0000, Vick, Matthew wrote:
> On 5/13/13 3:07 AM, "Dong Zhu" <[email protected]> wrote:
[...]
> >From 8a12932fd2a3bb5ca904bc72b20140247a5d81be Mon Sep 17 00:00:00 2001
> >From: Dong Zhu <[email protected]>
> >Date: Mon, 13 May 2013 17:27:59 +0800
> >
> >Currently kernel only support setting the hw time stamping policy
> >through ioctl,now add a method to check which packets(Outgoing and
> >Incoming) are time stamped by nic.
> >
> >Add this to igb_get_ts_info, we can query this by using the GET_TS_INFO
> >ethtool command. Testing on I350 and 82576NS it seems work well.
> >
> >Signed-off-by: Dong Zhu <[email protected]>
> >---
> > drivers/net/ethernet/intel/igb/igb_ethtool.c | 78
> >+++++++++++++++++++++++++++-
> > include/uapi/linux/ethtool.h | 3 ++
> > 2 files changed, 79 insertions(+), 2 deletions(-)
>
> [...]
>
> >diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> >index 0c9b448..06cdbc0 100644
> >--- a/include/uapi/linux/ethtool.h
> >+++ b/include/uapi/linux/ethtool.h
> >@@ -772,7 +772,10 @@ struct ethtool_sfeatures {
> > * @so_timestamping: bit mask of the sum of the supported
> >SO_TIMESTAMPING flags
> > * @phc_index: device index of the associated PHC, or -1 if there is none
> > * @tx_types: bit mask of the supported hwtstamp_tx_types enumeration
> >values
> >+ * @tx_reserved[0]: bit mask of the in use hwtstamp_tx_types enumeration
> >values
> > * @rx_filters: bit mask of the supported hwtstamp_rx_filters
> >enumeration values
> >+ * @rx_reserved[0]: bit mask of the in use hwtstamp_rx_filters
> >enumeration
> >+ * values
Why would we keep calling a field 'reserved' once it's been given a
specific meaning?
> > * The bits in the 'tx_types' and 'rx_filters' fields correspond to
> > * the 'hwtstamp_tx_types' and 'hwtstamp_rx_filters' enumeration values,
> >--
> >1.7.11.7
> >
> >
> >--
> >Best Regards,
> >Dong Zhu
>
> CCing Ben Hutchings, since now your patch is affecting ethtool.
[...]
Thanks, Matthew.
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.
> -----Original Message-----
> From: Dong Zhu [mailto:[email protected]]
> Sent: Monday, May 13, 2013 3:08 AM
> To: Richard Cochran
> Cc: Sergei Shtylyov; Kirsher, Jeffrey T; Brandeburg, Jesse; Allan, Bruce
> W; Wyborny, Carolyn; Skidmore, Donald C; Rose, Gregory V; Waskiewicz
> Jr, Peter P; Duyck, Alexander H; Ronciak, John; Dave, Tushar N; Vick,
> Matthew; Keller, Jacob E; Paul E. McKenney; David Howells; Dave Jones;
> Thomas Gleixner; [email protected]; e1000-
> [email protected]; [email protected]
> Subject: Re: [PATCH] igb: add a method to get the nic hw time stamping
> policy
>
> > You could use the flags field, as it has no definition yet.
> >
> > But you still need to explain why this new functionality is needed in
> > the first place:
> >
> > - You can query an interface's time stamping capabilities with the
> > GET_TS_INFO ethtool command.
> >
>
> Hi,
>
> I modified this patch and added the method to igb_get_ts_info function.
> For 82576 nic, through this method we can easily check which type of
> packets
> are time stamped now, such as (HWTSTAMP_FILTER_PTP_V1_L4_SYNC
> and
> HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ), then change or disable it
> up to your requests.
> I think it is convenient.The origial GET_TS_INFO method can only show
> the device’s
> time stamping capabilities which nics supported.
>
> I use the tx_reserved[0] and rx_reserved[0] to restore the
> hwtstamp_tx_types and
> hwtstamp_rx_filters enumeration values.
>
> Due to the limitation of 80 characters one line, I have to move the
> switch out of else judegment.
>
> I test it on I350 and 82576NS nics and it works as expect.
>
> Could help reviewing it again ? Any comments would be appreciated.
>
>
> From 8a12932fd2a3bb5ca904bc72b20140247a5d81be Mon Sep 17
> 00:00:00 2001
> From: Dong Zhu <[email protected]>
> Date: Mon, 13 May 2013 17:27:59 +0800
>
> Currently kernel only support setting the hw time stamping policy
> through ioctl,now add a method to check which packets(Outgoing and
> Incoming) are time stamped by nic.
>
> Add this to igb_get_ts_info, we can query this by using the GET_TS_INFO
> ethtool command. Testing on I350 and 82576NS it seems work well.
>
> Signed-off-by: Dong Zhu <[email protected]>
> ---
> drivers/net/ethernet/intel/igb/igb_ethtool.c | 78
> +++++++++++++++++++++++++++-
> include/uapi/linux/ethtool.h | 3 ++
> 2 files changed, 79 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c
> b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> index 7876240..49486b8 100644
> --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
> +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> @@ -2327,6 +2327,8 @@ static int igb_get_ts_info(struct net_device
> *dev,
> struct ethtool_ts_info *info)
> {
> struct igb_adapter *adapter = netdev_priv(dev);
> + struct e1000_hw *hw = &adapter->hw;
> + u32 regval;
>
> switch (adapter->hw.mac.type) {
> case e1000_82575:
> @@ -2360,10 +2362,29 @@ static int igb_get_ts_info(struct net_device
> *dev,
>
> info->rx_filters = 1 << HWTSTAMP_FILTER_NONE;
>
> + regval = rd32(E1000_TSYNCTXCTL);
> + if (regval & E1000_TSYNCTXCTL_ENABLED)
> + info->tx_reserved[0] = 1 << HWTSTAMP_TX_ON;
> + else
> + info->tx_reserved[0] = 1 << HWTSTAMP_TX_OFF;
> +
> + regval = rd32(E1000_TSYNCRXCTL);
> +
> /* 82576 does not support timestamping all packets. */
> - if (adapter->hw.mac.type >= e1000_82580)
> + if (adapter->hw.mac.type >= e1000_82580) {
> info->rx_filters |= 1 << HWTSTAMP_FILTER_ALL;
> - else
> +
> + if (!(regval & E1000_TSYNCRXCTL_ENABLED))
> + info->rx_reserved[0] =
> + 1 << HWTSTAMP_FILTER_NONE;
> + else if (E1000_TSYNCRXCTL_TYPE_ALL ==
> + (regval &
> E1000_TSYNCRXCTL_TYPE_MASK))
> + info->rx_reserved[0] = 1 <<
> HWTSTAMP_FILTER_ALL;
> + else
> + return -ERANGE;
> +
> + return 0;
> + } else {
> info->rx_filters |=
> (1 <<
> HWTSTAMP_FILTER_PTP_V1_L4_SYNC) |
> (1 <<
> HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ) |
> @@ -2373,6 +2394,59 @@ static int igb_get_ts_info(struct net_device
> *dev,
> (1 <<
> HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ) |
> (1 <<
> HWTSTAMP_FILTER_PTP_V2_EVENT);
>
> + if (!(regval & E1000_TSYNCRXCTL_ENABLED)) {
> + info->rx_reserved[0] =
> + 1 << HWTSTAMP_FILTER_NONE;
> + return 0;
> + }
> + }
> +
> + switch (regval & E1000_TSYNCRXCTL_TYPE_MASK) {
> + case E1000_TSYNCRXCTL_TYPE_L4_V1:
> + regval = rd32(E1000_TSYNCRXCFG);
> + if (E1000_TSYNCRXCFG_PTP_V1_SYNC_MESSAGE
> ==
> + (regval &
> +
> E1000_TSYNCRXCFG_PTP_V1_CTRLT_MASK))
> + info->rx_reserved[0] =
> + (1 <<
> HWTSTAMP_FILTER_PTP_V1_L4_SYNC);
> + else if
> (E1000_TSYNCRXCFG_PTP_V1_DELAY_REQ_MESSAGE
> + == (regval &
> +
> E1000_TSYNCRXCFG_PTP_V1_CTRLT_MASK))
> + info->rx_reserved[0] =
> + (1 <<
> +
> HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ);
> + else
> + return -ERANGE;
> + break;
> + case E1000_TSYNCRXCTL_TYPE_L2_L4_V2:
> + regval = rd32(E1000_TSYNCRXCFG);
> + if (E1000_TSYNCRXCFG_PTP_V2_SYNC_MESSAGE
> ==
> + (regval &
> +
> E1000_TSYNCRXCFG_PTP_V2_MSGID_MASK))
> + info->rx_reserved[0] =
> + (1 <<
> HWTSTAMP_FILTER_PTP_V2_L2_SYNC) |
> + (1 <<
> HWTSTAMP_FILTER_PTP_V2_L4_SYNC);
> + else if
> (E1000_TSYNCRXCFG_PTP_V2_DELAY_REQ_MESSAGE ==
> + (regval &
> +
> E1000_TSYNCRXCFG_PTP_V2_MSGID_MASK))
> + info->rx_reserved[0] =
> + (1 <<
> +
> HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ) |
> + (1 <<
> +
> HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ);
> + else
> + return -ERANGE;
> + break;
> + case E1000_TSYNCRXCTL_TYPE_EVENT_V2:
> + info->rx_reserved[0] =
> + (1 <<
> HWTSTAMP_FILTER_PTP_V2_EVENT) |
> + (1 << HWTSTAMP_FILTER_PTP_V2_SYNC)
> |
> + (1 <<
> HWTSTAMP_FILTER_PTP_V2_DELAY_REQ);
> + break;
> + default:
> + return -ERANGE;
> + }
> +
> return 0;
> default:
> return -EOPNOTSUPP;
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index 0c9b448..06cdbc0 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -772,7 +772,10 @@ struct ethtool_sfeatures {
> * @so_timestamping: bit mask of the sum of the supported
> SO_TIMESTAMPING flags
> * @phc_index: device index of the associated PHC, or -1 if there is
> none
> * @tx_types: bit mask of the supported hwtstamp_tx_types
> enumeration values
> + * @tx_reserved[0]: bit mask of the in use hwtstamp_tx_types
> enumeration values
> * @rx_filters: bit mask of the supported hwtstamp_rx_filters
> enumeration values
> + * @rx_reserved[0]: bit mask of the in use hwtstamp_rx_filters
> enumeration
> + * values
> *
> * The bits in the 'tx_types' and 'rx_filters' fields correspond to
> * the 'hwtstamp_tx_types' and 'hwtstamp_rx_filters' enumeration
> values,
> --
> 1.7.11.7
>
>
> --
> Best Regards,
> Dong Zhu
Yes. Matthew has it right. We already set the value via the hwtstamp_ioctl, and we set the actual value that we are timestamping. The ioctl will return at least what you requested, or an error. You can check the results of the ioctl by checking the hwtstamp_config structure after calling the ioctl and make sure you got what you want.
You can't modify the userspace ABI because that would break old applications, and cause even more headache for PTP application developers. Furthermore, you haven't sufficiently explained a use case that requires the ability to check without setting.
You can already query if the driver is capable of timestamping something, and can set and then query to ensure that you got at least what you requested... You need to explain why you need a new use case if you want to expose another method for accessing the information.
- Jake
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
Hi,
> I modified this patch and added the method to igb_get_ts_info function.
> For 82576 nic, through this method we can easily check which type of packets
> are time stamped now, such as (HWTSTAMP_FILTER_PTP_V1_L4_SYNC and
> HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ), then change or disable it up to your requests.
> I think it is convenient.The origial GET_TS_INFO method can only show the device’s
> time stamping capabilities which nics supported.
Through this method we can easily check which type of packets are
timestamped currently, it is useful because that we use the hwstamp_ctl
set time stamping policy first then we could verify what type of packets
does the nic timestamp and then do some other testing(regression) for each
policy.No matter PTP is running or not we can make sure whether timestamp packets
could cause other general network problems through viewing and setting the
timestamp policy.
My original idea is that:
The implementation of this method is calling the ioctl call, in order
not to break the userland ABI I use the flags field of hwtstamp_config,
as it has no definition yet.
- For I350 it only supports two types HWTSTAMP_FILTER_NONE and HWTSTAMP_FILTER_ALL
for timestamping. It is easy to handle just return the value to flags.
- For 82576NS it has more individual filters, so I need to do the
judement in the igb_ptp_hwtstamp_ioctl function for different nics,
the code of judgement might be reduplicated with igb_get_ts_info,then do
the OR operation for filters then return to the flags.
Could tell me whether the above method(ioctl) is feasible and better than this
one (ethtool)?
IMO checking this through the ethtool GET_TS_INFO is so convenient
without breaking any userland ABIs. Do you think so ?
>
> I use the tx_reserved[0] and rx_reserved[0] to restore the hwtstamp_tx_types and
> hwtstamp_rx_filters enumeration values.
>
> I test it on I350 and 82576NS nics and it works as expect.
>
> Could help reviewing it again ? Any comments would be appreciated.
>
>
> From 8a12932fd2a3bb5ca904bc72b20140247a5d81be Mon Sep 17 00:00:00 2001
> From: Dong Zhu <[email protected]>
> Date: Mon, 13 May 2013 17:27:59 +0800
>
> Currently kernel only support setting the hw time stamping policy
> through ioctl,now add a method to check which packets(Outgoing and
> Incoming) are time stamped by nic.
>
> Add this to igb_get_ts_info, we can query this by using the GET_TS_INFO
> ethtool command. Testing on I350 and 82576NS it seems work well.
>
> Signed-off-by: Dong Zhu <[email protected]>
> ---
> drivers/net/ethernet/intel/igb/igb_ethtool.c | 78 +++++++++++++++++++++++++++-
> include/uapi/linux/ethtool.h | 3 ++
> 2 files changed, 79 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> index 7876240..49486b8 100644
> --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
> +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> @@ -2327,6 +2327,8 @@ static int igb_get_ts_info(struct net_device *dev,
> struct ethtool_ts_info *info)
> {
> struct igb_adapter *adapter = netdev_priv(dev);
> + struct e1000_hw *hw = &adapter->hw;
> + u32 regval;
>
> switch (adapter->hw.mac.type) {
> case e1000_82575:
> @@ -2360,10 +2362,29 @@ static int igb_get_ts_info(struct net_device *dev,
>
> info->rx_filters = 1 << HWTSTAMP_FILTER_NONE;
>
> + regval = rd32(E1000_TSYNCTXCTL);
> + if (regval & E1000_TSYNCTXCTL_ENABLED)
> + info->tx_reserved[0] = 1 << HWTSTAMP_TX_ON;
> + else
> + info->tx_reserved[0] = 1 << HWTSTAMP_TX_OFF;
> +
> + regval = rd32(E1000_TSYNCRXCTL);
> +
> /* 82576 does not support timestamping all packets. */
> - if (adapter->hw.mac.type >= e1000_82580)
> + if (adapter->hw.mac.type >= e1000_82580) {
> info->rx_filters |= 1 << HWTSTAMP_FILTER_ALL;
> - else
> +
> + if (!(regval & E1000_TSYNCRXCTL_ENABLED))
> + info->rx_reserved[0] =
> + 1 << HWTSTAMP_FILTER_NONE;
> + else if (E1000_TSYNCRXCTL_TYPE_ALL ==
> + (regval & E1000_TSYNCRXCTL_TYPE_MASK))
> + info->rx_reserved[0] = 1 << HWTSTAMP_FILTER_ALL;
> + else
> + return -ERANGE;
> +
> + return 0;
> + } else {
> info->rx_filters |=
> (1 << HWTSTAMP_FILTER_PTP_V1_L4_SYNC) |
> (1 << HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ) |
> @@ -2373,6 +2394,59 @@ static int igb_get_ts_info(struct net_device *dev,
> (1 << HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ) |
> (1 << HWTSTAMP_FILTER_PTP_V2_EVENT);
>
> + if (!(regval & E1000_TSYNCRXCTL_ENABLED)) {
> + info->rx_reserved[0] =
> + 1 << HWTSTAMP_FILTER_NONE;
> + return 0;
> + }
> + }
> +
> + switch (regval & E1000_TSYNCRXCTL_TYPE_MASK) {
> + case E1000_TSYNCRXCTL_TYPE_L4_V1:
> + regval = rd32(E1000_TSYNCRXCFG);
> + if (E1000_TSYNCRXCFG_PTP_V1_SYNC_MESSAGE ==
> + (regval &
> + E1000_TSYNCRXCFG_PTP_V1_CTRLT_MASK))
> + info->rx_reserved[0] =
> + (1 << HWTSTAMP_FILTER_PTP_V1_L4_SYNC);
> + else if (E1000_TSYNCRXCFG_PTP_V1_DELAY_REQ_MESSAGE
> + == (regval &
> + E1000_TSYNCRXCFG_PTP_V1_CTRLT_MASK))
> + info->rx_reserved[0] =
> + (1 <<
> + HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ);
> + else
> + return -ERANGE;
> + break;
> + case E1000_TSYNCRXCTL_TYPE_L2_L4_V2:
> + regval = rd32(E1000_TSYNCRXCFG);
> + if (E1000_TSYNCRXCFG_PTP_V2_SYNC_MESSAGE ==
> + (regval &
> + E1000_TSYNCRXCFG_PTP_V2_MSGID_MASK))
> + info->rx_reserved[0] =
> + (1 << HWTSTAMP_FILTER_PTP_V2_L2_SYNC) |
> + (1 << HWTSTAMP_FILTER_PTP_V2_L4_SYNC);
> + else if (E1000_TSYNCRXCFG_PTP_V2_DELAY_REQ_MESSAGE ==
> + (regval &
> + E1000_TSYNCRXCFG_PTP_V2_MSGID_MASK))
> + info->rx_reserved[0] =
> + (1 <<
> + HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ) |
> + (1 <<
> + HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ);
> + else
> + return -ERANGE;
> + break;
> + case E1000_TSYNCRXCTL_TYPE_EVENT_V2:
> + info->rx_reserved[0] =
> + (1 << HWTSTAMP_FILTER_PTP_V2_EVENT) |
> + (1 << HWTSTAMP_FILTER_PTP_V2_SYNC) |
> + (1 << HWTSTAMP_FILTER_PTP_V2_DELAY_REQ);
> + break;
> + default:
> + return -ERANGE;
> + }
> +
> return 0;
> default:
> return -EOPNOTSUPP;
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index 0c9b448..06cdbc0 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -772,7 +772,10 @@ struct ethtool_sfeatures {
> * @so_timestamping: bit mask of the sum of the supported SO_TIMESTAMPING flags
> * @phc_index: device index of the associated PHC, or -1 if there is none
> * @tx_types: bit mask of the supported hwtstamp_tx_types enumeration values
> + * @tx_reserved[0]: bit mask of the in use hwtstamp_tx_types enumeration values
> * @rx_filters: bit mask of the supported hwtstamp_rx_filters enumeration values
> + * @rx_reserved[0]: bit mask of the in use hwtstamp_rx_filters enumeration
> + * values
> *
> * The bits in the 'tx_types' and 'rx_filters' fields correspond to
> * the 'hwtstamp_tx_types' and 'hwtstamp_rx_filters' enumeration values,
> --
> 1.7.11.7
--
Best Regards,
Dong Zhu
On 05/14/2013 02:51 AM, Dong Zhu wrote:
> Hi,
>
>> I modified this patch and added the method to igb_get_ts_info function.
>> For 82576 nic, through this method we can easily check which type of packets
>> are time stamped now, such as (HWTSTAMP_FILTER_PTP_V1_L4_SYNC and
>> HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ), then change or disable it up to your requests.
>> I think it is convenient.The origial GET_TS_INFO method can only show the device’s
>> time stamping capabilities which nics supported.
> Through this method we can easily check which type of packets are
> timestamped currently, it is useful because that we use the hwstamp_ctl
> set time stamping policy first then we could verify what type of packets
> does the nic timestamp and then do some other testing(regression) for each
> policy.No matter PTP is running or not we can make sure whether timestamp packets
> could cause other general network problems through viewing and setting the
> timestamp policy.
I'm trying to follow this thread, and Richard's question keeps coming up
in my head, and you've failed to answer it yet.
What are you trying to fix/solve?
It sounds like what you're trying to add can already be retrieved
through different mechanisms that already exist.
Please explain what you're trying to accomplish that doesn't already
exist today, and why it's important. If the facility exists already,
then use that. If it's not very usable, it can (maybe) be updated given
what's deficient. If it doesn't exist, then explaining why it needs to
exist will help the case for the patches.
Just providing code doesn't help answer the question of what you're
trying to do in the first place, and why what already exists isn't good
enough.
Regards,
-PJ
Hello,
> -----Original Message-----
> From: Dong Zhu [mailto:[email protected]]
> Sent: Tuesday, May 14, 2013 2:52 AM
> To: Keller, Jacob E; Ben Hutchings; Vick, Matthew; Richard Cochran
> Cc: Sergei Shtylyov; Kirsher, Jeffrey T; Brandeburg, Jesse; Allan, Bruce
> W; Wyborny, Carolyn; Skidmore, Donald C; Rose, Gregory V; Waskiewicz
> Jr, Peter P; Duyck, Alexander H; Ronciak, John; Dave, Tushar N;
> Abodunrin, Akeem G; David S. Miller; Paul E. McKenney; David Howells;
> Dave Jones; Thomas Gleixner; [email protected]; e1000-
> [email protected]; [email protected]
> Subject: Re: [PATCH] igb: add a method to get the nic hw time stamping
> policy
>
> Hi,
>
> > I modified this patch and added the method to igb_get_ts_info
> function.
> > For 82576 nic, through this method we can easily check which type of
> packets
> > are time stamped now, such as (HWTSTAMP_FILTER_PTP_V1_L4_SYNC
> and
> > HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ), then change or disable it
> up to your requests.
> > I think it is convenient.The origial GET_TS_INFO method can only show
> the device’s
> > time stamping capabilities which nics supported.
>
> Through this method we can easily check which type of packets are
> timestamped currently, it is useful because that we use the
> hwstamp_ctl
> set time stamping policy first then we could verify what type of packets
> does the nic timestamp and then do some other testing(regression) for
> each
> policy.No matter PTP is running or not we can make sure whether
> timestamp packets
> could cause other general network problems through viewing and
> setting the
> timestamp policy.
>
Correct. However, the ioctl already modifies the value of the hwtstamp_config so that it will tell you which packets are being timestamped. As long as the filter is a match for one of the supported filters, it will return that value, otherwise it will return HWTSTAMP_FILTER_SOME. But that means it will timestamp at least what you request. If you are concerned about returning more specifically what the driver is doing in FILTER_SOME mode, you could extend the filters, and add the one you're missing, but it would need to be common and justified enough... Which it probably isn't as the current list is pretty comprehensive.
There is no real need to add a second field to get this without setting, as you can simply call it with a new value, and then get the value you need. Effectively, if you don't know the value is correct, just try to set it how you want, and check the return value plus the hwtstamp_config structure which will be MODIFIED by the hwtstamp ioctl.
There is absolutely no reason to add this as a new field which would remove the ability to extend hwtstamp_ioctl in the future should that ever be necessary!!
> My original idea is that:
> The implementation of this method is calling the ioctl call, in order
> not to break the userland ABI I use the flags field of hwtstamp_config,
> as it has no definition yet.
>
That would remove the ability to extend the ioctl when it's really necessary.
> - For I350 it only supports two types HWTSTAMP_FILTER_NONE and
> HWTSTAMP_FILTER_ALL
> for timestamping. It is easy to handle just return the value to flags.
>
> - For 82576NS it has more individual filters, so I need to do the
> judement in the igb_ptp_hwtstamp_ioctl function for different nics,
> the code of judgement might be reduplicated with
> igb_get_ts_info,then do
> the OR operation for filters then return to the flags.
>
What are you trying to achieve. When you call hwtstamp_ioctl on the igb driver, it will modify the passed in structure (hwtstamp_config) and set it's rx_filter type to the current value after the function is called. Upon return, your application can check the value and then perform regression tests or what have you against that value. You already have this information. You don't need a new method to fix it.
If you are debugging a driver, you can have that driver print data to the kernel print buffer or the ftrace buffer in order to determine what is being done. But your application can already get the set filter and compare to the expected filter. Documentation/timestamping/timestamping.c already shows you how to do this!
> Could tell me whether the above method(ioctl) is feasible and better
> than this
> one (ethtool)?
>
> IMO checking this through the ethtool GET_TS_INFO is so convenient
> without breaking any userland ABIs. Do you think so ?
>
Modifying GET_TS_INFO would be potentially breaking a userland ABI.. Again, no one understands what you are trying to do that can't already be done via hwtstamp_ioctl (unmodified!) We understand what you are wanting, but telling you that it can already be done!
> >
> > I use the tx_reserved[0] and rx_reserved[0] to restore the
> hwtstamp_tx_types and
> > hwtstamp_rx_filters enumeration values.
> >
> > I test it on I350 and 82576NS nics and it works as expect.
> >
> > Could help reviewing it again ? Any comments would be appreciated.
> >
>
- Jake
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?