From: Jiri Pirko <jiri@resnulli.us>
To: Michal Kubecek <mkubecek@suse.cz>
Cc: David Miller <davem@davemloft.net>,
netdev@vger.kernel.org,
Jakub Kicinski <jakub.kicinski@netronome.com>,
Andrew Lunn <andrew@lunn.ch>,
Florian Fainelli <f.fainelli@gmail.com>,
John Linville <linville@tuxdriver.com>,
Stephen Hemminger <stephen@networkplumber.org>,
Johannes Berg <johannes@sipsolutions.net>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next v7 14/17] ethtool: set link settings with LINKINFO_SET request
Date: Thu, 10 Oct 2019 17:37:54 +0200 [thread overview]
Message-ID: <20191010153754.GA2901@nanopsycho> (raw)
In-Reply-To: <aef31ba798d1cfa2ae92d333ad1547f4b528ffa8.1570654310.git.mkubecek@suse.cz>
Wed, Oct 09, 2019 at 10:59:43PM CEST, mkubecek@suse.cz wrote:
>Implement LINKINFO_SET netlink request to set link settings queried by
>LINKINFO_GET message.
>
>Only physical port, phy MDIO address and MDI(-X) control can be set,
>attempt to modify MDI(-X) status and transceiver is rejected.
>
>When any data is modified, ETHTOOL_MSG_LINKINFO_NTF message in the same
>format as reply to LINKINFO_GET request is sent to notify userspace about
>the changes. The same notification is also sent when these settings are
>modified using the ioctl interface.
>
It is a bit confusing and harder to follow when you have set and notify
code in the same patch. Could you please split?
[...]
>+/* LINKINFO_SET */
>+
>+static const struct nla_policy linkinfo_hdr_policy[ETHTOOL_A_HEADER_MAX + 1] = {
>+ [ETHTOOL_A_HEADER_UNSPEC] = { .type = NLA_REJECT },
>+ [ETHTOOL_A_HEADER_DEV_INDEX] = { .type = NLA_U32 },
>+ [ETHTOOL_A_HEADER_DEV_NAME] = { .type = NLA_NUL_STRING,
>+ .len = IFNAMSIZ - 1 },
>+ [ETHTOOL_A_HEADER_GFLAGS] = { .type = NLA_U32 },
>+ [ETHTOOL_A_HEADER_RFLAGS] = { .type = NLA_REJECT },
>+};
This is what I was talking about in the other email. These common attrs
should have common policy and should be parsed by generic netlink code
by default and be available for ethnl_set_linkinfo() in info->attrs.
>+
>+static const struct nla_policy
>+linkinfo_set_policy[ETHTOOL_A_LINKINFO_MAX + 1] = {
>+ [ETHTOOL_A_LINKINFO_UNSPEC] = { .type = NLA_REJECT },
>+ [ETHTOOL_A_LINKINFO_HEADER] = { .type = NLA_NESTED },
>+ [ETHTOOL_A_LINKINFO_PORT] = { .type = NLA_U8 },
>+ [ETHTOOL_A_LINKINFO_PHYADDR] = { .type = NLA_U8 },
>+ [ETHTOOL_A_LINKINFO_TP_MDIX] = { .type = NLA_REJECT },
>+ [ETHTOOL_A_LINKINFO_TP_MDIX_CTRL] = { .type = NLA_U8 },
>+ [ETHTOOL_A_LINKINFO_TRANSCEIVER] = { .type = NLA_REJECT },
>+};
>+
>+int ethnl_set_linkinfo(struct sk_buff *skb, struct genl_info *info)
>+{
>+ struct nlattr *tb[ETHTOOL_A_LINKINFO_MAX + 1];
>+ struct ethtool_link_ksettings ksettings = {};
>+ struct ethtool_link_settings *lsettings;
>+ struct ethnl_req_info req_info = {};
>+ struct net_device *dev;
>+ bool mod = false;
>+ int ret;
>+
>+ ret = nlmsg_parse(info->nlhdr, GENL_HDRLEN, tb,
>+ ETHTOOL_A_LINKINFO_MAX, linkinfo_set_policy,
>+ info->extack);
Yeah, genl code should do this parse..
>+ if (ret < 0)
>+ return ret;
>+ ret = ethnl_parse_header(&req_info, tb[ETHTOOL_A_LINKINFO_HEADER],
>+ genl_info_net(info), info->extack,
>+ linkinfo_hdr_policy, true);
and pre_doit should do this one.
>+ if (ret < 0)
>+ return ret;
>+ dev = req_info.dev;
>+ if (!dev->ethtool_ops->get_link_ksettings ||
>+ !dev->ethtool_ops->set_link_ksettings)
>+ return -EOPNOTSUPP;
>+
>+ rtnl_lock();
>+ ret = ethnl_before_ops(dev);
>+ if (ret < 0)
>+ goto out_rtnl;
>+
>+ ret = __ethtool_get_link_ksettings(dev, &ksettings);
>+ if (ret < 0) {
>+ if (info)
>+ GENL_SET_ERR_MSG(info, "failed to retrieve link settings");
>+ goto out_ops;
>+ }
>+ lsettings = &ksettings.base;
>+
>+ ethnl_update_u8(&lsettings->port, tb[ETHTOOL_A_LINKINFO_PORT], &mod);
>+ ethnl_update_u8(&lsettings->phy_address, tb[ETHTOOL_A_LINKINFO_PHYADDR],
>+ &mod);
>+ ethnl_update_u8(&lsettings->eth_tp_mdix_ctrl,
>+ tb[ETHTOOL_A_LINKINFO_TP_MDIX_CTRL], &mod);
>+
>+ ret = 0;
>+ if (mod) {
if (!mod)
goto out_ops;
?
>+ ret = dev->ethtool_ops->set_link_ksettings(dev, &ksettings);
>+ if (ret < 0)
>+ GENL_SET_ERR_MSG(info, "link settings update failed");
>+ else
>+ ethtool_notify(dev, ETHTOOL_MSG_LINKINFO_NTF, NULL);
>+ }
>+
>+out_ops:
>+ ethnl_after_ops(dev);
>+out_rtnl:
>+ rtnl_unlock();
>+ dev_put(dev);
>+ return ret;
>+}
>diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
>index dc52d912e0dd..5b9d12656e97 100644
>--- a/net/ethtool/netlink.c
>+++ b/net/ethtool/netlink.c
>@@ -603,6 +603,11 @@ static int ethnl_get_done(struct netlink_callback *cb)
>
> static const struct get_request_ops *ethnl_std_notify_to_ops(unsigned int cmd)
> {
>+ switch (cmd) {
>+ case ETHTOOL_MSG_LINKINFO_NTF:
>+ return &linkinfo_request_ops;
>+ };
>+
> WARN_ONCE(1, "unexpected notification type %u\n", cmd);
> return NULL;
> }
>@@ -683,6 +688,7 @@ typedef void (*ethnl_notify_handler_t)(struct net_device *dev, unsigned int cmd,
> const void *data);
>
> static const ethnl_notify_handler_t ethnl_notify_handlers[] = {
>+ [ETHTOOL_MSG_LINKINFO_NTF] = ethnl_std_notify,
Correct me if I'm wrong, but this is the only notification I found in
this patchset. Do you expect other then ethnl_std_notify() handler?
Bacause otherwise this can ba simplified down to just a single table
similar you have for GET.
> };
>
> void ethtool_notify(struct net_device *dev, unsigned int cmd, const void *data)
>@@ -717,6 +723,11 @@ static const struct genl_ops ethtool_genl_ops[] = {
> .dumpit = ethnl_get_dumpit,
> .done = ethnl_get_done,
> },
>+ {
>+ .cmd = ETHTOOL_MSG_LINKINFO_SET,
>+ .flags = GENL_UNS_ADMIN_PERM,
>+ .doit = ethnl_set_linkinfo,
>+ },
> };
>
> static const struct genl_multicast_group ethtool_nl_mcgrps[] = {
>diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
>index 23e82a4dd265..ca136dd7ea02 100644
>--- a/net/ethtool/netlink.h
>+++ b/net/ethtool/netlink.h
>@@ -350,4 +350,6 @@ struct get_request_ops {
> extern const struct get_request_ops strset_request_ops;
> extern const struct get_request_ops linkinfo_request_ops;
>
>+int ethnl_set_linkinfo(struct sk_buff *skb, struct genl_info *info);
>+
> #endif /* _NET_ETHTOOL_NETLINK_H */
>--
>2.23.0
>
next prev parent reply other threads:[~2019-10-10 15:38 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-09 20:59 [PATCH net-next v7 00/17] ethtool netlink interface, part 1 Michal Kubecek
2019-10-09 20:59 ` [PATCH net-next v7 01/17] rtnetlink: provide permanent hardware address in RTM_NEWLINK Michal Kubecek
2019-10-09 20:59 ` [PATCH net-next v7 02/17] netlink: rename nl80211_validate_nested() to nla_validate_nested() Michal Kubecek
2019-10-09 20:59 ` [PATCH net-next v7 03/17] ethtool: move to its own directory Michal Kubecek
2019-10-09 20:59 ` [PATCH net-next v7 04/17] ethtool: introduce ethtool netlink interface Michal Kubecek
2019-10-09 20:59 ` [PATCH net-next v7 05/17] ethtool: helper functions for " Michal Kubecek
2019-10-10 13:42 ` Jiri Pirko
2019-10-10 17:13 ` Michal Kubecek
2019-10-09 20:59 ` [PATCH net-next v7 06/17] ethtool: netlink bitset handling Michal Kubecek
2019-10-11 13:34 ` Jiri Pirko
2019-10-14 11:18 ` Michal Kubecek
2019-10-14 13:02 ` Jiri Pirko
2019-10-21 7:18 ` Michal Kubecek
2019-10-09 20:59 ` [PATCH net-next v7 07/17] ethtool: support for netlink notifications Michal Kubecek
2019-10-09 20:59 ` [PATCH net-next v7 08/17] ethtool: move string arrays into common file Michal Kubecek
2019-10-10 13:27 ` Jiri Pirko
2019-10-09 20:59 ` [PATCH net-next v7 09/17] ethtool: generic handlers for GET requests Michal Kubecek
2019-10-10 13:56 ` Jiri Pirko
2019-10-10 18:04 ` Michal Kubecek
2019-10-10 18:18 ` Johannes Berg
2019-10-10 20:00 ` Michal Kubecek
2019-10-11 8:08 ` Johannes Berg
2019-10-11 6:06 ` Jiri Pirko
2019-10-10 15:23 ` Jiri Pirko
2019-10-09 20:59 ` [PATCH net-next v7 10/17] ethtool: provide string sets with STRSET_GET request Michal Kubecek
2019-10-10 13:59 ` Jiri Pirko
2019-10-10 18:05 ` Michal Kubecek
2019-10-09 20:59 ` [PATCH net-next v7 11/17] ethtool: provide link mode names as a string set Michal Kubecek
2019-10-09 20:59 ` [PATCH net-next v7 12/17] ethtool: provide link settings with LINKINFO_GET request Michal Kubecek
2019-10-10 15:59 ` Jiri Pirko
2019-10-10 20:15 ` Michal Kubecek
2019-10-09 20:59 ` [PATCH net-next v7 13/17] ethtool: add standard notification handler Michal Kubecek
2019-10-10 15:25 ` Jiri Pirko
2019-10-10 18:17 ` Michal Kubecek
2019-10-11 5:56 ` Jiri Pirko
2019-10-11 5:59 ` Michal Kubecek
2019-10-09 20:59 ` [PATCH net-next v7 14/17] ethtool: set link settings with LINKINFO_SET request Michal Kubecek
2019-10-10 15:37 ` Jiri Pirko [this message]
2019-10-10 19:30 ` Michal Kubecek
2019-10-11 5:59 ` Jiri Pirko
2019-10-12 16:33 ` Jiri Pirko
2019-10-14 8:48 ` Michal Kubecek
2019-10-09 20:59 ` [PATCH net-next v7 15/17] ethtool: provide link mode information with LINKMODES_GET request Michal Kubecek
2019-10-09 20:59 ` [PATCH net-next v7 16/17] ethtool: set link modes related data with LINKMODES_SET request Michal Kubecek
2019-10-09 20:59 ` [PATCH net-next v7 17/17] ethtool: provide link state with LINKSTATE_GET request Michal Kubecek
2019-10-11 0:48 ` [PATCH net-next v7 00/17] ethtool netlink interface, part 1 Jakub Kicinski
2019-10-11 6:46 ` Johannes Berg
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191010153754.GA2901@nanopsycho \
--to=jiri@resnulli.us \
--cc=andrew@lunn.ch \
--cc=davem@davemloft.net \
--cc=f.fainelli@gmail.com \
--cc=jakub.kicinski@netronome.com \
--cc=johannes@sipsolutions.net \
--cc=linux-kernel@vger.kernel.org \
--cc=linville@tuxdriver.com \
--cc=mkubecek@suse.cz \
--cc=netdev@vger.kernel.org \
--cc=stephen@networkplumber.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).