[RFC][PATCH 0/3] net_tstamp: Add SIOCGHWTSTAMP ioctl

[RFC][PATCH 0/3] net_tstamp: Add SIOCGHWTSTAMP ioctl

[Ben Hutchings]

SIOCSHWTSTAMP returns the real configuration to the application
using it, but there is currently no way for any other
application to find out the configuration non-destructively.

This series adds a new ioctl with the obvious name which just reads the
configuration, and an example implementation for the sfc driver.

This information could alternately be added to struct ethtool_ts_info
returned by the ethtool ETHTOOL_GET_TS_INFO command, although that
wouldn't be very symmetric.  There would also be a problem of how to
indicate whether this extra information is present, given that all
fields of hwtstamp_config can validly be 0.

Ben.

Ben Hutchings (3):
  net_tstamp: Improve kernel-doc for struct hwtstamp_config
  net_tstamp: Add SIOCGHWTSTAMP ioctl to match SIOCSHWTSTAMP
  sfc: Implement the SIOCGHWTSTAMP ioctl

 drivers/net/ethernet/sfc/efx.c  |  2 +-
 drivers/net/ethernet/sfc/ptp.c  | 14 +++++++++-----
 include/uapi/linux/net_tstamp.h | 16 ++++++++--------
 include/uapi/linux/sockios.h    |  3 ++-
 net/core/dev_ioctl.c            |  2 ++
 5 files changed, 22 insertions(+), 15 deletions(-)


-- 
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.

[RFC][PATCH 1/3] net_tstamp: Improve kernel-doc for struct hwtstamp_config

[Ben Hutchings]

Fix the name of the rx_filter field.

Remove text about 32/64-bit compatibility; this works just the same as
for most socket ioctls and as the structure is not allowed to grow
there is no need to remind anyone how to maintain it.

