[dpdk-dev] [PATCH v7 5/6] bus: add helper to find a bus from a device name

[Gaëtan Rivet]

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 <gaetan.rivet at 6wind.com>
> >---
> >  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