PCI: add kernel parameter to override devid<->driver mapping.

PATCH Superseded Marcel Apfelbaum 3 participants 11 messages Sep 30, 2014 – Oct 12, 2014

Subsystems (2)

Documentation PCI

lore

Share & tools

lore

Marcel Apfelbaum

author

2014-09-30 16:39 UTC #0 lore raw

PATCH 1/1 PCI: add kernel parameter to override devid<->driver mapping.

Scanning a lot of devices during boot requires a lot of time. On other scenarios there is a need to bind a driver to a specific slot.

Binding devices to pci-stub driver does not work, as it will not differentiate between devices of the same type. Using some start scripts is error prone.

The solution leverages driver_override functionality introduced by

	commit: 782a985d7af26db39e86070d28f987cad21313c0
	Author: Alex Williamson <[email protected]>
	Date:   Tue May 20 08:53:21 2014 -0600

    	PCI: Introduce new device binding path using pci_dev.driver_override

In order to bind PCI slot 0001:02:03.4 to pci-stub use:

	driver[0001:02:03.4]=pci-stub

Signed-off-by Marcel Apfelbaum <[email protected]>

---
Notes:
- For the moment only 32 slots can be assigned specific driver.
- I have further ideas on top of this patch based on your reviews. I thought of:
- Use wildcards to specify entire buses/devices, something like:

     	driver[0001:02:*.*]=pci-stub
   - Use comma to separate several devices:
     	driver[0001:02:03.4,0001:02:04.0,...]=pci-stub

- Make domain optional:
driver[00:03.0]=pci-stub

Comments will be appreciated,
Thanks,
Marcel

Documentation/kernel-parameters.txt | 4 +++

 drivers/pci/bus.c                   |  1 +
 drivers/pci/pci.c                   | 61 +++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h                   |  1 +
 4 files changed, 67 insertions(+)

Documentation/kernel-parameters.txt +4 -0

index 5ae8608..c1cbb4c 100644
@@ -2631,6 +2631,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 		pcie_scan_all	Scan all possible PCIe devices.  Otherwise we
 				only look for one device below a PCIe downstream
 				port.
+		driver		Provide an override to the devid<->driver mapping
+				for a specific slot.
+				Bind PCI slot 0001:02:03.4 to pci-stub by:
+					driver[0001:02:03.4]=pci-stub
 
 	pcie_aspm=	[PCIE] Forcibly enable or disable PCIe Active State Power
 			Management.

drivers/pci/bus.c +1 -0

index 73aef51..7a7ed85 100644
@@ -245,6 +245,7 @@ void pci_bus_add_device(struct pci_dev *dev)
 	 * are not assigned yet for some devices.
 	 */
 	pci_fixup_device(pci_fixup_final, dev);
+	pci_device_setup_driver_override(dev);
 	pci_create_sysfs_dev_files(dev);
 	pci_proc_attach_device(dev);

drivers/pci/pci.c +61 -0

73 lines not on this page · raw message

drivers/pci/pci.h +1 -0

index 0601890..023d78b 100644
@@ -197,6 +197,7 @@ enum pci_bar_type {
 bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *pl,
 				int crs_timeout);
 int pci_setup_device(struct pci_dev *dev);
+void pci_device_setup_driver_override(struct pci_dev *dev);
 int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 		    struct resource *res, unsigned int reg);
 int pci_resource_bar(struct pci_dev *dev, int resno, enum pci_bar_type *type);

Signature · 1 line

1.8.3.1

Michael S. Tsirkin

2014-10-01 06:49 UTC #1 lore raw

Quoted 93 lines · Marcel Apfelbaum

Scanning a lot of devices during boot requires a lot of time. On other scenarios there is a need to bind a driver to a specific slot.

Binding devices to pci-stub driver does not work, as it will not differentiate between devices of the same type. Using some start scripts is error prone.

The solution leverages driver_override functionality introduced by

	commit: 782a985d7af26db39e86070d28f987cad21313c0
	Author: Alex Williamson <[email protected]>
	Date:   Tue May 20 08:53:21 2014 -0600

    	PCI: Introduce new device binding path using pci_dev.driver_override

In order to bind PCI slot 0001:02:03.4 to pci-stub use:

	driver[0001:02:03.4]=pci-stub

Signed-off-by Marcel Apfelbaum <[email protected]>

     	driver[0001:02:*.*]=pci-stub
   - Use comma to separate several devices:
     	driver[0001:02:03.4,0001:02:04.0,...]=pci-stub
   - Make domain optional:
   	driver[00:03.0]=pci-stub

Comments will be appreciated,
Thanks,
Marcel

Documentation/kernel-parameters.txt | 4 +++

 drivers/pci/bus.c                   |  1 +
 drivers/pci/pci.c                   | 61 +++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h                   |  1 +
 4 files changed, 67 insertions(+)

Documentation/kernel-parameters.txt +4 -0

index 5ae8608..c1cbb4c 100644
@@ -2631,6 +2631,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 		pcie_scan_all	Scan all possible PCIe devices.  Otherwise we
 				only look for one device below a PCIe downstream
 				port.
+		driver		Provide an override to the devid<->driver mapping
+				for a specific slot.
+				Bind PCI slot 0001:02:03.4 to pci-stub by:
+					driver[0001:02:03.4]=pci-stub
 
 	pcie_aspm=	[PCIE] Forcibly enable or disable PCIe Active State Power
 			Management.

drivers/pci/bus.c +1 -0

index 73aef51..7a7ed85 100644
@@ -245,6 +245,7 @@ void pci_bus_add_device(struct pci_dev *dev)
 	 * are not assigned yet for some devices.
 	 */
 	pci_fixup_device(pci_fixup_final, dev);
+	pci_device_setup_driver_override(dev);
 	pci_create_sysfs_dev_files(dev);
 	pci_proc_attach_device(dev);

drivers/pci/pci.c +17 -0

index b7678be..0a3e07c 100644
@@ -4426,6 +4426,65 @@ void __weak pci_fixup_cardbus(struct pci_bus *bus)
 }
 EXPORT_SYMBOL(pci_fixup_cardbus);
 
+#define DRIVER_OVERRIDE_NAME_LENGTH 64
+#define DRIVER_OVERRIDE_MAP_SIZE 32
+
+struct driver_override_entry {
+	unsigned int domain;
+	unsigned char bus;
+	unsigned int devfn;
+	char driver_name[DRIVER_OVERRIDE_NAME_LENGTH];
+};
+
+static struct driver_override_entry __initdata
+	driver_override_map[DRIVER_OVERRIDE_MAP_SIZE];
+static int driver_override_num_entries __initdata;
+
+void pci_device_setup_driver_override(struct pci_dev *dev)
+{
+	int i;

drivers/pci/pci.c void __weak pci_fixup_cardbus(struct pci_bus *bus) · line 4455

Quoted 10 lines

drivers/pci/pci.c +10 -0

+
+	for (i = 0; i < driver_override_num_entries; i++) {
+		if (pci_domain_nr(dev->bus) != driver_override_map[i].domain ||
+		    dev->bus->number != driver_override_map[i].bus ||
+		    dev->devfn != driver_override_map[i].devfn)
+			continue;
+
+		dev->driver_override = kstrndup(driver_override_map[i].driver_name,
+						DRIVER_OVERRIDE_NAME_LENGTH,
+						GFP_KERNEL);

You allocate this entry but never free it? Shouldn't allocation failure be handled in some way?

Quoted 58 lines

drivers/pci/pci.c +34 -0

+		break;
+	}
+}
+
+static void __init pci_parse_driver_override(char *str)
+{
+	unsigned int domain, bus, dev, fn;
+	char driver_name[DRIVER_OVERRIDE_NAME_LENGTH];
+	int ret, next_entry;
+	char format[] = "[%4x:%2x:%2x.%2x]=%%%ds";
+
+	sprintf(format + 18, "%%%ds", DRIVER_OVERRIDE_NAME_LENGTH);
+	ret = sscanf(str, format, &domain, &bus, &dev, &fn, driver_name);
+
+	if (ret != 5) {
+		pr_err("PCI: Invalid command line: driver%s\n", str);
+		return;
+	}
+
+	if (driver_override_num_entries == DRIVER_OVERRIDE_MAP_SIZE) {
+		pr_err("PCI: Driver map overflow - ignoring driver%s\n", str);
+		return;
+	}
+
+	next_entry = driver_override_num_entries++;
+	driver_override_map[next_entry].domain = domain;
+	driver_override_map[next_entry].bus = bus && 0xff;
+	driver_override_map[next_entry].devfn = ((dev & 0x1f) << 3) | (fn & 0x7);
+	memcpy(driver_override_map[next_entry].driver_name,
+	       driver_name, sizeof(driver_name));
+}
+
 static int __init pci_setup(char *str)
 {
 	while (str) {

@@ -4468,6 +4527,8 @@ static int __init pci_setup(char *str)
 				pcie_bus_config = PCIE_BUS_PEER2PEER;
 			} else if (!strncmp(str, "pcie_scan_all", 13)) {
 				pci_add_flags(PCI_SCAN_ALL_PCIE_DEVS);
+			} else if (!strncmp(str, "driver", 6)) {
+				pci_parse_driver_override(str + 6);
 			} else {
 				printk(KERN_ERR "PCI: Unknown option `%s'\n",
 						str);

drivers/pci/pci.h +1 -0

index 0601890..023d78b 100644
@@ -197,6 +197,7 @@ enum pci_bar_type {
 bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *pl,
 				int crs_timeout);
 int pci_setup_device(struct pci_dev *dev);
+void pci_device_setup_driver_override(struct pci_dev *dev);
 int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 		    struct resource *res, unsigned int reg);
 int pci_resource_bar(struct pci_dev *dev, int resno, enum pci_bar_type *type);

Signature · 1 line

1.8.3.1

Marcel Apfelbaum

author

2014-10-01 07:07 UTC #2 lore raw

Quoted 96 lines · Michael S. Tsirkin

Quoted 95 lines · Marcel Apfelbaum

Scanning a lot of devices during boot requires a lot of time. On other scenarios there is a need to bind a driver to a specific slot.

Binding devices to pci-stub driver does not work, as it will not differentiate between devices of the same type. Using some start scripts is error prone.

The solution leverages driver_override functionality introduced by

	commit: 782a985d7af26db39e86070d28f987cad21313c0
	Author: Alex Williamson <[email protected]>
	Date:   Tue May 20 08:53:21 2014 -0600

    	PCI: Introduce new device binding path using pci_dev.driver_override

In order to bind PCI slot 0001:02:03.4 to pci-stub use:

	driver[0001:02:03.4]=pci-stub

Signed-off-by Marcel Apfelbaum <[email protected]>

     	driver[0001:02:*.*]=pci-stub
   - Use comma to separate several devices:
     	driver[0001:02:03.4,0001:02:04.0,...]=pci-stub
   - Make domain optional:
   	driver[00:03.0]=pci-stub

Comments will be appreciated,
Thanks,
Marcel

Documentation/kernel-parameters.txt | 4 +++

 drivers/pci/bus.c                   |  1 +
 drivers/pci/pci.c                   | 61 +++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h                   |  1 +
 4 files changed, 67 insertions(+)

Documentation/kernel-parameters.txt +4 -0

index 5ae8608..c1cbb4c 100644
@@ -2631,6 +2631,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 		pcie_scan_all	Scan all possible PCIe devices.  Otherwise we
 				only look for one device below a PCIe downstream
 				port.
+		driver		Provide an override to the devid<->driver mapping
+				for a specific slot.
+				Bind PCI slot 0001:02:03.4 to pci-stub by:
+					driver[0001:02:03.4]=pci-stub
 
 	pcie_aspm=	[PCIE] Forcibly enable or disable PCIe Active State Power
 			Management.

drivers/pci/bus.c +1 -0

index 73aef51..7a7ed85 100644
@@ -245,6 +245,7 @@ void pci_bus_add_device(struct pci_dev *dev)
 	 * are not assigned yet for some devices.
 	 */
 	pci_fixup_device(pci_fixup_final, dev);
+	pci_device_setup_driver_override(dev);
 	pci_create_sysfs_dev_files(dev);
 	pci_proc_attach_device(dev);

drivers/pci/pci.c +19 -0

index b7678be..0a3e07c 100644
@@ -4426,6 +4426,65 @@ void __weak pci_fixup_cardbus(struct pci_bus *bus)
 }
 EXPORT_SYMBOL(pci_fixup_cardbus);
 
