[PATCH RFC] pci: expose function reset capability in sysfs

[PATCH RFC] pci: expose function reset capability in sysfs

[Michael S. Tsirkin]

Some devices allow an individual function to be reset without affecting
other functions in the same device: that's what pci_reset_function does.
For devices that have this support, expose reset attribite in sysfs.

This is useful e.g. for virtualization, where a qemu userspace
process wants to reset the device when the guest is started/reset,
to emulate machine reboot as closely as possible.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Jesse, all,
could you please comment on whether the following approach looks sane?
Compile-tested only at this point.  I'm also not sure whether the
CAP_SYS_ADMIN check is necessary: maybe 400 permissions on the sysfs
file are sufficient?

 drivers/pci/pci-sysfs.c  |   37 +++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.c        |   16 ++++++++++++++++
 drivers/pci/pci.h        |    1 +
 include/linux/kvm_host.h |    1 +
 virt/kvm/irq_comm.c      |    4 +++-
 5 files changed, 58 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 85ebd02..92805e8 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -916,6 +916,28 @@ int __attribute__ ((weak)) pcibios_add_platform_entries(struct pci_dev *dev)
 	return 0;
 }
 
+static ssize_t 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;
+
+	/* this can crash the machine when done on the "wrong" device */
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	if (val != 1)
+		return -EINVAL;
+	return pci_reset_function(pdev);
+}
+
+static struct device_attribute reset_attr = __ATTR(reset, 0200, NULL, reset_store);
+
 static int pci_create_capabilities_sysfs(struct pci_dev *dev)
 {
 	int retval;
@@ -943,7 +965,21 @@ static int pci_create_capabilities_sysfs(struct pci_dev *dev)
 	/* Active State Power Management */
 	pcie_aspm_create_sysfs_dev_files(dev);
 
+	if (!pci_probe_reset_function(dev)) {
+		retval = device_create_file(&dev->dev, &reset_attr);
+		if (retval)
+			goto error;
+	}
 	return 0;
+
+error:
+	pcie_aspm_remove_sysfs_dev_files(dev);
+	if (dev->vpd && dev->vpd->attr) {
+		sysfs_remove_bin_file(&dev->dev.kobj, dev->vpd->attr);
+		kfree(dev->vpd->attr);
+	}
+
+	return retval;
 }
 
 int __must_check pci_create_sysfs_dev_files (struct pci_dev *pdev)
@@ -1037,6 +1073,7 @@ static void pci_remove_capabilities_sysfs(struct pci_dev *dev)
 	}
 
 	pcie_aspm_remove_sysfs_dev_files(dev);
+	device_remove_file(&dev->dev, &reset_attr);
 }
 
 /**
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index dbd0f94..f6d1c6c 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2260,6 +2260,22 @@ int __pci_reset_function(struct pci_dev *dev)
 EXPORT_SYMBOL_GPL(__pci_reset_function);
 
 /**
+ * pci_probe_reset_function - check whether the device can be safely reset
+ * @dev: PCI device to reset
+ *
+ * Some devices allow an individual function to be reset without affecting
+ * other functions in the same device.  The PCI device must be responsive
+ * to PCI config space in order to use this function.
+ *
+ * Returns 0 if the device function can be reset or negative if the
+ * device doesn't support resetting a single function.
+ */
+int pci_probe_reset_function(struct pci_dev *dev)
+{
+	return pci_dev_reset(dev, 1);
+}
+
+/**
  * pci_reset_function - quiesce and reset a PCI device function
  * @dev: PCI device to reset
  *
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index f73bcbe..60a3811 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -16,6 +16,7 @@ extern void pci_cleanup_rom(struct pci_dev *dev);
 extern int pci_mmap_fits(struct pci_dev *pdev, int resno,
 			 struct vm_area_struct *vma);
 #endif
+int pci_probe_reset_function(struct pci_dev *dev);
 
 /**
  * struct pci_platform_pm_ops - Firmware PM callbacks
-- 
1.6.2.5

Re: [PATCH RFC] pci: expose function reset capability in sysfs

[Greg KH]

On Sun, Jul 26, 2009 at 08:11:39PM +0300, Michael S. Tsirkin wrote:
> Some devices allow an individual function to be reset without affecting
> other functions in the same device: that's what pci_reset_function does.
> For devices that have this support, expose reset attribite in sysfs.

Please add the proper documentation to Documentation/ABI for any new
sysfs file you create.

>  static int pci_create_capabilities_sysfs(struct pci_dev *dev)
>  {
>  	int retval;
> @@ -943,7 +965,21 @@ static int pci_create_capabilities_sysfs(struct pci_dev *dev)
>  	/* Active State Power Management */
>  	pcie_aspm_create_sysfs_dev_files(dev);
>  
> +	if (!pci_probe_reset_function(dev)) {
> +		retval = device_create_file(&dev->dev, &reset_attr);
> +		if (retval)
> +			goto error;
> +	}

