From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759704AbdAFX26 convert rfc822-to-8bit (ORCPT ); Fri, 6 Jan 2017 18:28:58 -0500 Received: from mga05.intel.com ([192.55.52.43]:58002 "EHLO mga05.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751203AbdAFX2w (ORCPT ); Fri, 6 Jan 2017 18:28:52 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,326,1477983600"; d="scan'208";a="50418569" From: "Tantilov, Emil S" To: Greg CC: "linux-pci@vger.kernel.org" , "intel-wired-lan@lists.osuosl.org" , "linux-kernel@vger.kernel.org" , "netdev@vger.kernel.org" , Gavin Shan Subject: RE: [Intel-wired-lan] [PATCH v2] PCI: lock each enable/disable num_vfs operation in sysfs Thread-Topic: [Intel-wired-lan] [PATCH v2] PCI: lock each enable/disable num_vfs operation in sysfs Thread-Index: AQHSaHMacvLDIcl1RECZHNbZeG5yjaEsFuJg Date: Fri, 6 Jan 2017 23:28:51 +0000 Message-ID: <87618083B2453E4A8714035B62D6799250744DCD@FMSMSX105.amr.corp.intel.com> References: <20170106215908.20736.34632.stgit@localhost6.localdomain6> <1483744662.3868.1.camel@gmail.com> In-Reply-To: <1483744662.3868.1.camel@gmail.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMzI3ZDgxYzktZGEyOS00Y2VjLTgwYTQtNWE5ZmU0YTU0OTkxIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjIuMTEuMCIsIlRydXN0ZWRMYWJlbEhhc2giOiJCUTV5MGRSeVBnUWRLQ1gxa3hENGplR2s1ZFZcL0I5V3RVSUp4ZmtQU01QQT0ifQ== x-ctpclassification: CTP_IC x-originating-ip: [10.1.200.106] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org >-----Original Message----- >From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On >Behalf Of Greg >Sent: Friday, January 06, 2017 3:18 PM >To: Tantilov, Emil S >Cc: linux-pci@vger.kernel.org; intel-wired-lan@lists.osuosl.org; linux- >kernel@vger.kernel.org; netdev@vger.kernel.org >Subject: Re: [Intel-wired-lan] [PATCH v2] PCI: lock each enable/disable >num_vfs operation in sysfs > >On Fri, 2017-01-06 at 13:59 -0800, Emil Tantilov wrote: >> Enabling/disabling SRIOV via sysfs by echo-ing multiple values >> simultaneously: >> >> echo 63 > /sys/class/net/ethX/device/sriov_numvfs& >> echo 63 > /sys/class/net/ethX/device/sriov_numvfs >> >> sleep 5 >> >> echo 0 > /sys/class/net/ethX/device/sriov_numvfs& >> echo 0 > /sys/class/net/ethX/device/sriov_numvfs >> >> Results in the following bug: >> >> kernel BUG at drivers/pci/iov.c:495! >> invalid opcode: 0000 [#1] SMP >> CPU: 1 PID: 8050 Comm: bash Tainted: G W 4.9.0-rc7-net-next #2092 >> RIP: 0010:[] >> [] pci_iov_release+0x57/0x60 >> >> Call Trace: >> [] pci_release_dev+0x26/0x70 >> [] device_release+0x3e/0xb0 >> [] kobject_cleanup+0x67/0x180 >> [] kobject_put+0x2d/0x60 >> [] put_device+0x17/0x20 >> [] pci_dev_put+0x1a/0x20 >> [] pci_get_dev_by_id+0x5b/0x90 >> [] pci_get_subsys+0x35/0x40 >> [] pci_get_device+0x18/0x20 >> [] pci_get_domain_bus_and_slot+0x2b/0x60 >> [] pci_iov_remove_virtfn+0x57/0x180 >> [] pci_disable_sriov+0x65/0x140 >> [] ixgbe_disable_sriov+0xc7/0x1d0 [ixgbe] >> [] ixgbe_pci_sriov_configure+0x3d/0x170 [ixgbe] >> [] sriov_numvfs_store+0xdc/0x130 >> ... >> RIP [] pci_iov_release+0x57/0x60 >> >> Use the existing mutex lock to protect each enable/disable operation. >> >> -v2: move the existing lock from protecting the config of the IOV bus >> to protecting the writes to sriov_numvfs in sysfs without maintaining >> a "locked" version of pci_iov_add/remove_virtfn(). >> As suggested by Gavin Shan >> >> CC: Alexander Duyck >> Signed-off-by: Emil Tantilov >> --- >> drivers/pci/iov.c | 7 ------- >> drivers/pci/pci-sysfs.c | 23 ++++++++++++++++------- >> drivers/pci/pci.h | 2 +- >> 3 files changed, 17 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c >> index 4722782..2479ae8 100644 >> --- a/drivers/pci/iov.c >> +++ b/drivers/pci/iov.c >> @@ -124,7 +124,6 @@ int pci_iov_add_virtfn(struct pci_dev *dev, int id, >int reset) >> struct pci_sriov *iov = dev->sriov; >> struct pci_bus *bus; >> >> - mutex_lock(&iov->dev->sriov->lock); >> bus = virtfn_add_bus(dev->bus, pci_iov_virtfn_bus(dev, id)); >> if (!bus) >> goto failed; >> @@ -162,7 +161,6 @@ int pci_iov_add_virtfn(struct pci_dev *dev, int id, >int reset) >> __pci_reset_function(virtfn); >> >> pci_device_add(virtfn, virtfn->bus); >> - mutex_unlock(&iov->dev->sriov->lock); >> >> pci_bus_add_device(virtfn); >> sprintf(buf, "virtfn%u", id); >> @@ -181,12 +179,10 @@ int pci_iov_add_virtfn(struct pci_dev *dev, int id, >int reset) >> sysfs_remove_link(&dev->dev.kobj, buf); >> failed1: >> pci_dev_put(dev); >> - mutex_lock(&iov->dev->sriov->lock); >> pci_stop_and_remove_bus_device(virtfn); >> failed0: >> virtfn_remove_bus(dev->bus, bus); >> failed: >> - mutex_unlock(&iov->dev->sriov->lock); >> >> return rc; >> } >> @@ -195,7 +191,6 @@ void pci_iov_remove_virtfn(struct pci_dev *dev, int >id, int reset) >> { >> char buf[VIRTFN_ID_LEN]; >> struct pci_dev *virtfn; >> - struct pci_sriov *iov = dev->sriov; >> >> virtfn = pci_get_domain_bus_and_slot(pci_domain_nr(dev->bus), >> pci_iov_virtfn_bus(dev, id), >> @@ -218,10 +213,8 @@ void pci_iov_remove_virtfn(struct pci_dev *dev, int >id, int reset) >> if (virtfn->dev.kobj.sd) >> sysfs_remove_link(&virtfn->dev.kobj, "physfn"); >> >> - mutex_lock(&iov->dev->sriov->lock); >> pci_stop_and_remove_bus_device(virtfn); >> virtfn_remove_bus(dev->bus, virtfn->bus); >> - mutex_unlock(&iov->dev->sriov->lock); >> >> /* balance pci_get_domain_bus_and_slot() */ >> pci_dev_put(virtfn); >> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c >> index 0666287..25d010d 100644 >> --- a/drivers/pci/pci-sysfs.c >> +++ b/drivers/pci/pci-sysfs.c >> @@ -472,6 +472,7 @@ static ssize_t sriov_numvfs_store(struct device *dev, >> const char *buf, size_t count) >> { >> struct pci_dev *pdev = to_pci_dev(dev); >> + struct pci_sriov *iov = pdev->sriov; >> int ret; >> u16 num_vfs; >> >> @@ -482,38 +483,46 @@ static ssize_t sriov_numvfs_store(struct device >*dev, >> if (num_vfs > pci_sriov_get_totalvfs(pdev)) >> return -ERANGE; >> >> + mutex_lock(&iov->dev->sriov->lock); >> + >> if (num_vfs == pdev->sriov->num_VFs) >> - return count; /* no change */ >> + goto exit; >> >> /* is PF driver loaded w/callback */ >> if (!pdev->driver || !pdev->driver->sriov_configure) { >> dev_info(&pdev->dev, "Driver doesn't support SRIOV >configuration via sysfs\n"); >> - return -ENOSYS; >> + ret = -ENOENT; >> + goto exit; > >Hi Emil, > >Generally the patch looks fine to me - but I am wondering why the change >from -ENOSYS TO -ENOENT? Because checkpatch was complaining that ENOSYS should only be used for "invalid syscall nr". If you look at the discussion of the previous version of the patch you can see the details there. Basically Gavin preferred ENOENT and I don't really care so much (I kind of ran into this by accident as this patch does not need to change the error code). Thanks, Emil