+#define DRIVER_OVERRIDE_NAME_LENGTH 64
+#define DRIVER_OVERRIDE_MAP_SIZE 32
+
+struct driver_override_entry {
+	unsigned int domain;
+	unsigned char bus;
+	unsigned int devfn;
+	char driver_name[DRIVER_OVERRIDE_NAME_LENGTH];
+};
+
+static struct driver_override_entry __initdata
+	driver_override_map[DRIVER_OVERRIDE_MAP_SIZE];
+static int driver_override_num_entries __initdata;
+
+void pci_device_setup_driver_override(struct pci_dev *dev)
+{
+	int i;
+
+	for (i = 0; i < driver_override_num_entries; i++) {

Quoted 10 lines

drivers/pci/pci.c void __weak pci_fixup_cardbus(struct pci_bus *bus) · line 4455

Quoted 8 lines

drivers/pci/pci.c +8 -0

+		if (pci_domain_nr(dev->bus) != driver_override_map[i].domain ||
+		    dev->bus->number != driver_override_map[i].bus ||
+		    dev->devfn != driver_override_map[i].devfn)
+			continue;
+
+		dev->driver_override = kstrndup(driver_override_map[i].driver_name,
+						DRIVER_OVERRIDE_NAME_LENGTH,
+						GFP_KERNEL);

Hi,

Quoted 1 line

You allocate this entry but never free it?

Is being freed in pci_release_dev function.

Quoted 1 line

Shouldn't allocation failure be handled in some way?

I thought about it, but in my opinion there is no need for error handling in this situation.
- On the worst case scenario it will load the regular driver, no harm done. (kstrndup will return NULL)
- It is used in pci_bus_add_device workflow that has no error flow that I am aware of, and even if it were an error flow, in my opinion this is not enough to stop system boot.
But thinking of this again, maybe a pr_err message?

Your review is most appreciated.
Thanks,
Marcel

Quoted 59 lines

drivers/pci/pci.c +34 -0

+		break;
+	}
+}
+
+static void __init pci_parse_driver_override(char *str)
+{
+	unsigned int domain, bus, dev, fn;
+	char driver_name[DRIVER_OVERRIDE_NAME_LENGTH];
+	int ret, next_entry;
+	char format[] = "[%4x:%2x:%2x.%2x]=%%%ds";
+
+	sprintf(format + 18, "%%%ds", DRIVER_OVERRIDE_NAME_LENGTH);
+	ret = sscanf(str, format, &domain, &bus, &dev, &fn, driver_name);
+
+	if (ret != 5) {
+		pr_err("PCI: Invalid command line: driver%s\n", str);
+		return;
+	}
+
+	if (driver_override_num_entries == DRIVER_OVERRIDE_MAP_SIZE) {
+		pr_err("PCI: Driver map overflow - ignoring driver%s\n", str);
+		return;
+	}
+
+	next_entry = driver_override_num_entries++;
+	driver_override_map[next_entry].domain = domain;
+	driver_override_map[next_entry].bus = bus && 0xff;
+	driver_override_map[next_entry].devfn = ((dev & 0x1f) << 3) | (fn & 0x7);
+	memcpy(driver_override_map[next_entry].driver_name,
+	       driver_name, sizeof(driver_name));
+}
+
 static int __init pci_setup(char *str)
 {
 	while (str) {

@@ -4468,6 +4527,8 @@ static int __init pci_setup(char *str)
 				pcie_bus_config = PCIE_BUS_PEER2PEER;
 			} else if (!strncmp(str, "pcie_scan_all", 13)) {
 				pci_add_flags(PCI_SCAN_ALL_PCIE_DEVS);
+			} else if (!strncmp(str, "driver", 6)) {
+				pci_parse_driver_override(str + 6);
 			} else {
 				printk(KERN_ERR "PCI: Unknown option `%s'\n",
 						str);

drivers/pci/pci.h +1 -0

index 0601890..023d78b 100644
@@ -197,6 +197,7 @@ enum pci_bar_type {
 bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *pl,
 				int crs_timeout);
 int pci_setup_device(struct pci_dev *dev);
+void pci_device_setup_driver_override(struct pci_dev *dev);
 int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 		    struct resource *res, unsigned int reg);
 int pci_resource_bar(struct pci_dev *dev, int resno, enum pci_bar_type *type);

Signature · 1 line

1.8.3.1

Michael S. Tsirkin

2014-10-01 07:26 UTC #3 lore raw

Quoted 122 lines · Marcel Apfelbaum

Quoted 96 lines · Michael S. Tsirkin
Quoted 95 lines · Marcel Apfelbaum
Scanning a lot of devices during boot requires a lot of time. On other scenarios there is a need to bind a driver to a specific slot.
Binding devices to pci-stub driver does not work, as it will not differentiate between devices of the same type. Using some start scripts is error prone.
The solution leverages driver_override functionality introduced by
	commit: 782a985d7af26db39e86070d28f987cad21313c0
	Author: Alex Williamson <[email protected]>
	Date:   Tue May 20 08:53:21 2014 -0600

    	PCI: Introduce new device binding path using pci_dev.driver_override
In order to bind PCI slot 0001:02:03.4 to pci-stub use:
	driver[0001:02:03.4]=pci-stub
Signed-off-by Marcel Apfelbaum <[email protected]>
---
Notes:
- For the moment only 32 slots can be assigned specific driver.
- I have further ideas on top of this patch based on your reviews. I thought of:
- Use wildcards to specify entire buses/devices, something like:
     	driver[0001:02:*.*]=pci-stub
   - Use comma to separate several devices:
     	driver[0001:02:03.4,0001:02:04.0,...]=pci-stub
   - Make domain optional:
   	driver[00:03.0]=pci-stub
Comments will be appreciated,
Thanks,
Marcel
Documentation/kernel-parameters.txt | 4 +++
 drivers/pci/bus.c                   |  1 +
 drivers/pci/pci.c                   | 61 +++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h                   |  1 +
 4 files changed, 67 insertions(+)
Documentation/kernel-parameters.txt +4 -0
index 5ae8608..c1cbb4c 100644
@@ -2631,6 +2631,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 		pcie_scan_all	Scan all possible PCIe devices.  Otherwise we
 				only look for one device below a PCIe downstream
 				port.
+		driver		Provide an override to the devid<->driver mapping
+				for a specific slot.
+				Bind PCI slot 0001:02:03.4 to pci-stub by:
+					driver[0001:02:03.4]=pci-stub
 
 	pcie_aspm=	[PCIE] Forcibly enable or disable PCIe Active State Power
 			Management.
drivers/pci/bus.c +1 -0
index 73aef51..7a7ed85 100644
@@ -245,6 +245,7 @@ void pci_bus_add_device(struct pci_dev *dev)
 	 * are not assigned yet for some devices.
 	 */
 	pci_fixup_device(pci_fixup_final, dev);
+	pci_device_setup_driver_override(dev);
 	pci_create_sysfs_dev_files(dev);
 	pci_proc_attach_device(dev);
 
drivers/pci/pci.c +19 -0
index b7678be..0a3e07c 100644
@@ -4426,6 +4426,65 @@ void __weak pci_fixup_cardbus(struct pci_bus *bus)
 }
 EXPORT_SYMBOL(pci_fixup_cardbus);
 