Add explanation about drivers changing the filter mode.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 include/uapi/linux/net_tstamp.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h
index ae5df122e42f..c9a7de2a6276 100644
--- a/include/uapi/linux/net_tstamp.h
+++ b/include/uapi/linux/net_tstamp.h
@@ -30,13 +30,13 @@ enum {
  *
  * @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 HWTSTAMP_FILTER_*
  *
  * %SIOCSHWTSTAMP expects a &struct ifreq with a ifr_data pointer to
- * this structure. dev_ifsioc() in the kernel takes care of the
- * translation between 32 bit userspace and 64 bit kernel. The
- * structure is intentionally chosen so that it has the same layout on
- * 32 and 64 bit systems, don't break this!
+ * this structure.  If the driver or hardware does not support the
+ * requested @rx_filter value, the driver may use a more general
+ * filter mode.  In this case @rx_filter will indicate the actual mode
+ * on return.
  */
 struct hwtstamp_config {
 	int flags;


-- 
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.

[RFC][PATCH 2/3] net_tstamp: Add SIOCGHWTSTAMP ioctl to match SIOCSHWTSTAMP

[Ben Hutchings]

SIOCSHWTSTAMP returns the real configuration to the application
using it, but there is currently no way for any other
application to find out the configuration non-destructively.
Add a new ioctl for this, making it unprivileged.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 include/uapi/linux/net_tstamp.h | 14 +++++++-------
 include/uapi/linux/sockios.h    |  3 ++-
 net/core/dev_ioctl.c            |  2 ++
 3 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h
index c9a7de2a6276..f53879c0f590 100644
--- a/include/uapi/linux/net_tstamp.h
+++ b/include/uapi/linux/net_tstamp.h
@@ -26,17 +26,17 @@ enum {
 };
 
 /**
- * struct hwtstamp_config - %SIOCSHWTSTAMP parameter
+ * struct hwtstamp_config - %SIOCGHWTSTAMP and %SIOCSHWTSTAMP parameter
  *
- * @flags:	no flags defined right now, must be zero
+ * @flags:	no flags defined right now, must be zero for %SIOCSHWTSTAMP
  * @tx_type:	one of HWTSTAMP_TX_*
  * @rx_filter:	one of HWTSTAMP_FILTER_*
  *
- * %SIOCSHWTSTAMP expects a &struct ifreq with a ifr_data pointer to
- * this structure.  If the driver or hardware does not support the
- * requested @rx_filter value, the driver may use a more general
- * filter mode.  In this case @rx_filter will indicate the actual mode
- * on return.
+ * %SIOCGHWTSTAMP and %SIOCSHWTSTAMP expect a &struct ifreq with a
+ * ifr_data pointer to this structure.  For %SIOCSHWTSTAMP, if the
+ * driver or hardware does not support the requested @rx_filter value,
+ * the driver may use a more general filter mode.  In this case
+ * @rx_filter will indicate the actual mode on return.
  */
 struct hwtstamp_config {
 	int flags;
diff --git a/include/uapi/linux/sockios.h b/include/uapi/linux/sockios.h
index 7997a506ad41..e888b1aed69f 100644
--- a/include/uapi/linux/sockios.h
+++ b/include/uapi/linux/sockios.h
@@ -125,7 +125,8 @@
 #define SIOCBRDELIF	0x89a3		/* remove interface from bridge */
 
 /* hardware time stamping: parameters in linux/net_tstamp.h */
-#define SIOCSHWTSTAMP   0x89b0
+#define SIOCSHWTSTAMP	0x89b0		/* set and get config		*/
+#define SIOCGHWTSTAMP	0x89b1		/* get config			*/
 
 /* Device private ioctl calls */
 
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index 5b7d0e1d0664..cf999e09bcd2 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -327,6 +327,7 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, unsigned int cmd)
 		    cmd == SIOCBRADDIF ||
 		    cmd == SIOCBRDELIF ||
 		    cmd == SIOCSHWTSTAMP ||
+		    cmd == SIOCGHWTSTAMP ||
 		    cmd == SIOCWANDEV) {
 			err = -EOPNOTSUPP;
 			if (ops->ndo_do_ioctl) {
@@ -546,6 +547,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, void __user *arg)
 	 */
 	default:
 		if (cmd == SIOCWANDEV ||
+		    cmd == SIOCGHWTSTAMP ||
 		    (cmd >= SIOCDEVPRIVATE &&
 		     cmd <= SIOCDEVPRIVATE + 15)) {
 			dev_load(net, ifr.ifr_name);


-- 
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.

[RFC][PATCH 3/3] sfc: Implement the SIOCGHWTSTAMP ioctl

[Ben Hutchings]

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 drivers/net/ethernet/sfc/efx.c |  2 +-
 drivers/net/ethernet/sfc/ptp.c | 14 +++++++++-----
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c
index 07c9bc4c61bc..d5fbac313bf7 100644
--- a/drivers/net/ethernet/sfc/efx.c
+++ b/drivers/net/ethernet/sfc/efx.c
@@ -1856,7 +1856,7 @@ static int efx_ioctl(struct net_device *net_dev, struct ifreq *ifr, int cmd)
 	struct efx_nic *efx = netdev_priv(net_dev);
 	struct mii_ioctl_data *data = if_mii(ifr);
 
-	if (cmd == SIOCSHWTSTAMP)
+	if (cmd == SIOCGHWTSTAMP || cmd == SIOCSHWTSTAMP)
 		return efx_ptp_ioctl(efx, ifr, cmd);
 
 	/* Convert phy_id from older PRTAD/DEVAD format */
diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
index 03acf57df045..ba14a4d95dfb 100644
--- a/drivers/net/ethernet/sfc/ptp.c
+++ b/drivers/net/ethernet/sfc/ptp.c
@@ -1240,12 +1240,16 @@ int efx_ptp_ioctl(struct efx_nic *efx, struct ifreq *ifr, int cmd)
 	if (!efx->ptp_data)
 		return -EOPNOTSUPP;
 
-	if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
-		return -EFAULT;
+	if (cmd == SIOCSHWTSTAMP) {
+		if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
+			return -EFAULT;
 
-	rc = efx_ptp_ts_init(efx, &config);
-	if (rc != 0)
-		return rc;
+		rc = efx_ptp_ts_init(efx, &config);
+		if (rc != 0)
+			return rc;
+	} else {
+		config = efx->ptp_data->config;
+	}
 
 	return copy_to_user(ifr->ifr_data, &config, sizeof(config))
 		? -EFAULT : 0;

-- 
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.

Test program for SIOC{G,S}HWTSTAMP

[Ben Hutchings]

I used this to exercise the two ioctls.

Ben.

---
/* Test program for SIOC{G,S}HWTSTAMP - bhutchings@solarflare.com, 2013 */

#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#include <sys/socket.h>
#include <sys/ioctl.h>

#include <linux/if.h>
#include <linux/net_tstamp.h>
#include <linux/sockios.h>

#ifndef SIOCGHWTSTAMP
#define SIOCGHWTSTAMP 0x89b1
#endif

static int
lookup_value(const char **names, int size, const char *name)
{
    int value;

    for (value = 0; value < size; value++)
	if (names[value] && strcasecmp(names[value], name) == 0)
	    return value;

    return -1;
}

static const char *
lookup_name(const char **names, int size, int value)
{
    return (value >= 0 && value < size) ? names[value] : NULL;
}

static void list_names(FILE *f, const char **names, int size)
{
    int value;

    for (value = 0; value < size; value++)
	if (names[value])
	    fprintf(f, "    %s\n", names[value]);
}

static const char *tx_types[] = {
#define TX_TYPE(name) [HWTSTAMP_TX_ ## name] = #name
    TX_TYPE(OFF),
    TX_TYPE(ON),
    TX_TYPE(ONESTEP_SYNC)
#undef TX_TYPE
};
#define N_TX_TYPES ((int)(sizeof(tx_types) / sizeof(tx_types[0])))

static const char *rx_filters[] = {
#define RX_FILTER(name) [HWTSTAMP_FILTER_ ## name] = #name
    RX_FILTER(NONE),
    RX_FILTER(ALL),
    RX_FILTER(SOME),
    RX_FILTER(PTP_V1_L4_EVENT),
    RX_FILTER(PTP_V1_L4_SYNC),
    RX_FILTER(PTP_V1_L4_DELAY_REQ),
    RX_FILTER(PTP_V2_L4_EVENT),
    RX_FILTER(PTP_V2_L4_SYNC),
    RX_FILTER(PTP_V2_L4_DELAY_REQ),
    RX_FILTER(PTP_V2_L2_EVENT),
    RX_FILTER(PTP_V2_L2_SYNC),
    RX_FILTER(PTP_V2_L2_DELAY_REQ),
    RX_FILTER(PTP_V2_EVENT),
    RX_FILTER(PTP_V2_SYNC),
    RX_FILTER(PTP_V2_DELAY_REQ),
#undef RX_FILTER
};
#define N_RX_FILTERS ((int)(sizeof(rx_filters) / sizeof(rx_filters[0])))

static void usage(void)
{
    fputs("Usage: hwtstamp_config if_name [tx_type rx_filter]\n"
	  "tx_type is any of (case-insensitive):\n",
	  stderr);
    list_names(stderr, tx_types, N_TX_TYPES);
    fputs("rx_filter is any of (case-insensitive):\n", stderr);
    list_names(stderr, rx_filters, N_RX_FILTERS);
}

int main(int argc, char **argv)
{
    struct ifreq ifr;
    struct hwtstamp_config config;
    const char *name;
    int sock;

    if ((argc != 2 && argc != 4) || (strlen(argv[1]) >= IFNAMSIZ)) {
	usage();
	return 2;
    }

    if (argc == 4) {
	config.flags = 0;
	config.tx_type = lookup_value(tx_types, N_TX_TYPES, argv[2]);
	config.rx_filter = lookup_value(rx_filters, N_RX_FILTERS, argv[3]);
	if (config.tx_type < 0 || config.rx_filter < 0) {
	    usage();
	    return 2;
	}
    }

    sock = socket(AF_INET, SOCK_DGRAM, 0);
    if (sock < 0) {
	perror("socket");
	return 1;
    }

    strcpy(ifr.ifr_name, argv[1]);
    ifr.ifr_data = (caddr_t)&config;

    if (ioctl(sock, (argc == 2) ? SIOCGHWTSTAMP : SIOCSHWTSTAMP, &ifr)) {
	perror("ioctl");
	return 1;
    }

    printf("flags = %#x\n", config.flags);
    name = lookup_name(tx_types, N_TX_TYPES, config.tx_type);
    if (name)
	printf("tx_type = %s\n", name);
    else
	printf("tx_type = %d\n", config.tx_type);
    name = lookup_name(rx_filters, N_RX_FILTERS, config.rx_filter);
    if (name)
	printf("rx_filter = %s\n", name);
    else
	printf("rx_filter = %d\n", config.rx_filter);

    return 0;
}


-- 
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.

Re: Test program for SIOC{G,S}HWTSTAMP

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Thu, 14 Nov 2013 02:46:24 +0000

> I used this to exercise the two ioctls.

You could put this under Documentation/networking/timestamping/

There is already example code there.

Re: [RFC][PATCH 0/3] net_tstamp: Add SIOCGHWTSTAMP ioctl

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Thu, 14 Nov 2013 02:37:15 +0000

> SIOCSHWTSTAMP returns the real configuration to the application
> using it, but there is currently no way for any other
> application to find out the configuration non-destructively.
> 
> This series adds a new ioctl with the obvious name which just reads the
> configuration, and an example implementation for the sfc driver.
> 
> This information could alternately be added to struct ethtool_ts_info
> returned by the ethtool ETHTOOL_GET_TS_INFO command, although that
> wouldn't be very symmetric.  There would also be a problem of how to
> indicate whether this extra information is present, given that all
> fields of hwtstamp_config can validly be 0.

No objections to the new ioctl, but you absolutely need to implement
the appropriate compat ioctl handler in net/socket.c just as we have
one for the existing SIOCSHWTSTAMP.

Thanks.

Re: [RFC][PATCH 0/3] net_tstamp: Add SIOCGHWTSTAMP ioctl

[Richard Cochran]

On Thu, Nov 14, 2013 at 02:37:15AM +0000, Ben Hutchings wrote:
> SIOCSHWTSTAMP returns the real configuration to the application
> using it, but there is currently no way for any other
> application to find out the configuration non-destructively.

I guess that the reasoning behind the original interface is that an
application only wants to know the settings when it needs a particular
setting in order to operate correctly. This would make the query call
pointless, since the application might as well just go ahead and set
the values that it needs.

Do you have a particular use case in mind?

Thanks,
Richard

Re: [RFC][PATCH 0/3] net_tstamp: Add SIOCGHWTSTAMP ioctl

[Ben Hutchings]

On Thu, 2013-11-14 at 08:41 +0100, Richard Cochran wrote:
> On Thu, Nov 14, 2013 at 02:37:15AM +0000, Ben Hutchings wrote:
> > SIOCSHWTSTAMP returns the real configuration to the application
> > using it, but there is currently no way for any other
> > application to find out the configuration non-destructively.
> 
> I guess that the reasoning behind the original interface is that an
> application only wants to know the settings when it needs a particular
> setting in order to operate correctly. This would make the query call
> pointless, since the application might as well just go ahead and set
> the values that it needs.
> 
> Do you have a particular use case in mind?

The immediate motivation is wanting to include this information in
diagnostic reports.

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.

Re: [RFC][PATCH 0/3] net_tstamp: Add SIOCGHWTSTAMP ioctl

[Richard Cochran]

On Thu, Nov 14, 2013 at 03:10:19PM +0000, Ben Hutchings wrote:
> 
> The immediate motivation is wanting to include this information in
> diagnostic reports.

Fair enough.

Thanks,
Richard