[dpdk-dev,v3,20/28] eal/pci: Add rte_eal_pci_close_one_driver

The function is used for closing the specified driver and device.

Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
---
 lib/librte_eal/common/eal_private.h   | 15 +++++++++
 lib/librte_eal/linuxapp/eal/eal_pci.c | 61 +++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+)
  

On 12/9/2014 2:33 PM, Tetsuya Mukawa wrote:
> The function is used for closing the specified driver and device.
>
> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
> ---
>  lib/librte_eal/common/eal_private.h   | 15 +++++++++
>  lib/librte_eal/linuxapp/eal/eal_pci.c | 61 +++++++++++++++++++++++++++++++++++
>  2 files changed, 76 insertions(+)
>
> diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
> index a1127ab..f892ac4 100644
> --- a/lib/librte_eal/common/eal_private.h
> +++ b/lib/librte_eal/common/eal_private.h
> @@ -176,6 +176,21 @@ int rte_eal_pci_probe_one_driver(struct rte_pci_driver *dr,
>  		struct rte_pci_device *dev);
>  
>  /**
> + * Munmap memory for single PCI device
> + *
> + * This function is private to EAL.
> + *
> + * @param	dr
> + *  The pointer to the pci driver structure
> + * @param	dev
> + *  The pointer to the pci device structure
> + * @return
> + *   0 on success, negative on error
> + */
> +int rte_eal_pci_close_one_driver(struct rte_pci_driver *dr,
> +		struct rte_pci_device *dev);
> +
> +/**
>   * Init tail queues for non-EAL library structures. This is to allow
>   * the rings, mempools, etc. lists to be shared among multiple processes
>   *
> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
> index 8d683f5..93456f3 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_pci.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
> @@ -616,6 +616,67 @@ rte_eal_pci_probe_one_driver(struct rte_pci_driver *dr, struct rte_pci_device *d
>  	return 1;
>  }
>  
> +#if defined(RTE_LIBRTE_EAL_HOTPLUG) && defined(RTE_LIBRTE_EAL_LINUXAPP)
> +/*
> + * If vendor/device ID match, call the devshutdown() function of the
> + * driver.
> + */
> +int
> +rte_eal_pci_close_one_driver(struct rte_pci_driver *dr,
> +		struct rte_pci_device *dev)
> +{
> +	struct rte_pci_id *id_table;
> +
> +	if ((dr == NULL) || (dev == NULL))
> +		return -EINVAL;
> +
> +	for (id_table = dr->id_table ; id_table->vendor_id != 0; id_table++) {
> +
> +		/* check if device's identifiers match the driver's ones */
> +		if (id_table->vendor_id != dev->id.vendor_id &&
> +				id_table->vendor_id != PCI_ANY_ID)

Here and below, it is better to make indentation like this:

+		if (id_table->vendor_id != dev->id.vendor_id &&
+		    id_table->vendor_id != PCI_ANY_ID)

But I don't know if there are any strict rule of dpdk community :)

> +			continue;
> +		if (id_table->device_id != dev->id.device_id &&
> +				id_table->device_id != PCI_ANY_ID)
> +			continue;
> +		if (id_table->subsystem_vendor_id !=
> +				dev->id.subsystem_vendor_id &&
> +				id_table->subsystem_vendor_id != PCI_ANY_ID)
> +			continue;
> +		if (id_table->subsystem_device_id !=
> +				dev->id.subsystem_device_id &&
> +				id_table->subsystem_device_id != PCI_ANY_ID)
> +			continue;
> +
> +		struct rte_pci_addr *loc = &dev->addr;
> +
> +		RTE_LOG(DEBUG, EAL,
> +				"PCI device "PCI_PRI_FMT" on NUMA socket %i\n",
> +				loc->domain, loc->bus, loc->devid,
> +				loc->function, dev->numa_node);
> +
> +		RTE_LOG(DEBUG, EAL, "  remove driver: %x:%x %s\n",
> +				dev->id.vendor_id, dev->id.device_id,
> +				dr->name);
> +
> +		/* call the driver devshutdown() function */
> +		if (dr->devshutdown && (dr->devshutdown(dr, dev) < 0))
> +			return -1;	/* negative value is an error */
> +
> +		/* clear driver structure */
> +		dev->driver = NULL;
> +
> +		if (dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING)
> +			/* unmap resources for devices that use igb_uio */
> +			pci_unmap_device(dev);
> +
> +		return 0;
> +	}
> +	/* return positive value if driver is not found */
> +	return 1;
> +}
> +#endif /* RTE_LIBRTE_EAL_HOTPLUG & RTE_LIBRTE_EAL_LINUXAPP */
> +
>  /* Init the PCI EAL subsystem */
>  int
>  rte_eal_pci_init(void)
  

