Re: [RFC PATCH net-next v3 13/21] ethtool: provide timestamping information in GET_INFO request

2019-02-20 Thread Jakub Kicinski
On Wed, 20 Feb 2019 14:00:07 +0100, Michal Kubecek wrote:
> On Tue, Feb 19, 2019 at 07:00:48PM -0800, Jakub Kicinski wrote:
> > On Mon, 18 Feb 2019 19:22:29 +0100 (CET), Michal Kubecek wrote:  
> > > Add timestamping information as provided by ETHTOOL_GET_TS_INFO ioctl
> > > command in GET_INFO reply if ETH_INFO_IM_TSINFO flag is set in the 
> > > request.
> > > 
> > > Add constants for counts of HWTSTAMP_TX_* and HWTSTAM_FILTER_* constants
> > > and provide symbolic names for timestamping related values so that they 
> > > can
> > > be retrieved in GET_STRSET and GET_INFO requests.  
> > 
> > What's the reason for providing the symbolic names?  
> 
> One of the the goals I had was to reduce the need to keep the lists of
> possible values in sync between kernel and userspace ethtool and other
> users of the interface so that when a new value is added, we don't have
> to update all userspace tools to be able to use or present it.
> 
> This already works in ethtool for some newer commands (e.g. features)
> and obviously for those where the list of available options depends on
> the device (e.g. private flags or statistics). I would like to extend
> the principle also to older commands and new ones which do not work like
> this (e.g. device reset).

Let me try to argue that's the wrong direction.  People should learn to
update their user space tooling if they want access to new features.  
In my (limited) experience trying to solve forward compatibility leads
to short term gains, and long term warts in the APIs and increased
maintenance burden in the kernel.


Re: [RFC PATCH net-next v3 13/21] ethtool: provide timestamping information in GET_INFO request

2019-02-20 Thread Michal Kubecek
On Tue, Feb 19, 2019 at 07:00:48PM -0800, Jakub Kicinski wrote:
> On Mon, 18 Feb 2019 19:22:29 +0100 (CET), Michal Kubecek wrote:
> > Add timestamping information as provided by ETHTOOL_GET_TS_INFO ioctl
> > command in GET_INFO reply if ETH_INFO_IM_TSINFO flag is set in the request.
> > 
> > Add constants for counts of HWTSTAMP_TX_* and HWTSTAM_FILTER_* constants
> > and provide symbolic names for timestamping related values so that they can
> > be retrieved in GET_STRSET and GET_INFO requests.
> 
> What's the reason for providing the symbolic names?

One of the the goals I had was to reduce the need to keep the lists of
possible values in sync between kernel and userspace ethtool and other
users of the interface so that when a new value is added, we don't have
to update all userspace tools to be able to use or present it.

This already works in ethtool for some newer commands (e.g. features)
and obviously for those where the list of available options depends on
the device (e.g. private flags or statistics). I would like to extend
the principle also to older commands and new ones which do not work like
this (e.g. device reset).

Michal


Re: [RFC PATCH net-next v3 13/21] ethtool: provide timestamping information in GET_INFO request

2019-02-19 Thread Jakub Kicinski
On Mon, 18 Feb 2019 19:22:29 +0100 (CET), Michal Kubecek wrote:
> Add timestamping information as provided by ETHTOOL_GET_TS_INFO ioctl
> command in GET_INFO reply if ETH_INFO_IM_TSINFO flag is set in the request.
> 
> Add constants for counts of HWTSTAMP_TX_* and HWTSTAM_FILTER_* constants
> and provide symbolic names for timestamping related values so that they can
> be retrieved in GET_STRSET and GET_INFO requests.

What's the reason for providing the symbolic names?

> Signed-off-by: Michal Kubecek 


[RFC PATCH net-next v3 13/21] ethtool: provide timestamping information in GET_INFO request

2019-02-18 Thread Michal Kubecek
Add timestamping information as provided by ETHTOOL_GET_TS_INFO ioctl
command in GET_INFO reply if ETH_INFO_IM_TSINFO flag is set in the request.

Add constants for counts of HWTSTAMP_TX_* and HWTSTAM_FILTER_* constants
and provide symbolic names for timestamping related values so that they can
be retrieved in GET_STRSET and GET_INFO requests.

Signed-off-by: Michal Kubecek 
---
 Documentation/networking/ethtool-netlink.txt |  10 +-
 include/uapi/linux/ethtool.h |   6 +
 include/uapi/linux/ethtool_netlink.h |  14 +-
 include/uapi/linux/net_tstamp.h  |  13 ++
 net/ethtool/common.c |  24 
 net/ethtool/common.h |   2 +
 net/ethtool/info.c   | 138 +++
 net/ethtool/ioctl.c  |  20 +--
 net/ethtool/netlink.h|   9 ++
 net/ethtool/strset.c |  18 +++
 10 files changed, 234 insertions(+), 20 deletions(-)

