diff mbox

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

Message ID 1418106629-22227-21-git-send-email-mukawa@igel.co.jp (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Tetsuya Mukawa Dec. 9, 2014, 6:30 a.m. UTC
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(+)

Comments

Michael Qiu Dec. 11, 2014, 3:41 a.m. UTC | #1
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)
Bruce Richardson Dec. 11, 2014, 9:55 a.m. UTC | #2
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
Michael Qiu Dec. 11, 2014, 3:45 p.m. UTC | #3
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
>
diff mbox

Patch

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)
+			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)