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

[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 <***@redhat.com>
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 <***@redhat.com>
---
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(+)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 5ae8608..c1cbb4c 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -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.
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 73aef51..7a7ed85 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -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);

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b7678be..0a3e07c 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -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++) {
+ 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)
+{
+ 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);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 0601890..023d78b 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -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);

[Michael S. Tsirkin]

Post by 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
Date: Tue May 20 08:53:21 2014 -0600
PCI: Introduce new device binding path using pci_dev.driver_override
driver[0001:02:03.4]=pci-stub
---
- For the moment only 32 slots can be assigned specific driver.
- I have further ideas on top of this patch based on your reviews.
driver[0001:02:*.*]=pci-stub
driver[0001:02:03.4,0001:02:04.0,...]=pci-stub
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(+)
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 5ae8608..c1cbb4c 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -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.
+ driver[0001:02:03.4]=pci-stub
pcie_aspm= [PCIE] Forcibly enable or disable PCIe Active State Power
Management.
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 73aef51..7a7ed85 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -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);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b7678be..0a3e07c 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -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++) {
+ 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?

Post by Marcel Apfelbaum
+ 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);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 0601890..023d78b 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -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);
--
1.8.3.1

[Marcel Apfelbaum]

Post by Michael S. Tsirkin

Post by 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
Date: Tue May 20 08:53:21 2014 -0600
PCI: Introduce new device binding path using pci_dev.driver_override
driver[0001:02:03.4]=pci-stub
---
- For the moment only 32 slots can be assigned specific driver.
- I have further ideas on top of this patch based on your reviews.
driver[0001:02:*.*]=pci-stub
driver[0001:02:03.4,0001:02:04.0,...]=pci-stub
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(+)
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 5ae8608..c1cbb4c 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -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.
+ driver[0001:02:03.4]=pci-stub
pcie_aspm= [PCIE] Forcibly enable or disable PCIe Active State Power
Management.
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 73aef51..7a7ed85 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -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);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b7678be..0a3e07c 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -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++) {
+ 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,

Post by Michael S. Tsirkin
You allocate this entry but never free it?

Is being freed in pci_release_dev function.

Post by Michael S. Tsirkin
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

Post by Michael S. Tsirkin

Post by Marcel Apfelbaum
+ 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);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 0601890..023d78b 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -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);
--
1.8.3.1

[Michael S. Tsirkin]

Post by Marcel Apfelbaum

Post by Michael S. Tsirkin

Post by 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
Date: Tue May 20 08:53:21 2014 -0600
PCI: Introduce new device binding path using pci_dev.driver_override
driver[0001:02:03.4]=pci-stub
---
- For the moment only 32 slots can be assigned specific driver.
- I have further ideas on top of this patch based on your reviews.
driver[0001:02:*.*]=pci-stub
driver[0001:02:03.4,0001:02:04.0,...]=pci-stub
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(+)
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 5ae8608..c1cbb4c 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -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.
+ driver[0001:02:03.4]=pci-stub
pcie_aspm= [PCIE] Forcibly enable or disable PCIe Active State Power
Management.
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 73aef51..7a7ed85 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -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);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b7678be..0a3e07c 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -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++) {
+ 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,

Post by Michael S. Tsirkin
You allocate this entry but never free it?

Is being freed in pci_release_dev function.

Post by Michael S. Tsirkin
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

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.

Post by Marcel Apfelbaum

Post by Michael S. Tsirkin

Post by Marcel Apfelbaum
+ 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);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 0601890..023d78b 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -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);
--
1.8.3.1

[Marcel Apfelbaum]

Post by Michael S. Tsirkin

Post by Marcel Apfelbaum

Post by Michael S. Tsirkin

Post by 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
Date: Tue May 20 08:53:21 2014 -0600
PCI: Introduce new device binding path using pci_dev.driver_override
driver[0001:02:03.4]=pci-stub
---
- For the moment only 32 slots can be assigned specific driver.
- I have further ideas on top of this patch based on your reviews.
driver[0001:02:*.*]=pci-stub
driver[0001:02:03.4,0001:02:04.0,...]=pci-stub
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(+)
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 5ae8608..c1cbb4c 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -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.
+ driver[0001:02:03.4]=pci-stub
pcie_aspm= [PCIE] Forcibly enable or disable PCIe Active State Power
Management.
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 73aef51..7a7ed85 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -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);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b7678be..0a3e07c 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -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++) {
+ 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,

Post by Michael S. Tsirkin
You allocate this entry but never free it?

Is being freed in pci_release_dev function.

Post by Michael S. Tsirkin
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

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

Post by Michael S. Tsirkin

Post by Marcel Apfelbaum

Post by Michael S. Tsirkin

Post by Marcel Apfelbaum
+ 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);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 0601890..023d78b 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -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);
--
1.8.3.1

