From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shreyansh Jain Subject: Re: [PATCH v7 5/6] bus: add helper to find a bus from a device name Date: Wed, 5 Jul 2017 19:42:40 +0530 Message-ID: <71fa0d7b-a140-5d1b-f0e7-a6e87eebf85d@nxp.com> References: <3cedddd79f0f518e412431526c9e2b3058d3220b.1499211317.git.gaetan.rivet@6wind.com> <20170705134521.GI11154@bidouze.vm.6wind.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit Cc: To: =?UTF-8?Q?Ga=c3=abtan_Rivet?= Return-path: Received: from NAM03-CO1-obe.outbound.protection.outlook.com (mail-co1nam03on0048.outbound.protection.outlook.com [104.47.40.48]) by dpdk.org (Postfix) with ESMTP id 178782C48 for ; Wed, 5 Jul 2017 16:03:37 +0200 (CEST) In-Reply-To: <20170705134521.GI11154@bidouze.vm.6wind.com> Content-Language: en-US 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 Wednesday 05 July 2017 07:15 PM, Gaëtan Rivet wrote: > 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. Ah, yes, I see that now. I mixed up. > > 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). Ok. I hadn't yet gone through it yet - so, didn't notice. I will look through that as well. But, if this is already taken care of in that series, consider my comment resolved. > > 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? For the purpose of this patch, this is OK. I will read through that patch series and comment if there is something I have comments on. Thanks for highlighting. > > [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. I agree. There doesn't seem to be much devargs related documentation anyways. (is there?) If someone uses a ',' name (ignorantly), they would get a failure from this function because of partial name match. But its worth documenting - now that EAL (Bus scan) allows device an arbitrary naming (no more just PCI and VDEV). (I will try to write something and push as patch - that is, if I have enough understanding of devargs subsystem.) > >>> + 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 > Other than the 'device name length issue', which would be revisited in other patch series: Acked-by: Shreyansh Jain