Wed, Jan 30, 2019 at 08:05:13PM CET, jakub.kicin...@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.kicin...@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
>
