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

[Bruce Richardson <bruce.richardson@intel.com>]

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