So you only add the file if there is a reset function, which is fine,
but later on:

> @@ -1037,6 +1073,7 @@ static void pci_remove_capabilities_sysfs(struct pci_dev *dev)
>  	}
>  
>  	pcie_aspm_remove_sysfs_dev_files(dev);
> +	device_remove_file(&dev->dev, &reset_attr);

You always remove the file, if it has been created or not.  That could
cause problems in the future, please only remove a file if you have
actually added it to the sysfs tree.

thanks,

greg k-h

Re: [PATCH RFC] pci: expose function reset capability in sysfs

[Michael S. Tsirkin]

On Mon, Jul 27, 2009 at 08:01:46AM -0700, Greg KH wrote:
> > +	if (!pci_probe_reset_function(dev)) {
> > +		retval = device_create_file(&dev->dev, &reset_attr);
> > +		if (retval)
> > +			goto error;
> > +	}
> 
> So you only add the file if there is a reset function, which is fine,
> but later on:
> 
> > @@ -1037,6 +1073,7 @@ static void pci_remove_capabilities_sysfs(struct pci_dev *dev)
> >  	}
> >  
> >  	pcie_aspm_remove_sysfs_dev_files(dev);
> > +	device_remove_file(&dev->dev, &reset_attr);
> 
> You always remove the file, if it has been created or not.  That could
> cause problems in the future, please only remove a file if you have
> actually added it to the sysfs tree.
> 
> thanks,
> 
> greg k-h

OK. I think, however, that it's better to avoid probing the function
on cleanup path just to figure out whether the file needs to be removed.
Can this info be stored in struct pci_dev?
Along the lines of:

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 85ebd02..bfb9d21 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -916,6 +916,28 @@ int __attribute__ ((weak)) pcibios_add_platform_entries(struct pci_dev *dev)
 	return 0;
 }
 
+static ssize_t 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;
+
+	/* this can crash the machine when done on the "wrong" device */
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	if (val != 1)
+		return -EINVAL;
+	return pci_reset_function(pdev);
+}
+
+static struct device_attribute reset_attr = __ATTR(reset, 0200, NULL, reset_store);
+
 static int pci_create_capabilities_sysfs(struct pci_dev *dev)
 {
 	int retval;
@@ -943,7 +965,22 @@ static int pci_create_capabilities_sysfs(struct pci_dev *dev)
 	/* Active State Power Management */
 	pcie_aspm_create_sysfs_dev_files(dev);
 
+	if (!pci_probe_reset_function(dev)) {
+		retval = device_create_file(&dev->dev, &reset_attr);
+		if (retval)
+			goto error;
+		dev->reset_fn = 1;
+	}
 	return 0;
+
+error:
+	pcie_aspm_remove_sysfs_dev_files(dev);
+	if (dev->vpd && dev->vpd->attr) {
+		sysfs_remove_bin_file(&dev->dev.kobj, dev->vpd->attr);
+		kfree(dev->vpd->attr);
+	}
+
+	return retval;
 }
 
 int __must_check pci_create_sysfs_dev_files (struct pci_dev *pdev)
