From patchwork Mon Jul 27 15:52:38 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Michael S. Tsirkin" X-Patchwork-Id: 37565 Received: from vger.kernel.org (vger.kernel.org [209.132.176.167]) by demeter.kernel.org (8.14.2/8.14.2) with ESMTP id n6RFsN7G026570 for ; Mon, 27 Jul 2009 15:54:24 GMT Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752447AbZG0Pxl (ORCPT ); Mon, 27 Jul 2009 11:53:41 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1752714AbZG0Pxk (ORCPT ); Mon, 27 Jul 2009 11:53:40 -0400 Received: from mx2.redhat.com ([66.187.237.31]:40012 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752111AbZG0Pxj (ORCPT ); Mon, 27 Jul 2009 11:53:39 -0400 Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26]) by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id n6RFrddh023941; Mon, 27 Jul 2009 11:53:39 -0400 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx2.corp.redhat.com (8.13.1/8.13.1) with ESMTP id n6RFrcr5032755; Mon, 27 Jul 2009 11:53:39 -0400 Received: from redhat.com (vpn-6-222.tlv.redhat.com [10.35.6.222]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id n6RFrYfe002794; Mon, 27 Jul 2009 11:53:35 -0400 Date: Mon, 27 Jul 2009 18:52:38 +0300 From: "Michael S. Tsirkin" To: Greg KH Cc: jbarnes@virtuousgeek.org, linux-pci@vger.kernel.org, kvm@vger.kernel.org Subject: Re: [PATCH RFC] pci: expose function reset capability in sysfs Message-ID: <20090727155238.GA29853@redhat.com> References: <20090726171139.GA22202@redhat.com> <20090727150146.GA19079@kroah.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20090727150146.GA19079@kroah.com> User-Agent: Mutt/1.5.19 (2009-01-05) X-Scanned-By: MIMEDefang 2.58 on 172.16.27.26 Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org 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: --- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html 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 */