Re: [PATCH 1/4 v2] PCI: introduce new base functions

Previous thread: [PATCH 0/4 v2] PCI: Linux kernel SR-IOV support by Zhao, Yu on Monday, September 1, 2008 - 4:20 am. (1 message)

Next thread: [PATCH 2/4 v2] PCI: support ARI capability by Zhao, Yu on Monday, September 1, 2008 - 4:20 am. (7 messages)
From: Zhao, Yu
Date: Monday, September 1, 2008 - 4:20 am

Some basic changes to allocation bus range, MMIO resource for SR-IOV device.
And add new sysfs entry to hotplug core to pass parameter to a slot, which will be used by SR-IOV code.

Signed-off-by: Yu Zhao <yu.zhao@intel.com>
Signed-off-by: Eddie Dong <eddie.dong@intel.com>

---
 drivers/pci/bus.c                      |   63 +++++++++++++-------------
 drivers/pci/hotplug/pci_hotplug_core.c |   75 +++++++++++++++++++++++++++++---
 drivers/pci/pci-sysfs.c                |    4 +-
 drivers/pci/pci.c                      |   68 +++++++++++++++++++++--------
 drivers/pci/pci.h                      |    3 +
 drivers/pci/probe.c                    |   37 ++++++++-------
 drivers/pci/proc.c                     |    7 ++-
 drivers/pci/remove.c                   |    3 +-
 drivers/pci/setup-bus.c                |    9 ++--
 drivers/pci/setup-res.c                |   29 ++++++------
 include/linux/pci.h                    |   53 ++++++++++++++++-------
 include/linux/pci_hotplug.h            |   11 ++++-
 12 files changed, 246 insertions(+), 116 deletions(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 529d9d7..15f64c9 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -105,7 +105,7 @@ int pci_bus_add_device(struct pci_dev *dev)
 void pci_bus_add_devices(struct pci_bus *bus)
 {
 	struct pci_dev *dev;
-	struct pci_bus *child_bus;
+	struct pci_bus *child;
 	int retval;
 
 	list_for_each_entry(dev, &bus->devices, bus_list) {
@@ -115,43 +115,42 @@ void pci_bus_add_devices(struct pci_bus *bus)
 		retval = pci_bus_add_device(dev);
 		if (retval)
 			dev_err(&dev->dev, "Error adding device, continuing\n");
-	}
-
-	list_for_each_entry(dev, &bus->devices, bus_list) {
-		BUG_ON(!dev->is_added);
 
 		/*
 		 * If there is an unattached subordinate bus, attach
 		 * it and then scan for unattached PCI devices.
 		 */
-		if (dev->subordinate) {
-		       if (list_empty(&dev->subordinate->node)) {
-			       down_write(&pci_bus_sem);
-			       ...
From: Alex Chiang
Date: Monday, September 1, 2008 - 9:15 am

I was reading this patch, expecting to see a change to the
hotplug core _API_ taking a param, not just a new sysfs entry.

I would suggest rewording this part of the changelog as:

	Add new sysfs file 'param' to /sys/bus/pci/slots/.../
	which allows the user to pass a parameter to a slot. This
	parameter will be used by the SR-IOV code.


Please break the 80-column "rule" and make this all one line, to
help with greppability.

I know the prior version had it broken across two lines too, but


So is this new file only used for SR-IOV? Or do you imagine it to
be general-purpose in the future? If SR-IOV only, then I suggest
a different name other than 'param', since that's a very generic
name for a very specific feature.

If you do imagine 'param' to be generally useful, then ignore

CodingStyle?

	if (slot->ops->set_param || slot->ops->get_param)
		return 0;

Seems slightly more readable to me, but it's just a suggestion;
feel free to ignore.

I guess this comment applies to the above line too; you don't

Why not:

	for (i = 0; i < PCI_BRIDGE_RESOURCES; i++) {

Looks like the tradeoff is explicit mention of PCI_ROM_RESOURCE
vs using a non-standard C idiom (personally, I'm not a huge fan
of <= in a for loop, but ymmv).




Ouch, this enum makes my head hurt a little. Care to put in a
comment for PCI_BRIDGE_RES_END, saying that it has a value of 10?

Thanks,

/ac

--

From: Zhao, Yu
Date: Wednesday, September 10, 2008 - 1:17 am

It can be PCI_BRIDGE_RESOURCES, because there may be some non-standard resources following PCI_ROM_RESOURCE and before PCI_BRIDGE_RESOURCES.

For example, a standard PCI device has following resources:
	0 - 5   BARs
	6       ROM
	7 - 10  Bridge

After SR-IOV is enabled, it becomes
	0 - 5   standard BARs
	6       Rom
     7 - 12  SR-IOV BARs

Same as above, the PCI_BRIDGE_RES_END varies when some features is enabled or disabled.


--

From: Alex Chiang
Date: Wednesday, September 10, 2008 - 3:37 pm

Ok, makes sense now; was that documented somewhere else, and I


Sure, you can Cc me next time too.

Thanks.

/ac

--

Previous thread: [PATCH 0/4 v2] PCI: Linux kernel SR-IOV support by Zhao, Yu on Monday, September 1, 2008 - 4:20 am. (1 message)

Next thread: [PATCH 2/4 v2] PCI: support ARI capability by Zhao, Yu on Monday, September 1, 2008 - 4:20 am. (7 messages)