From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f46.google.com (mail-wm0-f46.google.com [74.125.82.46]) by dpdk.org (Postfix) with ESMTP id 67A0D2C8 for ; Wed, 5 Jul 2017 15:45:30 +0200 (CEST) Received: by mail-wm0-f46.google.com with SMTP id w126so222690107wme.0 for ; Wed, 05 Jul 2017 06:45:30 -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 :user-agent; bh=qqKl0yrBpMnhlvpXZkctQzVkPci6UQF8cElMRHNPVjg=; b=pPKUDxh9ENT28BapnUkJTvCC1mf4K6metrn8q0WngWRk5mnT8/tKa66itI+z4DT5HY N5SSRhamq2Pa70TwBHvVMoamKa1b2oNiQ4TCNpR7BVYyENs2oOvh3mUoxTTZi5sDCAGk pmh8s1SJp6iqzXgZGVlucRBZd+99k0KizomLpiKhv1ZSn1bdhhYdKdRvT4iHikFP1YYy kwbTLO/yP7Hic2eUuvTqGqyYr+NUfB6Tur6b/+tFnZbVKGIYPnPFtx8c2j6pBO/dMl/9 W//eYjyX3ClPe5qfVeB0bzaovutqD6KBQ8p/+StYESUnv/E9L5LCvUy5cML9qMOV5o/o UjGQ== 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:user-agent; bh=qqKl0yrBpMnhlvpXZkctQzVkPci6UQF8cElMRHNPVjg=; b=bVN7PUvARKxxcYcyHlAaHX/ZFndyz4x+WTuvsW7wzdEN43ZYfJ9UIEzh/e1wukITzd Bo2qsGHPpY8sardjX5v6fb5OSf5VnyTH+HSKSX6ebYUxYiHEIvkpYIIvFY+YN7xZQDej yFlLeoj7B7FRpT2FYD+ztrHCF2Alq7x61+cQYkfnOvTedTjNs11Hba+dV9HcQmyLprsD yMiHiPg3n4fA8vU7f0PJMWBaBOsclPJnGmIIEQhBye46edDNBRKPkYi4HxLk2mOOALd9 jB2hxtL8aqsxYq2tG+hTKQR1RddN+nrOy+mAXLvVuec3lr60ebaTj/bBzOBAWg00T7ut IWFQ== X-Gm-Message-State: AKS2vOxrc8+lvPDhwq4qkQnqkeSpL10znxPjJ/XA4gnjiynzqm2Aa1VV aOx9P1an6E8BtVzM X-Received: by 10.28.26.2 with SMTP id a2mr31360943wma.32.1499262329709; Wed, 05 Jul 2017 06:45:29 -0700 (PDT) Received: from bidouze.vm.6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id m188sm29513691wmg.34.2017.07.05.06.45.28 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 05 Jul 2017 06:45:28 -0700 (PDT) Date: Wed, 5 Jul 2017 15:45:21 +0200 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: Shreyansh Jain Cc: dev@dpdk.org Message-ID: <20170705134521.GI11154@bidouze.vm.6wind.com> References: <3cedddd79f0f518e412431526c9e2b3058d3220b.1499211317.git.gaetan.rivet@6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH v7 5/6] bus: add helper to find a bus from a device name 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: Wed, 05 Jul 2017 13:45:30 -0000 Hi Shreyansh, On Wed, Jul 05, 2017 at 07:05:40PM +0530, Shreyansh Jain wrote: > Hello Gaetan, > > Some comments inline: > > On Wednesday 05 July 2017 05:25 AM, Gaetan Rivet wrote: > >Find which bus should be able to parse this device name into an internal > >device representation. > > > >Signed-off-by: Gaetan Rivet > >--- > > lib/librte_eal/common/eal_common_bus.c | 21 +++++++++++++++++++++ > > lib/librte_eal/common/eal_private.h | 12 ++++++++++++ > > 2 files changed, 33 insertions(+) > > > >diff --git a/lib/librte_eal/common/eal_common_bus.c b/lib/librte_eal/common/eal_common_bus.c > >index 87b0c6e..34fcfa1 100644 > >--- a/lib/librte_eal/common/eal_common_bus.c > >+++ b/lib/librte_eal/common/eal_common_bus.c > >@@ -204,3 +204,24 @@ rte_bus_find_by_name(const char *busname) > > { > > return rte_bus_find(NULL, cmp_bus_name, (const void *)busname); > > } > >+ > >+static int > >+bus_can_parse(const struct rte_bus *bus, const void *_name) > >+{ > >+ const char *name = _name; > >+ > >+ return !(bus->parse && bus->parse(name, NULL) == 0); > >+} > >+ > >+struct rte_bus * > >+rte_bus_find_by_device_name(const char *str) > >+{ > >+ char name[32]; > > It is possible to use a constant here? Basically, I am not sure why '32' has > been chosen - or maybe, it might remind a reader in future the reason for > this value. > > Just to clarify: is there any documented limit on bus name? Until this > point, the name (and length) of bus was responsibility of bus driver > implementation. eal_common_bus.c doesn't codify any limit - so, this may > have to be advertised, even if just within the code. > I agree that this is not clear. First thing however: this is a limit on the device name length, not bus. This problem is fixed in [1], as a single common device name location is defined. The problem is that it is in another patchset (even if both are closely linked and were only separated to ease integration). This limit was currently taken from the arbitrary limit of device in name in the original rte_devargs for virtual devices. I think it could be exported as a define by rte_dev.h, used there and reused in [1] to define the new rte_devargs structure. Would that be ok with you? [1]: http://dpdk.org/ml/archives/dev/2017-July/070225.html > >+ char *c; > >+ > >+ snprintf(name, sizeof(name), "%s", str); > >+ c = strchr(name, ','); > > I saw a lot of discussion on the naming assumptions/presumptions. Though, I > am not sure I have an understood conclusion from that. > So, this might be incorrect/ill-informed: > > Is it assumed that ',' is not present in device name? > > Do you think it would be better if this is documented that ',' in a device > name is reserved (as per devargs)? (or, is this already known?) > > For example, in case of DPAA2 devices, I didn't consider this as a case > while generating names while scanning the bus (though, I didn't have a ',' > in the name). But, if a well known fact, bus drivers can normalize their > device names before creating device names. > For this release cycle the discussion was mostly about forbidding (or not forbidding) characters in bus names. This character is one limitation previously defined for device names, to separate the device name from its kvargs. I am thus following the current rte_devargs API. Not sure if that's documented. There are examples in doc, but maybe this device name limitation is not explicitly said. > >+ if (c != NULL) > >+ c[0] = '\0'; > >+ return rte_bus_find(NULL, bus_can_parse, name); > >+} > >diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h > >index 6cacce0..0836339 100644 > >--- a/lib/librte_eal/common/eal_private.h > >+++ b/lib/librte_eal/common/eal_private.h > >@@ -338,4 +338,16 @@ int rte_eal_hugepage_attach(void); > > */ > > bool rte_eal_using_phys_addrs(void); > >+/** > >+ * Find a bus capable of identifying a device. > >+ * > >+ * @param str > >+ * A device identifier (PCI address, virtual PMD name, ...). > >+ * > >+ * @return > >+ * A valid bus handle if found. > >+ * NULL if no bus is able to parse this device. > >+ */ > >+struct rte_bus *rte_bus_find_by_device_name(const char *str); > >+ > > #endif /* _EAL_PRIVATE_H_ */ > > > > (Apologies for commenting so late in series - feel free to ignore if this > disrupts your cycle or sounds out-of-context.) > No problem, thanks for reviewing so quickly this patchset. > - > Shreyansh -- Gaëtan Rivet 6WIND