| Message ID | 20221031124248.484405-14-jiri@resnulli.us (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: fix netdev to devlink_port linkage and expose to user | expand |
On Mon, 31 Oct 2022 13:42:48 +0100 Jiri Pirko wrote: > From: Jiri Pirko <jiri@nvidia.com> > > Expose devlink port handle related to netdev over rtnetlink. Introduce a > new nested IFLA attribute to carry the info. Call into devlink code to > fill-up the nest with existing devlink attributes that are used over > devlink netlink. > +int devlink_nl_port_handle_fill(struct sk_buff *msg, struct devlink_port *devlink_port) > +{ > + if (devlink_nl_put_handle(msg, devlink_port->devlink)) > + return -EMSGSIZE; > + if (nla_put_u32(msg, DEVLINK_ATTR_PORT_INDEX, devlink_port->index)) > + return -EMSGSIZE; > + return 0; > +} > +EXPORT_SYMBOL_GPL(devlink_nl_port_handle_fill); > + > +size_t devlink_nl_port_handle_size(struct devlink_port *devlink_port) > +{ > + struct devlink *devlink = devlink_port->devlink; > + > + return nla_total_size(strlen(devlink->dev->bus->name) + 1) /* DEVLINK_ATTR_BUS_NAME */ > + + nla_total_size(strlen(dev_name(devlink->dev)) + 1) /* DEVLINK_ATTR_DEV_NAME */ > + + nla_total_size(4); /* DEVLINK_ATTR_PORT_INDEX */ > +} > +EXPORT_SYMBOL_GPL(devlink_nl_port_handle_size); Why the exports? devlink is a boolean now IIRC. > +static int rtnl_fill_devlink_port(struct sk_buff *skb, > + const struct net_device *dev) > +{ > + struct nlattr *devlink_port_nest; > + int ret; > + > + devlink_port_nest = nla_nest_start(skb, IFLA_DEVLINK_PORT); > + if (!devlink_port_nest) > + return -EMSGSIZE; > + > + if (dev->devlink_port) { Why produce the empty nest if port is not set? > + ret = devlink_nl_port_handle_fill(skb, dev->devlink_port); > + if (ret < 0) > + goto nest_cancel; > + } > + > + nla_nest_end(skb, devlink_port_nest); > + return 0; > + > +nest_cancel: > + nla_nest_cancel(skb, devlink_port_nest); > + return ret; > +}
Tue, Nov 01, 2022 at 05:18:34PM CET, kuba@kernel.org wrote: >On Mon, 31 Oct 2022 13:42:48 +0100 Jiri Pirko wrote: >> From: Jiri Pirko <jiri@nvidia.com> >> >> Expose devlink port handle related to netdev over rtnetlink. Introduce a >> new nested IFLA attribute to carry the info. Call into devlink code to >> fill-up the nest with existing devlink attributes that are used over >> devlink netlink. > >> +int devlink_nl_port_handle_fill(struct sk_buff *msg, struct devlink_port *devlink_port) >> +{ >> + if (devlink_nl_put_handle(msg, devlink_port->devlink)) >> + return -EMSGSIZE; >> + if (nla_put_u32(msg, DEVLINK_ATTR_PORT_INDEX, devlink_port->index)) >> + return -EMSGSIZE; >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(devlink_nl_port_handle_fill); >> + >> +size_t devlink_nl_port_handle_size(struct devlink_port *devlink_port) >> +{ >> + struct devlink *devlink = devlink_port->devlink; >> + >> + return nla_total_size(strlen(devlink->dev->bus->name) + 1) /* DEVLINK_ATTR_BUS_NAME */ >> + + nla_total_size(strlen(dev_name(devlink->dev)) + 1) /* DEVLINK_ATTR_DEV_NAME */ >> + + nla_total_size(4); /* DEVLINK_ATTR_PORT_INDEX */ >> +} >> +EXPORT_SYMBOL_GPL(devlink_nl_port_handle_size); > >Why the exports? devlink is a boolean now IIRC. Right, removed. > >> +static int rtnl_fill_devlink_port(struct sk_buff *skb, >> + const struct net_device *dev) >> +{ >> + struct nlattr *devlink_port_nest; >> + int ret; >> + >> + devlink_port_nest = nla_nest_start(skb, IFLA_DEVLINK_PORT); >> + if (!devlink_port_nest) >> + return -EMSGSIZE; >> + >> + if (dev->devlink_port) { > >Why produce the empty nest if port is not set? Empty nest indicates that kernel supports this but there is no devlink port associated. I see no other way to indicate this :/ > >> + ret = devlink_nl_port_handle_fill(skb, dev->devlink_port); >> + if (ret < 0) >> + goto nest_cancel; >> + } >> + >> + nla_nest_end(skb, devlink_port_nest); >> + return 0; >> + >> +nest_cancel: >> + nla_nest_cancel(skb, devlink_port_nest); >> + return ret; >> +}
On Wed, 2 Nov 2022 12:22:09 +0100 Jiri Pirko wrote: >> Why produce the empty nest if port is not set? > > Empty nest indicates that kernel supports this but there is no devlink > port associated. I see no other way to indicate this :/ Maybe it's time to plumb policies thru to classic netlink, instead of creating weird attribute constructs?
On Wed, 2 Nov 2022 08:10:06 -0700 Jakub Kicinski wrote: > On Wed, 2 Nov 2022 12:22:09 +0100 Jiri Pirko wrote: > >> Why produce the empty nest if port is not set? > > > > Empty nest indicates that kernel supports this but there is no devlink > > port associated. I see no other way to indicate this :/ > > Maybe it's time to plumb policies thru to classic netlink, instead of > creating weird attribute constructs? Not a blocker, FWIW, just pointing out a better alternative.
Wed, Nov 02, 2022 at 04:13:25PM CET, kuba@kernel.org wrote: >On Wed, 2 Nov 2022 08:10:06 -0700 Jakub Kicinski wrote: >> On Wed, 2 Nov 2022 12:22:09 +0100 Jiri Pirko wrote: >> >> Why produce the empty nest if port is not set? >> > >> > Empty nest indicates that kernel supports this but there is no devlink >> > port associated. I see no other way to indicate this :/ >> >> Maybe it's time to plumb policies thru to classic netlink, instead of >> creating weird attribute constructs? > >Not a blocker, FWIW, just pointing out a better alternative. Or, even better, move RTnetlink to generic netlink. Really, there is no point to have it as non-generic netlink forever. We moved ethtool there, why not RTnetlink?
On Wed, 2 Nov 2022 16:37:00 +0100 Jiri Pirko wrote: > >> Maybe it's time to plumb policies thru to classic netlink, instead of > >> creating weird attribute constructs? > > > >Not a blocker, FWIW, just pointing out a better alternative. > > Or, even better, move RTnetlink to generic netlink. Really, there is no > point to have it as non-generic netlink forever. We moved ethtool there, > why not RTnetlink? As a rewrite? We could plug in the same callbacks into a genl family but the replies / notifications would have different headers depending on the socket type which gets hairy, no?
Wed, Nov 02, 2022 at 04:52:49PM CET, kuba@kernel.org wrote: >On Wed, 2 Nov 2022 16:37:00 +0100 Jiri Pirko wrote: >> >> Maybe it's time to plumb policies thru to classic netlink, instead of >> >> creating weird attribute constructs? >> > >> >Not a blocker, FWIW, just pointing out a better alternative. >> >> Or, even better, move RTnetlink to generic netlink. Really, there is no >> point to have it as non-generic netlink forever. We moved ethtool there, >> why not RTnetlink? > >As a rewrite? We could plug in the same callbacks into a genl family >but the replies / notifications would have different headers depending >on the socket type which gets hairy, no? I mean like ethtool, completely side iface, independent, new attrs etc. We can start with NetdevNetlink for example. Just cover netdev part of RTNetlink. That is probably most interesting anyway.
On Wed, 2 Nov 2022 16:59:28 +0100 Jiri Pirko wrote: > Wed, Nov 02, 2022 at 04:52:49PM CET, kuba@kernel.org wrote: > >On Wed, 2 Nov 2022 16:37:00 +0100 Jiri Pirko wrote: > >> Or, even better, move RTnetlink to generic netlink. Really, there is no > >> point to have it as non-generic netlink forever. We moved ethtool there, > >> why not RTnetlink? > > > >As a rewrite? We could plug in the same callbacks into a genl family > >but the replies / notifications would have different headers depending > >on the socket type which gets hairy, no? > > I mean like ethtool, completely side iface, independent, new attrs etc. > We can start with NetdevNetlink for example. Just cover netdev part of > RTNetlink. That is probably most interesting anyway. That came up in conversations about the YAML specs. Major effort but may be worth doing.
diff --git a/include/net/devlink.h b/include/net/devlink.h index 7befad57afd4..fa6e936af1a5 100644 --- a/include/net/devlink.h +++ b/include/net/devlink.h @@ -1873,6 +1873,9 @@ int devlink_compat_phys_port_name_get(struct net_device *dev, int devlink_compat_switch_id_get(struct net_device *dev, struct netdev_phys_item_id *ppid); +int devlink_nl_port_handle_fill(struct sk_buff *msg, struct devlink_port *devlink_port); +size_t devlink_nl_port_handle_size(struct devlink_port *devlink_port); + #else static inline struct devlink *devlink_try_get(struct devlink *devlink) @@ -1909,6 +1912,17 @@ devlink_compat_switch_id_get(struct net_device *dev, return -EOPNOTSUPP; } +static inline int +devlink_nl_port_handle_fill(struct sk_buff *msg, struct devlink_port *devlink_port) +{ + return 0; +} + +static inline size_t devlink_nl_port_handle_size(struct devlink_port *devlink_port) +{ + return 0; +} + #endif #endif /* _NET_DEVLINK_H_ */ diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h index 5e7a1041df3a..9af9da1db4e8 100644 --- a/include/uapi/linux/if_link.h +++ b/include/uapi/linux/if_link.h @@ -372,6 +372,8 @@ enum { IFLA_TSO_MAX_SEGS, IFLA_ALLMULTI, /* Allmulti count: > 0 means acts ALLMULTI */ + IFLA_DEVLINK_PORT, + __IFLA_MAX }; diff --git a/net/core/devlink.c b/net/core/devlink.c index 3a454d0045e5..7277169d0b1e 100644 --- a/net/core/devlink.c +++ b/net/core/devlink.c @@ -880,6 +880,26 @@ static int devlink_nl_put_nested_handle(struct sk_buff *msg, struct devlink *dev return -EMSGSIZE; } +int devlink_nl_port_handle_fill(struct sk_buff *msg, struct devlink_port *devlink_port) +{ + if (devlink_nl_put_handle(msg, devlink_port->devlink)) + return -EMSGSIZE; + if (nla_put_u32(msg, DEVLINK_ATTR_PORT_INDEX, devlink_port->index)) + return -EMSGSIZE; + return 0; +} +EXPORT_SYMBOL_GPL(devlink_nl_port_handle_fill); + +size_t devlink_nl_port_handle_size(struct devlink_port *devlink_port) +{ + struct devlink *devlink = devlink_port->devlink; + + return nla_total_size(strlen(devlink->dev->bus->name) + 1) /* DEVLINK_ATTR_BUS_NAME */ + + nla_total_size(strlen(dev_name(devlink->dev)) + 1) /* DEVLINK_ATTR_DEV_NAME */ + + nla_total_size(4); /* DEVLINK_ATTR_PORT_INDEX */ +} +EXPORT_SYMBOL_GPL(devlink_nl_port_handle_size); + struct devlink_reload_combination { enum devlink_reload_action action; enum devlink_reload_limit limit; diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index 74864dc46a7e..e034c0c8e6cc 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -53,6 +53,7 @@ #include <net/fib_rules.h> #include <net/rtnetlink.h> #include <net/net_namespace.h> +#include <net/devlink.h> #include "dev.h" @@ -1038,6 +1039,16 @@ static size_t rtnl_proto_down_size(const struct net_device *dev) return size; } +static size_t rtnl_devlink_port_size(const struct net_device *dev) +{ + size_t size = nla_total_size(0); /* nest IFLA_DEVLINK_PORT */ + + if (dev->devlink_port) + size += devlink_nl_port_handle_size(dev->devlink_port); + + return size; +} + static noinline size_t if_nlmsg_size(const struct net_device *dev, u32 ext_filter_mask) { @@ -1091,6 +1102,7 @@ static noinline size_t if_nlmsg_size(const struct net_device *dev, + nla_total_size(4) /* IFLA_MAX_MTU */ + rtnl_prop_list_size(dev) + nla_total_size(MAX_ADDR_LEN) /* IFLA_PERM_ADDRESS */ + + rtnl_devlink_port_size(dev) + 0; } @@ -1728,6 +1740,30 @@ static int rtnl_fill_proto_down(struct sk_buff *skb, return -EMSGSIZE; } +static int rtnl_fill_devlink_port(struct sk_buff *skb, + const struct net_device *dev) +{ + struct nlattr *devlink_port_nest; + int ret; + + devlink_port_nest = nla_nest_start(skb, IFLA_DEVLINK_PORT); + if (!devlink_port_nest) + return -EMSGSIZE; + + if (dev->devlink_port) { + ret = devlink_nl_port_handle_fill(skb, dev->devlink_port); + if (ret < 0) + goto nest_cancel; + } + + nla_nest_end(skb, devlink_port_nest); + return 0; + +nest_cancel: + nla_nest_cancel(skb, devlink_port_nest); + return ret; +} + static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev, struct net *src_net, int type, u32 pid, u32 seq, u32 change, @@ -1865,6 +1901,9 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, dev->dev.parent->bus->name)) goto nla_put_failure; + if (rtnl_fill_devlink_port(skb, dev)) + goto nla_put_failure; + nlmsg_end(skb, nlh); return 0;