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 <gaetan.ri...@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.
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 <shreyansh.j...@nxp.com>