[Alex Williamson]

Post by Marcel Apfelbaum

Post by Michael S. Tsirkin

Post by Marcel Apfelbaum

Post by Michael S. Tsirkin

Post by 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
Date: Tue May 20 08:53:21 2014 -0600
PCI: Introduce new device binding path using pci_dev.driver_override
driver[0001:02:03.4]=pci-stub
---
- For the moment only 32 slots can be assigned specific driver.
- I have further ideas on top of this patch based on your reviews.
driver[0001:02:*.*]=pci-stub
driver[0001:02:03.4,0001:02:04.0,...]=pci-stub
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(+)
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 5ae8608..c1cbb4c 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -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.
+ driver[0001:02:03.4]=pci-stub
pcie_aspm= [PCIE] Forcibly enable or disable PCIe Active State Power
Management.
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 73aef51..7a7ed85 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -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);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b7678be..0a3e07c 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -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++) {
+ 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,

Post by Michael S. Tsirkin
You allocate this entry but never free it?

Is being freed in pci_release_dev function.

Post by Michael S. Tsirkin
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

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

Post by Marcel Apfelbaum

Post by Michael S. Tsirkin

Post by Marcel Apfelbaum

Post by Michael S. Tsirkin

Post by Marcel Apfelbaum
+ 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);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 0601890..023d78b 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -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);
--
1.8.3.1

[Michael S. Tsirkin]

Post by Alex Williamson

Post by Marcel Apfelbaum

Post by Michael S. Tsirkin

Post by Marcel Apfelbaum

Post by Michael S. Tsirkin

Post by 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
Date: Tue May 20 08:53:21 2014 -0600
PCI: Introduce new device binding path using pci_dev.driver_override
driver[0001:02:03.4]=pci-stub
---
- For the moment only 32 slots can be assigned specific driver.
- I have further ideas on top of this patch based on your reviews.
driver[0001:02:*.*]=pci-stub
driver[0001:02:03.4,0001:02:04.0,...]=pci-stub
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(+)
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 5ae8608..c1cbb4c 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -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.
+ driver[0001:02:03.4]=pci-stub
pcie_aspm= [PCIE] Forcibly enable or disable PCIe Active State Power
Management.
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 73aef51..7a7ed85 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -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);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b7678be..0a3e07c 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -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++) {
+ 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,

Post by Michael S. Tsirkin
You allocate this entry but never free it?

Is being freed in pci_release_dev function.

Post by Michael S. Tsirkin
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

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

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.

Post by Alex Williamson

Post by Marcel Apfelbaum

Post by Michael S. Tsirkin

Post by Marcel Apfelbaum

Post by Michael S. Tsirkin

Post by Marcel Apfelbaum
+ 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);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 0601890..023d78b 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -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);
--
1.8.3.1

[Marcel Apfelbaum]

Post by Michael S. Tsirkin

Post by Alex Williamson

Post by Marcel Apfelbaum

Post by Michael S. Tsirkin

Post by Marcel Apfelbaum

Post by Michael S. Tsirkin

Post by 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
Date: Tue May 20 08:53:21 2014 -0600
PCI: Introduce new device binding path using pci_dev.driver_override
driver[0001:02:03.4]=pci-stub
---
- For the moment only 32 slots can be assigned specific driver.
- I have further ideas on top of this patch based on your reviews.
driver[0001:02:*.*]=pci-stub
driver[0001:02:03.4,0001:02:04.0,...]=pci-stub
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(+)
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 5ae8608..c1cbb4c 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -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.
+ driver[0001:02:03.4]=pci-stub
pcie_aspm= [PCIE] Forcibly enable or disable PCIe Active State Power
Management.
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 73aef51..7a7ed85 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -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);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b7678be..0a3e07c 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -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++) {
+ 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,

Post by Michael S. Tsirkin
You allocate this entry but never free it?

Is being freed in pci_release_dev function.

Post by Michael S. Tsirkin
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

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,

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.

Post by Michael S. Tsirkin

Post by Alex Williamson
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

Post by Michael S. Tsirkin

Post by Alex Williamson

Post by Marcel Apfelbaum

Post by Michael S. Tsirkin

Post by Marcel Apfelbaum

Post by Michael S. Tsirkin

Post by Marcel Apfelbaum
+ 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);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 0601890..023d78b 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -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);
--
1.8.3.1

