diff mbox series

[net-next,v2,7/7] ethtool: add compat for devlink info

Message ID 20190130190513.25718-8-jakub.kicinski@netronome.com
State Superseded
Delegated to: David Miller
Headers show
Series devlink: add device (driver) information API | expand

Commit Message

Jakub Kicinski Jan. 30, 2019, 7:05 p.m. UTC 
If driver did not fill the fw_version field, try to call into
the new devlink get_info op and collect the versions that way.
We assume ethtool was always reporting running versions.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 include/net/devlink.h |  7 ++++++
 net/core/devlink.c    | 52 ++++++++++++++++++++++++++++++++++++++++++-
 net/core/ethtool.c    |  7 ++++++
 3 files changed, 65 insertions(+), 1 deletion(-)

Comments

Jiri Pirko Jan. 30, 2019, 10:12 p.m. UTC | #1 
Wed, Jan 30, 2019 at 08:05:13PM CET, jakub.kicinski@netronome.com wrote:
>If driver did not fill the fw_version field, try to call into
>the new devlink get_info op and collect the versions that way.
>We assume ethtool was always reporting running versions.
>
>Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>---
> include/net/devlink.h |  7 ++++++
> net/core/devlink.c    | 52 ++++++++++++++++++++++++++++++++++++++++++-
> net/core/ethtool.c    |  7 ++++++
> 3 files changed, 65 insertions(+), 1 deletion(-)
>
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index c678ed0cb099..b4750e93303a 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -640,6 +640,8 @@ int devlink_info_report_version(struct devlink_info_req *req,
> 				enum devlink_version_type type,
> 				const char *version_name,
> 				const char *version_value);
>+void devlink_compat_running_versions(struct net_device *dev,
>+				     char *buf, size_t len);
> 
> #else
> 
>@@ -957,6 +959,11 @@ devlink_info_report_version(struct devlink_info_req *req,
> {
> 	return 0;
> }
>+
>+static inline void
>+devlink_compat_running_versions(struct net_device *dev, char *buf, size_t len)
>+{
>+}
> #endif
> 
> #endif /* _NET_DEVLINK_H_ */
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index e2027d3a5e40..5313e5918ee2 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -3715,12 +3715,18 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
> }
> 
> struct devlink_info_req {
>+	bool compat;
> 	struct sk_buff *msg;
>+	/* For compat call */
>+	char *buf;

union?


>+	size_t len;
> };
> 
> int devlink_info_report_driver_name(struct devlink_info_req *req,
> 				    const char *name)
> {
>+	if (req->compat)
>+		return 0;
> 	return nla_put_string(req->msg, DEVLINK_ATTR_INFO_DRV_NAME, name);
> }
> EXPORT_SYMBOL_GPL(devlink_info_report_driver_name);
>@@ -3728,6 +3734,8 @@ EXPORT_SYMBOL_GPL(devlink_info_report_driver_name);
> int devlink_info_report_serial_number(struct devlink_info_req *req,
> 				      const char *sn)
> {
>+	if (req->compat)
>+		return 0;
> 	return nla_put_string(req->msg, DEVLINK_ATTR_INFO_SERIAL_NUMBER, sn);
> }
> EXPORT_SYMBOL_GPL(devlink_info_report_serial_number);
>@@ -3743,7 +3751,15 @@ int devlink_info_report_version(struct devlink_info_req *req,
> 		[DEVLINK_VERSION_RUNNING] = DEVLINK_ATTR_INFO_VERSION_RUNNING,
> 	};
> 	struct nlattr *nest;
>-	int err;
>+	int len, err;
>+
>+	if (req->compat) {
>+		if (type == DEVLINK_VERSION_RUNNING) {
>+			len = strlcpy(req->buf, version_value, req->len);

If you have multiple running versions, shouldn't you concat them into a
single string for compat?

>+			req->len = max_t(size_t, 0, req->len - len);
>+		}
>+		return 0;
>+	}
> 
> 	if (type >= ARRAY_SIZE(type2attr) || !type2attr[type])
> 		return -EINVAL;
>@@ -3789,6 +3805,7 @@ devlink_nl_info_fill(struct sk_buff *msg, struct devlink *devlink,
> 	if (devlink_nl_put_handle(msg, devlink))
> 		goto err_cancel_msg;
> 
>+	memset(&req, 0, sizeof(req));
> 	req.msg = msg;
> 	err = devlink->ops->info_get(devlink, &req, extack);
> 	if (err)
>@@ -5263,6 +5280,39 @@ int devlink_region_snapshot_create(struct devlink_region *region, u64 data_len,
> }
> EXPORT_SYMBOL_GPL(devlink_region_snapshot_create);
> 
>+void devlink_compat_running_versions(struct net_device *dev,

Why "versionS?"


>+				     char *buf, size_t len)
>+{
>+	struct devlink_port *devlink_port;
>+	struct devlink_info_req req;
>+	struct devlink *devlink;
>+	bool found = false;
>+
>+	mutex_lock(&devlink_mutex);
>+	list_for_each_entry(devlink, &devlink_list, list) {
>+		mutex_lock(&devlink->lock);
>+		list_for_each_entry(devlink_port, &devlink->port_list, list) {
>+			if (devlink_port->type == DEVLINK_PORT_TYPE_ETH ||
>+			    devlink_port->type_dev == dev) {
>+				mutex_unlock(&devlink->lock);
>+				found = true;
>+				goto out;
>+			}
>+		}
>+		mutex_unlock(&devlink->lock);
>+	}
>+out:
>+	if (found && devlink->ops->info_get) {
>+		memset(&req, 0, sizeof(req));
>+		req.compat = true;
>+		req.buf = buf;
>+		req.len = len;
>+
>+		devlink->ops->info_get(devlink, &req, NULL);

I don't really like this compat bool and check in "put" functions.
I would rather like to run info_get() in case of both compat and
non-compat in the same way. Then for compat just pickup the info
which is needed and put to buf.
I don't see problem in using netlink skb for that direcly.


>+	}
>+	mutex_unlock(&devlink_mutex);
>+}
>+
> static int __init devlink_module_init(void)
> {
> 	return genl_register_family(&devlink_nl_family);
>diff --git a/net/core/ethtool.c b/net/core/ethtool.c
>index 158264f7cfaf..176b17d11f08 100644
>--- a/net/core/ethtool.c
>+++ b/net/core/ethtool.c
>@@ -27,6 +27,7 @@
> #include <linux/rtnetlink.h>
> #include <linux/sched/signal.h>
> #include <linux/net.h>
>+#include <net/devlink.h>
> #include <net/xdp_sock.h>
> 
> /*
>@@ -803,6 +804,12 @@ static noinline_for_stack int ethtool_get_drvinfo(struct net_device *dev,
> 	if (ops->get_eeprom_len)
> 		info.eedump_len = ops->get_eeprom_len(dev);
> 
>+	rtnl_unlock();
>+	if (!info.fw_version[0])
>+		devlink_compat_running_versions(dev, info.fw_version,
>+						ARRAY_SIZE(info.fw_version));

ARRAY_SIZE looks odd here. "sizeof()" would be better I think.


>+	rtnl_lock();
>+
> 	if (copy_to_user(useraddr, &info, sizeof(info)))
> 		return -EFAULT;
> 	return 0;
>-- 
>2.19.2
>
kernel test robot Jan. 31, 2019, 7:05 p.m. UTC | #2 
Hi Jakub,

I love your patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Jakub-Kicinski/devlink-add-device-driver-information-API/20190131-222221
config: i386-randconfig-n0-01300126 (attached as .config)
compiler: gcc-8 (Debian 8.2.0-15) 8.2.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   ld: net/core/ethtool.o: in function `ethtool_get_drvinfo':
>> net/core/ethtool.c:809: undefined reference to `devlink_compat_running_versions'

vim +809 net/core/ethtool.c

   760	
   761	static noinline_for_stack int ethtool_get_drvinfo(struct net_device *dev,
   762							  void __user *useraddr)
   763	{
   764		struct ethtool_drvinfo info;
   765		const struct ethtool_ops *ops = dev->ethtool_ops;
   766	
   767		memset(&info, 0, sizeof(info));
   768		info.cmd = ETHTOOL_GDRVINFO;
   769		if (ops->get_drvinfo) {
   770			ops->get_drvinfo(dev, &info);
   771		} else if (dev->dev.parent && dev->dev.parent->driver) {
   772			strlcpy(info.bus_info, dev_name(dev->dev.parent),
   773				sizeof(info.bus_info));
   774			strlcpy(info.driver, dev->dev.parent->driver->name,
   775				sizeof(info.driver));
   776		} else {
   777			return -EOPNOTSUPP;
   778		}
   779	
   780		/*
   781		 * this method of obtaining string set info is deprecated;
   782		 * Use ETHTOOL_GSSET_INFO instead.
   783		 */
   784		if (ops->get_sset_count) {
   785			int rc;
   786	
   787			rc = ops->get_sset_count(dev, ETH_SS_TEST);
   788			if (rc >= 0)
   789				info.testinfo_len = rc;
   790			rc = ops->get_sset_count(dev, ETH_SS_STATS);
   791			if (rc >= 0)
   792				info.n_stats = rc;
   793			rc = ops->get_sset_count(dev, ETH_SS_PRIV_FLAGS);
   794			if (rc >= 0)
   795				info.n_priv_flags = rc;
   796		}
   797		if (ops->get_regs_len) {
   798			int ret = ops->get_regs_len(dev);
   799	
   800			if (ret > 0)
   801				info.regdump_len = ret;
   802		}
   803	
   804		if (ops->get_eeprom_len)
   805			info.eedump_len = ops->get_eeprom_len(dev);
   806	
   807		rtnl_unlock();
   808		if (!info.fw_version[0])
 > 809			devlink_compat_running_versions(dev, info.fw_version,
   810							ARRAY_SIZE(info.fw_version));
   811		rtnl_lock();
   812	
   813		if (copy_to_user(useraddr, &info, sizeof(info)))
   814			return -EFAULT;
   815		return 0;
   816	}
   817	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
kernel test robot Feb. 1, 2019, 4:06 a.m. UTC | #3 
Hi Jakub,

I love your patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Jakub-Kicinski/devlink-add-device-driver-information-API/20190131-222221
config: m68k-sun3_defconfig (attached as .config)
compiler: m68k-linux-gnu-gcc (Debian 8.2.0-11) 8.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=8.2.0 make.cross ARCH=m68k 

All errors (new ones prefixed by >>):

   m68k-linux-gnu-ld: drivers/rtc/proc.o: in function `is_rtc_hctosys.isra.0':
   proc.c:(.text+0x178): undefined reference to `strcmp'
   m68k-linux-gnu-ld: net/core/ethtool.o: in function `ethtool_get_drvinfo':
>> ethtool.c:(.text+0xc08): undefined reference to `devlink_compat_running_versions'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
diff mbox series

Patch

diff --git a/include/net/devlink.h b/include/net/devlink.h
index c678ed0cb099..b4750e93303a 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -640,6 +640,8 @@  int devlink_info_report_version(struct devlink_info_req *req,
 				enum devlink_version_type type,
 				const char *version_name,
 				const char *version_value);
+void devlink_compat_running_versions(struct net_device *dev,
+				     char *buf, size_t len);
 
 #else
 
@@ -957,6 +959,11 @@  devlink_info_report_version(struct devlink_info_req *req,
 {
 	return 0;
 }
+
+static inline void
+devlink_compat_running_versions(struct net_device *dev, char *buf, size_t len)
+{
+}
 #endif
 
 #endif /* _NET_DEVLINK_H_ */
diff --git a/net/core/devlink.c b/net/core/devlink.c
index e2027d3a5e40..5313e5918ee2 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -3715,12 +3715,18 @@  static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
 }
 
 struct devlink_info_req {
+	bool compat;
 	struct sk_buff *msg;
+	/* For compat call */
+	char *buf;
+	size_t len;
 };
 
 int devlink_info_report_driver_name(struct devlink_info_req *req,
 				    const char *name)
 {
+	if (req->compat)
+		return 0;
 	return nla_put_string(req->msg, DEVLINK_ATTR_INFO_DRV_NAME, name);
 }
 EXPORT_SYMBOL_GPL(devlink_info_report_driver_name);
@@ -3728,6 +3734,8 @@  EXPORT_SYMBOL_GPL(devlink_info_report_driver_name);
 int devlink_info_report_serial_number(struct devlink_info_req *req,
 				      const char *sn)
 {
+	if (req->compat)
+		return 0;
 	return nla_put_string(req->msg, DEVLINK_ATTR_INFO_SERIAL_NUMBER, sn);
 }
 EXPORT_SYMBOL_GPL(devlink_info_report_serial_number);
@@ -3743,7 +3751,15 @@  int devlink_info_report_version(struct devlink_info_req *req,
 		[DEVLINK_VERSION_RUNNING] = DEVLINK_ATTR_INFO_VERSION_RUNNING,
 	};
 	struct nlattr *nest;
-	int err;
+	int len, err;
+
+	if (req->compat) {
+		if (type == DEVLINK_VERSION_RUNNING) {
+			len = strlcpy(req->buf, version_value, req->len);
+			req->len = max_t(size_t, 0, req->len - len);
+		}
+		return 0;
+	}
 
 	if (type >= ARRAY_SIZE(type2attr) || !type2attr[type])
 		return -EINVAL;
@@ -3789,6 +3805,7 @@  devlink_nl_info_fill(struct sk_buff *msg, struct devlink *devlink,
 	if (devlink_nl_put_handle(msg, devlink))
 		goto err_cancel_msg;
 
+	memset(&req, 0, sizeof(req));
 	req.msg = msg;
 	err = devlink->ops->info_get(devlink, &req, extack);
 	if (err)
@@ -5263,6 +5280,39 @@  int devlink_region_snapshot_create(struct devlink_region *region, u64 data_len,
 }
 EXPORT_SYMBOL_GPL(devlink_region_snapshot_create);
 
+void devlink_compat_running_versions(struct net_device *dev,
+				     char *buf, size_t len)
+{
+	struct devlink_port *devlink_port;
+	struct devlink_info_req req;
+	struct devlink *devlink;
+	bool found = false;
+
+	mutex_lock(&devlink_mutex);
+	list_for_each_entry(devlink, &devlink_list, list) {
+		mutex_lock(&devlink->lock);
+		list_for_each_entry(devlink_port, &devlink->port_list, list) {
+			if (devlink_port->type == DEVLINK_PORT_TYPE_ETH ||
+			    devlink_port->type_dev == dev) {
+				mutex_unlock(&devlink->lock);
+				found = true;
+				goto out;
+			}
+		}
+		mutex_unlock(&devlink->lock);
+	}
+out:
+	if (found && devlink->ops->info_get) {
+		memset(&req, 0, sizeof(req));
+		req.compat = true;
+		req.buf = buf;
+		req.len = len;
+
+		devlink->ops->info_get(devlink, &req, NULL);
+	}
+	mutex_unlock(&devlink_mutex);
+}
+
 static int __init devlink_module_init(void)
 {
 	return genl_register_family(&devlink_nl_family);
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 158264f7cfaf..176b17d11f08 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -27,6 +27,7 @@ 
 #include <linux/rtnetlink.h>
 #include <linux/sched/signal.h>
 #include <linux/net.h>
+#include <net/devlink.h>
 #include <net/xdp_sock.h>
 
 /*
@@ -803,6 +804,12 @@  static noinline_for_stack int ethtool_get_drvinfo(struct net_device *dev,
 	if (ops->get_eeprom_len)
 		info.eedump_len = ops->get_eeprom_len(dev);
 
+	rtnl_unlock();
+	if (!info.fw_version[0])
+		devlink_compat_running_versions(dev, info.fw_version,
+						ARRAY_SIZE(info.fw_version));
+	rtnl_lock();
+
 	if (copy_to_user(useraddr, &info, sizeof(info)))
 		return -EFAULT;
 	return 0;