From patchwork Mon Dec 17 23:24:39 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Bjorn Helgaas X-Patchwork-Id: 1889211 X-Patchwork-Delegate: bhelgaas@google.com Return-Path: X-Original-To: patchwork-linux-pci@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork1.kernel.org (Postfix) with ESMTP id C35FA3FCA5 for ; Mon, 17 Dec 2012 23:24:41 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753865Ab2LQXYl (ORCPT ); Mon, 17 Dec 2012 18:24:41 -0500 Received: from mail-yh0-f74.google.com ([209.85.213.74]:59837 "EHLO mail-yh0-f74.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753862Ab2LQXYk (ORCPT ); Mon, 17 Dec 2012 18:24:40 -0500 Received: by mail-yh0-f74.google.com with SMTP id 10so711963yhl.5 for ; Mon, 17 Dec 2012 15:24:39 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=x-received:date:from:to:cc:subject:message-id:references :mime-version:content-type:content-disposition:in-reply-to :user-agent; bh=wRDsRsG3CJPIQZk1lIWBxtoGOD/bYO9BYrJplrypKj8=; b=U+CPPI9qMn9dC+LImtjehoIqhtBGyVthKdY7dQD62a73ryy+w+9jBjx6KyqH9bPslm vKK9uNklz9f6Ck6oeTseJe46a4K+PSYCJdfP3KMdLgss9waPTUlpLDlQhE6N4WW76x+x Q/zNbvWQ8AwqvY5rDlPLCAA3jwOrfBvFb21YYiUUQDC1rjKUH7IdhZ56mj1uKquCuiIr RDJLXGvgU5LTxfWl3KUpCPhoSmnxr6ujl9USQso29w2KmzR39nn0zwqKO5B5LdoSzof4 pv2w4mgsfa9hg1qIDMnZlKWBLwdi0BNtClDER+tfEUI69N+MFKZ7MeGHVeUUKZGTgBcG Xe5w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=x-received:date:from:to:cc:subject:message-id:references :mime-version:content-type:content-disposition:in-reply-to :user-agent:x-gm-message-state; bh=wRDsRsG3CJPIQZk1lIWBxtoGOD/bYO9BYrJplrypKj8=; b=Kyk3789nw8J/OYbuTNEZgOdWARh5o2wd6xgRf889eEpU6lNXPqD56Rop6byeBzk0Zw AbYQA1uE34ts7W+fe4SAiofYjGWIRpcDPmidqawC25yt4MkiCRtft7npAnMvEn5jnWuX v9wHzq8UWvEw9tU94XMzgLZGBRnO5sjrpUAmYT1plSkRkCPNZwD/kSl2nMeDSmpfsynM hQm5mObce8JWCwNTrKtbFbFTWv2FY9x2OJ1DSIC2R16O0Vece5s3Q5p5Ti5dCMMtS7K8 kKSc1ljfzx7M4S4RWp3N+4I09curxdUaW1NdoeZ4mKGe668g7ubiVKIoK1HDUYErYc5j KmJQ== X-Received: by 10.236.127.145 with SMTP id d17mr2294yhi.28.1355786679823; Mon, 17 Dec 2012 15:24:39 -0800 (PST) Received: from wpzn3.hot.corp.google.com (216-239-44-65.google.com [216.239.44.65]) by gmr-mx.google.com with ESMTPS id r6si1316095yhc.7.2012.12.17.15.24.39 (version=TLSv1/SSLv3 cipher=AES128-SHA); Mon, 17 Dec 2012 15:24:39 -0800 (PST) Received: from bhelgaas.mtv.corp.google.com (bhelgaas.mtv.corp.google.com [172.17.131.112]) by wpzn3.hot.corp.google.com (Postfix) with ESMTP id 96607100047; Mon, 17 Dec 2012 15:24:39 -0800 (PST) Received: by bhelgaas.mtv.corp.google.com (Postfix, from userid 131485) id 408B8180283; Mon, 17 Dec 2012 15:24:39 -0800 (PST) Date: Mon, 17 Dec 2012 16:24:39 -0700 From: Bjorn Helgaas To: Don Dutile Cc: Greg Rose , "linux-pci@vger.kernel.org" , "yuvalmin@broadcom.com" , "bhutchings@solarflare.com" , "yinghai@kernel.org" , "davem@davemloft.net" Subject: Re: [PATCH v2] PCI SRIOV device enable and disable via sysfs Message-ID: <20121217232439.GA9746@google.com> References: <1352146841-64458-1-git-send-email-ddutile@redhat.com> <20121214101911.00002f59@unknown> <50CF7993.9040606@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <50CF7993.9040606@redhat.com> User-Agent: Mutt/1.5.20 (2009-06-14) X-Gm-Message-State: ALoCoQk5B6pQ41OBDsysBmAALIUr7Hzr43iiHCTAmJNw2P2E1UZmcUtoe+xBM6/nJQnjHl4jm0mNXsI4AiEWyze1/FneoLuw9kdM1R+l5hWPdxdtbxABL6XCVP9AjEIVjVJKmtBAcOppvSyu4bfjH/AYHOrg/YAXftLDGyf4jMJuNQ5lqEh/g2H0qNyr6jDhc5EHMeyeWvHqSm0rowEqwiTaZOsZdbybRQ== Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org On Mon, Dec 17, 2012 at 02:59:15PM -0500, Don Dutile wrote: > On 12/14/2012 01:19 PM, Greg Rose wrote: > >pci: Fix return code > > > >From: Greg Rose > > > >The return code from the sriov configure function was only returned if it > >was less than zero indicating an error. This caused the code to fall > >through to the default return of an error code even though the sriov > >configure function has returned the number of VFs it created - a positive > >number indicating success. > > > > > >Signed-off-by: Greg Rose > > Actually, it returned the number of VFs enabled if it exactly equalled > the number to be enabled. Otherwise, the basic testing would have failed. > If the number of vf's enabled was positive but not the same > as the number requested-to-be-enabled, then it incorrectly returned. > > But, the patch corrects the base problem, so > Acked-by: Donald Dutile Alternate proposal below. The patch is ugly; it might be easier to compare the before (http://pastebin.com/zneG8AuD) and after (http://pastebin.com/BEXEE8kc) versions. commit fae71e5dc9064dcb3077b8da6e0ed9d800c1f527 Author: Bjorn Helgaas Date: Thu Dec 13 20:22:44 2012 -0700 PCI: Cleanup sriov_numvfs_show() and handle common case without error If we request "num_vfs" and the driver's sriov_configure() method enables exactly that number ("num_vfs_enabled"), we complain "Invalid value for number of VFs to enable" and return an error. We should silently return success instead. Also, use kstrtou16() since numVFs is defined to be a 16-bit field and rework to simplify control flow. Reported-by: Greg Rose Link: http://lkml.kernel.org/r/20121214101911.00002f59@unknown Signed-off-by: Bjorn Helgaas Tested-by: Donald Dutile --- To unsubscribe from this list: send the line "unsubscribe linux-pci" 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 05b78b1..5e8af12 100644 --- a/drivers/pci/pci-sysfs.c +++ b/drivers/pci/pci-sysfs.c @@ -422,77 +422,60 @@ static ssize_t sriov_numvfs_show(struct device *dev, } /* - * num_vfs > 0; number of vfs to enable - * num_vfs = 0; disable all vfs + * num_vfs > 0; number of VFs to enable + * num_vfs = 0; disable all VFs * * Note: SRIOV spec doesn't allow partial VF - * disable, so its all or none. + * disable, so it's all or none. */ static ssize_t sriov_numvfs_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) { struct pci_dev *pdev = to_pci_dev(dev); - int num_vfs_enabled = 0; - int num_vfs; - int ret = 0; - u16 total; + int ret; + u16 num_vfs; - if (kstrtoint(buf, 0, &num_vfs) < 0) - return -EINVAL; + ret = kstrtou16(buf, 0, &num_vfs); + if (ret < 0) + return ret; + + if (num_vfs > pci_sriov_get_totalvfs(pdev)) + return -ERANGE; + + if (num_vfs == pdev->sriov->num_VFs) + return count; /* no change */ /* 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"); + dev_info(&pdev->dev, "Driver doesn't support SRIOV configuration via sysfs\n"); return -ENOSYS; } - /* if enabling vf's ... */ - total = pci_sriov_get_totalvfs(pdev); - /* Requested VFs to enable < totalvfs and none enabled already */ - if ((num_vfs > 0) && (num_vfs <= total)) { - if (pdev->sriov->num_VFs == 0) { - num_vfs_enabled = - pdev->driver->sriov_configure(pdev, num_vfs); - if ((num_vfs_enabled >= 0) && - (num_vfs_enabled != num_vfs)) { - dev_warn(&pdev->dev, - "Only %d VFs enabled\n", - num_vfs_enabled); - return count; - } else if (num_vfs_enabled < 0) - /* error code from driver callback */ - return num_vfs_enabled; - } else if (num_vfs == pdev->sriov->num_VFs) { - dev_warn(&pdev->dev, - "%d VFs already enabled; no enable action taken\n", - num_vfs); - return count; - } else { - dev_warn(&pdev->dev, - "%d VFs already enabled. Disable before enabling %d VFs\n", - pdev->sriov->num_VFs, num_vfs); - return -EINVAL; - } + if (num_vfs == 0) { + /* disable VFs */ + ret = pdev->driver->sriov_configure(pdev, 0); + if (ret < 0) + return ret; + return count; } - /* disable vfs */ - if (num_vfs == 0) { - if (pdev->sriov->num_VFs != 0) { - ret = pdev->driver->sriov_configure(pdev, 0); - return ret ? ret : count; - } else { - dev_warn(&pdev->dev, - "All VFs disabled; no disable action taken\n"); - return count; - } + /* enable VFs */ + if (pdev->sriov->num_VFs) { + dev_warn(&pdev->dev, "%d VFs already enabled. Disable before enabling %d VFs\n", + pdev->sriov->num_VFs, num_vfs); + return -EINVAL; } - dev_err(&pdev->dev, - "Invalid value for number of VFs to enable: %d\n", num_vfs); + ret = pdev->driver->sriov_configure(pdev, num_vfs); + if (ret < 0) + return ret; - return -EINVAL; + if (ret != num_vfs) + dev_warn(&pdev->dev, "%d VFs requested; only %d enabled\n", + num_vfs, ret); + + return count; } static struct device_attribute sriov_totalvfs_attr = __ATTR_RO(sriov_totalvfs);