>-----Original Message-----
>From: Thomas Monjalon <tho...@monjalon.net>
>Sent: Wednesday, March 31, 2021 6:20 PM
>To: Xueming(Steven) Li <xuemi...@nvidia.com>
>Cc: Gaetan Rivet <gaet...@nvidia.com>; dev@dpdk.org; Asaf Penso
><as...@nvidia.com>; david.march...@redhat.com;
>ferruh.yi...@intel.com; andrew.rybche...@oktetlabs.ru; hemant.agra...@nxp.com;
>step...@networkplumber.org;
>rosen...@intel.com; ajit.khapa...@broadcom.com; jer...@marvell.com
>Subject: Re: [dpdk-dev] [PATCH v3 4/5] bus: add device arguments name parsing
>API
>
>The commit log should start by explaining it is adding a callback to the bus
>drivers for the new devargs syntax.
OK.
>
>30/03/2021 14:15, Xueming Li:
>> To use Global Device Syntax as devargs, name is required for device
>> management.
>
>Context is missing.
>You mean the argument "name" for the vdev bus?
Devargs.name, it is used by probe and device iterator. To locate a device from
a devargs, devargs.name is compared
agains name of probed devices. This not an issue for legacy syntax, since the
string after "bus:" is saved as name.
>
>>
>> In legacy parsing API, devargs name was extracted after bus name:
>> bus:name,kv_params,,,
>>
>> To parse new Global Device Syntax, this patch introduces new bus API
>> to parse devargs and update name, different bus driver might choose
>> different keys from parameters with unified formating, example:
>> -a bus=pci,addr=83:00.0/class=eth/driver=mlx5,...
>> name: 0000:03:00.0
>> -a bus=vdev,name=pcap0/class=eth/driver=pcap,...
>> name:pcap0
>
>Only PCI and vdev buses are implemented.
>What can be the plan for others?
>We should track the progress somewhere, maybe with TODO comments.
Like legacy parser, how about using "name" as default name key, the new syntax
parser can resolve it by default.
Then PCI bus overrides by using "addr" key in new bus API,
vdev and other bus drivers simply use default implementation, i.e. using "name"
as key..
>
>This commit log could also state what is the status of the global syntax
>support, talking about class and device drivers.
Yes
>
>We could update this comment in ethdev:
> * A new syntax is in development (not yet supported):
> * - bus=X,paramX=x/class=Y,paramY=y/driver=Z,paramZ=z
Will do it in next patch - enable new syntax
>
>[...]
>> +int
>> +rte_pci_devargs_parse(struct rte_devargs *da) {
>> + struct rte_kvargs *kvargs;
>> + const char *addr_str;
>> + struct rte_pci_addr addr;
>> + int ret;
>> +
>> + if (da == NULL)
>> + return 0;
>> + RTE_ASSERT(da->bus_str != NULL);
>> +
>> + kvargs = rte_kvargs_parse(da->bus_str, NULL);
>> + if (kvargs == NULL) {
>> + RTE_LOG(ERR, EAL, "cannot parse argument list: %s\n",
>> + da->bus_str);
>> + ret = -ENODEV;
>> + goto out;
>> + }
>> +
>> + addr_str = rte_kvargs_get(kvargs, pci_params_keys[RTE_PCI_PARAM_ADDR]);
>> + if (addr_str == NULL) {
>> + RTE_LOG(ERR, EAL, "No PCI address specified using '%s=<id>' in:
>> %s\n",
>> + pci_params_keys[RTE_PCI_PARAM_ADDR], da->bus_str);
>> + ret = -ENODEV;
>> + goto out;
>> + }
>> +
>> + ret = rte_pci_addr_parse(addr_str, &addr);
>> + if (ret != 0) {
>> + RTE_LOG(ERR, EAL, "PCI address invalid: %s\n", da->bus_str);
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> + rte_pci_device_name(&addr, da->name, sizeof(da->name));
>> +
>> + /* TODO: class parse -> driver parse */
>
>Please could you give a longer explanation of what is missing?
Just an option to give class driver and device driver to override name parsing.
But this should happen in new devargs parser, not here if needed.
I'll remove this line, and mention it in commit log.
>
