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@xxxxxxxxxx]
> > Sent: Wednesday, October 22, 2014 1:33 PM
> > To: Marcel Apfelbaum
> > Cc: linux-pci@xxxxxxxxxxxxxxx; bhelgaas@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; marcel@xxxxxxxxxx;
> > mst@xxxxxxxxxx; 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@xxxxxxxxxx>
> > > 	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@xxxxxxxxxx>
> > > ---
> > > 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



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/