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

  Find which bus should be able to parse this device name into an internal
device representation.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/eal_common_bus.c | 21 +++++++++++++++++++++
 lib/librte_eal/common/eal_private.h    | 12 ++++++++++++
 2 files changed, 33 insertions(+)
  

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@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.

> +	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.

> +	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.)

-
Shreyansh
  

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@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
  

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.rivet@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.jain@nxp.com>
  

05/07/2017 15:45, Gaëtan Rivet:
> On Wed, Jul 05, 2017 at 07:05:40PM +0530, Shreyansh Jain wrote:
> > >+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.

Please check this patch which was integrated yesterday:
	http://dpdk.org/commit/48d8675c9cf
Now you can use RTE_DEV_NAME_MAX_LEN instead of hardcoded 32.
  

On Thu, Jul 06, 2017 at 12:10:10PM +0200, Thomas Monjalon wrote:
> 05/07/2017 15:45, Gaëtan Rivet:
> > On Wed, Jul 05, 2017 at 07:05:40PM +0530, Shreyansh Jain wrote:
> > > >+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.
> 
> Please check this patch which was integrated yesterday:
> 	http://dpdk.org/commit/48d8675c9cf
> Now you can use RTE_DEV_NAME_MAX_LEN instead of hardcoded 32.
> 

Yes I just saw that while rebasing, it's exactly what I needed. It will
be in the next version.