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

[PATCH v4] 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 <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 slots to specific drivers use:
	pci=driver[xxxx:xx:xx.x]=foo,driver[xxxx:xx:xx.x]=bar,...

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
v3 -> v4:
 - Addressed Alex Williamson's comments:
   - Modified the type of driver_override_entry's fields
   - Used PCI_DEVFN when appropriated
   - Removed redundant checks
   - Replaced BUG_ON with pr_err messages
   - Simpler command line parsing
 - Addressed Michael S. Tsirkin comments
   - removed DRIVER_OVERRIDE_NAME_LENGTH limitation
v2 -> v3:
 - Corrected subject line
v1 -> v2:
 - Addressed Michael S. Tsirkin comments
   - Removed 32 slots limitation
   - Better handling of memory allocation failures
     (preferred BUG_ON over error messages)
 - Addressed Alex Williamson's comments:
   - Modified commit message to show parameter usage more clear.
 - I preferred to re-use parse_args instead of manually using
   strstr in order to better comply with command line parsing
   rules.
 - I didn't use any locking when parsing the command line args
   (see parse_done usage) assuming that first call will be
   early in system boot and no race can occur. Please correct
   me if I am wrong.

Notes:
 - 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                   | 111 ++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.c                   |   2 +
 3 files changed, 117 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..b49f5cc 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -15,6 +15,8 @@
 #include <linux/proc_fs.h>
 #include <linux/slab.h>
 