+#define DRIVER_OVERRIDE_NAME_LENGTH 64
+#define DRIVER_OVERRIDE_MAP_SIZE 32
+
+struct driver_override_entry {
+	unsigned int domain;
+	unsigned char bus;
+	unsigned int devfn;
+	char driver_name[DRIVER_OVERRIDE_NAME_LENGTH];
+};
+
+static struct driver_override_entry __initdata
+	driver_override_map[DRIVER_OVERRIDE_MAP_SIZE];
+static int driver_override_num_entries __initdata;
+
+void pci_device_setup_driver_override(struct pci_dev *dev)
+{
+	int i;
+
+	for (i = 0; i < driver_override_num_entries; i++) {
Quoted 10 lines
drivers/pci/pci.c void __weak pci_fixup_cardbus(struct pci_bus *bus) · line 4455
Quoted 8 lines
drivers/pci/pci.c +8 -0
+		if (pci_domain_nr(dev->bus) != driver_override_map[i].domain ||
+		    dev->bus->number != driver_override_map[i].bus ||
+		    dev->devfn != driver_override_map[i].devfn)
+			continue;
+
+		dev->driver_override = kstrndup(driver_override_map[i].driver_name,
+						DRIVER_OVERRIDE_NAME_LENGTH,
+						GFP_KERNEL);
Hi,
Quoted 1 line
You allocate this entry but never free it?
Is being freed in pci_release_dev function.
Quoted 1 line
Shouldn't allocation failure be handled in some way?
I thought about it, but in my opinion there is no need for error handling in this situation.
- On the worst case scenario it will load the regular driver, no harm done. (kstrndup will return NULL)
- It is used in pci_bus_add_device workflow that has no error flow that I am aware of, and even if it were an error flow, in my opinion this is not enough to stop system boot.
But thinking of this again, maybe a pr_err message?

Quoted 5 lines

Your review is most appreciated.
Thanks,
Marcel

Thinking more about it, how about just using strstr to search a string of overrides? This will completely remove the artificial DRIVER_OVERRIDE_MAP_SIZE as well.

Quoted 61 lines

Quoted 59 lines

drivers/pci/pci.c +34 -0

+		break;
+	}
+}
+
+static void __init pci_parse_driver_override(char *str)
+{
+	unsigned int domain, bus, dev, fn;
+	char driver_name[DRIVER_OVERRIDE_NAME_LENGTH];
+	int ret, next_entry;
+	char format[] = "[%4x:%2x:%2x.%2x]=%%%ds";
+
+	sprintf(format + 18, "%%%ds", DRIVER_OVERRIDE_NAME_LENGTH);
+	ret = sscanf(str, format, &domain, &bus, &dev, &fn, driver_name);
+
+	if (ret != 5) {
+		pr_err("PCI: Invalid command line: driver%s\n", str);
+		return;
+	}
+
+	if (driver_override_num_entries == DRIVER_OVERRIDE_MAP_SIZE) {
+		pr_err("PCI: Driver map overflow - ignoring driver%s\n", str);
+		return;
+	}
+
+	next_entry = driver_override_num_entries++;
+	driver_override_map[next_entry].domain = domain;
+	driver_override_map[next_entry].bus = bus && 0xff;
+	driver_override_map[next_entry].devfn = ((dev & 0x1f) << 3) | (fn & 0x7);
+	memcpy(driver_override_map[next_entry].driver_name,
+	       driver_name, sizeof(driver_name));
+}
+
 static int __init pci_setup(char *str)
 {
 	while (str) {

@@ -4468,6 +4527,8 @@ static int __init pci_setup(char *str)
 				pcie_bus_config = PCIE_BUS_PEER2PEER;
 			} else if (!strncmp(str, "pcie_scan_all", 13)) {
 				pci_add_flags(PCI_SCAN_ALL_PCIE_DEVS);
+			} else if (!strncmp(str, "driver", 6)) {
+				pci_parse_driver_override(str + 6);
 			} else {
 				printk(KERN_ERR "PCI: Unknown option `%s'\n",
 						str);

drivers/pci/pci.h +1 -0

index 0601890..023d78b 100644
@@ -197,6 +197,7 @@ enum pci_bar_type {
 bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *pl,
 				int crs_timeout);
 int pci_setup_device(struct pci_dev *dev);
+void pci_device_setup_driver_override(struct pci_dev *dev);
 int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 		    struct resource *res, unsigned int reg);
 int pci_resource_bar(struct pci_dev *dev, int resno, enum pci_bar_type *type);

Signature · 1 line

1.8.3.1

Marcel Apfelbaum

author

2014-10-01 07:51 UTC #4 lore raw

Quoted 129 lines · Michael S. Tsirkin

Quoted 122 lines · Marcel Apfelbaum
Quoted 96 lines · Michael S. Tsirkin
Quoted 95 lines · Marcel Apfelbaum
Scanning a lot of devices during boot requires a lot of time. On other scenarios there is a need to bind a driver to a specific slot.
Binding devices to pci-stub driver does not work, as it will not differentiate between devices of the same type. Using some start scripts is error prone.
The solution leverages driver_override functionality introduced by
	commit: 782a985d7af26db39e86070d28f987cad21313c0
	Author: Alex Williamson <[email protected]>
	Date:   Tue May 20 08:53:21 2014 -0600

    	PCI: Introduce new device binding path using pci_dev.driver_override
In order to bind PCI slot 0001:02:03.4 to pci-stub use:
	driver[0001:02:03.4]=pci-stub
Signed-off-by Marcel Apfelbaum <[email protected]>
---
Notes:
- For the moment only 32 slots can be assigned specific driver.
- I have further ideas on top of this patch based on your reviews. I thought of:
- Use wildcards to specify entire buses/devices, something like:
     	driver[0001:02:*.*]=pci-stub
   - Use comma to separate several devices:
     	driver[0001:02:03.4,0001:02:04.0,...]=pci-stub
   - Make domain optional:
   	driver[00:03.0]=pci-stub
Comments will be appreciated,
Thanks,
Marcel
Documentation/kernel-parameters.txt | 4 +++
 drivers/pci/bus.c                   |  1 +
 drivers/pci/pci.c                   | 61 +++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h                   |  1 +
 4 files changed, 67 insertions(+)
Documentation/kernel-parameters.txt +4 -0
index 5ae8608..c1cbb4c 100644
@@ -2631,6 +2631,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 		pcie_scan_all	Scan all possible PCIe devices.  Otherwise we
 				only look for one device below a PCIe downstream
 				port.
+		driver		Provide an override to the devid<->driver mapping
+				for a specific slot.
+				Bind PCI slot 0001:02:03.4 to pci-stub by:
+					driver[0001:02:03.4]=pci-stub
 
 	pcie_aspm=	[PCIE] Forcibly enable or disable PCIe Active State Power
 			Management.
drivers/pci/bus.c +1 -0
index 73aef51..7a7ed85 100644
@@ -245,6 +245,7 @@ void pci_bus_add_device(struct pci_dev *dev)
 	 * are not assigned yet for some devices.
 	 */
 	pci_fixup_device(pci_fixup_final, dev);
+	pci_device_setup_driver_override(dev);
 	pci_create_sysfs_dev_files(dev);
 	pci_proc_attach_device(dev);
 
drivers/pci/pci.c +19 -0
index b7678be..0a3e07c 100644
@@ -4426,6 +4426,65 @@ void __weak pci_fixup_cardbus(struct pci_bus *bus)
 }
 EXPORT_SYMBOL(pci_fixup_cardbus);
 
