From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f67.google.com (mail-wr1-f67.google.com [209.85.221.67]) by dpdk.org (Postfix) with ESMTP id 4873014EC for ; Mon, 23 Jul 2018 08:53:46 +0200 (CEST) Received: by mail-wr1-f67.google.com with SMTP id t6-v6so16697910wrn.7 for ; Sun, 22 Jul 2018 23:53:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to:openpgp :user-agent; bh=Nc5Bio+HRMdDIZAqidCAeFA24G+WQ+Y56fRSCinZ5js=; b=HfhSSMdz0ZG7p6XqEK6dnYGqVrNjH9p/dBpTeKFiX4bzUeGjTPqUYuVgbINUE4Purg 2Xegumq8T2al1vhFOe2FcsCgE9mX8ZArYXlmWJjRAtO2p9IZuXWJDVdWIY7wZA+diUBl /smb3mhmguPg2Oq8tDaRna5Dr8VVvnSrrQ+PLmYRikQTqRavdpJ2+606TKNJcK20n/tu ey8zKl051cnaC514mgdp/3s32I5x8vDFKlb00GT+YJNqS65XA4U0eFwxac5mxk3QPdki dEppfTkFYTJe325xRT5Wg/YFIO0fgFHrolDP10k7mSqjXCiMY3b8o1F+Lw2Dhw7/xEKK fIgw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:openpgp:user-agent; bh=Nc5Bio+HRMdDIZAqidCAeFA24G+WQ+Y56fRSCinZ5js=; b=bAikKpOnYxVpQ5JGeGPz3iy1dxbCGS7Pd8wi/ebIXWywDTmB/gfjLzcASsczhprmze xkynZlCoLYlWI6hct1UkSK9ir1MelhcRX/2zB6MZ9YRFAsBYBTiWO/6wsBuG9xFH5YD3 y/EtwdH1C1SzsSYRz6hB6+P3tgsAQBZVZZ1Cxh6SIH8+a+rLEaZ1Nmdy0Bht6N28oof/ ATMgQ9/U6BkxyUuYpljzZnMplE1FgD7v2kN+glYOSrjrzIWRICSOxrb03nMjId9FQhVL Xr+PpBMggVx869LITsqZKp9RAGhU1F3eeeapVz+QeoT0GMQ6p7ui6qVav2oSlQIMY6eH QqCQ== X-Gm-Message-State: AOUpUlGBG4BWRLceYkk9oy0yUTAjdNfCAQoEnSv23fvAuPI7zOTs1qP4 PCuCaV3mR8GYq8F9ZECID3mN X-Google-Smtp-Source: AAOMgpe7EQmNgZ2gJ3NlHD6s9zrSc7oOOm6u1hBOI1kVcCAVJyjqMWQ9du1JSs83x1MhMXUpB83r8w== X-Received: by 2002:adf:cd0e:: with SMTP id w14-v6mr7050225wrm.226.1532328826000; Sun, 22 Jul 2018 23:53:46 -0700 (PDT) Received: from laranjeiro-vm.dev.6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id y102-v6sm10628987wmh.9.2018.07.22.23.53.45 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Sun, 22 Jul 2018 23:53:45 -0700 (PDT) Date: Mon, 23 Jul 2018 08:53:37 +0200 From: =?iso-8859-1?Q?N=E9lio?= Laranjeiro To: Shahaf Shuler Cc: "dev@dpdk.org" , Yongseok Koh Message-ID: <20180723065337.b4ckrtiobq2bixhv@laranjeiro-vm.dev.6wind.com> References: MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: OpenPGP: id=A0075DA8F66A5949 preference=signencrypt User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH] net/mlx5: fix representors detection X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 23 Jul 2018 06:53:46 -0000 On Sun, Jul 22, 2018 at 11:18:34AM +0000, Shahaf Shuler wrote: > Hi Nelio, > > Friday, July 13, 2018 4:54 PM, Nelio Laranjeiro: > > Subject: [PATCH] net/mlx5: fix representors detection > > > > On systems where the required Netlink commands are not supported but > > Mellanox OFED is installed, representors information must be retrieved > > through sysfs. > > > > Fixes: 4c10141488fe ("net/mlx5: add port representor awareness") > > > > Signed-off-by: Nelio Laranjeiro > > Acked-by: Adrien Mazarguil > > --- > > drivers/net/mlx5/mlx5.c | 7 +++-- > > drivers/net/mlx5/mlx5.h | 2 ++ > > drivers/net/mlx5/mlx5_ethdev.c | 53 > > ++++++++++++++++++++++++++++++++++ > > 3 files changed, 60 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c index > > df7f39844..8174947f3 100644 > > --- a/drivers/net/mlx5/mlx5.c > > +++ b/drivers/net/mlx5/mlx5.c > > @@ -1306,7 +1306,8 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv > > __rte_unused, > > * Netlink calls assuming kernel drivers are recent enough to > > * support them. > > * > > - * In the event of identification failure through Netlink, either: > > + * In the event of identification failure through Netlink, try again > > + * through sysfs, then either: > > * > > * 1. No device matches (n == 0), complain and bail out. > > * 2. A single IB device matches (n == 1) and is not a representor, @@ > > -1325,7 +1326,9 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv > > __rte_unused, > > if (nl_route < 0 || > > !list[i].ifindex || > > mlx5_nl_switch_info(nl_route, list[i].ifindex, > > - &list[i].info)) { > > + &list[i].info) || > > + ((!list[i].info.representor && !list[i].info.master) && > > + mlx5_sysfs_switch_info(list[i].ifindex, &list[i].info))) { > > I find this logic a bit complex to be under a single if statement. Can > you simplify it w/ local variables? It won't be so easy has the second step depends on the result of the mlx5_nl_switch_info() call. To split this if statement into two, it will be a full copy of the first block by replacing only the call to mlx5_nl_switch_info(). > > list[i].ifindex = 0; > > memset(&list[i].info, 0, sizeof(list[i].info)); > > continue; > > diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index > > cc01310e0..fff88b1d6 100644 > > --- a/drivers/net/mlx5/mlx5.h > > +++ b/drivers/net/mlx5/mlx5.h > > @@ -254,6 +254,8 @@ eth_rx_burst_t mlx5_select_rx_function(struct > > rte_eth_dev *dev); unsigned int mlx5_dev_to_port_id(const struct > > rte_device *dev, > > uint16_t *port_list, > > unsigned int port_list_n); > > +int mlx5_sysfs_switch_info(unsigned int ifindex, > > + struct mlx5_switch_info *info); > > > > /* mlx5_mac.c */ > > > > diff --git a/drivers/net/mlx5/mlx5_ethdev.c > > b/drivers/net/mlx5/mlx5_ethdev.c index 05f66f7b6..101be4791 100644 > > --- a/drivers/net/mlx5/mlx5_ethdev.c > > +++ b/drivers/net/mlx5/mlx5_ethdev.c > > @@ -1296,3 +1296,56 @@ mlx5_dev_to_port_id(const struct rte_device > > *dev, uint16_t *port_list, > > } > > return n; > > } > > + > > +/** > > + * Get switch information associated with network interface. > > + * > > + * @param ifindex > > + * Network interface index. > > + * @param[out] info > > + * Switch information object, populated in case of success. > > + * > > + * @return > > + * 0 on success, a negative errno value otherwise and rte_errno is set. > > + */ > > +int > > +mlx5_sysfs_switch_info(unsigned int ifindex, struct mlx5_switch_info > > +*info) { > > + char ifname[IF_NAMESIZE]; > > + FILE *file; > > + struct mlx5_switch_info data = { .master = 0, }; > > + bool port_name_set = false; > > + bool port_switch_id_set = false; > > + char c; > > + > > + if (!if_indextoname(ifindex, ifname)) { > > + rte_errno = errno; > > + return -rte_errno; > > + } > > + > > + MKSTR(phys_port_name, "/sys/class/net/%s/phys_port_name", > > + ifname); > > + MKSTR(phys_switch_id, "/sys/class/net/%s/phys_switch_id", > > + ifname); > > + > > + file = fopen(phys_port_name, "rb"); > > + if (file == NULL) > > + goto error; > > This goto is not correct. > Representors will indeed be able to read, but PFs will not be able to > open the phys_port_name (as there is none). >>From my testing, this is wrong, the PF can open the file, but it cannot read (verified with strace). > The phys_switch_id is accessible form both. > > I pasted a suggestion below[1], let me know what you think. > > > + port_name_set = fscanf(file, "%d%c", &data.port_name, &c) == 2 > > && > > + c == '\n'; > > + fclose(file); > > + file = fopen(phys_switch_id, "rb"); > > + if (file == NULL) > > + goto error; > > + port_switch_id_set = > > + fscanf(file, "%" SCNx64 "%c", &data.switch_id, &c) == 2 && > > + c == '\n'; > > + fclose(file); > > + data.master = port_switch_id_set && !port_name_set; > > + data.representor = port_switch_id_set && port_name_set; > > + *info = data; > > + return 0; > > +error: > > + rte_errno = errno; > > + return -rte_errno; > > +} > > -- > > 2.18.0 > > [1] > diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c > index 6cfe02d..b64eb20 100644 > --- a/drivers/net/mlx5/mlx5_ethdev.c > +++ b/drivers/net/mlx5/mlx5_ethdev.c > @@ -1328,23 +1328,20 @@ struct ethtool_link_settings { > ifname); > > file = fopen(phys_port_name, "rb"); > - if (file == NULL) > - goto error; > - port_name_set = fscanf(file, "%d%c", &data.port_name, &c) == 2 && > - c == '\n'; > - fclose(file); > + if (file) { > + port_name_set = fscanf(file, "%d%c", &data.port_name, &c) == 2 && > + c == '\n'; > + fclose(file); > + } > file = fopen(phys_switch_id, "rb"); > - if (file == NULL) > - goto error; > - port_switch_id_set = > - fscanf(file, "%" SCNx64 "%c", &data.switch_id, &c) == 2 && > - c == '\n'; > - fclose(file); > + if (file) { > + port_switch_id_set = > + fscanf(file, "%" SCNx64 "%c", &data.switch_id, &c) == 2 && > + c == '\n'; > + fclose(file); > + } > data.master = port_switch_id_set && !port_name_set; > data.representor = port_switch_id_set && port_name_set; Regards, -- Nélio Laranjeiro 6WIND