+#include <asm/setup.h>
+
 #include "pci.h"
 
 void pci_add_resource_offset(struct list_head *resources, struct resource *res,
@@ -230,6 +232,114 @@ EXPORT_SYMBOL(pci_bus_alloc_resource);
 
 void __weak pcibios_resource_survey_bus(struct pci_bus *bus) { }
 
+struct driver_override_entry {
+	u16 domain;
+	u8 bus;
+	u8 devfn;
+	char *driver_name;
+	struct list_head list;
+};
+
+static LIST_HEAD(driver_override_entries);
+
+static int pci_device_parse_driver_override(char *param, char *val,
+					    const char *unused)
+{
+	unsigned int domain, bus, dev, fn;
+	char  *buf;
+	struct driver_override_entry *entry;
+	int ret;
+
+	buf = kmalloc(COMMAND_LINE_SIZE, GFP_KERNEL);
+	if (!buf)
+		goto err_buf;
+
+	while (val) {
+		char *k = strchr(val, ',');
+
+		if (k)
+			*k++ = 0;
+
+		if (strncmp(val, "driver", 6)) {
+			val = k;
+			continue;
+		}
+
+		memset(buf, 0, COMMAND_LINE_SIZE);
+		ret = sscanf(val + 6, "[%4x:%2x:%2x.%2x]=%s",
+			     &domain, &bus, &dev, &fn, buf);
+		if (ret != 5) {
+			pr_warn("PCI: Invalid command line: %s\n", val);
+			val = k;
+			continue;
+		}
+
+		entry = kzalloc(sizeof(*entry), GFP_KERNEL);
+		if (!entry)
+			goto err_entry;
+
+		INIT_LIST_HEAD(&entry->list);
+		entry->domain = domain;
+		entry->bus = bus;
+		entry->devfn = PCI_DEVFN(dev, fn);
+		entry->driver_name = kstrdup(buf, GFP_KERNEL);
+		if (!entry->driver_name)
+			goto err_driver_name;
+
+		list_add_tail(&entry->list, &driver_override_entries);
+		val = k;
+	}
+
+	kfree(buf);
+	return 0;
+
+err_driver_name:
+	kfree(entry);
+
+err_entry:
+	kfree(buf);
+
+err_buf:
+	pr_err("PCI: Out of memory while parsing command line: %s\n", val);
+	return -ENOMEM;
+}
+
+static void pci_device_setup_driver_override(struct pci_dev *dev)
+{
+	static int parse_done;
+	struct driver_override_entry *entry;
+
+	if (!parse_done) {
+		char *cmdline = kstrdup(saved_command_line, GFP_KERNEL);
+
+		if (!cmdline)
+			goto err_out_of_mem;
+
+		parse_args("pci", cmdline, NULL,
+			   0, 0, 0, &pci_device_parse_driver_override);
+		kfree(cmdline);
+		parse_done = 1;
+	}
+
+	list_for_each_entry(entry, &driver_override_entries, list) {
+		if (pci_domain_nr(dev->bus) != entry->domain ||
+		    dev->bus->number != entry->bus ||
+		    dev->devfn != entry->devfn)
+			continue;
+
+		dev->driver_override = kstrdup(entry->driver_name, GFP_KERNEL);
+		if (!dev->driver_override)
+			goto err_out_of_mem;
+
+		break;
+	}
+
+	return;
+
+err_out_of_mem:
+	pr_err("PCI: Out of memory while setting up driver override\n");
+}
+
 /**
  * pci_bus_add_device - start driver for a single device
  * @dev: device to add
@@ -245,6 +355,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 625a4ac..37809d4 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4508,6 +4508,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)) {
+				/* lazy evaluation by the pci subsystem */
 			} else {
 				printk(KERN_ERR "PCI: Unknown option `%s'\n",
 						str);
-- 
1.8.3.1

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

[Alex Williamson]

[cc+ stuart]

On Mon, 2014-10-20 at 17:04 +0300, Marcel Apfelbaum wrote:
> 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 <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 slots to specific drivers use:
> 	pci=driver[xxxx:xx:xx.x]=foo,driver[xxxx:xx:xx.x]=bar,...
> 
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
> v3 -> v4:
>  - Addressed Alex Williamson's comments:
>    - Modified the type of driver_override_entry's fields
>    - Used PCI_DEVFN when appropriated
>    - Removed redundant checks
>    - Replaced BUG_ON with pr_err messages
>    - Simpler command line parsing
>  - Addressed Michael S. Tsirkin comments
>    - removed DRIVER_OVERRIDE_NAME_LENGTH limitation
> v2 -> v3:
>  - Corrected subject line
> v1 -> v2:
>  - Addressed Michael S. Tsirkin comments
>    - Removed 32 slots limitation
>    - Better handling of memory allocation failures
>      (preferred BUG_ON over error messages)
>  - Addressed Alex Williamson's comments:
>    - Modified commit message to show parameter usage more clear.
>  - I preferred to re-use parse_args instead of manually using
>    strstr in order to better comply with command line parsing
>    rules.
>  - I didn't use any locking when parsing the command line args
>    (see parse_done usage) assuming that first call will be
>    early in system boot and no race can occur. Please correct
>    me if I am wrong.
> 
> Notes:
>  - 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                   | 111 ++++++++++++++++++++++++++++++++++++
>  drivers/pci/pci.c                   |   2 +
>  3 files changed, 117 insertions(+)

The driver_override feature that we're making use of here is also going
to be supported by platform devices and potentially more bustypes in the
future, so I'm concerned that making a pci specific kernel parameter is
too shortsighted.  Instead we could hook on to BUS_NOTIFY_ADD_DEVICE for
bustypes that support driver_override so we can have a common interface.
Perhaps:

driver_override=pci,0000:02:00.0=pci-stub;platform,fakename=vfio-platform

Finding delimiters that don't conflict may be challenging.  Also, can we
assume that bus-name:dev-name is unique for every bustype?  It is for
pci, platform?

It also seems like there's a question of how long should this override
last and how does the user disable it?  I think with pci-stub.ids=
$VENDOR:$DEVICE a user can echo the IDs to the pci-stub/remove_id sysfs
entry to cancel the effect.  The only option here seems to be a reboot.
Do we need a /sys/bus/pci/driver_overrides/{add_name,remove_name} for
this interface?  Thanks,

Alex

> 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..b49f5cc 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -15,6 +15,8 @@
>  #include <linux/proc_fs.h>
>  #include <linux/slab.h>
>  
> +#include <asm/setup.h>
> +
>  #include "pci.h"
>  
>  void pci_add_resource_offset(struct list_head *resources, struct resource *res,
> @@ -230,6 +232,114 @@ EXPORT_SYMBOL(pci_bus_alloc_resource);
>  
>  void __weak pcibios_resource_survey_bus(struct pci_bus *bus) { }
>  
> +struct driver_override_entry {
> +	u16 domain;
> +	u8 bus;
> +	u8 devfn;
> +	char *driver_name;
> +	struct list_head list;
> +};
> +
> +static LIST_HEAD(driver_override_entries);
> +
> +static int pci_device_parse_driver_override(char *param, char *val,
> +					    const char *unused)
> +{
> +	unsigned int domain, bus, dev, fn;
> +	char  *buf;
> +	struct driver_override_entry *entry;
> +	int ret;
> +
> +	buf = kmalloc(COMMAND_LINE_SIZE, GFP_KERNEL);
> +	if (!buf)
> +		goto err_buf;
> +
> +	while (val) {
> +		char *k = strchr(val, ',');
> +
> +		if (k)
> +			*k++ = 0;
> +
> +		if (strncmp(val, "driver", 6)) {
> +			val = k;
> +			continue;
> +		}
> +
> +		memset(buf, 0, COMMAND_LINE_SIZE);
> +		ret = sscanf(val + 6, "[%4x:%2x:%2x.%2x]=%s",
> +			     &domain, &bus, &dev, &fn, buf);
> +		if (ret != 5) {
> +			pr_warn("PCI: Invalid command line: %s\n", val);
> +			val = k;
> +			continue;
> +		}
> +
> +		entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> +		if (!entry)
> +			goto err_entry;
> +
> +		INIT_LIST_HEAD(&entry->list);
> +		entry->domain = domain;
> +		entry->bus = bus;
> +		entry->devfn = PCI_DEVFN(dev, fn);
> +		entry->driver_name = kstrdup(buf, GFP_KERNEL);
> +		if (!entry->driver_name)
> +			goto err_driver_name;
> +
> +		list_add_tail(&entry->list, &driver_override_entries);
> +		val = k;
> +	}
> +
> +	kfree(buf);
> +	return 0;
> +
> +err_driver_name:
> +	kfree(entry);
> +
> +err_entry:
> +	kfree(buf);
> +
> +err_buf:
> +	pr_err("PCI: Out of memory while parsing command line: %s\n", val);
> +	return -ENOMEM;
> +}
> +
> +static void pci_device_setup_driver_override(struct pci_dev *dev)
> +{
> +	static int parse_done;
> +	struct driver_override_entry *entry;
> +
> +	if (!parse_done) {
> +		char *cmdline = kstrdup(saved_command_line, GFP_KERNEL);
> +
> +		if (!cmdline)
> +			goto err_out_of_mem;
> +
> +		parse_args("pci", cmdline, NULL,
> +			   0, 0, 0, &pci_device_parse_driver_override);
> +		kfree(cmdline);
> +		parse_done = 1;
> +	}
> +
> +	list_for_each_entry(entry, &driver_override_entries, list) {
> +		if (pci_domain_nr(dev->bus) != entry->domain ||
> +		    dev->bus->number != entry->bus ||
> +		    dev->devfn != entry->devfn)
> +			continue;
> +
> +		dev->driver_override = kstrdup(entry->driver_name, GFP_KERNEL);
> +		if (!dev->driver_override)
> +			goto err_out_of_mem;
> +
> +		break;
> +	}
> +
> +	return;
> +
> +err_out_of_mem:
> +	pr_err("PCI: Out of memory while setting up driver override\n");
> +}
> +
>  /**
>   * pci_bus_add_device - start driver for a single device
>   * @dev: device to add
> @@ -245,6 +355,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 625a4ac..37809d4 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4508,6 +4508,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)) {
> +				/* lazy evaluation by the pci subsystem */
>  			} else {
>  				printk(KERN_ERR "PCI: Unknown option `%s'\n",
>  						str);

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

[Bjorn Helgaas]

Hi Marcel,

I'm not quite clear on what the objective is here, so I apologize for
some questions that probably seem silly.

On Mon, Oct 20, 2014 at 8:04 AM, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> Scanning a lot of devices during boot requires a lot of time.

I think what takes a lot of time is the .probe() method for some
drivers, right?  I first thought you meant that it took a long time
for the PCI core to enumerate a lot of devices, but you're not
changing anything there.

If the intent is to reduce boot time, I don't think this is a general
solution.  Drivers should be able to schedule asynchronous things in
their .probe() methods if necessary.

> On other scenarios there is a need to bind a driver to a specific slot.

A short example here would be good.  Are you talking about something
like binding a NIC driver to one device while leaving others unbound
for use by guests?

> Binding devices to pci-stub driver does not work,
> as it will not differentiate between devices of the
> same type.

I assume you mean booting with "pci-stub.ids=$VENDOR:$DEVICE" will
make pci-stub bind to *all* matching devices, and you only want it to
bind to some.  Maybe pci-stub could be extended to pay attention to
PCI bus addresses in addition to vendor/device IDs.

> Using some start scripts is error prone.
>
> The solution leverages driver_override functionality introduced by
>
>         commit: 782a985d7af26db39e86070d28f987cad21313c0
>         Author: Alex Williamson <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 slots to specific drivers use:
>         pci=driver[xxxx:xx:xx.x]=foo,driver[xxxx:xx:xx.x]=bar,...

If/when you address Alex's comments about other bus types, can you
also update the changelog to use the canonical commit reference
format, i.e., 782a985d7af2 ("PCI: Introduce new device binding path
using pci_dev.driver_override") in this case?

PCI bus numbers are mutable, e.g., they can change with hotplug or
other configuration changes.  But I don't have any better suggestion,
so I guess all we can do is be aware of this.

Speaking of hotplug, this is only a boot-time kernel parameter, with
no opportunity to use this, e.g., to add slot/driver pairs, after
boot.  Do you not need that because of Alex's driver_override thing?
How can we integrate this all together into a coherent whole?  I'm a
little confused as to how this would all be documented in a form
usable by end-users.

Bjorn

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

[Marcel Apfelbaum]

On Wed, 2014-10-22 at 12:32 -0600, Alex Williamson wrote:
> [cc+ stuart]
> 
> On Mon, 2014-10-20 at 17:04 +0300, Marcel Apfelbaum wrote:
> > 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 <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 slots to specific drivers use:
> > 	pci=driver[xxxx:xx:xx.x]=foo,driver[xxxx:xx:xx.x]=bar,...
> > 
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> > v3 -> v4:
> >  - Addressed Alex Williamson's comments:
> >    - Modified the type of driver_override_entry's fields
> >    - Used PCI_DEVFN when appropriated
> >    - Removed redundant checks
> >    - Replaced BUG_ON with pr_err messages
> >    - Simpler command line parsing
> >  - Addressed Michael S. Tsirkin comments
> >    - removed DRIVER_OVERRIDE_NAME_LENGTH limitation
> > v2 -> v3:
> >  - Corrected subject line
> > v1 -> v2:
> >  - Addressed Michael S. Tsirkin comments
> >    - Removed 32 slots limitation
> >    - Better handling of memory allocation failures
> >      (preferred BUG_ON over error messages)
> >  - Addressed Alex Williamson's comments:
> >    - Modified commit message to show parameter usage more clear.
> >  - I preferred to re-use parse_args instead of manually using
> >    strstr in order to better comply with command line parsing
> >    rules.
> >  - I didn't use any locking when parsing the command line args
> >    (see parse_done usage) assuming that first call will be
> >    early in system boot and no race can occur. Please correct
> >    me if I am wrong.
> > 
> > Notes:
> >  - 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                   | 111 ++++++++++++++++++++++++++++++++++++
> >  drivers/pci/pci.c                   |   2 +
> >  3 files changed, 117 insertions(+)
> 
> The driver_override feature that we're making use of here is also going
> to be supported by platform devices and potentially more bustypes in the
> future, so I'm concerned that making a pci specific kernel parameter is
> too shortsighted.  Instead we could hook on to BUS_NOTIFY_ADD_DEVICE for
> bustypes that support driver_override so we can have a common interface.
The real question here if those bus types/devices would benefit from this
feature, and I also must confess that I have no knowledge of the other buses.
Can anyone confirm that it does make sense for them?

> Perhaps:
> 
> driver_override=pci,0000:02:00.0=pci-stub;platform,fakename=vfio-platform
> 
> Finding delimiters that don't conflict may be challenging.  Also, can we
> assume that bus-name:dev-name is unique for every bustype?  It is for
> pci, platform?
For PCI, sure the domain:bus:dev.func is unique, for platform I have no idea,
can anyone that knows "platform" confirm or deny?

> 
> It also seems like there's a question of how long should this override
> last and how does the user disable it?  
> I think with pci-stub.ids=
> $VENDOR:$DEVICE a user can echo the IDs to the pci-stub/remove_id sysfs
> entry to cancel the effect.
The way I see it is simple, the override specified in kernel command line
last as long as the user does not specifically remove it using
echo "" > /sys/.../driver_override
and then unbind and bind the device again.

>   The only option here seems to be a reboot.
Please see above

> Do we need a /sys/bus/pci/driver_overrides/{add_name,remove_name} for
> this interface?  Thanks,
While it does not hurt, I see it as optional since a simple removal of
driver_override and rebind does the same

Thanks,
Marcel

> 
> Alex
> 
> > 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..b49f5cc 100644
> > --- a/drivers/pci/bus.c
> > +++ b/drivers/pci/bus.c
> > @@ -15,6 +15,8 @@
> >  #include <linux/proc_fs.h>
> >  #include <linux/slab.h>
> >  
> > +#include <asm/setup.h>
> > +
> >  #include "pci.h"
> >  
> >  void pci_add_resource_offset(struct list_head *resources, struct resource *res,
> > @@ -230,6 +232,114 @@ EXPORT_SYMBOL(pci_bus_alloc_resource);
> >  
> >  void __weak pcibios_resource_survey_bus(struct pci_bus *bus) { }
> >  
> > +struct driver_override_entry {
> > +	u16 domain;
> > +	u8 bus;
> > +	u8 devfn;
> > +	char *driver_name;
> > +	struct list_head list;
> > +};
> > +
> > +static LIST_HEAD(driver_override_entries);
> > +
> > +static int pci_device_parse_driver_override(char *param, char *val,
> > +					    const char *unused)
> > +{
> > +	unsigned int domain, bus, dev, fn;
> > +	char  *buf;
> > +	struct driver_override_entry *entry;
> > +	int ret;
> > +
> > +	buf = kmalloc(COMMAND_LINE_SIZE, GFP_KERNEL);
> > +	if (!buf)
> > +		goto err_buf;
> > +
> > +	while (val) {
> > +		char *k = strchr(val, ',');
> > +
> > +		if (k)
> > +			*k++ = 0;
> > +
> > +		if (strncmp(val, "driver", 6)) {
> > +			val = k;
> > +			continue;
> > +		}
> > +
> > +		memset(buf, 0, COMMAND_LINE_SIZE);
> > +		ret = sscanf(val + 6, "[%4x:%2x:%2x.%2x]=%s",
> > +			     &domain, &bus, &dev, &fn, buf);
> > +		if (ret != 5) {
> > +			pr_warn("PCI: Invalid command line: %s\n", val);
> > +			val = k;
> > +			continue;
> > +		}
> > +
> > +		entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> > +		if (!entry)
> > +			goto err_entry;
> > +
> > +		INIT_LIST_HEAD(&entry->list);
> > +		entry->domain = domain;
> > +		entry->bus = bus;
> > +		entry->devfn = PCI_DEVFN(dev, fn);
> > +		entry->driver_name = kstrdup(buf, GFP_KERNEL);
> > +		if (!entry->driver_name)
> > +			goto err_driver_name;
> > +
> > +		list_add_tail(&entry->list, &driver_override_entries);
> > +		val = k;
> > +	}
> > +
> > +	kfree(buf);
> > +	return 0;
> > +
> > +err_driver_name:
> > +	kfree(entry);
> > +
> > +err_entry:
> > +	kfree(buf);
> > +
> > +err_buf:
> > +	pr_err("PCI: Out of memory while parsing command line: %s\n", val);
> > +	return -ENOMEM;
> > +}
> > +
> > +static void pci_device_setup_driver_override(struct pci_dev *dev)
> > +{
> > +	static int parse_done;
> > +	struct driver_override_entry *entry;
> > +
> > +	if (!parse_done) {
> > +		char *cmdline = kstrdup(saved_command_line, GFP_KERNEL);
> > +
> > +		if (!cmdline)
> > +			goto err_out_of_mem;
> > +
> > +		parse_args("pci", cmdline, NULL,
> > +			   0, 0, 0, &pci_device_parse_driver_override);
> > +		kfree(cmdline);
> > +		parse_done = 1;
> > +	}
> > +
> > +	list_for_each_entry(entry, &driver_override_entries, list) {
> > +		if (pci_domain_nr(dev->bus) != entry->domain ||
> > +		    dev->bus->number != entry->bus ||
> > +		    dev->devfn != entry->devfn)
> > +			continue;
> > +
> > +		dev->driver_override = kstrdup(entry->driver_name, GFP_KERNEL);
> > +		if (!dev->driver_override)
> > +			goto err_out_of_mem;
> > +
> > +		break;
> > +	}
> > +
> > +	return;
> > +
> > +err_out_of_mem:
> > +	pr_err("PCI: Out of memory while setting up driver override\n");
> > +}
> > +
> >  /**
> >   * pci_bus_add_device - start driver for a single device
> >   * @dev: device to add
> > @@ -245,6 +355,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 625a4ac..37809d4 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -4508,6 +4508,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)) {
> > +				/* lazy evaluation by the pci subsystem */
> >  			} else {
> >  				printk(KERN_ERR "PCI: Unknown option `%s'\n",
> >  						str);
> 
> 
> 

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

[Marcel Apfelbaum]

On Wed, 2014-10-22 at 15:28 -0600, Bjorn Helgaas wrote:
> Hi Marcel,
Hi Bjorn,
Thank you for the review!

> 
> I'm not quite clear on what the objective is here, so I apologize for
> some questions that probably seem silly.
I appreciate you took your time to go over it.

> 
> On Mon, Oct 20, 2014 at 8:04 AM, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> > Scanning a lot of devices during boot requires a lot of time.
> 
> I think what takes a lot of time is the .probe() method for some
> drivers, right?  I first thought you meant that it took a long time
> for the PCI core to enumerate a lot of devices, but you're not
> changing anything there.
Yes indeed.

> 
> If the intent is to reduce boot time, I don't think this is a general
> solution.  Drivers should be able to schedule asynchronous things in
> their .probe() methods if necessary.
I agree, but sadly we cannot go over *all* existing drivers and fix,
we can of course do the best effort :)
By the way this was not the only reason as you also thought, see bellow

> 
> > On other scenarios there is a need to bind a driver to a specific slot.
> 
> A short example here would be good.  Are you talking about something
> like binding a NIC driver to one device while leaving others unbound
> for use by guests?
Exactly! This is the "perfect" example, thanks!
> 
> > Binding devices to pci-stub driver does not work,
> > as it will not differentiate between devices of the
> > same type.
> 
> I assume you mean booting with "pci-stub.ids=$VENDOR:$DEVICE" will
> make pci-stub bind to *all* matching devices, and you only want it to
> bind to some.
You are right again.

>   Maybe pci-stub could be extended to pay attention to
> PCI bus addresses in addition to vendor/device IDs.
A few thoughts here:
- We will have a race here between the "native" driver and pci-stub, right?
- Why not leverage the existing driver_override feature that is already
there and gives us exactly what we want: slot<->driver mapping?
- Maybe there are other scenarios that can benefit from slot<->driver mapping,
not only pci-stub.

 


> 
> > Using some start scripts is error prone.
> >
> > The solution leverages driver_override functionality introduced by
> >
> >         commit: 782a985d7af26db39e86070d28f987cad21313c0
> >         Author: Alex Williamson <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 slots to specific drivers use:
> >         pci=driver[xxxx:xx:xx.x]=foo,driver[xxxx:xx:xx.x]=bar,...
> 
> If/when you address Alex's comments about other bus types, can you
> also update the changelog to use the canonical commit reference
> format, i.e., 782a985d7af2 ("PCI: Introduce new device binding path
> using pci_dev.driver_override") in this case?
Sure, thanks for the tip.

> 
> PCI bus numbers are mutable, e.g., they can change with hotplug or
> other configuration changes.  But I don't have any better suggestion,
> so I guess all we can do is be aware of this.
Well, indeed, there is so much that can be done. (We can listen to an event and remap...)

> 
> Speaking of hotplug, this is only a boot-time kernel parameter, with
> no opportunity to use this, e.g., to add slot/driver pairs, after
> boot.  Do you not need that because of Alex's driver_override thing?
Well actually Alex's "driver_override" feature does that for runtime
(adds slot/driver pair), the only thing missing is the boot time
mapping.

> How can we integrate this all together into a coherent whole?  I'm a
> little confused as to how this would all be documented in a form
> usable by end-users.
For end-users it will be like this:
They want to create a slot/driver mapping.
In order to do that they will use the "driver_override" feature:
1. Run-time use:
   - Use sysfs to edit driver_override file associated with the slot.
2. Boot-time use:
   - Use the pci's driver_override parameter. 

Thanks,
Marcel
> 
> Bjorn

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

[Michael S. Tsirkin]

On Wed, Oct 22, 2014 at 03:28:29PM -0600, Bjorn Helgaas wrote:
> Hi Marcel,
> 
> I'm not quite clear on what the objective is here, so I apologize for
> some questions that probably seem silly.
> 
> On Mon, Oct 20, 2014 at 8:04 AM, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> > Scanning a lot of devices during boot requires a lot of time.
> 
> I think what takes a lot of time is the .probe() method for some
> drivers, right?  I first thought you meant that it took a long time
> for the PCI core to enumerate a lot of devices, but you're not
> changing anything there.
> 
> If the intent is to reduce boot time, I don't think this is a general
> solution.  Drivers should be able to schedule asynchronous things in
> their .probe() methods if necessary.

If this worked for all devices, we could just make probe
asynchronous in PCI core.
Unfortunately this doesn't work esp for storage devices
since people expect disks to be available for mount immediately.

If the point of the patch is to speed up boot, we could
try to probe everything in parallel?
Probe is serialized now, right?


> > On other scenarios there is a need to bind a driver to a specific slot.
> 
> A short example here would be good.  Are you talking about something
> like binding a NIC driver to one device while leaving others unbound
> for use by guests?
> 
> > Binding devices to pci-stub driver does not work,
> > as it will not differentiate between devices of the
> > same type.
> 
> I assume you mean booting with "pci-stub.ids=$VENDOR:$DEVICE" will
> make pci-stub bind to *all* matching devices, and you only want it to
> bind to some.  Maybe pci-stub could be extended to pay attention to
> PCI bus addresses in addition to vendor/device IDs.
> 
> > Using some start scripts is error prone.
> >
> > The solution leverages driver_override functionality introduced by
> >
> >         commit: 782a985d7af26db39e86070d28f987cad21313c0
> >         Author: Alex Williamson <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 slots to specific drivers use:
> >         pci=driver[xxxx:xx:xx.x]=foo,driver[xxxx:xx:xx.x]=bar,...
> 
> If/when you address Alex's comments about other bus types, can you
> also update the changelog to use the canonical commit reference
> format, i.e., 782a985d7af2 ("PCI: Introduce new device binding path
> using pci_dev.driver_override") in this case?
> 
> PCI bus numbers are mutable, e.g., they can change with hotplug or
> other configuration changes.  But I don't have any better suggestion,
> so I guess all we can do is be aware of this.

We could use slot capability for addressing if that's available.

> Speaking of hotplug, this is only a boot-time kernel parameter, with
> no opportunity to use this, e.g., to add slot/driver pairs, after
> boot.  Do you not need that because of Alex's driver_override thing?
> How can we integrate this all together into a coherent whole?  I'm a
> little confused as to how this would all be documented in a form
> usable by end-users.
> 
> Bjorn

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

[Alex Williamson]

On Thu, 2014-10-23 at 15:32 +0300, Marcel Apfelbaum wrote:
> On Wed, 2014-10-22 at 12:32 -0600, Alex Williamson wrote:
> > [cc+ stuart]
> > 
> > On Mon, 2014-10-20 at 17:04 +0300, Marcel Apfelbaum wrote:
> > > 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 <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 slots to specific drivers use:
> > > 	pci=driver[xxxx:xx:xx.x]=foo,driver[xxxx:xx:xx.x]=bar,...
> > > 
> > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > > ---
> > > v3 -> v4:
> > >  - Addressed Alex Williamson's comments:
> > >    - Modified the type of driver_override_entry's fields
> > >    - Used PCI_DEVFN when appropriated
> > >    - Removed redundant checks
> > >    - Replaced BUG_ON with pr_err messages
> > >    - Simpler command line parsing
> > >  - Addressed Michael S. Tsirkin comments
> > >    - removed DRIVER_OVERRIDE_NAME_LENGTH limitation
> > > v2 -> v3:
> > >  - Corrected subject line
> > > v1 -> v2:
> > >  - Addressed Michael S. Tsirkin comments
> > >    - Removed 32 slots limitation
> > >    - Better handling of memory allocation failures
> > >      (preferred BUG_ON over error messages)
> > >  - Addressed Alex Williamson's comments:
> > >    - Modified commit message to show parameter usage more clear.
> > >  - I preferred to re-use parse_args instead of manually using
> > >    strstr in order to better comply with command line parsing
> > >    rules.
> > >  - I didn't use any locking when parsing the command line args
> > >    (see parse_done usage) assuming that first call will be
> > >    early in system boot and no race can occur. Please correct
> > >    me if I am wrong.
> > > 
> > > Notes:
> > >  - 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                   | 111 ++++++++++++++++++++++++++++++++++++
> > >  drivers/pci/pci.c                   |   2 +
> > >  3 files changed, 117 insertions(+)
> > 
> > The driver_override feature that we're making use of here is also going
> > to be supported by platform devices and potentially more bustypes in the
> > future, so I'm concerned that making a pci specific kernel parameter is
> > too shortsighted.  Instead we could hook on to BUS_NOTIFY_ADD_DEVICE for
> > bustypes that support driver_override so we can have a common interface.
> The real question here if those bus types/devices would benefit from this
> feature, and I also must confess that I have no knowledge of the other buses.
> Can anyone confirm that it does make sense for them?

Platform devices are adding vfio support, so I expect the next logical
question will be how to reserve devices for use by vfio at boot.

> > Perhaps:
> > 
> > driver_override=pci,0000:02:00.0=pci-stub;platform,fakename=vfio-platform
> > 
> > Finding delimiters that don't conflict may be challenging.  Also, can we
> > assume that bus-name:dev-name is unique for every bustype?  It is for
> > pci, platform?
> For PCI, sure the domain:bus:dev.func is unique, for platform I have no idea,
> can anyone that knows "platform" confirm or deny?
> 
> > 
> > It also seems like there's a question of how long should this override
> > last and how does the user disable it?  
> > I think with pci-stub.ids=
> > $VENDOR:$DEVICE a user can echo the IDs to the pci-stub/remove_id sysfs
> > entry to cancel the effect.
> The way I see it is simple, the override specified in kernel command line
> last as long as the user does not specifically remove it using
> echo "" > /sys/.../driver_override
> and then unbind and bind the device again.
> 
> >   The only option here seems to be a reboot.
> Please see above

That's only a temporary removal though, if the device is removed and
re-added, either via physical hotplug or sysfs, the override is
re-applied.  Thanks,

Alex

> > Do we need a /sys/bus/pci/driver_overrides/{add_name,remove_name} for
> > this interface?  Thanks,
> While it does not hurt, I see it as optional since a simple removal of
> driver_override and rebind does the same
> 
> Thanks,
> Marcel
> 
> > 
> > Alex
> > 
> > > 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..b49f5cc 100644
> > > --- a/drivers/pci/bus.c
> > > +++ b/drivers/pci/bus.c
> > > @@ -15,6 +15,8 @@
> > >  #include <linux/proc_fs.h>
> > >  #include <linux/slab.h>
> > >  
> > > +#include <asm/setup.h>
> > > +
> > >  #include "pci.h"
> > >  
> > >  void pci_add_resource_offset(struct list_head *resources, struct resource *res,
> > > @@ -230,6 +232,114 @@ EXPORT_SYMBOL(pci_bus_alloc_resource);
> > >  
> > >  void __weak pcibios_resource_survey_bus(struct pci_bus *bus) { }
> > >  
> > > +struct driver_override_entry {
> > > +	u16 domain;
> > > +	u8 bus;
> > > +	u8 devfn;
> > > +	char *driver_name;
> > > +	struct list_head list;
> > > +};
> > > +
> > > +static LIST_HEAD(driver_override_entries);
> > > +
> > > +static int pci_device_parse_driver_override(char *param, char *val,
> > > +					    const char *unused)
> > > +{
> > > +	unsigned int domain, bus, dev, fn;
> > > +	char  *buf;
> > > +	struct driver_override_entry *entry;
> > > +	int ret;
> > > +
> > > +	buf = kmalloc(COMMAND_LINE_SIZE, GFP_KERNEL);
> > > +	if (!buf)
> > > +		goto err_buf;
> > > +
> > > +	while (val) {
> > > +		char *k = strchr(val, ',');
> > > +
> > > +		if (k)
> > > +			*k++ = 0;
> > > +
> > > +		if (strncmp(val, "driver", 6)) {
> > > +			val = k;
> > > +			continue;
> > > +		}
> > > +
> > > +		memset(buf, 0, COMMAND_LINE_SIZE);
> > > +		ret = sscanf(val + 6, "[%4x:%2x:%2x.%2x]=%s",
> > > +			     &domain, &bus, &dev, &fn, buf);
> > > +		if (ret != 5) {
> > > +			pr_warn("PCI: Invalid command line: %s\n", val);
> > > +			val = k;
> > > +			continue;
> > > +		}
> > > +
> > > +		entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> > > +		if (!entry)
> > > +			goto err_entry;
> > > +
> > > +		INIT_LIST_HEAD(&entry->list);
> > > +		entry->domain = domain;
> > > +		entry->bus = bus;
> > > +		entry->devfn = PCI_DEVFN(dev, fn);
> > > +		entry->driver_name = kstrdup(buf, GFP_KERNEL);
> > > +		if (!entry->driver_name)
> > > +			goto err_driver_name;
> > > +
> > > +		list_add_tail(&entry->list, &driver_override_entries);
> > > +		val = k;
> > > +	}
> > > +
> > > +	kfree(buf);
> > > +	return 0;
> > > +
> > > +err_driver_name:
> > > +	kfree(entry);
> > > +
> > > +err_entry:
> > > +	kfree(buf);
> > > +
> > > +err_buf:
> > > +	pr_err("PCI: Out of memory while parsing command line: %s\n", val);
> > > +	return -ENOMEM;
> > > +}
> > > +
> > > +static void pci_device_setup_driver_override(struct pci_dev *dev)
> > > +{
> > > +	static int parse_done;
> > > +	struct driver_override_entry *entry;
> > > +
> > > +	if (!parse_done) {
> > > +		char *cmdline = kstrdup(saved_command_line, GFP_KERNEL);
> > > +
> > > +		if (!cmdline)
> > > +			goto err_out_of_mem;
> > > +
> > > +		parse_args("pci", cmdline, NULL,
> > > +			   0, 0, 0, &pci_device_parse_driver_override);
> > > +		kfree(cmdline);
> > > +		parse_done = 1;
> > > +	}
> > > +
> > > +	list_for_each_entry(entry, &driver_override_entries, list) {
> > > +		if (pci_domain_nr(dev->bus) != entry->domain ||
> > > +		    dev->bus->number != entry->bus ||
> > > +		    dev->devfn != entry->devfn)
> > > +			continue;
> > > +
> > > +		dev->driver_override = kstrdup(entry->driver_name, GFP_KERNEL);
> > > +		if (!dev->driver_override)
> > > +			goto err_out_of_mem;
> > > +
> > > +		break;
> > > +	}
> > > +
> > > +	return;
> > > +
> > > +err_out_of_mem:
> > > +	pr_err("PCI: Out of memory while setting up driver override\n");
> > > +}
> > > +
> > >  /**
> > >   * pci_bus_add_device - start driver for a single device
> > >   * @dev: device to add
> > > @@ -245,6 +355,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 625a4ac..37809d4 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -4508,6 +4508,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)) {
> > > +				/* lazy evaluation by the pci subsystem */
> > >  			} else {
> > >  				printk(KERN_ERR "PCI: Unknown option `%s'\n",
> > >  						str);
> > 
> > 
> > 
> 
> 
> 

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

[Marcel Apfelbaum]

On Thu, 2014-10-23 at 07:11 -0600, Alex Williamson wrote:
> On Thu, 2014-10-23 at 15:32 +0300, Marcel Apfelbaum wrote:
> > On Wed, 2014-10-22 at 12:32 -0600, Alex Williamson wrote:
> > > [cc+ stuart]
> > > 
> > > On Mon, 2014-10-20 at 17:04 +0300, Marcel Apfelbaum wrote:
> > > > 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 <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 slots to specific drivers use:
> > > > 	pci=driver[xxxx:xx:xx.x]=foo,driver[xxxx:xx:xx.x]=bar,...
> > > > 
> > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > > > ---
> > > > v3 -> v4:
> > > >  - Addressed Alex Williamson's comments:
> > > >    - Modified the type of driver_override_entry's fields
> > > >    - Used PCI_DEVFN when appropriated
> > > >    - Removed redundant checks
> > > >    - Replaced BUG_ON with pr_err messages
> > > >    - Simpler command line parsing
> > > >  - Addressed Michael S. Tsirkin comments
> > > >    - removed DRIVER_OVERRIDE_NAME_LENGTH limitation
> > > > v2 -> v3:
> > > >  - Corrected subject line
> > > > v1 -> v2:
> > > >  - Addressed Michael S. Tsirkin comments
> > > >    - Removed 32 slots limitation
> > > >    - Better handling of memory allocation failures
> > > >      (preferred BUG_ON over error messages)
> > > >  - Addressed Alex Williamson's comments:
> > > >    - Modified commit message to show parameter usage more clear.
> > > >  - I preferred to re-use parse_args instead of manually using
> > > >    strstr in order to better comply with command line parsing
> > > >    rules.
> > > >  - I didn't use any locking when parsing the command line args
> > > >    (see parse_done usage) assuming that first call will be
> > > >    early in system boot and no race can occur. Please correct
> > > >    me if I am wrong.
> > > > 
> > > > Notes:
> > > >  - 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                   | 111 ++++++++++++++++++++++++++++++++++++
> > > >  drivers/pci/pci.c                   |   2 +
> > > >  3 files changed, 117 insertions(+)
> > > 
> > > The driver_override feature that we're making use of here is also going
> > > to be supported by platform devices and potentially more bustypes in the
> > > future, so I'm concerned that making a pci specific kernel parameter is
> > > too shortsighted.  Instead we could hook on to BUS_NOTIFY_ADD_DEVICE for
> > > bustypes that support driver_override so we can have a common interface.
> > The real question here if those bus types/devices would benefit from this
> > feature, and I also must confess that I have no knowledge of the other buses.
> > Can anyone confirm that it does make sense for them?
> 
> Platform devices are adding vfio support, so I expect the next logical
> question will be how to reserve devices for use by vfio at boot.
Well, I'll have to learn more about vfio before saying anything,
but my question is if it can be deferred or it has to be part of
this patch. If the platform devices do not have a slot like hw address, 
maybe it can be configured separately.

I saw this patch as a PCI patch, and not "driver_override" expansion;
meaning that I only leveraged an existing feature, not trying to
extend it.
If I am wrong, please point me to a more robust solution.


> 
> > > Perhaps:
> > > 
> > > driver_override=pci,0000:02:00.0=pci-stub;platform,fakename=vfio-platform
> > > 
> > > Finding delimiters that don't conflict may be challenging.  Also, can we
> > > assume that bus-name:dev-name is unique for every bustype?  It is for
> > > pci, platform?
> > For PCI, sure the domain:bus:dev.func is unique, for platform I have no idea,
> > can anyone that knows "platform" confirm or deny?
> > 
> > > 
> > > It also seems like there's a question of how long should this override
> > > last and how does the user disable it?  
> > > I think with pci-stub.ids=
> > > $VENDOR:$DEVICE a user can echo the IDs to the pci-stub/remove_id sysfs
> > > entry to cancel the effect.
> > The way I see it is simple, the override specified in kernel command line
> > last as long as the user does not specifically remove it using
> > echo "" > /sys/.../driver_override
> > and then unbind and bind the device again.
> > 
> > >   The only option here seems to be a reboot.
> > Please see above
> 
> That's only a temporary removal though, if the device is removed and
> re-added, either via physical hotplug or sysfs, the override is
> re-applied.  Thanks,
Bear with me please,

If we empty the driver_override file (cat "" /sys/bus/.../slot/driver_override)
as suggested above, the override will not be reapplied.

So, it is a consistent model:
1. The end-user specified the driver in command-line, so he wants it there
   even after unbinding/binding or hotplug operations.
2. If the end-user "changes his mind", he doesn't need to reboot the system,
   only to clear the driver_override sysfs file and from this moment on
   the system behaves like usual. (at next unbind/bind or hotplug)

I hope I got it right,
Thanks,
Marcel


> 
> Alex
> 
> > > Do we need a /sys/bus/pci/driver_overrides/{add_name,remove_name} for
> > > this interface?  Thanks,
> > While it does not hurt, I see it as optional since a simple removal of
> > driver_override and rebind does the same
> > 
> > Thanks,
> > Marcel
> > 
> > > 
> > > Alex
> > > 
> > > > 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..b49f5cc 100644
> > > > --- a/drivers/pci/bus.c
> > > > +++ b/drivers/pci/bus.c
> > > > @@ -15,6 +15,8 @@
> > > >  #include <linux/proc_fs.h>
> > > >  #include <linux/slab.h>
> > > >  
> > > > +#include <asm/setup.h>
> > > > +
> > > >  #include "pci.h"
> > > >  
> > > >  void pci_add_resource_offset(struct list_head *resources, struct resource *res,
> > > > @@ -230,6 +232,114 @@ EXPORT_SYMBOL(pci_bus_alloc_resource);
> > > >  
> > > >  void __weak pcibios_resource_survey_bus(struct pci_bus *bus) { }
> > > >  
> > > > +struct driver_override_entry {
> > > > +	u16 domain;
> > > > +	u8 bus;
> > > > +	u8 devfn;
> > > > +	char *driver_name;
> > > > +	struct list_head list;
> > > > +};
> > > > +
> > > > +static LIST_HEAD(driver_override_entries);
> > > > +
> > > > +static int pci_device_parse_driver_override(char *param, char *val,
> > > > +					    const char *unused)
> > > > +{
> > > > +	unsigned int domain, bus, dev, fn;
> > > > +	char  *buf;
> > > > +	struct driver_override_entry *entry;
> > > > +	int ret;
> > > > +
> > > > +	buf = kmalloc(COMMAND_LINE_SIZE, GFP_KERNEL);
> > > > +	if (!buf)
> > > > +		goto err_buf;
> > > > +
> > > > +	while (val) {
> > > > +		char *k = strchr(val, ',');
> > > > +
> > > > +		if (k)
> > > > +			*k++ = 0;
> > > > +
> > > > +		if (strncmp(val, "driver", 6)) {
> > > > +			val = k;
> > > > +			continue;
> > > > +		}
> > > > +
> > > > +		memset(buf, 0, COMMAND_LINE_SIZE);
> > > > +		ret = sscanf(val + 6, "[%4x:%2x:%2x.%2x]=%s",
> > > > +			     &domain, &bus, &dev, &fn, buf);
> > > > +		if (ret != 5) {
> > > > +			pr_warn("PCI: Invalid command line: %s\n", val);
> > > > +			val = k;
> > > > +			continue;
> > > > +		}
> > > > +
> > > > +		entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> > > > +		if (!entry)
> > > > +			goto err_entry;
> > > > +
> > > > +		INIT_LIST_HEAD(&entry->list);
> > > > +		entry->domain = domain;
> > > > +		entry->bus = bus;
> > > > +		entry->devfn = PCI_DEVFN(dev, fn);
> > > > +		entry->driver_name = kstrdup(buf, GFP_KERNEL);
> > > > +		if (!entry->driver_name)
> > > > +			goto err_driver_name;
> > > > +
> > > > +		list_add_tail(&entry->list, &driver_override_entries);
> > > > +		val = k;
> > > > +	}
> > > > +
> > > > +	kfree(buf);
> > > > +	return 0;
> > > > +
> > > > +err_driver_name:
> > > > +	kfree(entry);
> > > > +
> > > > +err_entry:
> > > > +	kfree(buf);
> > > > +
> > > > +err_buf:
> > > > +	pr_err("PCI: Out of memory while parsing command line: %s\n", val);
> > > > +	return -ENOMEM;
> > > > +}
> > > > +
> > > > +static void pci_device_setup_driver_override(struct pci_dev *dev)
> > > > +{
> > > > +	static int parse_done;
> > > > +	struct driver_override_entry *entry;
> > > > +
> > > > +	if (!parse_done) {
> > > > +		char *cmdline = kstrdup(saved_command_line, GFP_KERNEL);
> > > > +
> > > > +		if (!cmdline)
> > > > +			goto err_out_of_mem;
> > > > +
> > > > +		parse_args("pci", cmdline, NULL,
> > > > +			   0, 0, 0, &pci_device_parse_driver_override);
> > > > +		kfree(cmdline);
> > > > +		parse_done = 1;
> > > > +	}
> > > > +
> > > > +	list_for_each_entry(entry, &driver_override_entries, list) {
> > > > +		if (pci_domain_nr(dev->bus) != entry->domain ||
> > > > +		    dev->bus->number != entry->bus ||
> > > > +		    dev->devfn != entry->devfn)
> > > > +			continue;
> > > > +
> > > > +		dev->driver_override = kstrdup(entry->driver_name, GFP_KERNEL);
> > > > +		if (!dev->driver_override)
> > > > +			goto err_out_of_mem;
> > > > +
> > > > +		break;
> > > > +	}
> > > > +
> > > > +	return;
> > > > +
> > > > +err_out_of_mem:
> > > > +	pr_err("PCI: Out of memory while setting up driver override\n");
> > > > +}
> > > > +
> > > >  /**
> > > >   * pci_bus_add_device - start driver for a single device
> > > >   * @dev: device to add
> > > > @@ -245,6 +355,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 625a4ac..37809d4 100644
> > > > --- a/drivers/pci/pci.c
> > > > +++ b/drivers/pci/pci.c
> > > > @@ -4508,6 +4508,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)) {
> > > > +				/* lazy evaluation by the pci subsystem */
> > > >  			} else {
> > > >  				printk(KERN_ERR "PCI: Unknown option `%s'\n",
> > > >  						str);
> > > 
> > > 
> > > 
> > 
> > 
> > 
> 
> 
> 
> 

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

[Stuart Yoder]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogQWxleCBXaWxsaWFtc29u
IFttYWlsdG86YWxleC53aWxsaWFtc29uQHJlZGhhdC5jb21dDQo+IFNlbnQ6IFdlZG5lc2RheSwg
T2N0b2JlciAyMiwgMjAxNCAxOjMzIFBNDQo+IFRvOiBNYXJjZWwgQXBmZWxiYXVtDQo+IENjOiBs
aW51eC1wY2lAdmdlci5rZXJuZWwub3JnOyBiaGVsZ2Fhc0Bnb29nbGUuY29tOyBsaW51eC1rZXJu
ZWxAdmdlci5rZXJuZWwub3JnOyBtYXJjZWxAcmVkaGF0LmNvbTsNCj4gbXN0QHJlZGhhdC5jb207
IFlvZGVyIFN0dWFydC1CMDgyNDgNCj4gU3ViamVjdDogUmU6IFtQQVRDSCB2NF0gUENJOiBhZGQg
a2VybmVsIHBhcmFtZXRlciB0byBvdmVycmlkZSBkZXZpZDwtPmRyaXZlciBtYXBwaW5nLg0KPiAN
Cj4gW2NjKyBzdHVhcnRdDQo+IA0KPiBPbiBNb24sIDIwMTQtMTAtMjAgYXQgMTc6MDQgKzAzMDAs
IE1hcmNlbCBBcGZlbGJhdW0gd3JvdGU6DQo+ID4gU2Nhbm5pbmcgYSBsb3Qgb2YgZGV2aWNlcyBk
dXJpbmcgYm9vdCByZXF1aXJlcyBhIGxvdCBvZiB0aW1lLg0KPiA+IE9uIG90aGVyIHNjZW5hcmlv
cyB0aGVyZSBpcyBhIG5lZWQgdG8gYmluZCBhIGRyaXZlciB0byBhIHNwZWNpZmljIHNsb3QuDQo+
ID4NCj4gPiBCaW5kaW5nIGRldmljZXMgdG8gcGNpLXN0dWIgZHJpdmVyIGRvZXMgbm90IHdvcmss
DQo+ID4gYXMgaXQgd2lsbCBub3QgZGlmZmVyZW50aWF0ZSBiZXR3ZWVuIGRldmljZXMgb2YgdGhl
DQo+ID4gc2FtZSB0eXBlLiBVc2luZyBzb21lIHN0YXJ0IHNjcmlwdHMgaXMgZXJyb3IgcHJvbmUu
DQo+ID4NCj4gPiBUaGUgc29sdXRpb24gbGV2ZXJhZ2VzIGRyaXZlcl9vdmVycmlkZSBmdW5jdGlv
bmFsaXR5IGludHJvZHVjZWQgYnkNCj4gPg0KPiA+IAljb21taXQ6IDc4MmE5ODVkN2FmMjZkYjM5
ZTg2MDcwZDI4Zjk4N2NhZDIxMzEzYzANCj4gPiAJQXV0aG9yOiBBbGV4IFdpbGxpYW1zb24gPGFs
ZXgud2lsbGlhbXNvbkByZWRoYXQuY29tPg0KPiA+IAlEYXRlOiAgIFR1ZSBNYXkgMjAgMDg6NTM6
MjEgMjAxNCAtMDYwMA0KPiA+DQo+ID4gICAgIAlQQ0k6IEludHJvZHVjZSBuZXcgZGV2aWNlIGJp
bmRpbmcgcGF0aCB1c2luZyBwY2lfZGV2LmRyaXZlcl9vdmVycmlkZQ0KPiA+DQo+ID4gSW4gb3Jk
ZXIgdG8gYmluZCBQQ0kgc2xvdHMgdG8gc3BlY2lmaWMgZHJpdmVycyB1c2U6DQo+ID4gCXBjaT1k
cml2ZXJbeHh4eDp4eDp4eC54XT1mb28sZHJpdmVyW3h4eHg6eHg6eHgueF09YmFyLC4uLg0KPiA+
DQo+ID4gU2lnbmVkLW9mZi1ieTogTWFyY2VsIEFwZmVsYmF1bSA8bWFyY2VsLmFAcmVkaGF0LmNv
bT4NCj4gPiAtLS0NCj4gPiB2MyAtPiB2NDoNCj4gPiAgLSBBZGRyZXNzZWQgQWxleCBXaWxsaWFt
c29uJ3MgY29tbWVudHM6DQo+ID4gICAgLSBNb2RpZmllZCB0aGUgdHlwZSBvZiBkcml2ZXJfb3Zl
cnJpZGVfZW50cnkncyBmaWVsZHMNCj4gPiAgICAtIFVzZWQgUENJX0RFVkZOIHdoZW4gYXBwcm9w
cmlhdGVkDQo+ID4gICAgLSBSZW1vdmVkIHJlZHVuZGFudCBjaGVja3MNCj4gPiAgICAtIFJlcGxh
Y2VkIEJVR19PTiB3aXRoIHByX2VyciBtZXNzYWdlcw0KPiA+ICAgIC0gU2ltcGxlciBjb21tYW5k
IGxpbmUgcGFyc2luZw0KPiA+ICAtIEFkZHJlc3NlZCBNaWNoYWVsIFMuIFRzaXJraW4gY29tbWVu
dHMNCj4gPiAgICAtIHJlbW92ZWQgRFJJVkVSX09WRVJSSURFX05BTUVfTEVOR1RIIGxpbWl0YXRp
b24NCj4gPiB2MiAtPiB2MzoNCj4gPiAgLSBDb3JyZWN0ZWQgc3ViamVjdCBsaW5lDQo+ID4gdjEg
LT4gdjI6DQo+ID4gIC0gQWRkcmVzc2VkIE1pY2hhZWwgUy4gVHNpcmtpbiBjb21tZW50cw0KPiA+
ICAgIC0gUmVtb3ZlZCAzMiBzbG90cyBsaW1pdGF0aW9uDQo+ID4gICAgLSBCZXR0ZXIgaGFuZGxp
bmcgb2YgbWVtb3J5IGFsbG9jYXRpb24gZmFpbHVyZXMNCj4gPiAgICAgIChwcmVmZXJyZWQgQlVH
X09OIG92ZXIgZXJyb3IgbWVzc2FnZXMpDQo+ID4gIC0gQWRkcmVzc2VkIEFsZXggV2lsbGlhbXNv
bidzIGNvbW1lbnRzOg0KPiA+ICAgIC0gTW9kaWZpZWQgY29tbWl0IG1lc3NhZ2UgdG8gc2hvdyBw
YXJhbWV0ZXIgdXNhZ2UgbW9yZSBjbGVhci4NCj4gPiAgLSBJIHByZWZlcnJlZCB0byByZS11c2Ug
cGFyc2VfYXJncyBpbnN0ZWFkIG9mIG1hbnVhbGx5IHVzaW5nDQo+ID4gICAgc3Ryc3RyIGluIG9y
ZGVyIHRvIGJldHRlciBjb21wbHkgd2l0aCBjb21tYW5kIGxpbmUgcGFyc2luZw0KPiA+ICAgIHJ1
bGVzLg0KPiA+ICAtIEkgZGlkbid0IHVzZSBhbnkgbG9ja2luZyB3aGVuIHBhcnNpbmcgdGhlIGNv
bW1hbmQgbGluZSBhcmdzDQo+ID4gICAgKHNlZSBwYXJzZV9kb25lIHVzYWdlKSBhc3N1bWluZyB0
aGF0IGZpcnN0IGNhbGwgd2lsbCBiZQ0KPiA+ICAgIGVhcmx5IGluIHN5c3RlbSBib290IGFuZCBu
byByYWNlIGNhbiBvY2N1ci4gUGxlYXNlIGNvcnJlY3QNCj4gPiAgICBtZSBpZiBJIGFtIHdyb25n
Lg0KPiA+DQo+ID4gTm90ZXM6DQo+ID4gIC0gSSBoYXZlIGZ1cnRoZXIgaWRlYXMgb24gdG9wIG9m
IHRoaXMgcGF0Y2ggYmFzZWQgb24geW91ciByZXZpZXdzLg0KPiA+ICAgIEkgdGhvdWdodCBvZjoN
Cj4gPiAgICAtIFVzZSB3aWxkY2FyZHMgdG8gc3BlY2lmeSBlbnRpcmUgYnVzZXMvZGV2aWNlcywg
c29tZXRoaW5nIGxpa2U6DQo+ID4gICAgICAJZHJpdmVyWzAwMDE6MDI6Ki4qXT1wY2ktc3R1Yg0K
PiA+ICAgIC0gVXNlIGNvbW1hIHRvIHNlcGFyYXRlIHNldmVyYWwgZGV2aWNlczoNCj4gPiAgICAg
IAlkcml2ZXJbMDAwMTowMjowMy40LDAwMDE6MDI6MDQuMCwuLi5dPXBjaS1zdHViDQo+ID4gICAg
LSBNYWtlIGRvbWFpbiBvcHRpb25hbDoNCj4gPiAgICAJZHJpdmVyWzAwOjAzLjBdPXBjaS1zdHVi
DQo+ID4NCj4gPiBDb21tZW50cyB3aWxsIGJlIGFwcHJlY2lhdGVkLA0KPiA+IFRoYW5rcywNCj4g
PiBNYXJjZWwNCj4gPiAgRG9jdW1lbnRhdGlvbi9rZXJuZWwtcGFyYW1ldGVycy50eHQgfCAgIDQg
KysNCj4gPiAgZHJpdmVycy9wY2kvYnVzLmMgICAgICAgICAgICAgICAgICAgfCAxMTEgKysrKysr
KysrKysrKysrKysrKysrKysrKysrKysrKysrKysrDQo+ID4gIGRyaXZlcnMvcGNpL3BjaS5jICAg
ICAgICAgICAgICAgICAgIHwgICAyICsNCj4gPiAgMyBmaWxlcyBjaGFuZ2VkLCAxMTcgaW5zZXJ0
aW9ucygrKQ0KPiANCj4gVGhlIGRyaXZlcl9vdmVycmlkZSBmZWF0dXJlIHRoYXQgd2UncmUgbWFr
aW5nIHVzZSBvZiBoZXJlIGlzIGFsc28gZ29pbmcNCj4gdG8gYmUgc3VwcG9ydGVkIGJ5IHBsYXRm
b3JtIGRldmljZXMgYW5kIHBvdGVudGlhbGx5IG1vcmUgYnVzdHlwZXMgaW4gdGhlDQo+IGZ1dHVy
ZSwgc28gSSdtIGNvbmNlcm5lZCB0aGF0IG1ha2luZyBhIHBjaSBzcGVjaWZpYyBrZXJuZWwgcGFy
YW1ldGVyIGlzDQo+IHRvbyBzaG9ydHNpZ2h0ZWQuICBJbnN0ZWFkIHdlIGNvdWxkIGhvb2sgb24g
dG8gQlVTX05PVElGWV9BRERfREVWSUNFIGZvcg0KPiBidXN0eXBlcyB0aGF0IHN1cHBvcnQgZHJp
dmVyX292ZXJyaWRlIHNvIHdlIGNhbiBoYXZlIGEgY29tbW9uIGludGVyZmFjZS4NCj4gUGVyaGFw
czoNCj4gDQo+IGRyaXZlcl9vdmVycmlkZT1wY2ksMDAwMDowMjowMC4wPXBjaS1zdHViO3BsYXRm
b3JtLGZha2VuYW1lPXZmaW8tcGxhdGZvcm0NCj4gDQo+IEZpbmRpbmcgZGVsaW1pdGVycyB0aGF0
IGRvbid0IGNvbmZsaWN0IG1heSBiZSBjaGFsbGVuZ2luZy4NCg0KSSB0aGluayB3aGF0IHlvdSBw
cm9wb3NlZCB3b3Jrcy0tICA8YnVzLW5hbWU+LDxidXMtZGV2Pj08ZHJpdmVyPjsNCg0KVGhpbmsg
dGhhdCB3aWxsIHdvcmsgZm9yIFBDSSwgcGxhdGZvcm0sIGFuZCB0aGUgbmV3IGZzbC1tYyBidXMg
d2UgYXJlIHdvcmtpbmcNCm9uLg0KDQo+IEFsc28sIGNhbiB3ZQ0KPiBhc3N1bWUgdGhhdCBidXMt
bmFtZTpkZXYtbmFtZSBpcyB1bmlxdWUgZm9yIGV2ZXJ5IGJ1c3R5cGU/ICBJdCBpcyBmb3INCj4g
cGNpLCBwbGF0Zm9ybT8NCg0KSSB0aGluayB0aGF0IGhhcyB0byBiZSB0aGUgY2FzZS4NCg0KPiBJ
dCBhbHNvIHNlZW1zIGxpa2UgdGhlcmUncyBhIHF1ZXN0aW9uIG9mIGhvdyBsb25nIHNob3VsZCB0
aGlzIG92ZXJyaWRlDQo+IGxhc3QgYW5kIGhvdyBkb2VzIHRoZSB1c2VyIGRpc2FibGUgaXQ/DQoN
Cklzbid0IHRoYXQgYSBnZW5lcmFsIHF1ZXN0aW9uIGZvciB0aGUgImRyaXZlcl9vdmVycnJpZGUi
IG1lY2hhbmlzbT8NCkknbSBmb3JnZXR0aW5nIGlmIHRoZSBtZWNoYW5pc20gaW4gdGhlIGtlcm5l
bCBub3cgaGFzIGEgd2F5IHRvIGRpc2FibGUNCml0LS0gIGUuZy4gZWNobyAvZGV2L251bGwgPiAv
c3lzL3BjaS9kZXZpY2VzLy4uLi9kcml2ZXJfb3ZlcnJpZGUgPz8NCg0KU28sIGl0IHdvdWxkIGxh
c3QgdW50aWwgZXhwbGljaXRseSBkaXNhYmxlZCB0aHJvdWdoIHN5c2ZzLg0KDQo+IEkgdGhpbmsg
d2l0aCBwY2ktc3R1Yi5pZHM9DQo+ICRWRU5ET1I6JERFVklDRSBhIHVzZXIgY2FuIGVjaG8gdGhl
IElEcyB0byB0aGUgcGNpLXN0dWIvcmVtb3ZlX2lkIHN5c2ZzDQo+IGVudHJ5IHRvIGNhbmNlbCB0
aGUgZWZmZWN0LiAgVGhlIG9ubHkgb3B0aW9uIGhlcmUgc2VlbXMgdG8gYmUgYSByZWJvb3QuDQo+
IERvIHdlIG5lZWQgYSAvc3lzL2J1cy9wY2kvZHJpdmVyX292ZXJyaWRlcy97YWRkX25hbWUscmVt
b3ZlX25hbWV9IGZvcg0KPiB0aGlzIGludGVyZmFjZT8gIFRoYW5rcywNCg0KVGhhbmtzLA0KU3R1
YXJ0DQo=

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

[Stuart Yoder]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogTWFyY2VsIEFwZmVsYmF1
bSBbbWFpbHRvOm1hcmNlbC5hQHJlZGhhdC5jb21dDQo+IFNlbnQ6IFRodXJzZGF5LCBPY3RvYmVy
IDIzLCAyMDE0IDc6MzIgQU0NCj4gVG86IEFsZXggV2lsbGlhbXNvbg0KPiBDYzogbGludXgtcGNp
QHZnZXIua2VybmVsLm9yZzsgYmhlbGdhYXNAZ29vZ2xlLmNvbTsgbGludXgta2VybmVsQHZnZXIu
a2VybmVsLm9yZzsgbWFyY2VsQHJlZGhhdC5jb207DQo+IG1zdEByZWRoYXQuY29tOyBZb2RlciBT
dHVhcnQtQjA4MjQ4DQo+IFN1YmplY3Q6IFJlOiBbUEFUQ0ggdjRdIFBDSTogYWRkIGtlcm5lbCBw
YXJhbWV0ZXIgdG8gb3ZlcnJpZGUgZGV2aWQ8LT5kcml2ZXIgbWFwcGluZy4NCj4gDQo+IE9uIFdl
ZCwgMjAxNC0xMC0yMiBhdCAxMjozMiAtMDYwMCwgQWxleCBXaWxsaWFtc29uIHdyb3RlOg0KPiA+
IFtjYysgc3R1YXJ0XQ0KPiA+DQo+ID4gT24gTW9uLCAyMDE0LTEwLTIwIGF0IDE3OjA0ICswMzAw
LCBNYXJjZWwgQXBmZWxiYXVtIHdyb3RlOg0KPiA+ID4gU2Nhbm5pbmcgYSBsb3Qgb2YgZGV2aWNl
cyBkdXJpbmcgYm9vdCByZXF1aXJlcyBhIGxvdCBvZiB0aW1lLg0KPiA+ID4gT24gb3RoZXIgc2Nl
bmFyaW9zIHRoZXJlIGlzIGEgbmVlZCB0byBiaW5kIGEgZHJpdmVyIHRvIGEgc3BlY2lmaWMgc2xv
dC4NCj4gPiA+DQo+ID4gPiBCaW5kaW5nIGRldmljZXMgdG8gcGNpLXN0dWIgZHJpdmVyIGRvZXMg
bm90IHdvcmssDQo+ID4gPiBhcyBpdCB3aWxsIG5vdCBkaWZmZXJlbnRpYXRlIGJldHdlZW4gZGV2
aWNlcyBvZiB0aGUNCj4gPiA+IHNhbWUgdHlwZS4gVXNpbmcgc29tZSBzdGFydCBzY3JpcHRzIGlz
IGVycm9yIHByb25lLg0KPiA+ID4NCj4gPiA+IFRoZSBzb2x1dGlvbiBsZXZlcmFnZXMgZHJpdmVy
X292ZXJyaWRlIGZ1bmN0aW9uYWxpdHkgaW50cm9kdWNlZCBieQ0KPiA+ID4NCj4gPiA+IAljb21t
aXQ6IDc4MmE5ODVkN2FmMjZkYjM5ZTg2MDcwZDI4Zjk4N2NhZDIxMzEzYzANCj4gPiA+IAlBdXRo
b3I6IEFsZXggV2lsbGlhbXNvbiA8YWxleC53aWxsaWFtc29uQHJlZGhhdC5jb20+DQo+ID4gPiAJ
RGF0ZTogICBUdWUgTWF5IDIwIDA4OjUzOjIxIDIwMTQgLTA2MDANCj4gPiA+DQo+ID4gPiAgICAg
CVBDSTogSW50cm9kdWNlIG5ldyBkZXZpY2UgYmluZGluZyBwYXRoIHVzaW5nIHBjaV9kZXYuZHJp
dmVyX292ZXJyaWRlDQo+ID4gPg0KPiA+ID4gSW4gb3JkZXIgdG8gYmluZCBQQ0kgc2xvdHMgdG8g
c3BlY2lmaWMgZHJpdmVycyB1c2U6DQo+ID4gPiAJcGNpPWRyaXZlclt4eHh4Onh4Onh4LnhdPWZv
byxkcml2ZXJbeHh4eDp4eDp4eC54XT1iYXIsLi4uDQo+ID4gPg0KPiA+ID4gU2lnbmVkLW9mZi1i
eTogTWFyY2VsIEFwZmVsYmF1bSA8bWFyY2VsLmFAcmVkaGF0LmNvbT4NCj4gPiA+IC0tLQ0KPiA+
ID4gdjMgLT4gdjQ6DQo+ID4gPiAgLSBBZGRyZXNzZWQgQWxleCBXaWxsaWFtc29uJ3MgY29tbWVu
dHM6DQo+ID4gPiAgICAtIE1vZGlmaWVkIHRoZSB0eXBlIG9mIGRyaXZlcl9vdmVycmlkZV9lbnRy
eSdzIGZpZWxkcw0KPiA+ID4gICAgLSBVc2VkIFBDSV9ERVZGTiB3aGVuIGFwcHJvcHJpYXRlZA0K
PiA+ID4gICAgLSBSZW1vdmVkIHJlZHVuZGFudCBjaGVja3MNCj4gPiA+ICAgIC0gUmVwbGFjZWQg
QlVHX09OIHdpdGggcHJfZXJyIG1lc3NhZ2VzDQo+ID4gPiAgICAtIFNpbXBsZXIgY29tbWFuZCBs
aW5lIHBhcnNpbmcNCj4gPiA+ICAtIEFkZHJlc3NlZCBNaWNoYWVsIFMuIFRzaXJraW4gY29tbWVu
dHMNCj4gPiA+ICAgIC0gcmVtb3ZlZCBEUklWRVJfT1ZFUlJJREVfTkFNRV9MRU5HVEggbGltaXRh
dGlvbg0KPiA+ID4gdjIgLT4gdjM6DQo+ID4gPiAgLSBDb3JyZWN0ZWQgc3ViamVjdCBsaW5lDQo+
ID4gPiB2MSAtPiB2MjoNCj4gPiA+ICAtIEFkZHJlc3NlZCBNaWNoYWVsIFMuIFRzaXJraW4gY29t
bWVudHMNCj4gPiA+ICAgIC0gUmVtb3ZlZCAzMiBzbG90cyBsaW1pdGF0aW9uDQo+ID4gPiAgICAt
IEJldHRlciBoYW5kbGluZyBvZiBtZW1vcnkgYWxsb2NhdGlvbiBmYWlsdXJlcw0KPiA+ID4gICAg
ICAocHJlZmVycmVkIEJVR19PTiBvdmVyIGVycm9yIG1lc3NhZ2VzKQ0KPiA+ID4gIC0gQWRkcmVz
c2VkIEFsZXggV2lsbGlhbXNvbidzIGNvbW1lbnRzOg0KPiA+ID4gICAgLSBNb2RpZmllZCBjb21t
aXQgbWVzc2FnZSB0byBzaG93IHBhcmFtZXRlciB1c2FnZSBtb3JlIGNsZWFyLg0KPiA+ID4gIC0g
SSBwcmVmZXJyZWQgdG8gcmUtdXNlIHBhcnNlX2FyZ3MgaW5zdGVhZCBvZiBtYW51YWxseSB1c2lu
Zw0KPiA+ID4gICAgc3Ryc3RyIGluIG9yZGVyIHRvIGJldHRlciBjb21wbHkgd2l0aCBjb21tYW5k
IGxpbmUgcGFyc2luZw0KPiA+ID4gICAgcnVsZXMuDQo+ID4gPiAgLSBJIGRpZG4ndCB1c2UgYW55
IGxvY2tpbmcgd2hlbiBwYXJzaW5nIHRoZSBjb21tYW5kIGxpbmUgYXJncw0KPiA+ID4gICAgKHNl
ZSBwYXJzZV9kb25lIHVzYWdlKSBhc3N1bWluZyB0aGF0IGZpcnN0IGNhbGwgd2lsbCBiZQ0KPiA+
ID4gICAgZWFybHkgaW4gc3lzdGVtIGJvb3QgYW5kIG5vIHJhY2UgY2FuIG9jY3VyLiBQbGVhc2Ug
Y29ycmVjdA0KPiA+ID4gICAgbWUgaWYgSSBhbSB3cm9uZy4NCj4gPiA+DQo+ID4gPiBOb3RlczoN
Cj4gPiA+ICAtIEkgaGF2ZSBmdXJ0aGVyIGlkZWFzIG9uIHRvcCBvZiB0aGlzIHBhdGNoIGJhc2Vk
IG9uIHlvdXIgcmV2aWV3cy4NCj4gPiA+ICAgIEkgdGhvdWdodCBvZjoNCj4gPiA+ICAgIC0gVXNl
IHdpbGRjYXJkcyB0byBzcGVjaWZ5IGVudGlyZSBidXNlcy9kZXZpY2VzLCBzb21ldGhpbmcgbGlr
ZToNCj4gPiA+ICAgICAgCWRyaXZlclswMDAxOjAyOiouKl09cGNpLXN0dWINCj4gPiA+ICAgIC0g
VXNlIGNvbW1hIHRvIHNlcGFyYXRlIHNldmVyYWwgZGV2aWNlczoNCj4gPiA+ICAgICAgCWRyaXZl
clswMDAxOjAyOjAzLjQsMDAwMTowMjowNC4wLC4uLl09cGNpLXN0dWINCj4gPiA+ICAgIC0gTWFr
ZSBkb21haW4gb3B0aW9uYWw6DQo+ID4gPiAgICAJZHJpdmVyWzAwOjAzLjBdPXBjaS1zdHViDQo+
ID4gPg0KPiA+ID4gQ29tbWVudHMgd2lsbCBiZSBhcHByZWNpYXRlZCwNCj4gPiA+IFRoYW5rcywN
Cj4gPiA+IE1hcmNlbA0KPiA+ID4gIERvY3VtZW50YXRpb24va2VybmVsLXBhcmFtZXRlcnMudHh0
IHwgICA0ICsrDQo+ID4gPiAgZHJpdmVycy9wY2kvYnVzLmMgICAgICAgICAgICAgICAgICAgfCAx
MTEgKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrDQo+ID4gPiAgZHJpdmVycy9w
Y2kvcGNpLmMgICAgICAgICAgICAgICAgICAgfCAgIDIgKw0KPiA+ID4gIDMgZmlsZXMgY2hhbmdl
ZCwgMTE3IGluc2VydGlvbnMoKykNCj4gPg0KPiA+IFRoZSBkcml2ZXJfb3ZlcnJpZGUgZmVhdHVy
ZSB0aGF0IHdlJ3JlIG1ha2luZyB1c2Ugb2YgaGVyZSBpcyBhbHNvIGdvaW5nDQo+ID4gdG8gYmUg
c3VwcG9ydGVkIGJ5IHBsYXRmb3JtIGRldmljZXMgYW5kIHBvdGVudGlhbGx5IG1vcmUgYnVzdHlw
ZXMgaW4gdGhlDQo+ID4gZnV0dXJlLCBzbyBJJ20gY29uY2VybmVkIHRoYXQgbWFraW5nIGEgcGNp
IHNwZWNpZmljIGtlcm5lbCBwYXJhbWV0ZXIgaXMNCj4gPiB0b28gc2hvcnRzaWdodGVkLiAgSW5z
dGVhZCB3ZSBjb3VsZCBob29rIG9uIHRvIEJVU19OT1RJRllfQUREX0RFVklDRSBmb3INCj4gPiBi
dXN0eXBlcyB0aGF0IHN1cHBvcnQgZHJpdmVyX292ZXJyaWRlIHNvIHdlIGNhbiBoYXZlIGEgY29t
bW9uIGludGVyZmFjZS4NCj4gVGhlIHJlYWwgcXVlc3Rpb24gaGVyZSBpZiB0aG9zZSBidXMgdHlw
ZXMvZGV2aWNlcyB3b3VsZCBiZW5lZml0IGZyb20gdGhpcw0KPiBmZWF0dXJlLCBhbmQgSSBhbHNv
IG11c3QgY29uZmVzcyB0aGF0IEkgaGF2ZSBubyBrbm93bGVkZ2Ugb2YgdGhlIG90aGVyIGJ1c2Vz
Lg0KPiBDYW4gYW55b25lIGNvbmZpcm0gdGhhdCBpdCBkb2VzIG1ha2Ugc2Vuc2UgZm9yIHRoZW0/
DQoNCldlIGRvbid0IGhhdmUgdmZpby1wbGF0Zm9ybSBpbiB1c2UgeWV0LCBzbyBub3QgbXVjaCBh
Y3R1YWwgcmVhbCB3b3JsZCB1c2VyDQpleHBlcmllbmNlIHlldC4gIEJ1dCwgSSB0aGluayBpdCBt
YWtlcyBzZW5zZS4gICBFc3BlY2lhbGx5LCBnaXZlbiB0aGF0IHdlIGFyZQ0KaW52ZW50aW5nIGEg
a2VybmVsIHBhcmFtZXRlciBJIHRoaW5rIHdlIHNob3VsZCBkZXNpZ24gdGhlIHN5bnRheCBzbyB0
aGF0IGl0DQpjYW4gd29yayBidXNlcyBjYW4gaW1wbGVtZW50IHN1cHBvcnQgZm9yIHRoaXMuICBU
aGUgZHJpdmVyX292ZXJyaWRlIG1lY2hhbmlzbQ0KaXMgbm90IGJ1cyBzcGVjaWZpYywgc28gbGV0
J3Mgbm90IG1ha2UgdGhlIGtlcm5lbCBwYXJhbWV0ZXIgYnVzIHNwZWNpZmljLg0KDQo+ID4gUGVy
aGFwczoNCj4gPg0KPiA+IGRyaXZlcl9vdmVycmlkZT1wY2ksMDAwMDowMjowMC4wPXBjaS1zdHVi
O3BsYXRmb3JtLGZha2VuYW1lPXZmaW8tcGxhdGZvcm0NCj4gPg0KPiA+IEZpbmRpbmcgZGVsaW1p
dGVycyB0aGF0IGRvbid0IGNvbmZsaWN0IG1heSBiZSBjaGFsbGVuZ2luZy4gIEFsc28sIGNhbiB3
ZQ0KPiA+IGFzc3VtZSB0aGF0IGJ1cy1uYW1lOmRldi1uYW1lIGlzIHVuaXF1ZSBmb3IgZXZlcnkg
YnVzdHlwZT8gIEl0IGlzIGZvcg0KPiA+IHBjaSwgcGxhdGZvcm0/DQo+IEZvciBQQ0ksIHN1cmUg
dGhlIGRvbWFpbjpidXM6ZGV2LmZ1bmMgaXMgdW5pcXVlLCBmb3IgcGxhdGZvcm0gSSBoYXZlIG5v
IGlkZWEsDQo+IGNhbiBhbnlvbmUgdGhhdCBrbm93cyAicGxhdGZvcm0iIGNvbmZpcm0gb3IgZGVu
eT8NCg0KWWVzLCBkZXYtbmFtZSB3aWxsIGJlIHVuaXF1ZS4gIEFsbCBwbGF0Zm9ybSBkZXZpY2Vz
IGFyZSB1bmRlciBhIHNpbmdsZQ0KcGxhdGZvcm0gYnVzLg0KDQpTdHVhcnQNCg==

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

[Marcel Apfelbaum]

On Thu, 2014-10-23 at 13:51 +0000, Stuart Yoder wrote:
> 
> > -----Original Message-----
> > From: Marcel Apfelbaum [mailto:marcel.a@redhat.com]
> > Sent: Thursday, October 23, 2014 7:32 AM
> > To: Alex Williamson
> > Cc: linux-pci@vger.kernel.org; bhelgaas@google.com; linux-kernel@vger.kernel.org; marcel@redhat.com;
> > mst@redhat.com; Yoder Stuart-B08248
> > Subject: Re: [PATCH v4] PCI: add kernel parameter to override devid<->driver mapping.
> > 
> > On Wed, 2014-10-22 at 12:32 -0600, Alex Williamson wrote:
> > > [cc+ stuart]
> > >
> > > On Mon, 2014-10-20 at 17:04 +0300, Marcel Apfelbaum wrote:
> > > > 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 <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 slots to specific drivers use:
> > > > 	pci=driver[xxxx:xx:xx.x]=foo,driver[xxxx:xx:xx.x]=bar,...
> > > >
> > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > > > ---
> > > > v3 -> v4:
> > > >  - Addressed Alex Williamson's comments:
> > > >    - Modified the type of driver_override_entry's fields
> > > >    - Used PCI_DEVFN when appropriated
> > > >    - Removed redundant checks
> > > >    - Replaced BUG_ON with pr_err messages
> > > >    - Simpler command line parsing
> > > >  - Addressed Michael S. Tsirkin comments
> > > >    - removed DRIVER_OVERRIDE_NAME_LENGTH limitation
> > > > v2 -> v3:
> > > >  - Corrected subject line
> > > > v1 -> v2:
> > > >  - Addressed Michael S. Tsirkin comments
> > > >    - Removed 32 slots limitation
> > > >    - Better handling of memory allocation failures
> > > >      (preferred BUG_ON over error messages)
> > > >  - Addressed Alex Williamson's comments:
> > > >    - Modified commit message to show parameter usage more clear.
> > > >  - I preferred to re-use parse_args instead of manually using
> > > >    strstr in order to better comply with command line parsing
> > > >    rules.
> > > >  - I didn't use any locking when parsing the command line args
> > > >    (see parse_done usage) assuming that first call will be
> > > >    early in system boot and no race can occur. Please correct
> > > >    me if I am wrong.
> > > >
> > > > Notes:
> > > >  - 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                   | 111 ++++++++++++++++++++++++++++++++++++
> > > >  drivers/pci/pci.c                   |   2 +
> > > >  3 files changed, 117 insertions(+)
> > >
> > > The driver_override feature that we're making use of here is also going
> > > to be supported by platform devices and potentially more bustypes in the
> > > future, so I'm concerned that making a pci specific kernel parameter is
> > > too shortsighted.  Instead we could hook on to BUS_NOTIFY_ADD_DEVICE for
> > > bustypes that support driver_override so we can have a common interface.
> > The real question here if those bus types/devices would benefit from this
> > feature, and I also must confess that I have no knowledge of the other buses.
> > Can anyone confirm that it does make sense for them?
> 
> We don't have vfio-platform in use yet, so not much actual real world user
> experience yet.  But, I think it makes sense.   Especially, given that we are
> inventing a kernel parameter I think we should design the syntax so that it
> can work buses can implement support for this.  The driver_override mechanism
> is not bus specific, so let's not make the kernel parameter bus specific.
Make sense, point taken.
I'll come up with something.

Thank you Stuart,
Marcel
> 
> > > Perhaps:
> > >
> > > driver_override=pci,0000:02:00.0=pci-stub;platform,fakename=vfio-platform
> > >
> > > Finding delimiters that don't conflict may be challenging.  Also, can we
> > > assume that bus-name:dev-name is unique for every bustype?  It is for
> > > pci, platform?
> > For PCI, sure the domain:bus:dev.func is unique, for platform I have no idea,
> > can anyone that knows "platform" confirm or deny?
> 
> Yes, dev-name will be unique.  All platform devices are under a single
> platform bus.
> 
> Stuart

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

[Alex Williamson]

On Thu, 2014-10-23 at 13:44 +0000, Stuart Yoder wrote:
> 
> > -----Original Message-----
> > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > Sent: Wednesday, October 22, 2014 1:33 PM
> > To: Marcel Apfelbaum
> > Cc: linux-pci@vger.kernel.org; bhelgaas@google.com; linux-kernel@vger.kernel.org; marcel@redhat.com;
> > mst@redhat.com; Yoder Stuart-B08248
> > Subject: Re: [PATCH v4] PCI: add kernel parameter to override devid<->driver mapping.
> > 
> > [cc+ stuart]
> > 
> > On Mon, 2014-10-20 at 17:04 +0300, Marcel Apfelbaum wrote:
> > > 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 <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 slots to specific drivers use:
> > > 	pci=driver[xxxx:xx:xx.x]=foo,driver[xxxx:xx:xx.x]=bar,...
> > >
> > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > > ---
> > > v3 -> v4:
> > >  - Addressed Alex Williamson's comments:
> > >    - Modified the type of driver_override_entry's fields
> > >    - Used PCI_DEVFN when appropriated
> > >    - Removed redundant checks
> > >    - Replaced BUG_ON with pr_err messages
> > >    - Simpler command line parsing
> > >  - Addressed Michael S. Tsirkin comments
> > >    - removed DRIVER_OVERRIDE_NAME_LENGTH limitation
> > > v2 -> v3:
> > >  - Corrected subject line
> > > v1 -> v2:
> > >  - Addressed Michael S. Tsirkin comments
> > >    - Removed 32 slots limitation
> > >    - Better handling of memory allocation failures
> > >      (preferred BUG_ON over error messages)
> > >  - Addressed Alex Williamson's comments:
> > >    - Modified commit message to show parameter usage more clear.
> > >  - I preferred to re-use parse_args instead of manually using
> > >    strstr in order to better comply with command line parsing
> > >    rules.
> > >  - I didn't use any locking when parsing the command line args
> > >    (see parse_done usage) assuming that first call will be
> > >    early in system boot and no race can occur. Please correct
> > >    me if I am wrong.
> > >
> > > Notes:
> > >  - 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                   | 111 ++++++++++++++++++++++++++++++++++++
> > >  drivers/pci/pci.c                   |   2 +
> > >  3 files changed, 117 insertions(+)
> > 
> > The driver_override feature that we're making use of here is also going
> > to be supported by platform devices and potentially more bustypes in the
> > future, so I'm concerned that making a pci specific kernel parameter is
> > too shortsighted.  Instead we could hook on to BUS_NOTIFY_ADD_DEVICE for
> > bustypes that support driver_override so we can have a common interface.
> > Perhaps:
> > 
> > driver_override=pci,0000:02:00.0=pci-stub;platform,fakename=vfio-platform
> > 
> > Finding delimiters that don't conflict may be challenging.
> 
> I think what you proposed works--  <bus-name>,<bus-dev>=<driver>;
> 
> Think that will work for PCI, platform, and the new fsl-mc bus we are working
> on.
> 
> > Also, can we
> > assume that bus-name:dev-name is unique for every bustype?  It is for
> > pci, platform?
> 
> I think that has to be the case.
> 
> > It also seems like there's a question of how long should this override
> > last and how does the user disable it?
> 
> Isn't that a general question for the "driver_overrride" mechanism?
> I'm forgetting if the mechanism in the kernel now has a way to disable
> it--  e.g. echo /dev/null > /sys/pci/devices/.../driver_override ??
> 
> So, it would last until explicitly disabled through sysfs.

Yes, when you set a driver_override on a device you cancel it by writing
a NULL string to the same interface.  The problem is that here we have a
new entity in the driver scan that keeps re-applying the driver_override
as devices are scanned with no way to stop it.  So you can certainly
undo the immediate effect and bind the device to another driver, but if
the device is removed and re-scanned there's no way to stop if from
re-applying the override.  Thanks,

Alex

> > I think with pci-stub.ids=
> > $VENDOR:$DEVICE a user can echo the IDs to the pci-stub/remove_id sysfs
> > entry to cancel the effect.  The only option here seems to be a reboot.
> > Do we need a /sys/bus/pci/driver_overrides/{add_name,remove_name} for
> > this interface?  Thanks,
> 
> Thanks,
> Stuart

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

[Marcel Apfelbaum]

On Thu, 2014-10-23 at 07:57 -0600, Alex Williamson wrote:
> On Thu, 2014-10-23 at 13:44 +0000, Stuart Yoder wrote:
> > 
> > > -----Original Message-----
> > > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > > Sent: Wednesday, October 22, 2014 1:33 PM
> > > To: Marcel Apfelbaum
> > > Cc: linux-pci@vger.kernel.org; bhelgaas@google.com; linux-kernel@vger.kernel.org; marcel@redhat.com;
> > > mst@redhat.com; Yoder Stuart-B08248
> > > Subject: Re: [PATCH v4] PCI: add kernel parameter to override devid<->driver mapping.
> > > 
> > > [cc+ stuart]
> > > 
> > > On Mon, 2014-10-20 at 17:04 +0300, Marcel Apfelbaum wrote:
> > > > 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 <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 slots to specific drivers use:
> > > > 	pci=driver[xxxx:xx:xx.x]=foo,driver[xxxx:xx:xx.x]=bar,...
> > > >
> > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > > > ---
> > > > v3 -> v4:
> > > >  - Addressed Alex Williamson's comments:
> > > >    - Modified the type of driver_override_entry's fields
> > > >    - Used PCI_DEVFN when appropriated
> > > >    - Removed redundant checks
> > > >    - Replaced BUG_ON with pr_err messages
> > > >    - Simpler command line parsing
> > > >  - Addressed Michael S. Tsirkin comments
> > > >    - removed DRIVER_OVERRIDE_NAME_LENGTH limitation
> > > > v2 -> v3:
> > > >  - Corrected subject line
> > > > v1 -> v2:
> > > >  - Addressed Michael S. Tsirkin comments
> > > >    - Removed 32 slots limitation
> > > >    - Better handling of memory allocation failures
> > > >      (preferred BUG_ON over error messages)
> > > >  - Addressed Alex Williamson's comments:
> > > >    - Modified commit message to show parameter usage more clear.
> > > >  - I preferred to re-use parse_args instead of manually using
> > > >    strstr in order to better comply with command line parsing
> > > >    rules.
> > > >  - I didn't use any locking when parsing the command line args
> > > >    (see parse_done usage) assuming that first call will be
> > > >    early in system boot and no race can occur. Please correct
> > > >    me if I am wrong.
> > > >
> > > > Notes:
> > > >  - 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                   | 111 ++++++++++++++++++++++++++++++++++++
> > > >  drivers/pci/pci.c                   |   2 +
> > > >  3 files changed, 117 insertions(+)
> > > 
> > > The driver_override feature that we're making use of here is also going
> > > to be supported by platform devices and potentially more bustypes in the
> > > future, so I'm concerned that making a pci specific kernel parameter is
> > > too shortsighted.  Instead we could hook on to BUS_NOTIFY_ADD_DEVICE for
> > > bustypes that support driver_override so we can have a common interface.
> > > Perhaps:
> > > 
> > > driver_override=pci,0000:02:00.0=pci-stub;platform,fakename=vfio-platform
> > > 
> > > Finding delimiters that don't conflict may be challenging.
> > 
> > I think what you proposed works--  <bus-name>,<bus-dev>=<driver>;
> > 
> > Think that will work for PCI, platform, and the new fsl-mc bus we are working
> > on.
> > 
> > > Also, can we
> > > assume that bus-name:dev-name is unique for every bustype?  It is for
> > > pci, platform?
> > 
> > I think that has to be the case.
> > 
> > > It also seems like there's a question of how long should this override
> > > last and how does the user disable it?
> > 
> > Isn't that a general question for the "driver_overrride" mechanism?
> > I'm forgetting if the mechanism in the kernel now has a way to disable
> > it--  e.g. echo /dev/null > /sys/pci/devices/.../driver_override ??
> > 
> > So, it would last until explicitly disabled through sysfs.
> 
> Yes, when you set a driver_override on a device you cancel it by writing
> a NULL string to the same interface.  The problem is that here we have a
> new entity in the driver scan that keeps re-applying the driver_override
> as devices are scanned with no way to stop it.  So you can certainly
> undo the immediate effect and bind the device to another driver, but if
> the device is removed and re-scanned there's no way to stop if from
> re-applying the override.  Thanks,
Hi Alex,

I checked the above scenario and after driver_override is cleared
an we do bind/unbind, the mapping defined in the command line
does not apply anymore.

My steps were:
1. define the override in command-line -> the mapped driver is used instead of the native one
2. unbind the device from the slot -> no driver for device
3. remove the driver_override mapping form the slot -> no mapping defined
3, bind the device again -> native driver in use.

Thanks,
Marcel


> 
> Alex
> 
> > > I think with pci-stub.ids=
> > > $VENDOR:$DEVICE a user can echo the IDs to the pci-stub/remove_id sysfs
> > > entry to cancel the effect.  The only option here seems to be a reboot.
> > > Do we need a /sys/bus/pci/driver_overrides/{add_name,remove_name} for
> > > this interface?  Thanks,
> > 
> > Thanks,
> > Stuart
> 
> 
> 

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

[Alex Williamson]

On Thu, 2014-10-23 at 17:33 +0300, Marcel Apfelbaum wrote:
> On Thu, 2014-10-23 at 07:57 -0600, Alex Williamson wrote:
> > On Thu, 2014-10-23 at 13:44 +0000, Stuart Yoder wrote:
> > > 
> > > > -----Original Message-----
> > > > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > > > Sent: Wednesday, October 22, 2014 1:33 PM
> > > > To: Marcel Apfelbaum
> > > > Cc: linux-pci@vger.kernel.org; bhelgaas@google.com; linux-kernel@vger.kernel.org; marcel@redhat.com;
> > > > mst@redhat.com; Yoder Stuart-B08248
> > > > Subject: Re: [PATCH v4] PCI: add kernel parameter to override devid<->driver mapping.
> > > > 
> > > > [cc+ stuart]
> > > > 
> > > > On Mon, 2014-10-20 at 17:04 +0300, Marcel Apfelbaum wrote:
> > > > > 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 <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 slots to specific drivers use:
> > > > > 	pci=driver[xxxx:xx:xx.x]=foo,driver[xxxx:xx:xx.x]=bar,...
> > > > >
> > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > > > > ---
> > > > > v3 -> v4:
> > > > >  - Addressed Alex Williamson's comments:
> > > > >    - Modified the type of driver_override_entry's fields
> > > > >    - Used PCI_DEVFN when appropriated
> > > > >    - Removed redundant checks
> > > > >    - Replaced BUG_ON with pr_err messages
> > > > >    - Simpler command line parsing
> > > > >  - Addressed Michael S. Tsirkin comments
> > > > >    - removed DRIVER_OVERRIDE_NAME_LENGTH limitation
> > > > > v2 -> v3:
> > > > >  - Corrected subject line
> > > > > v1 -> v2:
> > > > >  - Addressed Michael S. Tsirkin comments
> > > > >    - Removed 32 slots limitation
> > > > >    - Better handling of memory allocation failures
> > > > >      (preferred BUG_ON over error messages)
> > > > >  - Addressed Alex Williamson's comments:
> > > > >    - Modified commit message to show parameter usage more clear.
> > > > >  - I preferred to re-use parse_args instead of manually using
> > > > >    strstr in order to better comply with command line parsing
> > > > >    rules.
> > > > >  - I didn't use any locking when parsing the command line args
> > > > >    (see parse_done usage) assuming that first call will be
> > > > >    early in system boot and no race can occur. Please correct
> > > > >    me if I am wrong.
> > > > >
> > > > > Notes:
> > > > >  - 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                   | 111 ++++++++++++++++++++++++++++++++++++
> > > > >  drivers/pci/pci.c                   |   2 +
> > > > >  3 files changed, 117 insertions(+)
> > > > 
> > > > The driver_override feature that we're making use of here is also going
> > > > to be supported by platform devices and potentially more bustypes in the
> > > > future, so I'm concerned that making a pci specific kernel parameter is
> > > > too shortsighted.  Instead we could hook on to BUS_NOTIFY_ADD_DEVICE for
> > > > bustypes that support driver_override so we can have a common interface.
> > > > Perhaps:
> > > > 
> > > > driver_override=pci,0000:02:00.0=pci-stub;platform,fakename=vfio-platform
> > > > 
> > > > Finding delimiters that don't conflict may be challenging.
> > > 
> > > I think what you proposed works--  <bus-name>,<bus-dev>=<driver>;
> > > 
> > > Think that will work for PCI, platform, and the new fsl-mc bus we are working
> > > on.
> > > 
> > > > Also, can we
> > > > assume that bus-name:dev-name is unique for every bustype?  It is for
> > > > pci, platform?
> > > 
> > > I think that has to be the case.
> > > 
> > > > It also seems like there's a question of how long should this override
> > > > last and how does the user disable it?
> > > 
> > > Isn't that a general question for the "driver_overrride" mechanism?
> > > I'm forgetting if the mechanism in the kernel now has a way to disable
> > > it--  e.g. echo /dev/null > /sys/pci/devices/.../driver_override ??
> > > 
> > > So, it would last until explicitly disabled through sysfs.
> > 
> > Yes, when you set a driver_override on a device you cancel it by writing
> > a NULL string to the same interface.  The problem is that here we have a
> > new entity in the driver scan that keeps re-applying the driver_override
> > as devices are scanned with no way to stop it.  So you can certainly
> > undo the immediate effect and bind the device to another driver, but if
> > the device is removed and re-scanned there's no way to stop if from
> > re-applying the override.  Thanks,
> Hi Alex,
> 
> I checked the above scenario and after driver_override is cleared
> an we do bind/unbind, the mapping defined in the command line
> does not apply anymore.
> 
> My steps were:
> 1. define the override in command-line -> the mapped driver is used instead of the native one
> 2. unbind the device from the slot -> no driver for device
> 3. remove the driver_override mapping form the slot -> no mapping defined
> 3, bind the device again -> native driver in use.

That's not the scenario I'm describing.  Use the remove and rescan sysfs
attributes to do a software hotplug and you'll see that the
driver_override will always be re-applied to the device.  For example:

# echo "" > /sys/bus/pci/devices/0000:02:00.0/driver_override
# echo 1 > /sys/bus/pci/devices/0000:02:00.0/remove
# echo 1 > /sys/bus/pci/rescan
# cat /sys/bus/pci/devices/0000:02:00.0/driver_override

I expect the last step will show the original override re-applied.
Thanks,

Alex

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

[Marcel Apfelbaum]

On Thu, 2014-10-23 at 08:49 -0600, Alex Williamson wrote:
> On Thu, 2014-10-23 at 17:33 +0300, Marcel Apfelbaum wrote:
> > On Thu, 2014-10-23 at 07:57 -0600, Alex Williamson wrote:
> > > On Thu, 2014-10-23 at 13:44 +0000, Stuart Yoder wrote:
> > > > 
> > > > > -----Original Message-----
> > > > > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > > > > Sent: Wednesday, October 22, 2014 1:33 PM
> > > > > To: Marcel Apfelbaum
> > > > > Cc: linux-pci@vger.kernel.org; bhelgaas@google.com; linux-kernel@vger.kernel.org; marcel@redhat.com;
> > > > > mst@redhat.com; Yoder Stuart-B08248
> > > > > Subject: Re: [PATCH v4] PCI: add kernel parameter to override devid<->driver mapping.
> > > > > 
> > > > > [cc+ stuart]
> > > > > 
> > > > > On Mon, 2014-10-20 at 17:04 +0300, Marcel Apfelbaum wrote:
> > > > > > 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 <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 slots to specific drivers use:
> > > > > > 	pci=driver[xxxx:xx:xx.x]=foo,driver[xxxx:xx:xx.x]=bar,...
> > > > > >
> > > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > > > > > ---
> > > > > > v3 -> v4:
> > > > > >  - Addressed Alex Williamson's comments:
> > > > > >    - Modified the type of driver_override_entry's fields
> > > > > >    - Used PCI_DEVFN when appropriated
> > > > > >    - Removed redundant checks
> > > > > >    - Replaced BUG_ON with pr_err messages
> > > > > >    - Simpler command line parsing
> > > > > >  - Addressed Michael S. Tsirkin comments
> > > > > >    - removed DRIVER_OVERRIDE_NAME_LENGTH limitation
> > > > > > v2 -> v3:
> > > > > >  - Corrected subject line
> > > > > > v1 -> v2:
> > > > > >  - Addressed Michael S. Tsirkin comments
> > > > > >    - Removed 32 slots limitation
> > > > > >    - Better handling of memory allocation failures
> > > > > >      (preferred BUG_ON over error messages)
> > > > > >  - Addressed Alex Williamson's comments:
> > > > > >    - Modified commit message to show parameter usage more clear.
> > > > > >  - I preferred to re-use parse_args instead of manually using
> > > > > >    strstr in order to better comply with command line parsing
> > > > > >    rules.
> > > > > >  - I didn't use any locking when parsing the command line args
> > > > > >    (see parse_done usage) assuming that first call will be
> > > > > >    early in system boot and no race can occur. Please correct
> > > > > >    me if I am wrong.
> > > > > >
> > > > > > Notes:
> > > > > >  - 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                   | 111 ++++++++++++++++++++++++++++++++++++
> > > > > >  drivers/pci/pci.c                   |   2 +
> > > > > >  3 files changed, 117 insertions(+)
> > > > > 
> > > > > The driver_override feature that we're making use of here is also going
> > > > > to be supported by platform devices and potentially more bustypes in the
> > > > > future, so I'm concerned that making a pci specific kernel parameter is
> > > > > too shortsighted.  Instead we could hook on to BUS_NOTIFY_ADD_DEVICE for
> > > > > bustypes that support driver_override so we can have a common interface.
> > > > > Perhaps:
> > > > > 
> > > > > driver_override=pci,0000:02:00.0=pci-stub;platform,fakename=vfio-platform
> > > > > 
> > > > > Finding delimiters that don't conflict may be challenging.
> > > > 
> > > > I think what you proposed works--  <bus-name>,<bus-dev>=<driver>;
> > > > 
> > > > Think that will work for PCI, platform, and the new fsl-mc bus we are working
> > > > on.
> > > > 
> > > > > Also, can we
> > > > > assume that bus-name:dev-name is unique for every bustype?  It is for
> > > > > pci, platform?
> > > > 
> > > > I think that has to be the case.
> > > > 
> > > > > It also seems like there's a question of how long should this override
> > > > > last and how does the user disable it?
> > > > 
> > > > Isn't that a general question for the "driver_overrride" mechanism?
> > > > I'm forgetting if the mechanism in the kernel now has a way to disable
> > > > it--  e.g. echo /dev/null > /sys/pci/devices/.../driver_override ??
> > > > 
> > > > So, it would last until explicitly disabled through sysfs.
> > > 
> > > Yes, when you set a driver_override on a device you cancel it by writing
> > > a NULL string to the same interface.  The problem is that here we have a
> > > new entity in the driver scan that keeps re-applying the driver_override
> > > as devices are scanned with no way to stop it.  So you can certainly
> > > undo the immediate effect and bind the device to another driver, but if
> > > the device is removed and re-scanned there's no way to stop if from
> > > re-applying the override.  Thanks,
> > Hi Alex,
> > 
> > I checked the above scenario and after driver_override is cleared
> > an we do bind/unbind, the mapping defined in the command line
> > does not apply anymore.
> > 
> > My steps were:
> > 1. define the override in command-line -> the mapped driver is used instead of the native one
> > 2. unbind the device from the slot -> no driver for device
> > 3. remove the driver_override mapping form the slot -> no mapping defined
> > 3, bind the device again -> native driver in use.
> 
> That's not the scenario I'm describing.  Use the remove and rescan sysfs
> attributes to do a software hotplug and you'll see that the
> driver_override will always be re-applied to the device.  For example:
> 
> # echo "" > /sys/bus/pci/devices/0000:02:00.0/driver_override
> # echo 1 > /sys/bus/pci/devices/0000:02:00.0/remove
> # echo 1 > /sys/bus/pci/rescan
> # cat /sys/bus/pci/devices/0000:02:00.0/driver_override
> 
> I expect the last step will show the original override re-applied.
I can check this, but I am sure you are right.
We need to think about this, if is acceptable or not.

Thanks,
Marcel

> Thanks,
> 
> Alex
> 

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

[Stuart Yoder]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogTWFyY2VsIEFwZmVsYmF1
bSBbbWFpbHRvOm1hcmNlbC5hQHJlZGhhdC5jb21dDQo+IFNlbnQ6IFRodXJzZGF5LCBPY3RvYmVy
IDIzLCAyMDE0IDg6MzcgQU0NCj4gVG86IEFsZXggV2lsbGlhbXNvbg0KPiBDYzogbGludXgtcGNp
QHZnZXIua2VybmVsLm9yZzsgYmhlbGdhYXNAZ29vZ2xlLmNvbTsgbGludXgta2VybmVsQHZnZXIu
a2VybmVsLm9yZzsgbWFyY2VsQHJlZGhhdC5jb207DQo+IG1zdEByZWRoYXQuY29tOyBZb2RlciBT
dHVhcnQtQjA4MjQ4DQo+IFN1YmplY3Q6IFJlOiBbUEFUQ0ggdjRdIFBDSTogYWRkIGtlcm5lbCBw
YXJhbWV0ZXIgdG8gb3ZlcnJpZGUgZGV2aWQ8LT5kcml2ZXIgbWFwcGluZy4NCj4gDQo+IE9uIFRo
dSwgMjAxNC0xMC0yMyBhdCAwNzoxMSAtMDYwMCwgQWxleCBXaWxsaWFtc29uIHdyb3RlOg0KPiA+
IE9uIFRodSwgMjAxNC0xMC0yMyBhdCAxNTozMiArMDMwMCwgTWFyY2VsIEFwZmVsYmF1bSB3cm90
ZToNCj4gPiA+IE9uIFdlZCwgMjAxNC0xMC0yMiBhdCAxMjozMiAtMDYwMCwgQWxleCBXaWxsaWFt
c29uIHdyb3RlOg0KPiA+ID4gPiBbY2MrIHN0dWFydF0NCj4gPiA+ID4NCj4gPiA+ID4gT24gTW9u
LCAyMDE0LTEwLTIwIGF0IDE3OjA0ICswMzAwLCBNYXJjZWwgQXBmZWxiYXVtIHdyb3RlOg0KPiA+
ID4gPiA+IFNjYW5uaW5nIGEgbG90IG9mIGRldmljZXMgZHVyaW5nIGJvb3QgcmVxdWlyZXMgYSBs
b3Qgb2YgdGltZS4NCj4gPiA+ID4gPiBPbiBvdGhlciBzY2VuYXJpb3MgdGhlcmUgaXMgYSBuZWVk
IHRvIGJpbmQgYSBkcml2ZXIgdG8gYSBzcGVjaWZpYyBzbG90Lg0KPiA+ID4gPiA+DQo+ID4gPiA+
ID4gQmluZGluZyBkZXZpY2VzIHRvIHBjaS1zdHViIGRyaXZlciBkb2VzIG5vdCB3b3JrLA0KPiA+
ID4gPiA+IGFzIGl0IHdpbGwgbm90IGRpZmZlcmVudGlhdGUgYmV0d2VlbiBkZXZpY2VzIG9mIHRo
ZQ0KPiA+ID4gPiA+IHNhbWUgdHlwZS4gVXNpbmcgc29tZSBzdGFydCBzY3JpcHRzIGlzIGVycm9y
IHByb25lLg0KPiA+ID4gPiA+DQo+ID4gPiA+ID4gVGhlIHNvbHV0aW9uIGxldmVyYWdlcyBkcml2
ZXJfb3ZlcnJpZGUgZnVuY3Rpb25hbGl0eSBpbnRyb2R1Y2VkIGJ5DQo+ID4gPiA+ID4NCj4gPiA+
ID4gPiAJY29tbWl0OiA3ODJhOTg1ZDdhZjI2ZGIzOWU4NjA3MGQyOGY5ODdjYWQyMTMxM2MwDQo+
ID4gPiA+ID4gCUF1dGhvcjogQWxleCBXaWxsaWFtc29uIDxhbGV4LndpbGxpYW1zb25AcmVkaGF0
LmNvbT4NCj4gPiA+ID4gPiAJRGF0ZTogICBUdWUgTWF5IDIwIDA4OjUzOjIxIDIwMTQgLTA2MDAN
Cj4gPiA+ID4gPg0KPiA+ID4gPiA+ICAgICAJUENJOiBJbnRyb2R1Y2UgbmV3IGRldmljZSBiaW5k
aW5nIHBhdGggdXNpbmcgcGNpX2Rldi5kcml2ZXJfb3ZlcnJpZGUNCj4gPiA+ID4gPg0KPiA+ID4g
PiA+IEluIG9yZGVyIHRvIGJpbmQgUENJIHNsb3RzIHRvIHNwZWNpZmljIGRyaXZlcnMgdXNlOg0K
PiA+ID4gPiA+IAlwY2k9ZHJpdmVyW3h4eHg6eHg6eHgueF09Zm9vLGRyaXZlclt4eHh4Onh4Onh4
LnhdPWJhciwuLi4NCj4gPiA+ID4gPg0KPiA+ID4gPiA+IFNpZ25lZC1vZmYtYnk6IE1hcmNlbCBB
cGZlbGJhdW0gPG1hcmNlbC5hQHJlZGhhdC5jb20+DQo+ID4gPiA+ID4gLS0tDQo+ID4gPiA+ID4g
djMgLT4gdjQ6DQo+ID4gPiA+ID4gIC0gQWRkcmVzc2VkIEFsZXggV2lsbGlhbXNvbidzIGNvbW1l
bnRzOg0KPiA+ID4gPiA+ICAgIC0gTW9kaWZpZWQgdGhlIHR5cGUgb2YgZHJpdmVyX292ZXJyaWRl
X2VudHJ5J3MgZmllbGRzDQo+ID4gPiA+ID4gICAgLSBVc2VkIFBDSV9ERVZGTiB3aGVuIGFwcHJv
cHJpYXRlZA0KPiA+ID4gPiA+ICAgIC0gUmVtb3ZlZCByZWR1bmRhbnQgY2hlY2tzDQo+ID4gPiA+
ID4gICAgLSBSZXBsYWNlZCBCVUdfT04gd2l0aCBwcl9lcnIgbWVzc2FnZXMNCj4gPiA+ID4gPiAg
ICAtIFNpbXBsZXIgY29tbWFuZCBsaW5lIHBhcnNpbmcNCj4gPiA+ID4gPiAgLSBBZGRyZXNzZWQg
TWljaGFlbCBTLiBUc2lya2luIGNvbW1lbnRzDQo+ID4gPiA+ID4gICAgLSByZW1vdmVkIERSSVZF
Ul9PVkVSUklERV9OQU1FX0xFTkdUSCBsaW1pdGF0aW9uDQo+ID4gPiA+ID4gdjIgLT4gdjM6DQo+
ID4gPiA+ID4gIC0gQ29ycmVjdGVkIHN1YmplY3QgbGluZQ0KPiA+ID4gPiA+IHYxIC0+IHYyOg0K
PiA+ID4gPiA+ICAtIEFkZHJlc3NlZCBNaWNoYWVsIFMuIFRzaXJraW4gY29tbWVudHMNCj4gPiA+
ID4gPiAgICAtIFJlbW92ZWQgMzIgc2xvdHMgbGltaXRhdGlvbg0KPiA+ID4gPiA+ICAgIC0gQmV0
dGVyIGhhbmRsaW5nIG9mIG1lbW9yeSBhbGxvY2F0aW9uIGZhaWx1cmVzDQo+ID4gPiA+ID4gICAg
ICAocHJlZmVycmVkIEJVR19PTiBvdmVyIGVycm9yIG1lc3NhZ2VzKQ0KPiA+ID4gPiA+ICAtIEFk
ZHJlc3NlZCBBbGV4IFdpbGxpYW1zb24ncyBjb21tZW50czoNCj4gPiA+ID4gPiAgICAtIE1vZGlm
aWVkIGNvbW1pdCBtZXNzYWdlIHRvIHNob3cgcGFyYW1ldGVyIHVzYWdlIG1vcmUgY2xlYXIuDQo+
ID4gPiA+ID4gIC0gSSBwcmVmZXJyZWQgdG8gcmUtdXNlIHBhcnNlX2FyZ3MgaW5zdGVhZCBvZiBt
YW51YWxseSB1c2luZw0KPiA+ID4gPiA+ICAgIHN0cnN0ciBpbiBvcmRlciB0byBiZXR0ZXIgY29t
cGx5IHdpdGggY29tbWFuZCBsaW5lIHBhcnNpbmcNCj4gPiA+ID4gPiAgICBydWxlcy4NCj4gPiA+
ID4gPiAgLSBJIGRpZG4ndCB1c2UgYW55IGxvY2tpbmcgd2hlbiBwYXJzaW5nIHRoZSBjb21tYW5k
IGxpbmUgYXJncw0KPiA+ID4gPiA+ICAgIChzZWUgcGFyc2VfZG9uZSB1c2FnZSkgYXNzdW1pbmcg
dGhhdCBmaXJzdCBjYWxsIHdpbGwgYmUNCj4gPiA+ID4gPiAgICBlYXJseSBpbiBzeXN0ZW0gYm9v
dCBhbmQgbm8gcmFjZSBjYW4gb2NjdXIuIFBsZWFzZSBjb3JyZWN0DQo+ID4gPiA+ID4gICAgbWUg
aWYgSSBhbSB3cm9uZy4NCj4gPiA+ID4gPg0KPiA+ID4gPiA+IE5vdGVzOg0KPiA+ID4gPiA+ICAt
IEkgaGF2ZSBmdXJ0aGVyIGlkZWFzIG9uIHRvcCBvZiB0aGlzIHBhdGNoIGJhc2VkIG9uIHlvdXIg
cmV2aWV3cy4NCj4gPiA+ID4gPiAgICBJIHRob3VnaHQgb2Y6DQo+ID4gPiA+ID4gICAgLSBVc2Ug
d2lsZGNhcmRzIHRvIHNwZWNpZnkgZW50aXJlIGJ1c2VzL2RldmljZXMsIHNvbWV0aGluZyBsaWtl
Og0KPiA+ID4gPiA+ICAgICAgCWRyaXZlclswMDAxOjAyOiouKl09cGNpLXN0dWINCj4gPiA+ID4g
PiAgICAtIFVzZSBjb21tYSB0byBzZXBhcmF0ZSBzZXZlcmFsIGRldmljZXM6DQo+ID4gPiA+ID4g
ICAgICAJZHJpdmVyWzAwMDE6MDI6MDMuNCwwMDAxOjAyOjA0LjAsLi4uXT1wY2ktc3R1Yg0KPiA+
ID4gPiA+ICAgIC0gTWFrZSBkb21haW4gb3B0aW9uYWw6DQo+ID4gPiA+ID4gICAgCWRyaXZlclsw
MDowMy4wXT1wY2ktc3R1Yg0KPiA+ID4gPiA+DQo+ID4gPiA+ID4gQ29tbWVudHMgd2lsbCBiZSBh
cHByZWNpYXRlZCwNCj4gPiA+ID4gPiBUaGFua3MsDQo+ID4gPiA+ID4gTWFyY2VsDQo+ID4gPiA+
ID4gIERvY3VtZW50YXRpb24va2VybmVsLXBhcmFtZXRlcnMudHh0IHwgICA0ICsrDQo+ID4gPiA+
ID4gIGRyaXZlcnMvcGNpL2J1cy5jICAgICAgICAgICAgICAgICAgIHwgMTExICsrKysrKysrKysr
KysrKysrKysrKysrKysrKysrKysrKysrKw0KPiA+ID4gPiA+ICBkcml2ZXJzL3BjaS9wY2kuYyAg
ICAgICAgICAgICAgICAgICB8ICAgMiArDQo+ID4gPiA+ID4gIDMgZmlsZXMgY2hhbmdlZCwgMTE3
IGluc2VydGlvbnMoKykNCj4gPiA+ID4NCj4gPiA+ID4gVGhlIGRyaXZlcl9vdmVycmlkZSBmZWF0
dXJlIHRoYXQgd2UncmUgbWFraW5nIHVzZSBvZiBoZXJlIGlzIGFsc28gZ29pbmcNCj4gPiA+ID4g
dG8gYmUgc3VwcG9ydGVkIGJ5IHBsYXRmb3JtIGRldmljZXMgYW5kIHBvdGVudGlhbGx5IG1vcmUg
YnVzdHlwZXMgaW4gdGhlDQo+ID4gPiA+IGZ1dHVyZSwgc28gSSdtIGNvbmNlcm5lZCB0aGF0IG1h
a2luZyBhIHBjaSBzcGVjaWZpYyBrZXJuZWwgcGFyYW1ldGVyIGlzDQo+ID4gPiA+IHRvbyBzaG9y
dHNpZ2h0ZWQuICBJbnN0ZWFkIHdlIGNvdWxkIGhvb2sgb24gdG8gQlVTX05PVElGWV9BRERfREVW
SUNFIGZvcg0KPiA+ID4gPiBidXN0eXBlcyB0aGF0IHN1cHBvcnQgZHJpdmVyX292ZXJyaWRlIHNv
IHdlIGNhbiBoYXZlIGEgY29tbW9uIGludGVyZmFjZS4NCj4gPiA+IFRoZSByZWFsIHF1ZXN0aW9u
IGhlcmUgaWYgdGhvc2UgYnVzIHR5cGVzL2RldmljZXMgd291bGQgYmVuZWZpdCBmcm9tIHRoaXMN
Cj4gPiA+IGZlYXR1cmUsIGFuZCBJIGFsc28gbXVzdCBjb25mZXNzIHRoYXQgSSBoYXZlIG5vIGtu
b3dsZWRnZSBvZiB0aGUgb3RoZXIgYnVzZXMuDQo+ID4gPiBDYW4gYW55b25lIGNvbmZpcm0gdGhh
dCBpdCBkb2VzIG1ha2Ugc2Vuc2UgZm9yIHRoZW0/DQo+ID4NCj4gPiBQbGF0Zm9ybSBkZXZpY2Vz
IGFyZSBhZGRpbmcgdmZpbyBzdXBwb3J0LCBzbyBJIGV4cGVjdCB0aGUgbmV4dCBsb2dpY2FsDQo+
ID4gcXVlc3Rpb24gd2lsbCBiZSBob3cgdG8gcmVzZXJ2ZSBkZXZpY2VzIGZvciB1c2UgYnkgdmZp
byBhdCBib290Lg0KPiBXZWxsLCBJJ2xsIGhhdmUgdG8gbGVhcm4gbW9yZSBhYm91dCB2ZmlvIGJl
Zm9yZSBzYXlpbmcgYW55dGhpbmcsDQo+IGJ1dCBteSBxdWVzdGlvbiBpcyBpZiBpdCBjYW4gYmUg
ZGVmZXJyZWQgb3IgaXQgaGFzIHRvIGJlIHBhcnQgb2YNCj4gdGhpcyBwYXRjaC4gSWYgdGhlIHBs
YXRmb3JtIGRldmljZXMgZG8gbm90IGhhdmUgYSBzbG90IGxpa2UgaHcgYWRkcmVzcywNCj4gbWF5
YmUgaXQgY2FuIGJlIGNvbmZpZ3VyZWQgc2VwYXJhdGVseS4NCj4gDQo+IEkgc2F3IHRoaXMgcGF0
Y2ggYXMgYSBQQ0kgcGF0Y2gsIGFuZCBub3QgImRyaXZlcl9vdmVycmlkZSIgZXhwYW5zaW9uOw0K
PiBtZWFuaW5nIHRoYXQgSSBvbmx5IGxldmVyYWdlZCBhbiBleGlzdGluZyBmZWF0dXJlLCBub3Qg
dHJ5aW5nIHRvDQo+IGV4dGVuZCBpdC4NCg0KSSB0aGluayBvdGhlciBidXNlcyBtYXkgd2FudCB0
byB1c2UgdGhlIHNhbWUgbWVjaGFuaXNtIHRvIHNwZWNpZnkgZHJpdmVyDQpiaW5kaW5ncywgc28g
SSB0aGluayB0aGUgbWFpbiB0aGluZyBpcyB0byBub3QgZGVmaW5lIGEgc3ludGF4IHRoYXQgbWFr
ZXMNCnRoYXQgcHJvYmxlbWF0aWMuDQoNCkxvb2tpbmcgYXQgeW91ciBvcmlnaW5hbCBzeW50YXgs
IEkgdGhpbmsgaXQgY291bGQgd29yayBmb3IgZGlmZmVyZW50DQpidXMgdHlwZXMuICBDb3VsZCBo
YXZlIHNvbWV0aGluZyBsaWtlOg0KDQogICBwbGF0Zm9ybT1kcml2ZXJbeHh4eF09Zm9vLGRyaXZl
clt4eHh4XT12ZmlvLXBsYXRmb3JtDQogICBwY2k9ZHJpdmVyW3h4eHg6eHg6eHgueF09Zm9vLGRy
aXZlclt4eHh4Onh4Onh4LnhdPWJhcg0KDQpTbyBubyBzdHJvbmcgb3BpbmlvbiBvbiB0aGF0IHZz
IEFsZXgncyBwcm9wb3NhbDoNCg0KICAgZHJpdmVyX292ZXJyaWRlPXBjaSwwMDAwOjAyOjAwLjA9
cGNpLXN0dWI7cGxhdGZvcm0seHh4eD12ZmlvLXBsYXRmb3JtDQoNCkl0IHNlZW1zIHRvIG1lIHRo
YXQgZWl0aGVyIGNvdWxkIHdvcmsuDQoNClRoYW5rcywNClN0dWFydA0K

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

[Alex Williamson]

On Thu, 2014-10-23 at 18:00 +0300, Marcel Apfelbaum wrote:
> On Thu, 2014-10-23 at 08:49 -0600, Alex Williamson wrote:
> > On Thu, 2014-10-23 at 17:33 +0300, Marcel Apfelbaum wrote:
> > > On Thu, 2014-10-23 at 07:57 -0600, Alex Williamson wrote:
> > > > On Thu, 2014-10-23 at 13:44 +0000, Stuart Yoder wrote:
> > > > > 
> > > > > > -----Original Message-----
> > > > > > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > > > > > Sent: Wednesday, October 22, 2014 1:33 PM
> > > > > > To: Marcel Apfelbaum
> > > > > > Cc: linux-pci@vger.kernel.org; bhelgaas@google.com; linux-kernel@vger.kernel.org; marcel@redhat.com;
> > > > > > mst@redhat.com; Yoder Stuart-B08248
> > > > > > Subject: Re: [PATCH v4] PCI: add kernel parameter to override devid<->driver mapping.
> > > > > > 
> > > > > > [cc+ stuart]
> > > > > > 
> > > > > > On Mon, 2014-10-20 at 17:04 +0300, Marcel Apfelbaum wrote:
> > > > > > > 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 <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 slots to specific drivers use:
> > > > > > > 	pci=driver[xxxx:xx:xx.x]=foo,driver[xxxx:xx:xx.x]=bar,...
> > > > > > >
> > > > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > > > > > > ---
> > > > > > > v3 -> v4:
> > > > > > >  - Addressed Alex Williamson's comments:
> > > > > > >    - Modified the type of driver_override_entry's fields
> > > > > > >    - Used PCI_DEVFN when appropriated
> > > > > > >    - Removed redundant checks
> > > > > > >    - Replaced BUG_ON with pr_err messages
> > > > > > >    - Simpler command line parsing
> > > > > > >  - Addressed Michael S. Tsirkin comments
> > > > > > >    - removed DRIVER_OVERRIDE_NAME_LENGTH limitation
> > > > > > > v2 -> v3:
> > > > > > >  - Corrected subject line
> > > > > > > v1 -> v2:
> > > > > > >  - Addressed Michael S. Tsirkin comments
> > > > > > >    - Removed 32 slots limitation
> > > > > > >    - Better handling of memory allocation failures
> > > > > > >      (preferred BUG_ON over error messages)
> > > > > > >  - Addressed Alex Williamson's comments:
> > > > > > >    - Modified commit message to show parameter usage more clear.
> > > > > > >  - I preferred to re-use parse_args instead of manually using
> > > > > > >    strstr in order to better comply with command line parsing
> > > > > > >    rules.
> > > > > > >  - I didn't use any locking when parsing the command line args
> > > > > > >    (see parse_done usage) assuming that first call will be
> > > > > > >    early in system boot and no race can occur. Please correct
> > > > > > >    me if I am wrong.
> > > > > > >
> > > > > > > Notes:
> > > > > > >  - 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                   | 111 ++++++++++++++++++++++++++++++++++++
> > > > > > >  drivers/pci/pci.c                   |   2 +
> > > > > > >  3 files changed, 117 insertions(+)
> > > > > > 
> > > > > > The driver_override feature that we're making use of here is also going
> > > > > > to be supported by platform devices and potentially more bustypes in the
> > > > > > future, so I'm concerned that making a pci specific kernel parameter is
> > > > > > too shortsighted.  Instead we could hook on to BUS_NOTIFY_ADD_DEVICE for
> > > > > > bustypes that support driver_override so we can have a common interface.
> > > > > > Perhaps:
> > > > > > 
> > > > > > driver_override=pci,0000:02:00.0=pci-stub;platform,fakename=vfio-platform
> > > > > > 
> > > > > > Finding delimiters that don't conflict may be challenging.
> > > > > 
> > > > > I think what you proposed works--  <bus-name>,<bus-dev>=<driver>;
> > > > > 
> > > > > Think that will work for PCI, platform, and the new fsl-mc bus we are working
> > > > > on.
> > > > > 
> > > > > > Also, can we
> > > > > > assume that bus-name:dev-name is unique for every bustype?  It is for
> > > > > > pci, platform?
> > > > > 
> > > > > I think that has to be the case.
> > > > > 
> > > > > > It also seems like there's a question of how long should this override
> > > > > > last and how does the user disable it?
> > > > > 
> > > > > Isn't that a general question for the "driver_overrride" mechanism?
> > > > > I'm forgetting if the mechanism in the kernel now has a way to disable
> > > > > it--  e.g. echo /dev/null > /sys/pci/devices/.../driver_override ??
> > > > > 
> > > > > So, it would last until explicitly disabled through sysfs.
> > > > 
> > > > Yes, when you set a driver_override on a device you cancel it by writing
> > > > a NULL string to the same interface.  The problem is that here we have a
> > > > new entity in the driver scan that keeps re-applying the driver_override
> > > > as devices are scanned with no way to stop it.  So you can certainly
> > > > undo the immediate effect and bind the device to another driver, but if
> > > > the device is removed and re-scanned there's no way to stop if from
> > > > re-applying the override.  Thanks,
> > > Hi Alex,
> > > 
> > > I checked the above scenario and after driver_override is cleared
> > > an we do bind/unbind, the mapping defined in the command line
> > > does not apply anymore.
> > > 
> > > My steps were:
> > > 1. define the override in command-line -> the mapped driver is used instead of the native one
> > > 2. unbind the device from the slot -> no driver for device
> > > 3. remove the driver_override mapping form the slot -> no mapping defined
> > > 3, bind the device again -> native driver in use.
> > 
> > That's not the scenario I'm describing.  Use the remove and rescan sysfs
> > attributes to do a software hotplug and you'll see that the
> > driver_override will always be re-applied to the device.  For example:
> > 
> > # echo "" > /sys/bus/pci/devices/0000:02:00.0/driver_override
> > # echo 1 > /sys/bus/pci/devices/0000:02:00.0/remove
> > # echo 1 > /sys/bus/pci/rescan
> > # cat /sys/bus/pci/devices/0000:02:00.0/driver_override
> > 
> > I expect the last step will show the original override re-applied.
> I can check this, but I am sure you are right.
> We need to think about this, if is acceptable or not.

I think we're going to need a sysfs way to manipulate the set of active
overrides.  I was thinking about whether a one-shot implementation might
be acceptable, ie. discard the override after use, but I think that
would look just as unpredictable to users as the current approach.
Thanks,

Alex

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

[Marcel Apfelbaum]

On Thu, 2014-10-23 at 09:54 -0600, Alex Williamson wrote:
> On Thu, 2014-10-23 at 18:00 +0300, Marcel Apfelbaum wrote:
> > On Thu, 2014-10-23 at 08:49 -0600, Alex Williamson wrote:
> > > On Thu, 2014-10-23 at 17:33 +0300, Marcel Apfelbaum wrote:
> > > > On Thu, 2014-10-23 at 07:57 -0600, Alex Williamson wrote:
> > > > > On Thu, 2014-10-23 at 13:44 +0000, Stuart Yoder wrote:
> > > > > > 
> > > > > > > -----Original Message-----
> > > > > > > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > > > > > > Sent: Wednesday, October 22, 2014 1:33 PM
> > > > > > > To: Marcel Apfelbaum
> > > > > > > Cc: linux-pci@vger.kernel.org; bhelgaas@google.com; linux-kernel@vger.kernel.org; marcel@redhat.com;
> > > > > > > mst@redhat.com; Yoder Stuart-B08248
> > > > > > > Subject: Re: [PATCH v4] PCI: add kernel parameter to override devid<->driver mapping.
> > > > > > > 
> > > > > > > [cc+ stuart]
> > > > > > > 
> > > > > > > On Mon, 2014-10-20 at 17:04 +0300, Marcel Apfelbaum wrote:
> > > > > > > > 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 <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 slots to specific drivers use:
> > > > > > > > 	pci=driver[xxxx:xx:xx.x]=foo,driver[xxxx:xx:xx.x]=bar,...
> > > > > > > >
> > > > > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > > > > > > > ---
> > > > > > > > v3 -> v4:
> > > > > > > >  - Addressed Alex Williamson's comments:
> > > > > > > >    - Modified the type of driver_override_entry's fields
> > > > > > > >    - Used PCI_DEVFN when appropriated
> > > > > > > >    - Removed redundant checks
> > > > > > > >    - Replaced BUG_ON with pr_err messages
> > > > > > > >    - Simpler command line parsing
> > > > > > > >  - Addressed Michael S. Tsirkin comments
> > > > > > > >    - removed DRIVER_OVERRIDE_NAME_LENGTH limitation
> > > > > > > > v2 -> v3:
> > > > > > > >  - Corrected subject line
> > > > > > > > v1 -> v2:
> > > > > > > >  - Addressed Michael S. Tsirkin comments
> > > > > > > >    - Removed 32 slots limitation
> > > > > > > >    - Better handling of memory allocation failures
> > > > > > > >      (preferred BUG_ON over error messages)
> > > > > > > >  - Addressed Alex Williamson's comments:
> > > > > > > >    - Modified commit message to show parameter usage more clear.
> > > > > > > >  - I preferred to re-use parse_args instead of manually using
> > > > > > > >    strstr in order to better comply with command line parsing
> > > > > > > >    rules.
> > > > > > > >  - I didn't use any locking when parsing the command line args
> > > > > > > >    (see parse_done usage) assuming that first call will be
> > > > > > > >    early in system boot and no race can occur. Please correct
> > > > > > > >    me if I am wrong.
> > > > > > > >
> > > > > > > > Notes:
> > > > > > > >  - 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                   | 111 ++++++++++++++++++++++++++++++++++++
> > > > > > > >  drivers/pci/pci.c                   |   2 +
> > > > > > > >  3 files changed, 117 insertions(+)
> > > > > > > 
> > > > > > > The driver_override feature that we're making use of here is also going
> > > > > > > to be supported by platform devices and potentially more bustypes in the
> > > > > > > future, so I'm concerned that making a pci specific kernel parameter is
> > > > > > > too shortsighted.  Instead we could hook on to BUS_NOTIFY_ADD_DEVICE for
> > > > > > > bustypes that support driver_override so we can have a common interface.
> > > > > > > Perhaps:
> > > > > > > 
> > > > > > > driver_override=pci,0000:02:00.0=pci-stub;platform,fakename=vfio-platform
> > > > > > > 
> > > > > > > Finding delimiters that don't conflict may be challenging.
> > > > > > 
> > > > > > I think what you proposed works--  <bus-name>,<bus-dev>=<driver>;
> > > > > > 
> > > > > > Think that will work for PCI, platform, and the new fsl-mc bus we are working
> > > > > > on.
> > > > > > 
> > > > > > > Also, can we
> > > > > > > assume that bus-name:dev-name is unique for every bustype?  It is for
> > > > > > > pci, platform?
> > > > > > 
> > > > > > I think that has to be the case.
> > > > > > 
> > > > > > > It also seems like there's a question of how long should this override
> > > > > > > last and how does the user disable it?
> > > > > > 
> > > > > > Isn't that a general question for the "driver_overrride" mechanism?
> > > > > > I'm forgetting if the mechanism in the kernel now has a way to disable
> > > > > > it--  e.g. echo /dev/null > /sys/pci/devices/.../driver_override ??
> > > > > > 
> > > > > > So, it would last until explicitly disabled through sysfs.
> > > > > 
> > > > > Yes, when you set a driver_override on a device you cancel it by writing
> > > > > a NULL string to the same interface.  The problem is that here we have a
> > > > > new entity in the driver scan that keeps re-applying the driver_override
> > > > > as devices are scanned with no way to stop it.  So you can certainly
> > > > > undo the immediate effect and bind the device to another driver, but if
> > > > > the device is removed and re-scanned there's no way to stop if from
> > > > > re-applying the override.  Thanks,
> > > > Hi Alex,
> > > > 
> > > > I checked the above scenario and after driver_override is cleared
> > > > an we do bind/unbind, the mapping defined in the command line
> > > > does not apply anymore.
> > > > 
> > > > My steps were:
> > > > 1. define the override in command-line -> the mapped driver is used instead of the native one
> > > > 2. unbind the device from the slot -> no driver for device
> > > > 3. remove the driver_override mapping form the slot -> no mapping defined
> > > > 3, bind the device again -> native driver in use.
> > > 
> > > That's not the scenario I'm describing.  Use the remove and rescan sysfs
> > > attributes to do a software hotplug and you'll see that the
> > > driver_override will always be re-applied to the device.  For example:
> > > 
> > > # echo "" > /sys/bus/pci/devices/0000:02:00.0/driver_override
> > > # echo 1 > /sys/bus/pci/devices/0000:02:00.0/remove
> > > # echo 1 > /sys/bus/pci/rescan
> > > # cat /sys/bus/pci/devices/0000:02:00.0/driver_override
> > > 
> > > I expect the last step will show the original override re-applied.
> > I can check this, but I am sure you are right.
> > We need to think about this, if is acceptable or not.
> 
> I think we're going to need a sysfs way to manipulate the set of active
> overrides.  I was thinking about whether a one-shot implementation might
> be acceptable, ie. discard the override after use, but I think that
> would look just as unpredictable to users as the current approach.
> Thanks,
Sure, maybe a /sys/bus/<bus type>/driver_overrides file that has
the same format as the kernel parameter (and can be edited by the end user)
or something based on operations like you suggested:
 /sys/bus/pci/driver_overrides/{add_name,remove_name}

Thanks for the ideas,
Marcel


> 
> Alex
>