|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] xen-pciback: provide a "reset" sysfs file to try harder at an SBR
On Wed, Jul 09, 2014 at 03:09:59PM +0100, David Vrabel wrote:
> The standard implementation of pci_reset_function() does not try an
> SBR if there are other sibling devices. This is a common
> configuration for multi-function devices (e.g., GPUs with a HDMI audio
> device function).
>
> If all the functions are co-assigned to the same domain at the same
> time, then it is safe to perform an SBR to reset all functions at the
> same time.
Is there a particular reason you don't want to lift some of the
code (see my pcistub_reset_pci_dev function in xen/pciback: Implement
PCI reset slot or bus with 'do_flr' SysFS attribute) to check
for this?
>
> Add a "reset" sysfs file with the same interface as the standard one
> (i.e., write 1 to reset) that will try an SBR if all sibling devices
> are unbound or bound to pciback.
>
> Note that this is weaker than the requirement for functions to be
> co-assigned, but the toolstack is expected to ensure this.
>
> Signed-off-by: David Vrabel <david.vrabel@xxxxxxxxxx>
> ---
> Changes in v2:
> - defer creating sysfs node to late init.
> ---
> drivers/xen/xen-pciback/pci_stub.c | 125
> +++++++++++++++++++++++++++++++++++-
> drivers/xen/xen-pciback/pciback.h | 1 +
> 2 files changed, 123 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/xen/xen-pciback/pci_stub.c
> b/drivers/xen/xen-pciback/pci_stub.c
> index d57a173..5697c2a 100644
> --- a/drivers/xen/xen-pciback/pci_stub.c
> +++ b/drivers/xen/xen-pciback/pci_stub.c
> @@ -17,6 +17,7 @@
> #include <linux/wait.h>
> #include <linux/sched.h>
> #include <linux/atomic.h>
> +#include <linux/delay.h>
> #include <xen/events.h>
> #include <asm/xen/pci.h>
> #include <asm/xen/hypervisor.h>
> @@ -63,6 +64,117 @@ static LIST_HEAD(pcistub_devices);
> static int initialize_devices;
> static LIST_HEAD(seized_devices);
>
> +/*
> + * pci_reset_function() will only work if there is a mechanism to
> + * reset that single function (e.g., FLR or a D-state transition).
> + * For PCI hardware that has two or more functions but no per-function
> + * reset, we can do a bus reset iff all the functions are co-assigned
> + * to the same domain.
> + *
> + * If a function has no per-function reset mechanism the 'reset' sysfs
> + * file that the toolstack uses to reset a function prior to assigning
> + * the device will be missing. In this case, pciback adds its own
> + * which will try a bus reset.
> + *
> + * Note: pciback does not check for co-assigment before doing a bus
> + * reset, only that the devices are bound to pciback. The toolstack
> + * is assumed to have done the right thing.
> + */
> +static int __pcistub_reset_function(struct pci_dev *dev)
> +{
> + struct pci_dev *pdev;
> + u16 ctrl;
> + int ret;
> +
> + ret = __pci_reset_function_locked(dev);
> + if (ret == 0)
> + return 0;
> +
> + if (pci_is_root_bus(dev->bus) || dev->subordinate || !dev->bus->self)
> + return -ENOTTY;
> +
> + list_for_each_entry(pdev, &dev->bus->devices, bus_list) {
> + if (pdev != dev && (!pdev->driver
> + || strcmp(pdev->driver->name, "pciback")))
> + return -ENOTTY;
> + pci_save_state(pdev);
> + }
> +
> + pci_read_config_word(dev->bus->self, PCI_BRIDGE_CONTROL, &ctrl);
> + ctrl |= PCI_BRIDGE_CTL_BUS_RESET;
> + pci_write_config_word(dev->bus->self, PCI_BRIDGE_CONTROL, ctrl);
> + msleep(200);
> +
> + ctrl &= ~PCI_BRIDGE_CTL_BUS_RESET;
> + pci_write_config_word(dev->bus->self, PCI_BRIDGE_CONTROL, ctrl);
> + msleep(200);
Why not use pci_try_reset_slot(dev->slot) or pci_try_reset_bus(dev->bus)?
> +
> + list_for_each_entry(pdev, &dev->bus->devices, bus_list)
> + pci_restore_state(pdev);
> +
> + return 0;
> +}
> +
> +static int pcistub_reset_function(struct pci_dev *dev)
> +{
> + int ret;
> +
> + device_lock(&dev->dev);
> + ret = __pcistub_reset_function(dev);
> + device_unlock(&dev->dev);
> +
> + return ret;
> +}
> +
> +static ssize_t pcistub_reset_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct pci_dev *pdev = to_pci_dev(dev);
> + unsigned long val;
> + ssize_t result = strict_strtoul(buf, 0, &val);
> +
> + if (result < 0)
> + return result;
> +
> + if (val != 1)
> + return -EINVAL;
> +
> + result = pcistub_reset_function(pdev);
> + if (result < 0)
> + return result;
> + return count;
> +}
> +static DEVICE_ATTR(reset, 0200, NULL, pcistub_reset_store);
> +
> +static int pcistub_try_create_reset_file(struct pci_dev *pci)
> +{
> + struct xen_pcibk_dev_data *dev_data = pci_get_drvdata(pci);
> + struct device *dev = &pci->dev;
> + struct kernfs_node *reset_dirent;
> + int ret;
> +
> + reset_dirent = sysfs_get_dirent(dev->kobj.sd, "reset");
> + if (reset_dirent) {
> + sysfs_put(reset_dirent);
> + return 0;
> + }
> +
> + ret = device_create_file(dev, &dev_attr_reset);
> + if (ret < 0)
> + return ret;
> + dev_data->created_reset_file = true;
> + return 0;
> +}
> +
> +static void pcistub_remove_reset_file(struct pci_dev *pci)
> +{
> + struct xen_pcibk_dev_data *dev_data = pci_get_drvdata(pci);
> +
> + if (dev_data && dev_data->created_reset_file)
> + device_remove_file(&pci->dev, &dev_attr_reset);
> +}
> +
> static struct pcistub_device *pcistub_device_alloc(struct pci_dev *dev)
> {
> struct pcistub_device *psdev;
> @@ -103,7 +215,8 @@ static void pcistub_device_release(struct kref *kref)
> /* Call the reset function which does not take lock as this
> * is called from "unbind" which takes a device_lock mutex.
> */
> - __pci_reset_function_locked(dev);
> + __pcistub_reset_function(psdev->dev);
> +
> if (pci_load_and_free_saved_state(dev, &dev_data->pci_saved_state))
> dev_dbg(&dev->dev, "Could not reload PCI state\n");
> else
> @@ -280,7 +393,7 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
> /* This is OK - we are running from workqueue context
> * and want to inhibit the user from fiddling with 'reset'
> */
> - pci_reset_function(dev);
> + pcistub_reset_function(dev);
> pci_restore_state(dev);
>
> /* This disables the device. */
> @@ -404,7 +517,7 @@ static int pcistub_init_device(struct pci_dev *dev)
> dev_err(&dev->dev, "Could not store PCI conf saved state!\n");
> else {
> dev_dbg(&dev->dev, "resetting (FLR, D3, etc) the device\n");
> - __pci_reset_function_locked(dev);
> + __pcistub_reset_function(dev);
> pci_restore_state(dev);
> }
> /* Now disable the device (this also ensures some private device
> @@ -413,6 +526,10 @@ static int pcistub_init_device(struct pci_dev *dev)
> dev_dbg(&dev->dev, "reset device\n");
> xen_pcibk_reset_device(dev);
>
> + err = pcistub_try_create_reset_file(dev);
> + if (err < 0)
> + goto config_release;
> +
> dev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
> return 0;
>
> @@ -540,6 +657,8 @@ static void pcistub_remove(struct pci_dev *dev)
>
> dev_dbg(&dev->dev, "removing\n");
>
> + pcistub_remove_reset_file(dev);
> +
> spin_lock_irqsave(&pcistub_devices_lock, flags);
>
> xen_pcibk_config_quirk_release(dev);
> diff --git a/drivers/xen/xen-pciback/pciback.h
> b/drivers/xen/xen-pciback/pciback.h
> index f72af87..708ade9 100644
> --- a/drivers/xen/xen-pciback/pciback.h
> +++ b/drivers/xen/xen-pciback/pciback.h
> @@ -42,6 +42,7 @@ struct xen_pcibk_device {
> struct xen_pcibk_dev_data {
> struct list_head config_fields;
> struct pci_saved_state *pci_saved_state;
> + bool created_reset_file;
> unsigned int permissive:1;
> unsigned int warned_on_write:1;
> unsigned int enable_intx:1;
> --
> 1.7.10.4
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |