RE: [PATCH 2/2 v2] PCI: add remove_id sysfs entry

Han, Weidong
2009-02-26T05:37:40Z

Chris Wright wrote:
> This adds a remove_id sysfs entry to allow users of new_id to later
> remove the added dynid. One use case is management tools that want to
> dynamically bind/unbind devices to pci-stub driver while devices are
> assigned to KVM guests. Rather than having to track which driver was
> originally bound to the driver, a mangement tool can simply:
>
> # echo "8086 10f5" > /sys/bus/pci/drivers/pci-stub/new_id
> # echo 0000:00:19.0 > /sys/bus/pci/devices/0000:00:19.0/driver/unbind
> # echo 0000:00:19.0 > /sys/bus/pci/drivers/pci-stub/bind
>
> Guest uses device
>
> # echo "8086 10f5" > /sys/bus/pci/drivers/pci-stub/remove_id
> # echo 0000:00:19.0 > /sys/bus/pci/drivers_probe
>

After above two commands, I found device 00:19.0 was still bound to pci-stub, and doesn't work. I found it needs following unbind command between remove_id and drivers_probe to make it work with original driver:
# echo "8086 10f5" > /sys/bus/pci/drivers/pci-stub/unbind

Chris, did it also happen on your side? or did I miss something?

Regards,
Weidong

> Signed-off-by: Chris Wright <chrisw@sous-sol.org>
> ---
> v2
> - update changelog to match ABI docs (remove echo -n)
>
> Documentation/ABI/testing/sysfs-bus-pci | 16 ++++++
> drivers/pci/pci-driver.c | 80
> +++++++++++++++++++++++++++++++- 2 files changed, 94 insertions(+),
> 2 deletions(-)
>
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -99,6 +99,52 @@ store_new_id(struct device_driver *drive
> }
> static DRIVER_ATTR(new_id, S_IWUSR, NULL, store_new_id);
>
> +/**
> + * store_remove_id - remove a PCI device ID from this driver
> + * @driver: target device driver
> + * @buf: buffer for scanning device ID data
> + * @count: input size
> + *
> + * Removes a dynamic pci device ID to this driver.
> + */
> +static ssize_t
> +store_remove_id(struct device_driver *driver, const char *buf,
> size_t count) +{
> + struct pci_dynid *dynid, *n;
> + struct pci_driver *pdrv = to_pci_driver(driver);
> + __u32 vendor, device, subvendor = PCI_ANY_ID,
> + subdevice = PCI_ANY_ID, class = 0, class_mask = 0;
> + int fields = 0;
> + int retval = -ENODEV;
> +
> + fields = sscanf(buf, "%x %x %x %x %x %x",
> + &vendor, &device, &subvendor, &subdevice,
> + &class, &class_mask);
> + if (fields < 2)
> + return -EINVAL;
> +
> + spin_lock(&pdrv->dynids.lock);
> + list_for_each_entry_safe(dynid, n, &pdrv->dynids.list, node) {
> + struct pci_device_id *id = &dynid->id;
> + if ((id->vendor == vendor) &&
> + (id->device == device) &&
> + (subvendor == PCI_ANY_ID || id->subvendor == subvendor) &&
> + (subdevice == PCI_ANY_ID || id->subdevice == subdevice) &&
> + !((id->class ^ class) & class_mask)) {
> + list_del(&dynid->node);
> + kfree(dynid);
> + retval = 0;
> + break;
> + }
> + }
> + spin_unlock(&pdrv->dynids.lock);
> +
> + if (retval)
> + return retval;
> + return count;
> +}
> +static DRIVER_ATTR(remove_id, S_IWUSR, NULL, store_remove_id);
> +
> static void
> pci_free_dynids(struct pci_driver *drv)
> {
> @@ -125,6 +171,20 @@ static void pci_remove_newid_file(struct
> {
> driver_remove_file(&drv->driver, &driver_attr_new_id);
> }
> +
> +static int
> +pci_create_removeid_file(struct pci_driver *drv)
> +{
> + int error = 0;
> + if (drv->probe != NULL)
> + error = driver_create_file(&drv->driver,&driver_attr_remove_id);
> + return error;
> +}
> +
> +static void pci_remove_removeid_file(struct pci_driver *drv)
> +{
> + driver_remove_file(&drv->driver, &driver_attr_remove_id);
> +}
> #else /* !CONFIG_HOTPLUG */
> static inline void pci_free_dynids(struct pci_driver *drv) {}
> static inline int pci_create_newid_file(struct pci_driver *drv)
> @@ -132,6 +192,11 @@ static inline int pci_create_newid_file(
> return 0;
> }
> static inline void pci_remove_newid_file(struct pci_driver *drv) {}
> +static inline int pci_create_removeid_file(struct pci_driver *drv)
> +{
> + return 0;
> +}
> +static inline void pci_remove_removeid_file(struct pci_driver *drv)
> {} #endif
>
> /**
> @@ -852,13 +917,23 @@ int __pci_register_driver(struct pci_dri
> /* register with core */
> error = driver_register(&drv->driver);
> if (error)
> - return error;
> + goto out;
>
> error = pci_create_newid_file(drv);
> if (error)
> - driver_unregister(&drv->driver);
> + goto out_newid;
>
> + error = pci_create_removeid_file(drv);
> + if (error)
> + goto out_removeid;
> +out:
> return error;
> +
> +out_removeid:
> + pci_remove_newid_file(drv);
> +out_newid:
> + driver_unregister(&drv->driver);
> + goto out;
> }
>
> /**
> @@ -874,6 +949,7 @@ int __pci_register_driver(struct pci_dri
> void
> pci_unregister_driver(struct pci_driver *drv)
> {
> + pci_remove_removeid_file(drv);
> pci_remove_newid_file(drv);
> driver_unregister(&drv->driver);
> pci_free_dynids(drv);
> --- a/Documentation/ABI/testing/sysfs-bus-pci
> +++ b/Documentation/ABI/testing/sysfs-bus-pci
> @@ -41,6 +41,22 @@ Description:
> for the device and attempt to bind to it. For example:
> # echo "8086 10f5" > /sys/bus/pci/drivers/foo/new_id
>
> +What: /sys/bus/pci/drivers/.../remove_id
> +Date: February 2009
> +Contact: Chris Wright <chrisw@sous-sol.org>
> +Description:
> + Writing a device ID to this file will remove an ID
> + that was dynamically added via the new_id sysfs entry.
> + The format for the device ID is:
> + VVVV DDDD SVVV SDDD CCCC MMMM. That is Vendor ID, Device
> + ID, Subsystem Vendor ID, Subsystem Device ID, Class,
> + and Class Mask. The Vendor ID and Device ID fields are
> + required, the rest are optional. After successfully
> + removing an ID, the driver will no longer support the
> + device. This is useful to ensure auto probing won't
> + match the driver to the device. For example:
> + # echo "8086 10f5" > /sys/bus/pci/drivers/foo/remove_id
> +
> What: /sys/bus/pci/devices/.../vpd
> Date: February 2008
> Contact: Ben Hutchings <bhutchings@solarflare.com>