@@ -1037,6 +1074,10 @@ static void pci_remove_capabilities_sysfs(struct pci_dev *dev)
 	}
 
 	pcie_aspm_remove_sysfs_dev_files(dev);
+	if (dev->reset_fn) {
+		device_remove_file(&dev->dev, &reset_attr);
+		dev->reset_fn = 0;
+	}
 }
 
 /**
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index dbd0f94..f6d1c6c 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2260,6 +2260,22 @@ int __pci_reset_function(struct pci_dev *dev)
 EXPORT_SYMBOL_GPL(__pci_reset_function);
 
 /**
+ * pci_probe_reset_function - check whether the device can be safely reset
+ * @dev: PCI device to reset
+ *
+ * Some devices allow an individual function to be reset without affecting
+ * other functions in the same device.  The PCI device must be responsive
+ * to PCI config space in order to use this function.
+ *
+ * Returns 0 if the device function can be reset or negative if the
+ * device doesn't support resetting a single function.
+ */
+int pci_probe_reset_function(struct pci_dev *dev)
+{
+	return pci_dev_reset(dev, 1);
+}
+
+/**
  * pci_reset_function - quiesce and reset a PCI device function
  * @dev: PCI device to reset
  *
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index f73bcbe..60a3811 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -16,6 +16,7 @@ extern void pci_cleanup_rom(struct pci_dev *dev);
 extern int pci_mmap_fits(struct pci_dev *pdev, int resno,
 			 struct vm_area_struct *vma);
 #endif
+int pci_probe_reset_function(struct pci_dev *dev);
 
 /**
  * struct pci_platform_pm_ops - Firmware PM callbacks
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 115fb7b..a90f940 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -276,6 +276,7 @@ struct pci_dev {
 	unsigned int	state_saved:1;
 	unsigned int	is_physfn:1;
 	unsigned int	is_virtfn:1;
+	unsigned int	reset_fn:1;
 	pci_dev_flags_t dev_flags;
 	atomic_t	enable_cnt;	/* pci_enable_device has been called */
 

Re: [PATCH RFC] pci: expose function reset capability in sysfs

[Greg KH]

On Mon, Jul 27, 2009 at 06:52:38PM +0300, Michael S. Tsirkin wrote:
> On Mon, Jul 27, 2009 at 08:01:46AM -0700, Greg KH wrote:
> > > +	if (!pci_probe_reset_function(dev)) {
> > > +		retval = device_create_file(&dev->dev, &reset_attr);
> > > +		if (retval)
> > > +			goto error;
> > > +	}
> > 
> > So you only add the file if there is a reset function, which is fine,
> > but later on:
> > 
> > > @@ -1037,6 +1073,7 @@ static void pci_remove_capabilities_sysfs(struct pci_dev *dev)
> > >  	}
> > >  
> > >  	pcie_aspm_remove_sysfs_dev_files(dev);
> > > +	device_remove_file(&dev->dev, &reset_attr);
> > 
> > You always remove the file, if it has been created or not.  That could
> > cause problems in the future, please only remove a file if you have
> > actually added it to the sysfs tree.
> > 
> > thanks,
> > 
> > greg k-h
> 
> OK. I think, however, that it's better to avoid probing the function
> on cleanup path just to figure out whether the file needs to be removed.
> Can this info be stored in struct pci_dev?
> Along the lines of:

Fine with me.  You forgot the documentation though :)

thanks,

greg k-h

Re: [PATCH RFC] pci: expose function reset capability in sysfs

[Jesse Barnes]

On Mon, 27 Jul 2009 09:14:23 -0700
Greg KH <greg@kroah.com> wrote:

