From mboxrd@z Thu Jan 1 00:00:00 1970 From: Adrien Mazarguil Subject: Re: [PATCH 2/5] net/mlx5: retrieve device index from Netlink Date: Wed, 14 Mar 2018 18:10:26 +0100 Message-ID: <20180314171026.GM3994@6wind.com> References: <205f899062e6acf2d8886bbbd6dac159023a2c04.1520944256.git.nelio.laranjeiro@6wind.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Yongseok Koh , dev@dpdk.org To: Nelio Laranjeiro Return-path: Received: from mail-wr0-f195.google.com (mail-wr0-f195.google.com [209.85.128.195]) by dpdk.org (Postfix) with ESMTP id 0F17E23B for ; Wed, 14 Mar 2018 18:10:41 +0100 (CET) Received: by mail-wr0-f195.google.com with SMTP id v65so5475966wrc.11 for ; Wed, 14 Mar 2018 10:10:41 -0700 (PDT) Content-Disposition: inline In-Reply-To: <205f899062e6acf2d8886bbbd6dac159023a2c04.1520944256.git.nelio.laranjeiro@6wind.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Tue, Mar 13, 2018 at 01:50:36PM +0100, Nelio Laranjeiro wrote: > This patch new file is not compiled yet, it starts a series necessary to > fix some issues with VF devices. > > Signed-off-by: Nelio Laranjeiro The fact this new file is not compiled in at this point won't be obvious when running git bisect and makes validation more difficult. I suggest to simply merge it into the next patch. A few more comments on this code below. > --- > drivers/net/mlx5/mlx5_vf.c | 134 +++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 134 insertions(+) > create mode 100644 drivers/net/mlx5/mlx5_vf.c > > diff --git a/drivers/net/mlx5/mlx5_vf.c b/drivers/net/mlx5/mlx5_vf.c Regarding the name of this file, it looks to me like all the enclosed functions could work equally well with non-VF devices. While this series is targeted at VFs, internal functions are actually more versatile. I suggest a more generic name, e.g. "mlx5_nl.c". > new file mode 100644 > index 000000000..357407f56 > --- /dev/null > +++ b/drivers/net/mlx5/mlx5_vf.c > @@ -0,0 +1,134 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright 2018 6WIND S.A. > + * Copyright 2018 Mellanox Technologies, Ltd. > + */ > + > +#include > +#include > + > +#include > + > +#include "mlx5.h" > +#include "mlx5_utils.h" > + > +/* Data exchanged between Netlink library and PMD. */ > +struct mlx5_vf_iface { > + struct rte_eth_dev *dev; /**< Pointer to Ethernet device. */ > + int iface_idx; /**< Network Interface index. */ > +}; Minor documentation nits here and elsewhere, you should remove the random capitalization of "Interface", "Message", "Header" and so on in the middle of sentences. > + > +/** > + * Parse Netlink message to retrieve the interface index. > + * > + * @param nh > + * Pointer to Netlink Message Header. > + * @param arg > + * PMD data register with this callback. > + * > + * @return > + * 0 on success, -1 otherwise. Looks like it only returns 0, never -1. You should probably describe that ((struct mlx5_vf_iface)arg)->iface_idx is left to -1 when its index can't be found. > + */ > +static int > +mlx5_vf_iface_idx_cb(struct nlmsghdr *nh, void *arg) > +{ > + struct mlx5_vf_iface *data = arg; > + struct rte_eth_dev *dev = data->dev; > + const struct ether_addr *mac = &dev->data->mac_addrs[0]; > + struct ifinfomsg *iface; > + struct rtattr *attribute; > + int len; > + > + /** > + * Leave right away if the index does not match its initialised value. > + * Interface index has already been found. > + */ > + if (data->iface_idx != -1) > + return 0; > + iface = NLMSG_DATA(nh); > + len = nh->nlmsg_len - NLMSG_LENGTH(sizeof(*iface)); > + for (attribute = IFLA_RTA(iface); > + RTA_OK(attribute, len); > + attribute = RTA_NEXT(attribute, len)) { > + if (attribute->rta_type == IFLA_ADDRESS && > + !memcmp(RTA_DATA(attribute), mac, ETHER_ADDR_LEN)) { Hmm, I'm not sure a matching MAC address is safe enough to determine it's the right netdevice. Could be a bridge (br0) interface or any other virtual thing. Although not great, you should probably rely on /sys as in mlx5_get_ifname(). > +#ifndef NDEBUG > + struct ether_addr m; > + > + memcpy(&m, RTA_DATA(attribute), ETHER_ADDR_LEN); > + DRV_LOG(DEBUG, > + "port %u interace %d MAC address" interace => interface > + " %02X:%02X:%02X:%02X:%02X:%02X", > + dev->data->port_id, > + iface->ifi_index, > + m.addr_bytes[0], m.addr_bytes[1], > + m.addr_bytes[2], m.addr_bytes[3], > + m.addr_bytes[4], m.addr_bytes[5]); > +#endif > + data->iface_idx = iface->ifi_index; > + return 0; > + } > + } > + data->iface_idx = -1; > + return 0; > +} > + > +/** > + * Retrieve interface Netlink interface index. > + * > + * @param dev > + * Pointer to Ethernet device. > + * > + * @return > + * Interface index, a negative errno value otherwise and rte_errno is set. > + */ > +static int > +mlx5_vf_iface_idx(struct rte_eth_dev *dev) > +{ > + struct nl_req { > + struct nlmsghdr hdr; > + struct rtgenmsg gen; > + } req = { > + .hdr = { > + .nlmsg_len = NLMSG_LENGTH(sizeof(struct rtgenmsg)), > + .nlmsg_type = RTM_GETLINK, > + .nlmsg_flags = NLM_F_REQUEST | NLM_F_DUMP | NLM_F_ROOT, > + }, > + .gen = { > + .rtgen_family = AF_UNSPEC, Extra space before AF_UNSPEC. > + }, > + }; > + struct mlx5_vf_iface data = { > + .dev = dev, > + .iface_idx = -1, > + }; > + int fd; > + int ret; > + > + fd = rte_nl_init(RTMGRP_LINK); > + if (fd < 0) { > + rte_errno = errno; > + goto error; > + } > + ret = rte_nl_send(fd, &req.hdr); > + if (ret == -1) { > + rte_errno = errno; > + goto error; > + } > + ret = rte_nl_recv(fd, mlx5_vf_iface_idx_cb, &data); > + if (ret == -1) { > + rte_errno = errno; > + goto error; > + } > + rte_nl_final(fd); > + if (data.iface_idx == -1) { > + rte_errno = EAGAIN; > + goto error; > + } > + return data.iface_idx; > +error: > + if (fd >= 0) > + rte_nl_final(fd); > + DRV_LOG(DEBUG, "port %u cannot retrieve Netlink Interface index %s", > + dev->data->port_id, strerror(rte_errno)); > + return -rte_errno; > +} > -- > 2.11.0 > Due to possible MAC addresses collisions, I suggest a simpler approach: replacing all this code with a combination of mlx5_get_ifname() and if_nametoindex(). Thoughts? -- Adrien Mazarguil 6WIND