diff --git a/Documentation/networking/ethtool-netlink.txt 
b/Documentation/networking/ethtool-netlink.txt
index 1e615e111262..c6c7475340e2 100644
--- a/Documentation/networking/ethtool-netlink.txt
+++ b/Documentation/networking/ethtool-netlink.txt
@@ -242,6 +242,11 @@ Kernel response contents:
 ETHA_INFO_PERMADDR (nested)
 ETHA_PERMADDR_ADDRESS  (binary)permanent HW address
 ETHA_PERMADDR_TYPE (u16)   dev->type
+ETHA_INFO_TSINFO   (nested)timestamping information
+ETHA_TSINFO_TIMESTAMPING   (bitset)supported flags
+ETHA_TSINFO_PHC_INDEX  (u32)   associated PHC
+ETHA_TSINFO_TX_TYPES   (bitset)Tx types
+ETHA_TSINFO_RX_FILTERS (bitset)Rx filters
 
 The meaning of DRVINFO attributes follows the corresponding fields of
 ETHTOOL_GDRVINFO response. Second part with various counts and sizes is
@@ -253,6 +258,9 @@ There is no separate attribute for permanent address length 
as the length can
 be determined from attribute length (nla_len()). ETHA_PERMADDR_TYPE provides
 net_device::type value to give a hint about what kind of address device has.
 
+ETHA_TSINFO_PHC_INDEX can be unsigned as there is no need for special value;
+if no PHC is associated, the attribute is not present.
+
 GET_INFO requests allow dumps.
 
 
@@ -328,7 +336,7 @@ ETHTOOL_SCHANNELS   n/a
 ETHTOOL_SET_DUMP   n/a
 ETHTOOL_GET_DUMP_FLAG  n/a
 ETHTOOL_GET_DUMP_DATA  n/a
-ETHTOOL_GET_TS_INFOn/a
+ETHTOOL_GET_TS_INFOETHNL_CMD_GET_INFO
 ETHTOOL_GMODULEINFOn/a
 ETHTOOL_GMODULEEEPROM  n/a
 ETHTOOL_GEEE   n/a
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 9ea278961d80..1b58637d3a4d 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -563,6 +563,9 @@ struct ethtool_pauseparam {
  * @ETH_SS_RSS_HASH_FUNCS: RSS hush function names
  * @ETH_SS_PHY_STATS: Statistic names, for use with %ETHTOOL_GPHYSTATS
  * @ETH_SS_PHY_TUNABLES: PHY tunable names
+ * @ETH_SS_TSTAMP_SOF: timestamping flag names
+ * @ETH_SS_TSTAMP_TX_TYPE: timestamping Tx type names
+ * @ETH_SS_TSTAMP_RX_FILTER: timestamping Rx filter names
  */
 enum ethtool_stringset {
ETH_SS_TEST = 0,
@@ -574,6 +577,9 @@ enum ethtool_stringset {
ETH_SS_TUNABLES,
ETH_SS_PHY_STATS,
ETH_SS_PHY_TUNABLES,
+   ETH_SS_TSTAMP_SOF,
+   ETH_SS_TSTAMP_TX_TYPE,
+   ETH_SS_TSTAMP_RX_FILTER,
 
ETH_SS_COUNT
 };
diff --git a/include/uapi/linux/ethtool_netlink.h 
b/include/uapi/linux/ethtool_netlink.h
index fb756b6a8592..8ab2b7454e81 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -154,8 +154,9 @@ enum {
 
 #define ETH_INFO_IM_DRVINFO0x01
 #define ETH_INFO_IM_PERMADDR   0x02
+#define ETH_INFO_IM_TSINFO 0x04
 
-#define ETH_INFO_IM_ALL0x03
+#define ETH_INFO_IM_ALL0x07
 
 enum {
ETHA_DRVINFO_UNSPEC,
@@ -177,6 +178,17 @@ enum {
ETHA_PERMADDR_MAX = (__ETHA_PERMADDR_CNT - 1)
 };
 
+enum {
+   ETHA_TSINFO_UNSPEC,
+   ETHA_TSINFO_TIMESTAMPING,   /* bitset */
+   ETHA_TSINFO_PHC_INDEX,  /* u32 */
+   ETHA_TSINFO_TX_TYPES,   /* bitset */
+   ETHA_TSINFO_RX_FILTERS, /* bitset */
+
+   __ETHA_TSINFO_CNT,
+   ETHA_TSINFO_MAX = (__ETHA_TSINFO_CNT - 1)
+};
+
 /* generic netlink info */
 #define ETHTOOL_GENL_NAME "ethtool"
 #define ETHTOOL_GENL_VERSION 1
diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h
index e5b39721c6e4..e5b0472c4416 100644
--- a/include/uapi/linux/net_tstamp.h
+++ b/include/uapi/linux/net_tstamp.h
@@ -30,6 +30,9 @@ enum {
SOF_TIMESTAMPING_OPT_S