> On Mon, Jul 27, 2009 at 06:52:38PM +0300, Michael S. Tsirkin wrote:
> > On Mon, Jul 27, 2009 at 08:01:46AM -0700, Greg KH wrote:
> > > > +	if (!pci_probe_reset_function(dev)) {
> > > > +		retval = device_create_file(&dev->dev,
> > > > &reset_attr);
> > > > +		if (retval)
> > > > +			goto error;
> > > > +	}
> > > 
> > > So you only add the file if there is a reset function, which is
> > > fine, but later on:
> > > 
> > > > @@ -1037,6 +1073,7 @@ static void
> > > > pci_remove_capabilities_sysfs(struct pci_dev *dev) }
> > > >  
> > > >  	pcie_aspm_remove_sysfs_dev_files(dev);
> > > > +	device_remove_file(&dev->dev, &reset_attr);
> > > 
> > > You always remove the file, if it has been created or not.  That
> > > could cause problems in the future, please only remove a file if
> > > you have actually added it to the sysfs tree.
> > > 
> > > thanks,
> > > 
> > > greg k-h
> > 
> > OK. I think, however, that it's better to avoid probing the function
> > on cleanup path just to figure out whether the file needs to be
> > removed. Can this info be stored in struct pci_dev?
> > Along the lines of:
> 
> Fine with me.  You forgot the documentation though :)

Aside from vague worries about encouraging more userland drivers, this
seems like a reasonable feature to me (and certainly no more
encouraging than our current set of config and resource files).

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH RFC] pci: expose function reset capability in sysfs

[Michael S. Tsirkin]

On Mon, Jul 27, 2009 at 09:14:23AM -0700, Greg KH wrote:
> Fine with me.  You forgot the documentation though :)

This enough?

pci: expose function reset capability in sysfs

Some devices allow an individual function to be reset without affecting
other functions in the same device: that's what pci_reset_function does.
For devices that have this support, expose reset attribite in sysfs.

This is useful e.g. for virtualization, where a qemu userspace
process wants to reset the device when the guest is reset,
to emulate machine reboot as closely as possible.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
index 6bf6805..bc52035 100644
--- a/Documentation/ABI/testing/sysfs-bus-pci
+++ b/Documentation/ABI/testing/sysfs-bus-pci
@@ -84,6 +84,16 @@ Description:
 		from this part of the device tree.
 		Depends on CONFIG_HOTPLUG.
 
+What:		/sys/bus/pci/devices/.../reset
+Date:		July 2009
+Contact:	Michael S. Tsirkin <mst@redhat.com>
+Description:
+		Some devices allow an individual function to be reset
+		without affecting other functions in the same device.
+		For devices that have this support, a file named reset
+		will be present in sysfs.  Writing 1 to this file
+		will perform reset.
+
 What:		/sys/bus/pci/devices/.../vpd
 Date:		February 2008
 Contact:	Ben Hutchings <bhutchings@solarflare.com>
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 85ebd02..bfb9d21 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -916,6 +916,24 @@ int __attribute__ ((weak)) pcibios_add_platform_entries(struct pci_dev *dev)
 	return 0;
 }
 
+static ssize_t 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;
+	return pci_reset_function(pdev);
+}
+
+static struct device_attribute reset_attr = __ATTR(reset, 0200, NULL, reset_store);
+
 static int pci_create_capabilities_sysfs(struct pci_dev *dev)
 {
 	int retval;
@@ -943,7 +965,22 @@ static int pci_create_capabilities_sysfs(struct pci_dev *dev)
 	/* Active State Power Management */
 	pcie_aspm_create_sysfs_dev_files(dev);
 
+	if (!pci_probe_reset_function(dev)) {
+		retval = device_create_file(&dev->dev, &reset_attr);
+		if (retval)
+			goto error;
+		dev->reset_fn = 1;
+	}
 	return 0;
+
+error:
+	pcie_aspm_remove_sysfs_dev_files(dev);
+	if (dev->vpd && dev->vpd->attr) {
+		sysfs_remove_bin_file(&dev->dev.kobj, dev->vpd->attr);
+		kfree(dev->vpd->attr);
+	}
+
+	return retval;
 }
 
 int __must_check pci_create_sysfs_dev_files (struct pci_dev *pdev)