+#define DRIVER_OVERRIDE_NAME_LENGTH 64
+#define DRIVER_OVERRIDE_MAP_SIZE 32
+
+struct driver_override_entry {
+	unsigned int domain;
+	unsigned char bus;
+	unsigned int devfn;
+	char driver_name[DRIVER_OVERRIDE_NAME_LENGTH];
+};
+
+static struct driver_override_entry __initdata
+	driver_override_map[DRIVER_OVERRIDE_MAP_SIZE];
+static int driver_override_num_entries __initdata;
+
+void pci_device_setup_driver_override(struct pci_dev *dev)
+{
+	int i;
+
+	for (i = 0; i < driver_override_num_entries; i++) {
Quoted 10 lines
drivers/pci/pci.c void __weak pci_fixup_cardbus(struct pci_bus *bus) · line 4455
Quoted 8 lines
drivers/pci/pci.c +8 -0
+		if (pci_domain_nr(dev->bus) != driver_override_map[i].domain ||
+		    dev->bus->number != driver_override_map[i].bus ||
+		    dev->devfn != driver_override_map[i].devfn)
+			continue;
+
+		dev->driver_override = kstrndup(driver_override_map[i].driver_name,
+						DRIVER_OVERRIDE_NAME_LENGTH,
+						GFP_KERNEL);
Hi,
Quoted 1 line
You allocate this entry but never free it?
Is being freed in pci_release_dev function.
Quoted 1 line
Shouldn't allocation failure be handled in some way?
I thought about it, but in my opinion there is no need for error handling in this situation.
- On the worst case scenario it will load the regular driver, no harm done. (kstrndup will return NULL)
- It is used in pci_bus_add_device workflow that has no error flow that I am aware of, and even if it were an error flow, in my opinion this is not enough to stop system boot.
But thinking of this again, maybe a pr_err message?
Quoted 5 lines
Your review is most appreciated.
Thanks,
Marcel

Quoted 3 lines

Thinking more about it, how about just using strstr to search a string of overrides? This will completely remove the artificial DRIVER_OVERRIDE_MAP_SIZE as well.

I'll try it, thanks, hope it will not be too ugly.

I will still need to reserve a COMMAND_LINE_SIZE string for "string of overrides", right? Or can I dynamically allocate a buffer to hold the string so early in boot process? If I can, it will be no problem to create a dynamic sized driver_override_map.

Thanks for the help,
Marcel

Quoted 65 lines

Quoted 63 lines

drivers/pci/pci.c +34 -0

+		break;
+	}
+}
+
+static void __init pci_parse_driver_override(char *str)
+{
+	unsigned int domain, bus, dev, fn;
+	char driver_name[DRIVER_OVERRIDE_NAME_LENGTH];
+	int ret, next_entry;
+	char format[] = "[%4x:%2x:%2x.%2x]=%%%ds";
+
+	sprintf(format + 18, "%%%ds", DRIVER_OVERRIDE_NAME_LENGTH);
+	ret = sscanf(str, format, &domain, &bus, &dev, &fn, driver_name);
+
+	if (ret != 5) {
+		pr_err("PCI: Invalid command line: driver%s\n", str);
+		return;
+	}
+
+	if (driver_override_num_entries == DRIVER_OVERRIDE_MAP_SIZE) {
+		pr_err("PCI: Driver map overflow - ignoring driver%s\n", str);
+		return;
+	}
+
+	next_entry = driver_override_num_entries++;
+	driver_override_map[next_entry].domain = domain;
+	driver_override_map[next_entry].bus = bus && 0xff;
+	driver_override_map[next_entry].devfn = ((dev & 0x1f) << 3) | (fn & 0x7);
+	memcpy(driver_override_map[next_entry].driver_name,
+	       driver_name, sizeof(driver_name));
+}
+
 static int __init pci_setup(char *str)
 {
 	while (str) {

@@ -4468,6 +4527,8 @@ static int __init pci_setup(char *str)
 				pcie_bus_config = PCIE_BUS_PEER2PEER;
 			} else if (!strncmp(str, "pcie_scan_all", 13)) {
 				pci_add_flags(PCI_SCAN_ALL_PCIE_DEVS);
+			} else if (!strncmp(str, "driver", 6)) {
+				pci_parse_driver_override(str + 6);
 			} else {
 				printk(KERN_ERR "PCI: Unknown option `%s'\n",
 						str);

drivers/pci/pci.h +1 -0

index 0601890..023d78b 100644
@@ -197,6 +197,7 @@ enum pci_bar_type {
 bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *pl,
 				int crs_timeout);
 int pci_setup_device(struct pci_dev *dev);
+void pci_device_setup_driver_override(struct pci_dev *dev);
 int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 		    struct resource *res, unsigned int reg);
 int pci_resource_bar(struct pci_dev *dev, int resno, enum pci_bar_type *type);

Signature · 1 line

1.8.3.1

Alex Williamson

2014-10-01 18:51 UTC #5 lore raw

Quoted 130 lines · Marcel Apfelbaum

Quoted 129 lines · Michael S. Tsirkin
Quoted 127 lines · Marcel Apfelbaum
Quoted 96 lines · Michael S. Tsirkin
Quoted 95 lines · Marcel Apfelbaum
Scanning a lot of devices during boot requires a lot of time. On other scenarios there is a need to bind a driver to a specific slot.
Binding devices to pci-stub driver does not work, as it will not differentiate between devices of the same type. Using some start scripts is error prone.
The solution leverages driver_override functionality introduced by
	commit: 782a985d7af26db39e86070d28f987cad21313c0
	Author: Alex Williamson <[email protected]>
	Date:   Tue May 20 08:53:21 2014 -0600

    	PCI: Introduce new device binding path using pci_dev.driver_override
In order to bind PCI slot 0001:02:03.4 to pci-stub use:
	driver[0001:02:03.4]=pci-stub
Signed-off-by Marcel Apfelbaum <[email protected]>
---
Notes:
- For the moment only 32 slots can be assigned specific driver.
- I have further ideas on top of this patch based on your reviews. I thought of:
- Use wildcards to specify entire buses/devices, something like:
     	driver[0001:02:*.*]=pci-stub
   - Use comma to separate several devices:
     	driver[0001:02:03.4,0001:02:04.0,...]=pci-stub
   - Make domain optional:
   	driver[00:03.0]=pci-stub
Comments will be appreciated,
Thanks,
Marcel
Documentation/kernel-parameters.txt | 4 +++
 drivers/pci/bus.c                   |  1 +
 drivers/pci/pci.c                   | 61 +++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h                   |  1 +
 4 files changed, 67 insertions(+)
Documentation/kernel-parameters.txt +4 -0
index 5ae8608..c1cbb4c 100644
@@ -2631,6 +2631,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 		pcie_scan_all	Scan all possible PCIe devices.  Otherwise we
 				only look for one device below a PCIe downstream
 				port.
+		driver		Provide an override to the devid<->driver mapping
+				for a specific slot.
+				Bind PCI slot 0001:02:03.4 to pci-stub by:
+					driver[0001:02:03.4]=pci-stub
 
 	pcie_aspm=	[PCIE] Forcibly enable or disable PCIe Active State Power
 			Management.
drivers/pci/bus.c +1 -0
index 73aef51..7a7ed85 100644
@@ -245,6 +245,7 @@ void pci_bus_add_device(struct pci_dev *dev)
 	 * are not assigned yet for some devices.
 	 */
 	pci_fixup_device(pci_fixup_final, dev);
+	pci_device_setup_driver_override(dev);
 	pci_create_sysfs_dev_files(dev);
 	pci_proc_attach_device(dev);
 
drivers/pci/pci.c +19 -0
index b7678be..0a3e07c 100644
@@ -4426,6 +4426,65 @@ void __weak pci_fixup_cardbus(struct pci_bus *bus)
 }
 EXPORT_SYMBOL(pci_fixup_cardbus);
 
+#define DRIVER_OVERRIDE_NAME_LENGTH 64
+#define DRIVER_OVERRIDE_MAP_SIZE 32
+
+struct driver_override_entry {
+	unsigned int domain;
+	unsigned char bus;
+	unsigned int devfn;
+	char driver_name[DRIVER_OVERRIDE_NAME_LENGTH];
+};
+
+static struct driver_override_entry __initdata
+	driver_override_map[DRIVER_OVERRIDE_MAP_SIZE];
+static int driver_override_num_entries __initdata;
+
+void pci_device_setup_driver_override(struct pci_dev *dev)
+{
+	int i;
+
+	for (i = 0; i < driver_override_num_entries; i++) {
Quoted 10 lines
drivers/pci/pci.c void __weak pci_fixup_cardbus(struct pci_bus *bus) · line 4455
Quoted 8 lines
drivers/pci/pci.c +8 -0
+		if (pci_domain_nr(dev->bus) != driver_override_map[i].domain ||
+		    dev->bus->number != driver_override_map[i].bus ||
+		    dev->devfn != driver_override_map[i].devfn)
+			continue;
+
+		dev->driver_override = kstrndup(driver_override_map[i].driver_name,
+						DRIVER_OVERRIDE_NAME_LENGTH,
+						GFP_KERNEL);
Hi,
Quoted 1 line
You allocate this entry but never free it?
Is being freed in pci_release_dev function.
Quoted 1 line
Shouldn't allocation failure be handled in some way?
I thought about it, but in my opinion there is no need for error handling in this situation.
- On the worst case scenario it will load the regular driver, no harm done. (kstrndup will return NULL)
- It is used in pci_bus_add_device workflow that has no error flow that I am aware of, and even if it were an error flow, in my opinion this is not enough to stop system boot.
But thinking of this again, maybe a pr_err message?
Your review is most appreciated.
Thanks,
Marcel

Quoted 8 lines

Quoted 3 lines
Thinking more about it, how about just using strstr to search a string of overrides? This will completely remove the artificial DRIVER_OVERRIDE_MAP_SIZE as well.
I'll try it, thanks, hope it will not be too ugly.
I will still need to reserve a COMMAND_LINE_SIZE string for "string of overrides", right? Or can I dynamically allocate a buffer to hold the string so early in boot process? If I can, it will be no problem to create a dynamic sized driver_override_map.

I think Michael is suggesting searching the existing commandline on each pass so that you don't need to allocate any memory at all. Thanks,

Alex

Quoted 64 lines

Quoted 61 lines

Quoted 59 lines

drivers/pci/pci.c +34 -0

+		break;
+	}
+}
+
+static void __init pci_parse_driver_override(char *str)
+{
+	unsigned int domain, bus, dev, fn;
+	char driver_name[DRIVER_OVERRIDE_NAME_LENGTH];
+	int ret, next_entry;
+	char format[] = "[%4x:%2x:%2x.%2x]=%%%ds";
+
+	sprintf(format + 18, "%%%ds", DRIVER_OVERRIDE_NAME_LENGTH);
+	ret = sscanf(str, format, &domain, &bus, &dev, &fn, driver_name);
+
+	if (ret != 5) {
+		pr_err("PCI: Invalid command line: driver%s\n", str);
+		return;
+	}
+
+	if (driver_override_num_entries == DRIVER_OVERRIDE_MAP_SIZE) {
+		pr_err("PCI: Driver map overflow - ignoring driver%s\n", str);
+		return;
+	}
+
+	next_entry = driver_override_num_entries++;
+	driver_override_map[next_entry].domain = domain;
+	driver_override_map[next_entry].bus = bus && 0xff;
+	driver_override_map[next_entry].devfn = ((dev & 0x1f) << 3) | (fn & 0x7);
+	memcpy(driver_override_map[next_entry].driver_name,
+	       driver_name, sizeof(driver_name));
+}
+
 static int __init pci_setup(char *str)
 {
 	while (str) {

@@ -4468,6 +4527,8 @@ static int __init pci_setup(char *str)
 				pcie_bus_config = PCIE_BUS_PEER2PEER;
 			} else if (!strncmp(str, "pcie_scan_all", 13)) {
 				pci_add_flags(PCI_SCAN_ALL_PCIE_DEVS);
+			} else if (!strncmp(str, "driver", 6)) {
+				pci_parse_driver_override(str + 6);
 			} else {
 				printk(KERN_ERR "PCI: Unknown option `%s'\n",
 						str);

drivers/pci/pci.h +1 -0

index 0601890..023d78b 100644
@@ -197,6 +197,7 @@ enum pci_bar_type {
 bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *pl,
 				int crs_timeout);
 int pci_setup_device(struct pci_dev *dev);
+void pci_device_setup_driver_override(struct pci_dev *dev);
 int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 		    struct resource *res, unsigned int reg);
 int pci_resource_bar(struct pci_dev *dev, int resno, enum pci_bar_type *type);

Signature · 1 line

1.8.3.1

Michael S. Tsirkin

2014-10-02 08:09 UTC #6 lore raw

Quoted 136 lines · Alex Williamson

Quoted 135 lines · Marcel Apfelbaum
Quoted 129 lines · Michael S. Tsirkin
Quoted 122 lines · Marcel Apfelbaum
Quoted 96 lines · Michael S. Tsirkin
Quoted 95 lines · Marcel Apfelbaum
Scanning a lot of devices during boot requires a lot of time. On other scenarios there is a need to bind a driver to a specific slot.
Binding devices to pci-stub driver does not work, as it will not differentiate between devices of the same type. Using some start scripts is error prone.
The solution leverages driver_override functionality introduced by
	commit: 782a985d7af26db39e86070d28f987cad21313c0
	Author: Alex Williamson <[email protected]>
	Date:   Tue May 20 08:53:21 2014 -0600

    	PCI: Introduce new device binding path using pci_dev.driver_override
In order to bind PCI slot 0001:02:03.4 to pci-stub use:
	driver[0001:02:03.4]=pci-stub
Signed-off-by Marcel Apfelbaum <[email protected]>
---
Notes:
- For the moment only 32 slots can be assigned specific driver.
- I have further ideas on top of this patch based on your reviews. I thought of:
- Use wildcards to specify entire buses/devices, something like:
     	driver[0001:02:*.*]=pci-stub
   - Use comma to separate several devices:
     	driver[0001:02:03.4,0001:02:04.0,...]=pci-stub
   - Make domain optional:
   	driver[00:03.0]=pci-stub
Comments will be appreciated,
Thanks,
Marcel
Documentation/kernel-parameters.txt | 4 +++
 drivers/pci/bus.c                   |  1 +
 drivers/pci/pci.c                   | 61 +++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h                   |  1 +
 4 files changed, 67 insertions(+)
Documentation/kernel-parameters.txt +4 -0
index 5ae8608..c1cbb4c 100644
@@ -2631,6 +2631,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 		pcie_scan_all	Scan all possible PCIe devices.  Otherwise we
 				only look for one device below a PCIe downstream
 				port.
+		driver		Provide an override to the devid<->driver mapping
+				for a specific slot.
+				Bind PCI slot 0001:02:03.4 to pci-stub by:
+					driver[0001:02:03.4]=pci-stub
 
 	pcie_aspm=	[PCIE] Forcibly enable or disable PCIe Active State Power
 			Management.
drivers/pci/bus.c +1 -0
index 73aef51..7a7ed85 100644
@@ -245,6 +245,7 @@ void pci_bus_add_device(struct pci_dev *dev)
 	 * are not assigned yet for some devices.
 	 */
 	pci_fixup_device(pci_fixup_final, dev);
+	pci_device_setup_driver_override(dev);
 	pci_create_sysfs_dev_files(dev);
 	pci_proc_attach_device(dev);
 
drivers/pci/pci.c +19 -0
index b7678be..0a3e07c 100644
@@ -4426,6 +4426,65 @@ void __weak pci_fixup_cardbus(struct pci_bus *bus)
 }
 EXPORT_SYMBOL(pci_fixup_cardbus);
 
