Re: RFC [PATCH 2/3] PTP: use flags to request HW features

Re: RFC [PATCH 2/3] PTP: use flags to request HW features

[Flavio Leitner]

Adding CC to netdev.

On Wed, Oct 30, 2013 at 05:00:36PM -0200, Flavio Leitner wrote:
> Currently the user space can't tell which delay mechanism
> (E2E/P2P) or transport is needed in the ioctl(). Therefore,
> PTP silently fails when the hardware doesn't support a
> certain delay mechanism or transport.
> 
> This patch uses the ioctl flags field to pass that information
> from the user space to kernel. If the hardware supports all
> the desired features, the ioctl continues as before, otherwise
> the unsupported bits are reseted to 0 and this information is
> returned to user space.
> 
> It's backwards compatible. If an older PTP applications calls
> the ioctl(), the flags field will be zero and no feature is
> checked.
> 
> Signed-off-by: Flavio Leitner <fbl@redhat.com>
> ---
>  include/uapi/linux/net_tstamp.h | 21 +++++++++++++++++++++
>  net/core/dev_ioctl.c            |  3 ---
>  2 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h
> index ae5df12..d63f8e9 100644
> --- a/include/uapi/linux/net_tstamp.h
> +++ b/include/uapi/linux/net_tstamp.h
> @@ -44,6 +44,27 @@ struct hwtstamp_config {
>  	int rx_filter;
>  };
>  
> +/* possible values for hwtstamp_config->flags */
> +#define HWTSTAMP_FEATURE_FLAGS_MASK 0xFFFF
> +enum hwtstamp_feature_flags {
> +	/* End-to-End Delay Mechanism */
> +	HWTSTAMP_DM_E2E = (1<<0),
> +	/* Peer-to-Peer Delay Mechanism */
> +	HWTSTAMP_DM_P2P = (1<<1),
> +	/* hole: 3 bits for additional mechanisms */
> +
> +	HWTSTAMP_TRANS_UDS = (1<<5),
> +	/* Message Transport: UDP over IPv4 */
> +	HWTSTAMP_TRANS_UDP_IPV4 = (1<<6),
> +	/* Message Transport: UDP over IPv6 */
> +	HWTSTAMP_TRANS_UDP_IPV6 = (1<<7),
> +	/* Message Transport: IEEE 802.3 Ethernet */
> +	HWTSTAMP_TRANS_IEEE_802_3 = (1<<8),
> +	HWTSTAMP_TRANS_DEVICENET = (1<<9),
> +	HWTSTAMP_TRANS_CONTROLNET = (1<<10),
> +	HWTSTAMP_TRANS_PROFINET = (1<<11),
> +};
> +
>  /* possible values for hwtstamp_config->tx_type */
>  enum hwtstamp_tx_types {
>  	/*
> diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
> index 5b7d0e1..9e2407e 100644
> --- a/net/core/dev_ioctl.c
> +++ b/net/core/dev_ioctl.c
> @@ -193,9 +193,6 @@ static int net_hwtstamp_validate(struct ifreq *ifr)
>  	if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
>  		return -EFAULT;
>  
> -	if (cfg.flags) /* reserved for future extensions */
> -		return -EINVAL;
> -
>  	tx_type = cfg.tx_type;
>  	rx_filter = cfg.rx_filter;
>  
> -- 
> 1.8.3.1
> 

Re: [Linuxptp-devel] RFC [PATCH 2/3] PTP: use flags to request HW features

[Richard Cochran]

On Wed, Oct 30, 2013 at 07:48:41PM -0200, Flavio Leitner wrote:
> On Wed, Oct 30, 2013 at 05:00:36PM -0200, Flavio Leitner wrote:
> > Currently the user space can't tell which delay mechanism
> > (E2E/P2P) or transport is needed in the ioctl(). Therefore,
> > PTP silently fails when the hardware doesn't support a
> > certain delay mechanism or transport.

[ For netdev readers, the text below repeats what I posted to the
  linuxptp-devel list. ]

SIOCSHWTSTAMP is bad enough already. Let's not make it even
uglier. ETHTOOL is a better place for this kind of thing. The
ethtool_ts_info has plenty of reserved fields, and drivers could
use them to advertise these capabilities without the risk of
breaking user space.

We already check each port's capabilities using the ethtool ioctl.
It makes perfect sense to have two calls. First, tell me what is
available, then, I'll tell you what I want. That is easy.

In contrast, the closed loop feedback of SIOCSHWTSTAMP is really
awful. The whole rx_filter field is designed around the inadequacies
of an early intel card, and the choices are obsolete and mostly
useless. There isn't even any mention of pdelay. Ptp4l is already
bad enough with the filter1, filter2 probing. Let's just leave that
ioctl alone.

On Wed, Oct 30, 2013 at 07:46:58PM -0200, Flavio Leitner wrote:
> What about the transport?

I suggest offering the following bits in the ethtool rx|tx_reserved
fields:

TS_SYNC        (1<<0) /* silly, PTP is not possible without this */
TS_DELAY_REQ   (1<<1)
TS_PDELAY_REQ  (1<<2)
TS_PDELAY_RESP (1<<3)

TS_ON_LAYER2   (1<<4)
TS_ON_IPV4     (1<<5)
TS_ON_IPV6     (1<<6)

> And feature combination?

My goal for linuxptp (and for the PHC subsystem) is to offer a kind of
reference implementation. I have no inclination to support every last
broken hardware out there. However, I agree that we should try to
support poorly designed cards if we can, but not at the price of
making ugly interfaces for the everyone.

Drivers with weird restrictions (for example, supports PDelay_Req but
only on IPv6) will have to just advertise one working combination.

The intel ixp465 is an example of design that we cannot ever support
in linuxptp. It can time stamp *either* the master role's events or
the slave role's events, but not both. Brilliant.

[ This chip is thankfully now obsolete, but the time stamping IP core
  found its way into the phc_gbe, but with the worst mistakes
  corrected. ]

Thanks,
Richard

Re: [Linuxptp-devel] RFC [PATCH 2/3] PTP: use flags to request HW features

[Flavio Leitner]

On Thu, Oct 31, 2013 at 08:12:58AM +0100, Richard Cochran wrote:
> On Wed, Oct 30, 2013 at 07:48:41PM -0200, Flavio Leitner wrote:
> > On Wed, Oct 30, 2013 at 05:00:36PM -0200, Flavio Leitner wrote:
> > > Currently the user space can't tell which delay mechanism
> > > (E2E/P2P) or transport is needed in the ioctl(). Therefore,
> > > PTP silently fails when the hardware doesn't support a
> > > certain delay mechanism or transport.
> 
> [ For netdev readers, the text below repeats what I posted to the
>   linuxptp-devel list. ]

I am reconsidering about extending ethtool instead, so I'd like to
drop these patches for now.
Thanks Richard and Jacob for the contributions,
fbl

Re: [PATCH 3/3] e1000e: PTP: provide hardware features

[Flavio Leitner]

Adding CC to netdev.

The first patch [1/3] is the userlevel linuxptp patch, so
I am not forwarding to netdev.

This is just an example which I used for testing locally. If
the idea is acceptable, this patch must be replaced since the
card does support more features than what is listed below.

fbl

On Wed, Oct 30, 2013 at 05:00:37PM -0200, Flavio Leitner wrote:
> Provide the modes and transports supported by the hardware.
> 
> Signed-off-by: Flavio Leitner <fbl@redhat.com>
> ---
>  drivers/net/ethernet/intel/e1000e/netdev.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> index 4ef7867..8bcf167 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -3498,10 +3498,6 @@ static int e1000e_config_hwtstamp(struct e1000_adapter *adapter)
>  	if (!(adapter->flags & FLAG_HAS_HW_TIMESTAMP))
>  		return -EINVAL;
>  
> -	/* flags reserved for future extensions - must be zero */
> -	if (config->flags)
> -		return -EINVAL;
> -
>  	switch (config->tx_type) {
>  	case HWTSTAMP_TX_OFF:
>  		tsync_tx_ctl = 0;
> @@ -5772,6 +5768,9 @@ static int e1000_mii_ioctl(struct net_device *netdev, struct ifreq *ifr,
>  	return 0;
>  }
>  
> +#define E1000E_HWSTAMP_FLAGS_MASK (HWTSTAMP_DM_E2E | HWTSTAMP_DM_P2P |\
> +				   HWTSTAMP_TRANS_IEEE_802_3 |\
> +				   HWTSTAMP_TRANS_UDP_IPV4)
>  /**
>   * e1000e_hwtstamp_ioctl - control hardware time stamping
>   * @netdev: network interface device structure
> @@ -5792,11 +5791,23 @@ static int e1000e_hwtstamp_ioctl(struct net_device *netdev, struct ifreq *ifr)
>  {
>  	struct e1000_adapter *adapter = netdev_priv(netdev);
>  	struct hwtstamp_config config;
> +	int flags_req;
> +	int flags_unsup;
>  	int ret_val;
>  
>  	if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
>  		return -EFAULT;
>  
> +	if (config.flags & ~HWTSTAMP_FEATURE_FLAGS_MASK)
> +		return -EINVAL;
> +
> +	flags_req = config.flags & HWTSTAMP_FEATURE_FLAGS_MASK;
> +	flags_unsup = flags_req & ~E1000E_HWSTAMP_FLAGS_MASK;
> +	if (flags_unsup) {
> +		config.flags &= ~flags_unsup;
> +		goto out;
> +	}
> +
>  	adapter->hwtstamp_config = config;
>  
>  	ret_val = e1000e_config_hwtstamp(adapter);
> @@ -5823,6 +5834,7 @@ static int e1000e_hwtstamp_ioctl(struct net_device *netdev, struct ifreq *ifr)
>  		break;
>  	}
>  
> +out:
>  	return copy_to_user(ifr->ifr_data, &config,
>  			    sizeof(config)) ? -EFAULT : 0;
>  }
> -- 
> 1.8.3.1
>