pci: expose function reset capability in sysfs

RFC PATCH Applied Michael S. Tsirkin 4 participants 8 messages Jul 26, 2009 – Jul 28, 2009

Subsystems (2)

KVM PCI

lore

Share & tools

lore

Michael S. Tsirkin

author

2009-07-26 17:11 UTC #0 lore raw

RFC PATCH 1/1 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 started/reset, to emulate machine reboot as closely as possible.

Signed-off-by Michael S. Tsirkin <[email protected]>

---

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(-)

drivers/pci/pci-sysfs.c +37 -0

index 85ebd02..92805e8 100644
@@ -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);
 }
 
 /**

drivers/pci/pci.c +16 -0

index dbd0f94..f6d1c6c 100644
@@ -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
  *

drivers/pci/pci.h +1 -0

index f73bcbe..60a3811 100644
@@ -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

Signature · 1 line

1.6.2.5

Greg KH

2009-07-27 15:02 UTC #1 lore raw

Quoted 3 lines · 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.

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

drivers/pci/pci-sysfs.c static int pci_create_capabilities_sysfs(struct pci_dev *dev) · line 972

Quoted 12 lines

static int pci_create_capabilities_sysfs(struct pci_dev *dev) {

drivers/pci/pci-sysfs.c +5 -0

 	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:

drivers/pci/pci-sysfs.c static void pci_remove_capabilities_sysfs(struct pci_dev *dev) · line 1076

Quoted 5 lines

drivers/pci/pci-sysfs.c +1 -0

@@ -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

Michael S. Tsirkin

author

2009-07-27 15:53 UTC #2 lore raw

Quoted 19 lines · Greg KH

Review
Quoted 5 lines
(quoted diff) +5 -0
+	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:
Review static void pci_remove_capabilities_sysfs(struct pci_dev *dev) · line 1076
Quoted 5 lines
(quoted diff) +1 -0
@@ -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.

Quoted 3 lines

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:

drivers/pci/pci-sysfs.c +41 -0

index 85ebd02..bfb9d21 100644
@@ -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;
+	}
 }
 
 /**

drivers/pci/pci.c +16 -0

index dbd0f94..f6d1c6c 100644
@@ -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
  *

drivers/pci/pci.h +1 -0

index f73bcbe..60a3811 100644
@@ -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

include/linux/pci.h +1 -0

index 115fb7b..a90f940 100644
@@ -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 */

Greg KH

2009-07-27 16:15 UTC #3 lore raw

Quoted 20 lines · Michael S. Tsirkin

Quoted 19 lines · Greg KH
drivers/pci/pci-sysfs.c static int pci_create_capabilities_sysfs(struct pci_dev *dev) · line 973
Quoted 5 lines
drivers/pci/pci-sysfs.c +5 -0
+	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:
drivers/pci/pci-sysfs.c static void pci_remove_capabilities_sysfs(struct pci_dev *dev) · line 1076
Quoted 5 lines
drivers/pci/pci-sysfs.c +1 -0
@@ -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.

Quoted 8 lines

Quoted 3 lines
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

Jesse Barnes

2009-07-27 16:20 UTC #4 lore raw

Quoted 24 lines · On Mon, 27 Jul 2009 09:14:23 -0700 Greg KH <[email protected]> wrote:

Quoted 23 lines · Michael S. Tsirkin
Quoted 22 lines · Greg KH
drivers/pci/pci-sysfs.c static int pci_create_capabilities_sysfs(struct pci_dev *dev) · line 973
Quoted 6 lines
(quoted diff) +2 -0
+	if (!pci_probe_reset_function(dev)) {
+		retval = device_create_file(&dev->dev,
&reset_attr);
drivers/pci/pci-sysfs.c +3 -0
+		if (retval)
+			goto error;
+	}
So you only add the file if there is a reset function, which is fine, but later on:
drivers/pci/pci-sysfs.c static void · line 1076
Quoted 5 lines
drivers/pci/pci-sysfs.c +1 -0
@@ -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,

Quoted 8 lines

Quoted 6 lines
Quoted 1 line
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).

Signature · 1 line

Jesse Barnes, Intel Open Source Technology Center

Michael S. Tsirkin

author

2009-07-27 20:38 UTC #5 lore raw

In reply to Greg KH

Quoted 1 line · Greg KH

Fine with me. You forgot the documentation though :)

This enough?

pci: expose function reset capability 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 <[email protected]>

Documentation/ABI/testing/sysfs-bus-pci +10 -0

index 6bf6805..bc52035 100644
@@ -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 <[email protected]>
+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 <[email protected]>

drivers/pci/pci-sysfs.c +37 -0

index 85ebd02..bfb9d21 100644
@@ -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;
+	}
 }
 
 /**

drivers/pci/pci.c +16 -0

index dbd0f94..f6d1c6c 100644
@@ -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
  *

drivers/pci/pci.h +1 -0

index f73bcbe..60a3811 100644
@@ -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

include/linux/pci.h +1 -0

index 115fb7b..a90f940 100644
@@ -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 */

Greg KH

Acked-by Greg Kroah-Hartman

2009-07-27 21:03 UTC #6 lore raw

Quoted 11 lines · Michael S. Tsirkin

Quoted 1 line · Greg KH
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.

Quoted 5 lines

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 <[email protected]>

Looks good to me:

Acked-by Greg Kroah-Hartman <[email protected]>

Jesse Barnes

Applied

2009-07-28 16:57 UTC #7 lore raw

In reply to Michael S. Tsirkin

Quoted 12 lines · On Mon, 27 Jul 2009 23:37:48 +0300 "Michael S. Tsirkin" <[email protected]> wrote:

Quoted 1 line · Greg KH
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.

Quoted 5 lines

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 <[email protected]>

Applied to my linux-next branch, thanks.

Signature · 1 line

Jesse Barnes, Intel Open Source Technology Center