+#define DRIVER_OVERRIDE_NAME_LENGTH 64
+#define DRIVER_OVERRIDE_MAP_SIZE 32
+
+struct driver_override_entry {
+	unsigned int domain;
+	unsigned char bus;
+	unsigned int devfn;
+	char driver_name[DRIVER_OVERRIDE_NAME_LENGTH];
+};
+
+static struct driver_override_entry __initdata
+	driver_override_map[DRIVER_OVERRIDE_MAP_SIZE];
+static int driver_override_num_entries __initdata;
+
+void pci_device_setup_driver_override(struct pci_dev *dev)
+{
+	int i;
+
+	for (i = 0; i < driver_override_num_entries; i++) {
Quoted 10 lines
drivers/pci/pci.c void __weak pci_fixup_cardbus(struct pci_bus *bus) · line 4455
Quoted 8 lines
drivers/pci/pci.c +8 -0
+		if (pci_domain_nr(dev->bus) != driver_override_map[i].domain ||
+		    dev->bus->number != driver_override_map[i].bus ||
+		    dev->devfn != driver_override_map[i].devfn)
+			continue;
+
+		dev->driver_override = kstrndup(driver_override_map[i].driver_name,
+						DRIVER_OVERRIDE_NAME_LENGTH,
+						GFP_KERNEL);
Hi,
Quoted 1 line
You allocate this entry but never free it?
Is being freed in pci_release_dev function.
Quoted 1 line
Shouldn't allocation failure be handled in some way?
I thought about it, but in my opinion there is no need for error handling in this situation.
- On the worst case scenario it will load the regular driver, no harm done. (kstrndup will return NULL)
- It is used in pci_bus_add_device workflow that has no error flow that I am aware of, and even if it were an error flow, in my opinion this is not enough to stop system boot.
But thinking of this again, maybe a pr_err message?
Quoted 5 lines
Your review is most appreciated.
Thanks,
Marcel
Quoted 3 lines
Thinking more about it, how about just using strstr to search a string of overrides? This will completely remove the artificial DRIVER_OVERRIDE_MAP_SIZE as well.
I'll try it, thanks, hope it will not be too ugly.

Quoted 8 lines

Quoted 3 lines
I will still need to reserve a COMMAND_LINE_SIZE string for "string of overrides", right? Or can I dynamically allocate a buffer to hold the string so early in boot process? If I can, it will be no problem to create a dynamic sized driver_override_map.
I think Michael is suggesting searching the existing commandline on each pass so that you don't need to allocate any memory at all. Thanks,
Alex

Conditional on having the option specified.
BTW, API-wise, I think that it's a good idea to make the fact that pci addresses are used, explicit.

Quoted 66 lines

Quoted 64 lines

Quoted 61 lines

Quoted 59 lines

drivers/pci/pci.c +34 -0

+		break;
+	}
+}
+
+static void __init pci_parse_driver_override(char *str)
+{
+	unsigned int domain, bus, dev, fn;
+	char driver_name[DRIVER_OVERRIDE_NAME_LENGTH];
+	int ret, next_entry;
+	char format[] = "[%4x:%2x:%2x.%2x]=%%%ds";
+
+	sprintf(format + 18, "%%%ds", DRIVER_OVERRIDE_NAME_LENGTH);
+	ret = sscanf(str, format, &domain, &bus, &dev, &fn, driver_name);
+
+	if (ret != 5) {
+		pr_err("PCI: Invalid command line: driver%s\n", str);
+		return;
+	}
+
+	if (driver_override_num_entries == DRIVER_OVERRIDE_MAP_SIZE) {
+		pr_err("PCI: Driver map overflow - ignoring driver%s\n", str);
+		return;
+	}
+
+	next_entry = driver_override_num_entries++;
+	driver_override_map[next_entry].domain = domain;
+	driver_override_map[next_entry].bus = bus && 0xff;
+	driver_override_map[next_entry].devfn = ((dev & 0x1f) << 3) | (fn & 0x7);
+	memcpy(driver_override_map[next_entry].driver_name,
+	       driver_name, sizeof(driver_name));
+}
+
 static int __init pci_setup(char *str)
 {
 	while (str) {

@@ -4468,6 +4527,8 @@ static int __init pci_setup(char *str)
 				pcie_bus_config = PCIE_BUS_PEER2PEER;
 			} else if (!strncmp(str, "pcie_scan_all", 13)) {
 				pci_add_flags(PCI_SCAN_ALL_PCIE_DEVS);
+			} else if (!strncmp(str, "driver", 6)) {
+				pci_parse_driver_override(str + 6);
 			} else {
 				printk(KERN_ERR "PCI: Unknown option `%s'\n",
 						str);

drivers/pci/pci.h +1 -0

index 0601890..023d78b 100644
@@ -197,6 +197,7 @@ enum pci_bar_type {
 bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *pl,
 				int crs_timeout);
 int pci_setup_device(struct pci_dev *dev);
+void pci_device_setup_driver_override(struct pci_dev *dev);
 int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 		    struct resource *res, unsigned int reg);
 int pci_resource_bar(struct pci_dev *dev, int resno, enum pci_bar_type *type);

Signature · 1 line

1.8.3.1

Marcel Apfelbaum

author

2014-10-02 08:21 UTC #7 lore raw

Quoted 135 lines · Michael S. Tsirkin

Quoted 134 lines · Alex Williamson
Quoted 133 lines · Marcel Apfelbaum
Quoted 132 lines · Michael S. Tsirkin
Quoted 122 lines · Marcel Apfelbaum
Quoted 96 lines · Michael S. Tsirkin
Quoted 95 lines · Marcel Apfelbaum
Scanning a lot of devices during boot requires a lot of time. On other scenarios there is a need to bind a driver to a specific slot.
Binding devices to pci-stub driver does not work, as it will not differentiate between devices of the same type. Using some start scripts is error prone.
The solution leverages driver_override functionality introduced by
	commit: 782a985d7af26db39e86070d28f987cad21313c0
	Author: Alex Williamson <[email protected]>
	Date:   Tue May 20 08:53:21 2014 -0600

    	PCI: Introduce new device binding path using pci_dev.driver_override
In order to bind PCI slot 0001:02:03.4 to pci-stub use:
	driver[0001:02:03.4]=pci-stub
Signed-off-by Marcel Apfelbaum <[email protected]>
---
Notes:
- For the moment only 32 slots can be assigned specific driver.
- I have further ideas on top of this patch based on your reviews. I thought of:
- Use wildcards to specify entire buses/devices, something like:
     	driver[0001:02:*.*]=pci-stub
   - Use comma to separate several devices:
     	driver[0001:02:03.4,0001:02:04.0,...]=pci-stub
   - Make domain optional:
   	driver[00:03.0]=pci-stub
Comments will be appreciated,
Thanks,
Marcel
Documentation/kernel-parameters.txt | 4 +++
 drivers/pci/bus.c                   |  1 +
 drivers/pci/pci.c                   | 61 +++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h                   |  1 +
 4 files changed, 67 insertions(+)
Documentation/kernel-parameters.txt +4 -0
index 5ae8608..c1cbb4c 100644
@@ -2631,6 +2631,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 		pcie_scan_all	Scan all possible PCIe devices.  Otherwise we
 				only look for one device below a PCIe downstream
 				port.
+		driver		Provide an override to the devid<->driver mapping
+				for a specific slot.
+				Bind PCI slot 0001:02:03.4 to pci-stub by:
+					driver[0001:02:03.4]=pci-stub
 
 	pcie_aspm=	[PCIE] Forcibly enable or disable PCIe Active State Power
 			Management.
drivers/pci/bus.c +1 -0
index 73aef51..7a7ed85 100644
@@ -245,6 +245,7 @@ void pci_bus_add_device(struct pci_dev *dev)
 	 * are not assigned yet for some devices.
 	 */
 	pci_fixup_device(pci_fixup_final, dev);
+	pci_device_setup_driver_override(dev);
 	pci_create_sysfs_dev_files(dev);
 	pci_proc_attach_device(dev);
 
drivers/pci/pci.c +19 -0
index b7678be..0a3e07c 100644
@@ -4426,6 +4426,65 @@ void __weak pci_fixup_cardbus(struct pci_bus *bus)
 }
 EXPORT_SYMBOL(pci_fixup_cardbus);
 
