RE: [PATCH] igb: add a method to get the nic hw time stamping policy

[Keller, Jacob E]

> -----Original Message-----
> From: Dong Zhu [mailto:bluezhudong@xxxxxxxxx]
> 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; linux-kernel@xxxxxxxxxxxxxxx; e1000-
> devel@xxxxxxxxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx
> 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 <bluezhudong@xxxxxxxxx>
> 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 <bluezhudong@xxxxxxxxx>
> ---
>  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‹§²æìr¸›yúèšØb²X¬¶ÇvØ^–)Þ{.nÇ+‰·¥Š{±‘êçzX§¶›¡Ü}©ž²ÆzÚ&j:+v‰¨¾«‘êçzZ+€Ê+zf£¢·hšˆ§~†­†Ûiÿûàz¹®w¥¢¸?™¨è­Ú&¢)ßf”ù^jÇy§m…á@A«a¶Úÿ0¶ìh®å’i