@@ -1037,6 +1074,10 @@ static void pci_remove_capabilities_sysfs(struct pci_dev *dev)
 	}
 
 	pcie_aspm_remove_sysfs_dev_files(dev);
+	if (dev->reset_fn) {
+		device_remove_file(&dev->dev, &reset_attr);
+		dev->reset_fn = 0;
+	}
 }
 
 /**
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index dbd0f94..f6d1c6c 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2260,6 +2260,22 @@ int __pci_reset_function(struct pci_dev *dev)
 EXPORT_SYMBOL_GPL(__pci_reset_function);
 
 /**
+ * pci_probe_reset_function - check whether the device can be safely reset
+ * @dev: PCI device to reset
+ *
+ * Some devices allow an individual function to be reset without affecting
+ * other functions in the same device.  The PCI device must be responsive
+ * to PCI config space in order to use this function.
+ *
+ * Returns 0 if the device function can be reset or negative if the
+ * device doesn't support resetting a single function.
+ */
+int pci_probe_reset_function(struct pci_dev *dev)
+{
+	return pci_dev_reset(dev, 1);
+}
+
+/**
  * pci_reset_function - quiesce and reset a PCI device function
  * @dev: PCI device to reset
  *
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index f73bcbe..60a3811 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -16,6 +16,7 @@ extern void pci_cleanup_rom(struct pci_dev *dev);
 extern int pci_mmap_fits(struct pci_dev *pdev, int resno,
 			 struct vm_area_struct *vma);
 #endif
+int pci_probe_reset_function(struct pci_dev *dev);
 
 /**
  * struct pci_platform_pm_ops - Firmware PM callbacks
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 115fb7b..a90f940 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -276,6 +276,7 @@ struct pci_dev {
 	unsigned int	state_saved:1;
 	unsigned int	is_physfn:1;
 	unsigned int	is_virtfn:1;
+	unsigned int	reset_fn:1;
 	pci_dev_flags_t dev_flags;
 	atomic_t	enable_cnt;	/* pci_enable_device has been called */
 

Re: [PATCH RFC] pci: expose function reset capability in sysfs

[Greg KH]

On Mon, Jul 27, 2009 at 11:37:48PM +0300, Michael S. Tsirkin wrote:
> On Mon, Jul 27, 2009 at 09:14:23AM -0700, Greg KH wrote:
> > Fine with me.  You forgot the documentation though :)
> 
> This enough?
> 
> pci: expose function reset capability in sysfs
> 
> Some devices allow an individual function to be reset without affecting
> other functions in the same device: that's what pci_reset_function does.
> For devices that have this support, expose reset attribite in sysfs.
> 
> This is useful e.g. for virtualization, where a qemu userspace
> process wants to reset the device when the guest is reset,
> to emulate machine reboot as closely as possible.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Looks good to me:
	Acked-by: Greg Kroah-Hartman <gregkh@suse.de>

Re: [PATCH RFC] pci: expose function reset capability in sysfs

[Jesse Barnes]

On Mon, 27 Jul 2009 23:37:48 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Jul 27, 2009 at 09:14:23AM -0700, Greg KH wrote:
> > Fine with me.  You forgot the documentation though :)
> 
> This enough?
> 
> pci: expose function reset capability in sysfs
> 
> Some devices allow an individual function to be reset without
> affecting other functions in the same device: that's what
> pci_reset_function does. For devices that have this support, expose
> reset attribite in sysfs.
> 
> This is useful e.g. for virtualization, where a qemu userspace
> process wants to reset the device when the guest is reset,
> to emulate machine reboot as closely as possible.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Applied to my linux-next branch, thanks.

-- 
Jesse Barnes, Intel Open Source Technology Center