+#define DRIVER_OVERRIDE_NAME_LENGTH 64
+#define DRIVER_OVERRIDE_MAP_SIZE 32
+
+struct driver_override_entry {
+	unsigned int domain;
+	unsigned char bus;
+	unsigned int devfn;
+	char driver_name[DRIVER_OVERRIDE_NAME_LENGTH];
+};
+
+static struct driver_override_entry __initdata
+	driver_override_map[DRIVER_OVERRIDE_MAP_SIZE];
+static int driver_override_num_entries __initdata;
+
+void pci_device_setup_driver_override(struct pci_dev *dev)
+{
+	int i;
+
+	for (i = 0; i < driver_override_num_entries; i++) {
Quoted 10 lines
drivers/pci/pci.c void __weak pci_fixup_cardbus(struct pci_bus *bus) · line 4455
Quoted 8 lines
drivers/pci/pci.c +8 -0
+		if (pci_domain_nr(dev->bus) != driver_override_map[i].domain ||
+		    dev->bus->number != driver_override_map[i].bus ||
+		    dev->devfn != driver_override_map[i].devfn)
+			continue;
+
+		dev->driver_override = kstrndup(driver_override_map[i].driver_name,
+						DRIVER_OVERRIDE_NAME_LENGTH,
+						GFP_KERNEL);
Hi,
Quoted 1 line
You allocate this entry but never free it?
Is being freed in pci_release_dev function.
Quoted 1 line
Shouldn't allocation failure be handled in some way?
I thought about it, but in my opinion there is no need for error handling in this situation.
- On the worst case scenario it will load the regular driver, no harm done. (kstrndup will return NULL)
- It is used in pci_bus_add_device workflow that has no error flow that I am aware of, and even if it were an error flow, in my opinion this is not enough to stop system boot.
But thinking of this again, maybe a pr_err message?
Quoted 5 lines
Your review is most appreciated.
Thanks,
Marcel
Thinking more about it, how about just using strstr to search a string of overrides? This will completely remove the artificial DRIVER_OVERRIDE_MAP_SIZE as well.

Quoted 8 lines

Quoted 8 lines
Quoted 5 lines
I'll try it, thanks, hope it will not be too ugly.
I will still need to reserve a COMMAND_LINE_SIZE string for "string of overrides", right? Or can I dynamically allocate a buffer to hold the string so early in boot process? If I can, it will be no problem to create a dynamic sized driver_override_map.
I think Michael is suggesting searching the existing commandline on each pass so that you don't need to allocate any memory at all. Thanks,

I thought about doing that, but this would mean too add <linux/bootmem.h> header to drivers/pci/pci.c and it seems too much to expose all of that stuff only for this feature. Please see the alternative bellow.

Quoted 6 lines

Quoted 2 lines
Alex
Conditional on having the option specified.
BTW, API-wise, I think that it's a good idea to make the fact that pci addresses are used, explicit.

I am working on a way to use a dynamically allocated list to store the driver/slot matches and in this way we will have:
- no need to expose <linux/bootmem.h> to drivers/pci/pci.c
- memory will be used if needed and no pre-allocated
- no limit for the nr of slots to override.

V2 will be sent shortly.

Thanks,
Marcel

Quoted 67 lines

Quoted 65 lines

Quoted 62 lines

Quoted 60 lines

drivers/pci/pci.c +34 -0

+		break;
+	}
+}
+
+static void __init pci_parse_driver_override(char *str)
+{
+	unsigned int domain, bus, dev, fn;
+	char driver_name[DRIVER_OVERRIDE_NAME_LENGTH];
+	int ret, next_entry;
+	char format[] = "[%4x:%2x:%2x.%2x]=%%%ds";
+
+	sprintf(format + 18, "%%%ds", DRIVER_OVERRIDE_NAME_LENGTH);
+	ret = sscanf(str, format, &domain, &bus, &dev, &fn, driver_name);
+
+	if (ret != 5) {
+		pr_err("PCI: Invalid command line: driver%s\n", str);
+		return;
+	}
+
+	if (driver_override_num_entries == DRIVER_OVERRIDE_MAP_SIZE) {
+		pr_err("PCI: Driver map overflow - ignoring driver%s\n", str);
+		return;
+	}
+
+	next_entry = driver_override_num_entries++;
+	driver_override_map[next_entry].domain = domain;
+	driver_override_map[next_entry].bus = bus && 0xff;
+	driver_override_map[next_entry].devfn = ((dev & 0x1f) << 3) | (fn & 0x7);
+	memcpy(driver_override_map[next_entry].driver_name,
+	       driver_name, sizeof(driver_name));
+}
+
 static int __init pci_setup(char *str)
 {
 	while (str) {

@@ -4468,6 +4527,8 @@ static int __init pci_setup(char *str)
 				pcie_bus_config = PCIE_BUS_PEER2PEER;
 			} else if (!strncmp(str, "pcie_scan_all", 13)) {
 				pci_add_flags(PCI_SCAN_ALL_PCIE_DEVS);
+			} else if (!strncmp(str, "driver", 6)) {
+				pci_parse_driver_override(str + 6);
 			} else {
 				printk(KERN_ERR "PCI: Unknown option `%s'\n",
 						str);

drivers/pci/pci.h +1 -0

index 0601890..023d78b 100644
@@ -197,6 +197,7 @@ enum pci_bar_type {
 bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *pl,
 				int crs_timeout);
 int pci_setup_device(struct pci_dev *dev);
+void pci_device_setup_driver_override(struct pci_dev *dev);
 int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 		    struct resource *res, unsigned int reg);
 int pci_resource_bar(struct pci_dev *dev, int resno, enum pci_bar_type *type);

Signature · 1 line

1.8.3.1

Alex Williamson

2014-10-01 18:48 UTC #8 lore raw

In reply to Marcel Apfelbaum

Quoted 9 lines · Marcel Apfelbaum

Scanning a lot of devices during boot requires a lot of time. On other scenarios there is a need to bind a driver to a specific slot.
Binding devices to pci-stub driver does not work, as it will not differentiate between devices of the same type. Using some start scripts is error prone.
The solution leverages driver_override functionality introduced by

Quoted 8 lines

	commit: 782a985d7af26db39e86070d28f987cad21313c0
	Author: Alex Williamson <[email protected]>
	Date:   Tue May 20 08:53:21 2014 -0600

    	PCI: Introduce new device binding path using pci_dev.driver_override

In order to bind PCI slot 0001:02:03.4 to pci-stub use:

	driver[0001:02:03.4]=pci-stub

So the option is actually:

pci=driver=[...]=foo,driver=[...]=bar

That's not very clear from the commit log.

Quoted 56 lines

Signed-off-by Marcel Apfelbaum <[email protected]>

     	driver[0001:02:*.*]=pci-stub
   - Use comma to separate several devices:
     	driver[0001:02:03.4,0001:02:04.0,...]=pci-stub
   - Make domain optional:
   	driver[00:03.0]=pci-stub

Comments will be appreciated,
Thanks,
Marcel

Documentation/kernel-parameters.txt | 4 +++

 drivers/pci/bus.c                   |  1 +
 drivers/pci/pci.c                   | 61 +++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h                   |  1 +
 4 files changed, 67 insertions(+)

Documentation/kernel-parameters.txt +4 -0

index 5ae8608..c1cbb4c 100644
@@ -2631,6 +2631,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 		pcie_scan_all	Scan all possible PCIe devices.  Otherwise we
 				only look for one device below a PCIe downstream
 				port.
+		driver		Provide an override to the devid<->driver mapping
+				for a specific slot.
+				Bind PCI slot 0001:02:03.4 to pci-stub by:
+					driver[0001:02:03.4]=pci-stub
 
 	pcie_aspm=	[PCIE] Forcibly enable or disable PCIe Active State Power
 			Management.

drivers/pci/bus.c +1 -0

index 73aef51..7a7ed85 100644
@@ -245,6 +245,7 @@ void pci_bus_add_device(struct pci_dev *dev)
 	 * are not assigned yet for some devices.
 	 */
 	pci_fixup_device(pci_fixup_final, dev);
+	pci_device_setup_driver_override(dev);
 	pci_create_sysfs_dev_files(dev);
 	pci_proc_attach_device(dev);

drivers/pci/pci.c +0 -0

index b7678be..0a3e07c 100644

drivers/pci/pci.c void __weak pci_fixup_cardbus(struct pci_bus *bus) · line 4433

Quoted 8 lines

drivers/pci/pci.c +5 -0

@@ -4426,6 +4426,65 @@ void __weak pci_fixup_cardbus(struct pci_bus *bus)
 }
 EXPORT_SYMBOL(pci_fixup_cardbus);
 
