[dpdk-dev] [PATCH v6 16/17] ethdev: convert to eal hotplug
Jan Viktorin
viktorin at rehivetech.com
Fri Jul 15 13:27:59 CEST 2016
On Fri, 15 Jul 2016 16:06:25 +0530
Shreyansh jain <shreyansh.jain at nxp.com> wrote:
> 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?
At least a loud log might be helpful. If there is really no path to reach this point, I'd put RTE_VERIFY here.
>
> >
> >> + 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.
OK, we just should shout loudly if it means a bug...
[...]
--
Jan Viktorin E-mail: Viktorin at RehiveTech.com
System Architect Web: www.RehiveTech.com
RehiveTech
Brno, Czech Republic
More information about the dev
mailing list