Re: [PATCH v2 12/18] eal: add generic device declaration parameter

From: Shreyansh Jain <shreyansh.jain@nxp.com>
To: "Gaëtan Rivet" <gaetan.rivet@6wind.com>
Cc: <dev@dpdk.org>
Subject: Re: [PATCH v2 12/18] eal: add generic device declaration parameter
Date: Wed, 13 Dec 2017 20:54:25 +0530	[thread overview]
Message-ID: <83cbd5e1-fc40-534d-09f6-d47f094028df@nxp.com> (raw)
In-Reply-To: <20171213144704.4d7675gw7iaasvtb@bidouze.vm.6wind.com>

On Wednesday 13 December 2017 08:17 PM, Gaëtan Rivet wrote:
> On Wed, Dec 13, 2017 at 07:56:42PM +0530, Shreyansh Jain wrote:
>> On Thursday 12 October 2017 01:51 PM, Gaetan Rivet wrote:
>>> Add a new generic device declaration parameter:
>>>
>>>      --dev=<device_declaration>
>>>
>>
>> [...]
>>
>>>
>>> diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
>>> index 603df27..b7591fd 100644
>>> --- a/lib/librte_eal/common/eal_common_options.c
>>> +++ b/lib/librte_eal/common/eal_common_options.c
>>> @@ -95,6 +95,7 @@ eal_long_options[] = {
>>>    	{OPT_PROC_TYPE,         1, NULL, OPT_PROC_TYPE_NUM        },
>>>    	{OPT_SOCKET_MEM,        1, NULL, OPT_SOCKET_MEM_NUM       },
>>>    	{OPT_SYSLOG,            1, NULL, OPT_SYSLOG_NUM           },
>>> +	{OPT_DEV,               1, NULL, OPT_DEV_NUM              },
>>>    	{OPT_VDEV,              1, NULL, OPT_VDEV_NUM             },
>>>    	{OPT_VFIO_INTR,         1, NULL, OPT_VFIO_INTR_NUM        },
>>>    	{OPT_VMWARE_TSC_MAP,    0, NULL, OPT_VMWARE_TSC_MAP_NUM   },
>>> @@ -1120,6 +1121,21 @@ eal_parse_common_option(int opt, const char *optarg,
>>>    		}
>>>    		break;
>>> +	case OPT_DEV_NUM: {
>>> +		struct rte_devargs da;
>>> +		int ret;
>>> +
>>> +		if (rte_eal_devargs_parse(&da, optarg) < 0)
>>> +			return -1;
>>> +		ret = rte_bus_probe_mode_set(da.bus->name,
>>> +					RTE_BUS_PROBE_WHITELIST);
>>> +		if (ret < 0 && ret != -ENOTSUP)
>>> +			return -1;
>>> +		if (eal_option_device_add(NULL, optarg) < 0)
>>> +			return -1;
>>> +	}
>>
>> Might be a naive question: Any specific reason why we don't add the devices
>> directly into devargs_list here (eal_parse_args -> eal_parse_common_option
>> -> OPT_DEV ->) rather than wait for eal to call eal_option_device_parse
>> again?
>>
>> Is it to allow eal_plugins_init() to finish?
>>
> 
> Yes. And actually this makes me aware of an issue with this
> implementation.
> 
> Calling rte_eal_devargs_parse here is premature, and
> rte_bus_probe_mode_set as well.
> 
> eal_plugins_init() must be executed before calling rte_devargs to allow
> for buses introduced as plugins to be able to recognize their devices.

There might be one more catch. Maybe eal_parse_args also finds all the 
plugins to load (-d ...).

> 
> I will reorder a few things in eal_options, thanks for catching this.
> 
>>> +		break;
>>> +
>>>    	case OPT_VDEV_NUM:
>>>    		if (eal_option_device_add("vdev", optarg) < 0)
>>>    			return -1;
>>
>> [...]
>>
> 