+#define DRIVER_OVERRIDE_NAME_LENGTH 64
+#define DRIVER_OVERRIDE_MAP_SIZE 32
+
+struct driver_override_entry {
+	unsigned int domain;

u16

drivers/pci/pci.c void __weak pci_fixup_cardbus(struct pci_bus *bus) · line 4434

Quoted 1 line

drivers/pci/pci.c +1 -0
+	unsigned char bus;

drivers/pci/pci.c void __weak pci_fixup_cardbus(struct pci_bus *bus) · line 4435

Quoted 1 line

drivers/pci/pci.c +1 -0
+	unsigned int devfn;

Quoted 22 lines

drivers/pci/pci.c +22 -0

+	char driver_name[DRIVER_OVERRIDE_NAME_LENGTH];
+};
+
+static struct driver_override_entry __initdata
+	driver_override_map[DRIVER_OVERRIDE_MAP_SIZE];
+static int driver_override_num_entries __initdata;
+
+void pci_device_setup_driver_override(struct pci_dev *dev)
+{
+	int i;
+
+	for (i = 0; i < driver_override_num_entries; i++) {
+		if (pci_domain_nr(dev->bus) != driver_override_map[i].domain ||
+		    dev->bus->number != driver_override_map[i].bus ||
+		    dev->devfn != driver_override_map[i].devfn)
+			continue;
+
+		dev->driver_override = kstrndup(driver_override_map[i].driver_name,
+						DRIVER_OVERRIDE_NAME_LENGTH,
+						GFP_KERNEL);
+		break;
+	}

drivers/pci/pci.c void __weak pci_fixup_cardbus(struct pci_bus *bus) · line 4467

Quoted 10 lines

drivers/pci/pci.c +10 -0

+}
+
+static void __init pci_parse_driver_override(char *str)
+{
+	unsigned int domain, bus, dev, fn;
+	char driver_name[DRIVER_OVERRIDE_NAME_LENGTH];
+	int ret, next_entry;
+	char format[] = "[%4x:%2x:%2x.%2x]=%%%ds";
+
+	sprintf(format + 18, "%%%ds", DRIVER_OVERRIDE_NAME_LENGTH);

There must be a less confusing way to do this.

Quoted 5 lines

drivers/pci/pci.c +5 -0

+	ret = sscanf(str, format, &domain, &bus, &dev, &fn, driver_name);
+
+	if (ret != 5) {
+		pr_err("PCI: Invalid command line: driver%s\n", str);
+		return;

drivers/pci/pci.c void __weak pci_fixup_cardbus(struct pci_bus *bus) · line 4482

Quoted 10 lines

drivers/pci/pci.c +10 -0

+	}
+
+	if (driver_override_num_entries == DRIVER_OVERRIDE_MAP_SIZE) {
+		pr_err("PCI: Driver map overflow - ignoring driver%s\n", str);
+		return;
+	}
+
+	next_entry = driver_override_num_entries++;
+	driver_override_map[next_entry].domain = domain;
+	driver_override_map[next_entry].bus = bus && 0xff;

How could it not be given our format?

drivers/pci/pci.c void __weak pci_fixup_cardbus(struct pci_bus *bus) · line 4483

Quoted 1 line

drivers/pci/pci.c +1 -0

+	driver_override_map[next_entry].devfn = ((dev & 0x1f) << 3) | (fn & 0x7);

PCI_DEVFN(dev, fn)

Quoted 28 lines

drivers/pci/pci.c +6 -0

+	memcpy(driver_override_map[next_entry].driver_name,
+	       driver_name, sizeof(driver_name));
+}
+
 static int __init pci_setup(char *str)
 {
 	while (str) {

@@ -4468,6 +4527,8 @@ static int __init pci_setup(char *str)
 				pcie_bus_config = PCIE_BUS_PEER2PEER;
 			} else if (!strncmp(str, "pcie_scan_all", 13)) {
 				pci_add_flags(PCI_SCAN_ALL_PCIE_DEVS);
+			} else if (!strncmp(str, "driver", 6)) {
+				pci_parse_driver_override(str + 6);
 			} else {
 				printk(KERN_ERR "PCI: Unknown option `%s'\n",
 						str);

drivers/pci/pci.h +1 -0

index 0601890..023d78b 100644
@@ -197,6 +197,7 @@ enum pci_bar_type {
 bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *pl,
 				int crs_timeout);
 int pci_setup_device(struct pci_dev *dev);
+void pci_device_setup_driver_override(struct pci_dev *dev);
 int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 		    struct resource *res, unsigned int reg);
 int pci_resource_bar(struct pci_dev *dev, int resno, enum pci_bar_type *type);

Marcel Apfelbaum

author

2014-10-02 08:11 UTC #9 lore raw

Quoted 16 lines · Alex Williamson

Quoted 15 lines · Marcel Apfelbaum
Scanning a lot of devices during boot requires a lot of time. On other scenarios there is a need to bind a driver to a specific slot.
Binding devices to pci-stub driver does not work, as it will not differentiate between devices of the same type. Using some start scripts is error prone.
The solution leverages driver_override functionality introduced by
	commit: 782a985d7af26db39e86070d28f987cad21313c0
	Author: Alex Williamson <[email protected]>
	Date:   Tue May 20 08:53:21 2014 -0600

    	PCI: Introduce new device binding path using pci_dev.driver_override

Quoted 8 lines

Quoted 2 lines
In order to bind PCI slot 0001:02:03.4 to pci-stub use:
	driver[0001:02:03.4]=pci-stub
So the option is actually:
pci=driver=[...]=foo,driver=[...]=bar
That's not very clear from the commit log.

Hi,
I will make it more clear.

Thanks for the review,
Marcel

Quoted 164 lines

Quoted 57 lines

Signed-off-by Marcel Apfelbaum <[email protected]>

     	driver[0001:02:*.*]=pci-stub
   - Use comma to separate several devices:
     	driver[0001:02:03.4,0001:02:04.0,...]=pci-stub
   - Make domain optional:
   	driver[00:03.0]=pci-stub

Comments will be appreciated,
Thanks,
Marcel

Documentation/kernel-parameters.txt | 4 +++

 drivers/pci/bus.c                   |  1 +
 drivers/pci/pci.c                   | 61 +++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h                   |  1 +
 4 files changed, 67 insertions(+)

Documentation/kernel-parameters.txt +4 -0

index 5ae8608..c1cbb4c 100644
@@ -2631,6 +2631,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 		pcie_scan_all	Scan all possible PCIe devices.  Otherwise we
 				only look for one device below a PCIe downstream
 				port.
+		driver		Provide an override to the devid<->driver mapping
+				for a specific slot.
+				Bind PCI slot 0001:02:03.4 to pci-stub by:
+					driver[0001:02:03.4]=pci-stub
 
 	pcie_aspm=	[PCIE] Forcibly enable or disable PCIe Active State Power
 			Management.

drivers/pci/bus.c +1 -0

index 73aef51..7a7ed85 100644
@@ -245,6 +245,7 @@ void pci_bus_add_device(struct pci_dev *dev)
 	 * are not assigned yet for some devices.
 	 */
 	pci_fixup_device(pci_fixup_final, dev);
+	pci_device_setup_driver_override(dev);
 	pci_create_sysfs_dev_files(dev);
 	pci_proc_attach_device(dev);

drivers/pci/pci.c +0 -0

index b7678be..0a3e07c 100644

drivers/pci/pci.c void __weak pci_fixup_cardbus(struct pci_bus *bus) · line 4433

Quoted 8 lines

drivers/pci/pci.c +5 -0

@@ -4426,6 +4426,65 @@ void __weak pci_fixup_cardbus(struct pci_bus *bus)
 }
 EXPORT_SYMBOL(pci_fixup_cardbus);
 
