From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932592AbdE0BUi (ORCPT ); Fri, 26 May 2017 21:20:38 -0400 Received: from mail.kernel.org ([198.145.29.99]:56848 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1948854AbdEZXr3 (ORCPT ); Fri, 26 May 2017 19:47:29 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4B71C23958 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=helgaas@kernel.org Date: Fri, 26 May 2017 18:47:26 -0500 From: Bjorn Helgaas To: Jakub Kicinski Cc: Bjorn Helgaas , linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, Greg Kroah-Hartman , Emil Tantilov , Alexander Duyck , oss-drivers@netronome.com Subject: Re: [PATCH] pci: iov: use device lock to protect IOV sysfs accesses Message-ID: <20170526234726.GD21531@bhelgaas-glaptop.roam.corp.google.com> References: <20170522225023.14010-1-jakub.kicinski@netronome.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170522225023.14010-1-jakub.kicinski@netronome.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Jakub, On Mon, May 22, 2017 at 03:50:23PM -0700, Jakub Kicinski wrote: > PCI core sets the driver pointer before calling ->probe() and only > clears it after ->remove(). This means driver's ->sriov_configure() > callback will happily race with probe() and remove(), most likely > leading to BUGs, since drivers don't expect this. I guess you're referring to the pci_dev->driver pointer set by local_pci_probe(), and this is important because sriov_numvfs_store() checks that pointer, right? I was trying to make sure there weren't other similar problems elsewhere. But I don't see any other sysfs functions that use pci_dev->driver in that way, so I think this is the only one. I think this looks good. pci_bus_add_devices pci_bus_add_device pci_create_sysfs_dev_files device_attach __device_attach device_lock(dev) __device_attach_driver ... local_pci_probe pci_dev->driver = pci_drv <--- set pci_drv->probe() device_unlock(dev) sriov_numvfs_store - mutex_lock(&iov->dev->sriov->lock) + device_lock(&pdev->dev) if (pdev->driver && pdev->driver->sriov_configure) <--- test pdev->driver->sriov_configure - mutex_unlock(&iov->dev->sriov->lock) + device_unlock(&pdev->dev) > We could reorder pointer assignments, or try detecting races in all > drivers, but it seems simpler and cleaner to just hold the device lock > instead of special SR-IOV lock, since that lock is already supposed > to synchronize the driver callbacks. > > Remove the iov lock completely, since we remove the last user. > > Signed-off-by: Jakub Kicinski > --- > drivers/pci/iov.c | 4 ---- > drivers/pci/pci-sysfs.c | 5 ++--- > drivers/pci/pci.h | 1 - > 3 files changed, 2 insertions(+), 8 deletions(-) > > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c > index d9dc7363ac77..120485d6f352 100644 > --- a/drivers/pci/iov.c > +++ b/drivers/pci/iov.c > @@ -461,8 +461,6 @@ static int sriov_init(struct pci_dev *dev, int pos) > else > iov->dev = dev; > > - mutex_init(&iov->lock); > - > dev->sriov = iov; > dev->is_physfn = 1; > rc = compute_max_vf_buses(dev); > @@ -491,8 +489,6 @@ static void sriov_release(struct pci_dev *dev) > if (dev != dev->sriov->dev) > pci_dev_put(dev->sriov->dev); > > - mutex_destroy(&dev->sriov->lock); > - > kfree(dev->sriov); > dev->sriov = NULL; > } > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c > index 31e99613a12e..7755559558df 100644 > --- a/drivers/pci/pci-sysfs.c > +++ b/drivers/pci/pci-sysfs.c > @@ -472,7 +472,6 @@ 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; > > @@ -483,7 +482,7 @@ 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); > + device_lock(&pdev->dev); > > if (num_vfs == pdev->sriov->num_VFs) > goto exit; > @@ -518,7 +517,7 @@ static ssize_t sriov_numvfs_store(struct device *dev, > num_vfs, ret); > > exit: > - mutex_unlock(&iov->dev->sriov->lock); > + device_unlock(&pdev->dev); > > if (ret < 0) > return ret; > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > index f8113e5b9812..93f4044b8f4b 100644 > --- a/drivers/pci/pci.h > +++ b/drivers/pci/pci.h > @@ -272,7 +272,6 @@ struct pci_sriov { > u16 driver_max_VFs; /* max num VFs driver supports */ > struct pci_dev *dev; /* lowest numbered PF */ > struct pci_dev *self; /* this PF */ > - struct mutex lock; /* lock for setting sriov_numvfs in sysfs */ > resource_size_t barsz[PCI_SRIOV_NUM_BARS]; /* VF BAR size */ > bool drivers_autoprobe; /* auto probing of VFs by driver */ > }; > -- > 2.11.0 >