On Thu, Dec 11, 2014 at 03:41:06AM +0000, Qiu, Michael wrote:
> On 12/9/2014 2:33 PM, Tetsuya Mukawa wrote:
> > The function is used for closing the specified driver and device.
> >
> > Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
> > ---
> >  lib/librte_eal/common/eal_private.h   | 15 +++++++++
> >  lib/librte_eal/linuxapp/eal/eal_pci.c | 61 +++++++++++++++++++++++++++++++++++
> >  2 files changed, 76 insertions(+)
> >
> > diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
> > index a1127ab..f892ac4 100644
> > --- a/lib/librte_eal/common/eal_private.h
> > +++ b/lib/librte_eal/common/eal_private.h
> > @@ -176,6 +176,21 @@ int rte_eal_pci_probe_one_driver(struct rte_pci_driver *dr,
> >  		struct rte_pci_device *dev);
> >  
> >  /**
> > + * Munmap memory for single PCI device
> > + *
> > + * This function is private to EAL.
> > + *
> > + * @param	dr
> > + *  The pointer to the pci driver structure
> > + * @param	dev
> > + *  The pointer to the pci device structure
> > + * @return
> > + *   0 on success, negative on error
> > + */
> > +int rte_eal_pci_close_one_driver(struct rte_pci_driver *dr,
> > +		struct rte_pci_device *dev);
> > +
> > +/**
> >   * Init tail queues for non-EAL library structures. This is to allow
> >   * the rings, mempools, etc. lists to be shared among multiple processes
> >   *
> > diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
> > index 8d683f5..93456f3 100644
> > --- a/lib/librte_eal/linuxapp/eal/eal_pci.c
> > +++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
> > @@ -616,6 +616,67 @@ rte_eal_pci_probe_one_driver(struct rte_pci_driver *dr, struct rte_pci_device *d
> >  	return 1;
> >  }
> >  
> > +#if defined(RTE_LIBRTE_EAL_HOTPLUG) && defined(RTE_LIBRTE_EAL_LINUXAPP)
> > +/*
> > + * If vendor/device ID match, call the devshutdown() function of the
> > + * driver.
> > + */
> > +int
> > +rte_eal_pci_close_one_driver(struct rte_pci_driver *dr,
> > +		struct rte_pci_device *dev)
> > +{
> > +	struct rte_pci_id *id_table;
> > +
> > +	if ((dr == NULL) || (dev == NULL))
> > +		return -EINVAL;
> > +
> > +	for (id_table = dr->id_table ; id_table->vendor_id != 0; id_table++) {
> > +
> > +		/* check if device's identifiers match the driver's ones */
> > +		if (id_table->vendor_id != dev->id.vendor_id &&
> > +				id_table->vendor_id != PCI_ANY_ID)
> 
> Here and below, it is better to make indentation like this:
> 
> +		if (id_table->vendor_id != dev->id.vendor_id &&
> +		    id_table->vendor_id != PCI_ANY_ID)
> 
> But I don't know if there are any strict rule of dpdk community :)
> 

I don't think we have any strict rules - I'm sure Thomas can confirm. However,
I prefer the original suggested version, as for the second version you propose
the second half of the if will appear indended as part of the if body for those
people who use a 4-character tab-stop in their editor. By indenting the second
line with two tabs, or tab + some space, you can guarantee that the rest of the
if statement never lines up with the if body, making the separation between
statement and body clearer.

Just my 2c.

/Bruce
  

On 2014/12/11 17:56, Richardson, Bruce wrote:
> On Thu, Dec 11, 2014 at 03:41:06AM +0000, Qiu, Michael wrote:
>> On 12/9/2014 2:33 PM, Tetsuya Mukawa wrote:
>>> The function is used for closing the specified driver and device.
>>>
>>> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
>>> ---
>>>  lib/librte_eal/common/eal_private.h   | 15 +++++++++
>>>  lib/librte_eal/linuxapp/eal/eal_pci.c | 61 +++++++++++++++++++++++++++++++++++
>>>  2 files changed, 76 insertions(+)
>>>
>>> diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
>>> index a1127ab..f892ac4 100644
>>> --- a/lib/librte_eal/common/eal_private.h
>>> +++ b/lib/librte_eal/common/eal_private.h
>>> @@ -176,6 +176,21 @@ int rte_eal_pci_probe_one_driver(struct rte_pci_driver *dr,
>>>  		struct rte_pci_device *dev);
>>>  
>>>  /**
>>> + * Munmap memory for single PCI device
>>> + *
>>> + * This function is private to EAL.
>>> + *
>>> + * @param	dr
>>> + *  The pointer to the pci driver structure
>>> + * @param	dev
>>> + *  The pointer to the pci device structure
>>> + * @return
>>> + *   0 on success, negative on error
>>> + */
>>> +int rte_eal_pci_close_one_driver(struct rte_pci_driver *dr,
>>> +		struct rte_pci_device *dev);
>>> +
>>> +/**
>>>   * Init tail queues for non-EAL library structures. This is to allow
>>>   * the rings, mempools, etc. lists to be shared among multiple processes
>>>   *
>>> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
>>> index 8d683f5..93456f3 100644
>>> --- a/lib/librte_eal/linuxapp/eal/eal_pci.c
>>> +++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
>>> @@ -616,6 +616,67 @@ rte_eal_pci_probe_one_driver(struct rte_pci_driver *dr, struct rte_pci_device *d
>>>  	return 1;
>>>  }
>>>  
>>> +#if defined(RTE_LIBRTE_EAL_HOTPLUG) && defined(RTE_LIBRTE_EAL_LINUXAPP)
>>> +/*
>>> + * If vendor/device ID match, call the devshutdown() function of the
>>> + * driver.
>>> + */
>>> +int
>>> +rte_eal_pci_close_one_driver(struct rte_pci_driver *dr,
>>> +		struct rte_pci_device *dev)
>>> +{
>>> +	struct rte_pci_id *id_table;
>>> +
>>> +	if ((dr == NULL) || (dev == NULL))
>>> +		return -EINVAL;
>>> +
>>> +	for (id_table = dr->id_table ; id_table->vendor_id != 0; id_table++) {
>>> +
>>> +		/* check if device's identifiers match the driver's ones */
>>> +		if (id_table->vendor_id != dev->id.vendor_id &&
>>> +				id_table->vendor_id != PCI_ANY_ID)
>> Here and below, it is better to make indentation like this:
>>
>> +		if (id_table->vendor_id != dev->id.vendor_id &&
>> +		    id_table->vendor_id != PCI_ANY_ID)
>>
>> But I don't know if there are any strict rule of dpdk community :)
>>
> I don't think we have any strict rules - I'm sure Thomas can confirm. However,
> I prefer the original suggested version, as for the second version you propose
> the second half of the if will appear indended as part of the if body for those
> people who use a 4-character tab-stop in their editor. By indenting the second
> line with two tabs, or tab + some space, you can guarantee that the rest of the
> if statement never lines up with the if body, making the separation between
> statement and body clearer.

My suggestion is just my habit, as I ever worked on kernel and qemu.

It is not a big deal. But I think we guys should be better to reach an
agreement on this, then all dpdk code should keep the style. Otherwise
the code is mixed style which is very ugly.

Do you agree me?

Thanks,
Michael
> Just my 2c.
>
> /Bruce
>