+#define DRIVER_OVERRIDE_NAME_LENGTH 64
+#define DRIVER_OVERRIDE_MAP_SIZE 32
+
+struct driver_override_entry {
+	unsigned int domain;

u16

drivers/pci/pci.c void __weak pci_fixup_cardbus(struct pci_bus *bus) · line 4434

Quoted 1 line

drivers/pci/pci.c +1 -0
+	unsigned char bus;

drivers/pci/pci.c void __weak pci_fixup_cardbus(struct pci_bus *bus) · line 4435

Quoted 1 line

drivers/pci/pci.c +1 -0
+	unsigned int devfn;

Quoted 22 lines

drivers/pci/pci.c +22 -0

+	char driver_name[DRIVER_OVERRIDE_NAME_LENGTH];
+};
+
+static struct driver_override_entry __initdata
+	driver_override_map[DRIVER_OVERRIDE_MAP_SIZE];
+static int driver_override_num_entries __initdata;
+
+void pci_device_setup_driver_override(struct pci_dev *dev)
+{
+	int i;
+
+	for (i = 0; i < driver_override_num_entries; i++) {
+		if (pci_domain_nr(dev->bus) != driver_override_map[i].domain ||
+		    dev->bus->number != driver_override_map[i].bus ||
+		    dev->devfn != driver_override_map[i].devfn)
+			continue;
+
+		dev->driver_override = kstrndup(driver_override_map[i].driver_name,
+						DRIVER_OVERRIDE_NAME_LENGTH,
+						GFP_KERNEL);
+		break;
+	}

drivers/pci/pci.c void __weak pci_fixup_cardbus(struct pci_bus *bus) · line 4467

Quoted 10 lines

drivers/pci/pci.c +10 -0

+}
+
+static void __init pci_parse_driver_override(char *str)
+{
+	unsigned int domain, bus, dev, fn;
+	char driver_name[DRIVER_OVERRIDE_NAME_LENGTH];
+	int ret, next_entry;
+	char format[] = "[%4x:%2x:%2x.%2x]=%%%ds";
+
+	sprintf(format + 18, "%%%ds", DRIVER_OVERRIDE_NAME_LENGTH);

There must be a less confusing way to do this.

Quoted 5 lines

drivers/pci/pci.c +5 -0

+	ret = sscanf(str, format, &domain, &bus, &dev, &fn, driver_name);
+
+	if (ret != 5) {
+		pr_err("PCI: Invalid command line: driver%s\n", str);
+		return;

drivers/pci/pci.c void __weak pci_fixup_cardbus(struct pci_bus *bus) · line 4482

Quoted 10 lines

drivers/pci/pci.c +10 -0

+	}
+
+	if (driver_override_num_entries == DRIVER_OVERRIDE_MAP_SIZE) {
+		pr_err("PCI: Driver map overflow - ignoring driver%s\n", str);
+		return;
+	}
+
+	next_entry = driver_override_num_entries++;
+	driver_override_map[next_entry].domain = domain;
+	driver_override_map[next_entry].bus = bus && 0xff;

How could it not be given our format?

drivers/pci/pci.c void __weak pci_fixup_cardbus(struct pci_bus *bus) · line 4483

Quoted 1 line

drivers/pci/pci.c +1 -0

+	driver_override_map[next_entry].devfn = ((dev & 0x1f) << 3) | (fn & 0x7);

PCI_DEVFN(dev, fn)

Quoted 28 lines

drivers/pci/pci.c +6 -0

+	memcpy(driver_override_map[next_entry].driver_name,
+	       driver_name, sizeof(driver_name));
+}
+
 static int __init pci_setup(char *str)
 {
 	while (str) {

@@ -4468,6 +4527,8 @@ static int __init pci_setup(char *str)
 				pcie_bus_config = PCIE_BUS_PEER2PEER;
 			} else if (!strncmp(str, "pcie_scan_all", 13)) {
 				pci_add_flags(PCI_SCAN_ALL_PCIE_DEVS);
+			} else if (!strncmp(str, "driver", 6)) {
+				pci_parse_driver_override(str + 6);
 			} else {
 				printk(KERN_ERR "PCI: Unknown option `%s'\n",
 						str);

drivers/pci/pci.h +1 -0

index 0601890..023d78b 100644
@@ -197,6 +197,7 @@ enum pci_bar_type {
 bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *pl,
 				int crs_timeout);
 int pci_setup_device(struct pci_dev *dev);
+void pci_device_setup_driver_override(struct pci_dev *dev);
 int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 		    struct resource *res, unsigned int reg);
 int pci_resource_bar(struct pci_dev *dev, int resno, enum pci_bar_type *type);

Marcel Apfelbaum

author

2014-10-12 12:20 UTC #10 lore raw

In reply to Alex Williamson

Quoted 16 lines · Alex Williamson

Quoted 15 lines · Marcel Apfelbaum
Scanning a lot of devices during boot requires a lot of time. On other scenarios there is a need to bind a driver to a specific slot.
Binding devices to pci-stub driver does not work, as it will not differentiate between devices of the same type. Using some start scripts is error prone.
The solution leverages driver_override functionality introduced by
	commit: 782a985d7af26db39e86070d28f987cad21313c0
	Author: Alex Williamson <[email protected]>
	Date:   Tue May 20 08:53:21 2014 -0600

    	PCI: Introduce new device binding path using pci_dev.driver_override

Quoted 9 lines

Quoted 2 lines
In order to bind PCI slot 0001:02:03.4 to pci-stub use:
	driver[0001:02:03.4]=pci-stub
So the option is actually:
pci=driver=[...]=foo,driver=[...]=bar
That's not very clear from the commit log.

[...]
Hi Alex,
I am sorry, but I missed the bellow comments, I am going to fix them for the next version.

Quoted 5 lines

drivers/pci/pci.c void __weak pci_fixup_cardbus(struct pci_bus *bus) · line 4433
Quoted 3 lines
drivers/pci/pci.c +3 -0
+
+struct driver_override_entry {
+	unsigned int domain;
u16

Sure

Quoted 4 lines

drivers/pci/pci.c void __weak pci_fixup_cardbus(struct pci_bus *bus) · line 4434
Quoted 2 lines
drivers/pci/pci.c +1 -0
+	unsigned char bus;
u8

Sure

Quoted 4 lines

drivers/pci/pci.c void __weak pci_fixup_cardbus(struct pci_bus *bus) · line 4435
Quoted 2 lines
drivers/pci/pci.c +1 -0
+	unsigned int devfn;
u8

Sure

Quoted 2 lines

Review
Quoted 2 lines
(quoted diff) +1 -0
+	char driver_name[DRIVER_OVERRIDE_NAME_LENGTH];

I'll allocate it dynamically

Quoted 25 lines

drivers/pci/pci.c +25 -0

+};
+
+static struct driver_override_entry __initdata
+	driver_override_map[DRIVER_OVERRIDE_MAP_SIZE];
+static int driver_override_num_entries __initdata;
+
+void pci_device_setup_driver_override(struct pci_dev *dev)
+{
+	int i;
+
+	for (i = 0; i < driver_override_num_entries; i++) {
+		if (pci_domain_nr(dev->bus) != driver_override_map[i].domain ||
+		    dev->bus->number != driver_override_map[i].bus ||
+		    dev->devfn != driver_override_map[i].devfn)
+			continue;
+
+		dev->driver_override = kstrndup(driver_override_map[i].driver_name,
+						DRIVER_OVERRIDE_NAME_LENGTH,
+						GFP_KERNEL);
+		break;
+	}
+}
+
+static void __init pci_parse_driver_override(char *str)
+{

Quoted 8 lines

drivers/pci/pci.c void __weak pci_fixup_cardbus(struct pci_bus *bus) · line 4467
Quoted 6 lines
drivers/pci/pci.c +6 -0
+	unsigned int domain, bus, dev, fn;
+	char driver_name[DRIVER_OVERRIDE_NAME_LENGTH];
+	int ret, next_entry;
+	char format[] = "[%4x:%2x:%2x.%2x]=%%%ds";
+
+	sprintf(format + 18, "%%%ds", DRIVER_OVERRIDE_NAME_LENGTH);
There must be a less confusing way to do this.

I'll try it, and also I'll get rid of DRIVER_OVERRIDE_NAME_LENGTH restriction.

Quoted 10 lines

drivers/pci/pci.c +9 -0

+	ret = sscanf(str, format, &domain, &bus, &dev, &fn, driver_name);
+
+	if (ret != 5) {
+		pr_err("PCI: Invalid command line: driver%s\n", str);
+		return;
+	}
+
+	if (driver_override_num_entries == DRIVER_OVERRIDE_MAP_SIZE) {
+		pr_err("PCI: Driver map overflow - ignoring driver%s\n", str);

Quoted 8 lines

drivers/pci/pci.c void __weak pci_fixup_cardbus(struct pci_bus *bus) · line 4482
Quoted 6 lines
drivers/pci/pci.c +6 -0
+		return;
+	}
+
+	next_entry = driver_override_num_entries++;
+	driver_override_map[next_entry].domain = domain;
+	driver_override_map[next_entry].bus = bus && 0xff;
How could it not be given our format?

Right, I'll remove the check.

Quoted 4 lines

drivers/pci/pci.c void __weak pci_fixup_cardbus(struct pci_bus *bus) · line 4483
Quoted 2 lines
drivers/pci/pci.c +1 -0
+	driver_override_map[next_entry].devfn = ((dev & 0x1f) << 3) | (fn & 0x7);
PCI_DEVFN(dev, fn)

Sure,

Thank you for the review,
Marcel

Quoted 32 lines

Quoted 29 lines

drivers/pci/pci.c +6 -0

+	memcpy(driver_override_map[next_entry].driver_name,
+	       driver_name, sizeof(driver_name));
+}
+
 static int __init pci_setup(char *str)
 {
 	while (str) {

@@ -4468,6 +4527,8 @@ static int __init pci_setup(char *str)
 				pcie_bus_config = PCIE_BUS_PEER2PEER;
 			} else if (!strncmp(str, "pcie_scan_all", 13)) {
 				pci_add_flags(PCI_SCAN_ALL_PCIE_DEVS);
+			} else if (!strncmp(str, "driver", 6)) {
+				pci_parse_driver_override(str + 6);
 			} else {
 				printk(KERN_ERR "PCI: Unknown option `%s'\n",
 						str);

drivers/pci/pci.h +1 -0

index 0601890..023d78b 100644
@@ -197,6 +197,7 @@ enum pci_bar_type {
 bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *pl,
 				int crs_timeout);
 int pci_setup_device(struct pci_dev *dev);
+void pci_device_setup_driver_override(struct pci_dev *dev);
 int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 		    struct resource *res, unsigned int reg);
 int pci_resource_bar(struct pci_dev *dev, int resno, enum pci_bar_type *type);