[Alex Williamson]

Post by 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
Date: Tue May 20 08:53:21 2014 -0600
PCI: Introduce new device binding path using pci_dev.driver_override
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.

Post by Marcel Apfelbaum
---
- For the moment only 32 slots can be assigned specific driver.
- I have further ideas on top of this patch based on your reviews.
driver[0001:02:*.*]=pci-stub
driver[0001:02:03.4,0001:02:04.0,...]=pci-stub
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(+)
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 5ae8608..c1cbb4c 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -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.
+ driver[0001:02:03.4]=pci-stub
pcie_aspm= [PCIE] Forcibly enable or disable PCIe Active State Power
Management.
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 73aef51..7a7ed85 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -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);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b7678be..0a3e07c 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -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

Post by Marcel Apfelbaum
+ unsigned char bus;

u8

Post by Marcel Apfelbaum
+ unsigned int devfn;

u8

Post by Marcel Apfelbaum
+ 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;
+ }
+}
+
+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.

Post by Marcel Apfelbaum
+ 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;

How could it not be given our format?

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

PCI_DEVFN(dev, fn)

Post by Marcel Apfelbaum
+ 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);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 0601890..023d78b 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -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]

Post by Alex Williamson

Post by 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
Date: Tue May 20 08:53:21 2014 -0600
PCI: Introduce new device binding path using pci_dev.driver_override
driver[0001:02:03.4]=pci-stub

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

Post by Alex Williamson

Post by Marcel Apfelbaum
---
- For the moment only 32 slots can be assigned specific driver.
- I have further ideas on top of this patch based on your reviews.
driver[0001:02:*.*]=pci-stub
driver[0001:02:03.4,0001:02:04.0,...]=pci-stub
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(+)
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 5ae8608..c1cbb4c 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -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.
+ driver[0001:02:03.4]=pci-stub
pcie_aspm= [PCIE] Forcibly enable or disable PCIe Active State Power
Management.
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 73aef51..7a7ed85 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -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);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b7678be..0a3e07c 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -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

Post by Marcel Apfelbaum
+ unsigned char bus;

u8

Post by Marcel Apfelbaum
+ unsigned int devfn;

u8

Post by Marcel Apfelbaum
+ 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;
+ }
+}
+
+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.

Post by Marcel Apfelbaum
+ 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;

How could it not be given our format?

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

PCI_DEVFN(dev, fn)

Post by Marcel Apfelbaum
+ 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);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 0601890..023d78b 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -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]

Post by Alex Williamson

Post by 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
Date: Tue May 20 08:53:21 2014 -0600
PCI: Introduce new device binding path using pci_dev.driver_override
driver[0001:02:03.4]=pci-stub

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.

Post by Alex Williamson

Post by Marcel Apfelbaum
+
+struct driver_override_entry {
+ unsigned int domain;

u16

Sure

Post by Alex Williamson

Post by Marcel Apfelbaum
+ unsigned char bus;

u8

Sure

Post by Alex Williamson

Post by Marcel Apfelbaum
+ unsigned int devfn;

u8

Sure

Post by Alex Williamson

Post by Marcel Apfelbaum
+ char driver_name[DRIVER_OVERRIDE_NAME_LENGTH];

I'll allocate it dynamically

Post by Alex Williamson

Post by Marcel Apfelbaum
+};
+
+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)
+{
+ 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.

Post by Alex Williamson

Post by Marcel Apfelbaum
+ 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;

How could it not be given our format?

Right, I'll remove the check.

Post by Alex Williamson

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

PCI_DEVFN(dev, fn)

Sure,

Thank you for the review,
Marcel

Post by Alex Williamson

Post by Marcel Apfelbaum
+ 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);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 0601890..023d78b 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -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);