[dpdk-dev] [PATCH v6 16/17] ethdev: convert to eal hotplug
Shreyansh jain
shreyansh.jain at nxp.com
Fri Jul 15 12:36:25 CEST 2016
On Thursday 14 July 2016 10:21 PM, Jan Viktorin wrote:
> On Tue, 12 Jul 2016 11:31:21 +0530
> Shreyansh Jain <shreyansh.jain at nxp.com> wrote:
>
>> Remove bus logic from ethdev hotplug by using eal for this.
>>
>> Current api is preserved:
>> - the last port that has been created is tracked to return it to the
>> application when attaching,
>> - the internal device name is reused when detaching.
>>
>> We can not get rid of ethdev hotplug yet since we still need some mechanism
>> to inform applications of port creation/removal to substitute for ethdev
>> hotplug api.
>>
>> dev_type field in struct rte_eth_dev and rte_eth_dev_allocate are kept as
>> is, but this information is not needed anymore and is removed in the following
>> commit.
>>
>> Signed-off-by: David Marchand <david.marchand at 6wind.com>
>> Signed-off-by: Shreyansh Jain <shreyansh.jain at nxp.com>
>> ---
>> lib/librte_ether/rte_ethdev.c | 207 +++++++-----------------------------------
>> 1 file changed, 33 insertions(+), 174 deletions(-)
>>
>> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
>> index a667012..8d14fd7 100644
>> --- a/lib/librte_ether/rte_ethdev.c
>> +++ b/lib/librte_ether/rte_ethdev.c
>> @@ -72,6 +72,7 @@
>> static const char *MZ_RTE_ETH_DEV_DATA = "rte_eth_dev_data";
>> struct rte_eth_dev rte_eth_devices[RTE_MAX_ETHPORTS];
>> static struct rte_eth_dev_data *rte_eth_dev_data;
>> +static uint8_t eth_dev_last_created_port;
>> static uint8_t nb_ports;
>>
>
> [...]
>
>> -
>> /* attach the new device, then store port_id of the device */
>> int
>> rte_eth_dev_attach(const char *devargs, uint8_t *port_id)
>> {
>> - struct rte_pci_addr addr;
>> int ret = -1;
>> + int current = eth_dev_last_created_port;
>> + char *name = NULL;
>> + char *args = NULL;
>>
>> if ((devargs == NULL) || (port_id == NULL)) {
>> ret = -EINVAL;
>> goto err;
>> }
>>
>> - if (eal_parse_pci_DomBDF(devargs, &addr) == 0) {
>> - ret = rte_eth_dev_attach_pdev(&addr, port_id);
>> - if (ret < 0)
>> - goto err;
>> - } else {
>> - ret = rte_eth_dev_attach_vdev(devargs, port_id);
>> - if (ret < 0)
>> - goto err;
>> + /* parse devargs, then retrieve device name and args */
>> + if (rte_eal_parse_devargs_str(devargs, &name, &args))
>> + goto err;
>> +
>> + ret = rte_eal_dev_attach(name, args);
>> + if (ret < 0)
>> + goto err;
>> +
>> + /* no point looking at eth_dev_last_created_port if no port exists */
>
> I am not sure about this comment. What is "no point"?
>
> Isn't this also a potential bug? (Like the one below.) How could it
> happen there is no port after a successful attach?
Yes, even searching through code path I couldn't find a positive case where control would reach here without nb_ports>0.
Though, i am not sure if some rough application attempts to mix-up calls - and that, in my opinion, is not worth checking.
Should I remove it?
>
>> + if (!nb_ports) {
>> + ret = -1;
>> + goto err;
>> + }
>> + /* if nothing happened, there is a bug here, since some driver told us
>> + * it did attach a device, but did not create a port */
>> + if (current == eth_dev_last_created_port) {
>> + ret = -1;
>> + goto err;
>
> Should we log this? Or call some kind panic?
I will place a log because applications should have a chance to ignore a device which cannot be attached for whatever reason.
>
>> }
>> + *port_id = eth_dev_last_created_port;
>> + ret = 0;
>>
>> - return 0;
>> err:
>> - RTE_LOG(ERR, EAL, "Driver, cannot attach the device\n");
>> + free(name);
>> + free(args);
>> return ret;
>> }
>>
>> @@ -590,7 +464,6 @@ err:
>> int
>> rte_eth_dev_detach(uint8_t port_id, char *name)
>> {
>> - struct rte_pci_addr addr;
>> int ret = -1;
>>
>> if (name == NULL) {
>> @@ -598,33 +471,19 @@ rte_eth_dev_detach(uint8_t port_id, char *name)
>> goto err;
>> }
>>
>> - /* check whether the driver supports detach feature, or not */
>> + /* FIXME: move this to eal, once device flags are relocated there */
>> if (rte_eth_dev_is_detachable(port_id))
>> goto err;
>>
>> - if (rte_eth_dev_get_device_type(port_id) == RTE_ETH_DEV_PCI) {
>> - ret = rte_eth_dev_get_addr_by_port(port_id, &addr);
>> - if (ret < 0)
>> - goto err;
>> -
>> - ret = rte_eth_dev_detach_pdev(port_id, &addr);
>> - if (ret < 0)
>> - goto err;
>> -
>> - snprintf(name, RTE_ETH_NAME_MAX_LEN,
>> - "%04x:%02x:%02x.%d",
>> - addr.domain, addr.bus,
>> - addr.devid, addr.function);
>> - } else {
>> - ret = rte_eth_dev_detach_vdev(port_id, name);
>> - if (ret < 0)
>> - goto err;
>> - }
>> + snprintf(name, sizeof(rte_eth_devices[port_id].data->name),
>> + "%s", rte_eth_devices[port_id].data->name);
>> + ret = rte_eal_dev_detach(name);
>> + if (ret < 0)
>> + goto err;
>>
>> return 0;
>>
>> err:
>> - RTE_LOG(ERR, EAL, "Driver, cannot detach the device\n");
>
> I'd be more specific about the failing device, we have its name.
Agree. I will add 'name' to this.
>> return ret;
>> }
>>
>
More information about the dev
mailing list