From patchwork Fri Jun 16 22:54:46 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Bjorn Helgaas X-Patchwork-Id: 9793813 X-Patchwork-Delegate: bhelgaas@google.com Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 62A8B6038E for ; Fri, 16 Jun 2017 22:55:06 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 531C528683 for ; Fri, 16 Jun 2017 22:55:06 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 447D4286A0; Fri, 16 Jun 2017 22:55:06 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 68CA028683 for ; Fri, 16 Jun 2017 22:55:05 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750967AbdFPWyu (ORCPT ); Fri, 16 Jun 2017 18:54:50 -0400 Received: from mail.kernel.org ([198.145.29.99]:47944 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750863AbdFPWyt (ORCPT ); Fri, 16 Jun 2017 18:54:49 -0400 Received: from localhost (unknown [69.71.4.159]) (using TLSv1.2 with cipher DHE-RSA-AES128-SHA (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id D44F822CB4; Fri, 16 Jun 2017 22:54:47 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org D44F822CB4 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, 16 Jun 2017 17:54:46 -0500 From: Bjorn Helgaas To: Piotr Gregor Cc: Alex Williamson , Bjorn Helgaas , "Michael S. Tsirkin" , Greg Kroah-Hartman , Neo Jia , Kirti Wankhede , Vlad Tsyrklevich , Arvind Yadav , Yongji Xie , linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, kvm@vger.kernel.org Subject: Re: [PATCH] PCI: Move test of INTx masking to pci_setup_device Message-ID: <20170616225446.GG11129@bhelgaas-glaptop.roam.corp.google.com> References: <20170526210222.GA22204@westernst> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20170526210222.GA22204@westernst> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Fri, May 26, 2017 at 10:02:25PM +0100, Piotr Gregor wrote: > The test for INTx masking via config space command performed > in pci_intx_mask_supported() should be performed before PCI device > can be used. This is to avoid reading/writing of PCI_COMMAND_INTX_DISABLE > register which may collide with MSI/MSI-X interrupts. > > This patch moves test performed in pci_intx_mask_supported() to > > static void pci_test_intx_masking(struct pci_dev *dev) > > defined in drivers/pci/probe.c. > > This function is called from pci_setup_device(). It skips the test > if the device has been already marked to have broken INTx masking > feature. Otherwise the test is executed and broken_intx_masking > field of struct pci_dev is set accordingly. broken_intx_masking > meaning is: if it is true then the test has been either skipped > because the device has been already known to have broken INTx > masking support, or the test's been done and it has detected INTx > masking support to be broken. > The test result can be queried at any time later from the pci_dev > using same interface as before (though whith changed implementation) > > static inline bool pci_intx_mask_supported(struct pci_dev *pdev) > { > /* > * INTx masking is supported if device has not been marked > * to have this feature broken and it has passed > * pci_test_intx_masking() test. > */ > return !pdev->broken_intx_masking; > } > > so current users of pci_intx_mask_supported: uio and vfio, keep > their code unchanged. > > Signed-off-by: Piotr Gregor > --- > drivers/pci/pci.c | 42 +----------------------------------------- > drivers/pci/probe.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ > include/linux/pci.h | 13 +++++++++++-- > 3 files changed, 56 insertions(+), 43 deletions(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index b01bd5b..7c4e1aa 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -3708,46 +3708,6 @@ void pci_intx(struct pci_dev *pdev, int enable) > } > EXPORT_SYMBOL_GPL(pci_intx); > > -/** > - * pci_intx_mask_supported - probe for INTx masking support > - * @dev: the PCI device to operate on > - * > - * Check if the device dev support INTx masking via the config space > - * command word. > - */ > -bool pci_intx_mask_supported(struct pci_dev *dev) > -{ > - bool mask_supported = false; > - u16 orig, new; > - > - if (dev->broken_intx_masking) > - return false; > - > - pci_cfg_access_lock(dev); > - > - pci_read_config_word(dev, PCI_COMMAND, &orig); > - pci_write_config_word(dev, PCI_COMMAND, > - orig ^ PCI_COMMAND_INTX_DISABLE); > - pci_read_config_word(dev, PCI_COMMAND, &new); > - > - /* > - * There's no way to protect against hardware bugs or detect them > - * reliably, but as long as we know what the value should be, let's > - * go ahead and check it. > - */ > - if ((new ^ orig) & ~PCI_COMMAND_INTX_DISABLE) { > - dev_err(&dev->dev, "Command register changed from 0x%x to 0x%x: driver or hardware bug?\n", > - orig, new); > - } else if ((new ^ orig) & PCI_COMMAND_INTX_DISABLE) { > - mask_supported = true; > - pci_write_config_word(dev, PCI_COMMAND, orig); > - } > - > - pci_cfg_access_unlock(dev); > - return mask_supported; > -} > -EXPORT_SYMBOL_GPL(pci_intx_mask_supported); > - > static bool pci_check_and_set_intx_mask(struct pci_dev *dev, bool mask) > { > struct pci_bus *bus = dev->bus; > @@ -3798,7 +3758,7 @@ static bool pci_check_and_set_intx_mask(struct pci_dev *dev, bool mask) > * @dev: the PCI device to operate on > * > * Check if the device dev has its INTx line asserted, mask it and > - * return true in that case. False is returned if not interrupt was > + * return true in that case. False is returned if no interrupt was > * pending. > */ > bool pci_check_and_mask_intx(struct pci_dev *dev) > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 19c8950..ee6b55c 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -1330,6 +1330,48 @@ static void pci_msi_setup_pci_dev(struct pci_dev *dev) > } > > /** > + * pci_test_intx_masking - probe for INTx masking support > + * @dev: the PCI device to operate on > + * > + * Check if the @dev supports INTx masking via the config space > + * command word. Executed when PCI device is setup. Result is saved > + * in broken_intx_masking field of struct pci_dev and can be checked > + * with pci_intx_mask_supported at any time later, after the PCI device > + * has been setup (this avoids testing of PCI_COMMAND_INTX_DISABLE > + * register at runtime). > + */ > +static void pci_test_intx_masking(struct pci_dev *dev) > +{ > + u16 orig, toggle, new; > + > + /* > + * If device doesn't support this feature though it could pass the test. > + */ > + if (dev->broken_intx_masking) > + return; I don't think we need this check anymore, because broken_intx_masking is only set by "FINAL" quirks, which run long after this code. > + > + pci_cfg_access_lock(dev); I don't think we need this locking either. This lock protects against user-space config access, and the device isn't accessible to user space yet. > + /* > + * Perform the test. > + */ > + pci_read_config_word(dev, PCI_COMMAND, &orig); > + toggle = orig ^ PCI_COMMAND_INTX_DISABLE; > + pci_write_config_word(dev, PCI_COMMAND, toggle); > + pci_read_config_word(dev, PCI_COMMAND, &new); > + > + /* > + * Restore initial state. > + */ > + pci_write_config_word(dev, PCI_COMMAND, orig); > + > + pci_cfg_access_unlock(dev); > + > + if (new != toggle) > + dev->broken_intx_masking = 1; > +} > + > +/** > * pci_setup_device - fill in class and map information of a device > * @dev: the device structure to fill > * > @@ -1399,6 +1441,8 @@ int pci_setup_device(struct pci_dev *dev) > } > } > > + pci_test_intx_masking(dev); I think it's nicer to read the code if the assignment isn't hidden away, e.g., something like this: dev->broken_intx_masking = pci_test_intx_masking(dev); That's more like what we do for dev->cfg_size and similar things. I provisionally made these changes and attached the updated patch below. Let me know if it looks OK. This is on my pci/enumeration branch, intended for v4.13. > switch (dev->hdr_type) { /* header type */ > case PCI_HEADER_TYPE_NORMAL: /* standard header */ > if (class == PCI_CLASS_BRIDGE_PCI) > diff --git a/include/linux/pci.h b/include/linux/pci.h > index 33c2b0b..8a307fc 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -366,7 +366,7 @@ struct pci_dev { > unsigned int is_thunderbolt:1; /* Thunderbolt controller */ > unsigned int __aer_firmware_first_valid:1; > unsigned int __aer_firmware_first:1; > - unsigned int broken_intx_masking:1; > + unsigned int broken_intx_masking:1; /* INTx masking can't be used */ > unsigned int io_window_1k:1; /* Intel P2P bridge 1K I/O windows */ > unsigned int irq_managed:1; > unsigned int has_secondary_link:1; > @@ -1003,6 +1003,16 @@ int __must_check pci_reenable_device(struct pci_dev *); > int __must_check pcim_enable_device(struct pci_dev *pdev); > void pcim_pin_device(struct pci_dev *pdev); > > +static inline bool pci_intx_mask_supported(struct pci_dev *pdev) > +{ > + /* > + * INTx masking is supported if device has not been marked > + * to have this feature broken and it has passed > + * pci_test_intx_masking() test. > + */ > + return !pdev->broken_intx_masking; > +} > + > static inline int pci_is_enabled(struct pci_dev *pdev) > { > return (atomic_read(&pdev->enable_cnt) > 0); > @@ -1026,7 +1036,6 @@ int __must_check pci_set_mwi(struct pci_dev *dev); > int pci_try_set_mwi(struct pci_dev *dev); > void pci_clear_mwi(struct pci_dev *dev); > void pci_intx(struct pci_dev *dev, int enable); > -bool pci_intx_mask_supported(struct pci_dev *dev); > bool pci_check_and_mask_intx(struct pci_dev *dev); > bool pci_check_and_unmask_intx(struct pci_dev *dev); > int pci_wait_for_pending(struct pci_dev *dev, int pos, u16 mask); commit 73ab6bc006ba966e233785577a1742c6cace9c8a Author: Piotr Gregor Date: Fri May 26 22:02:25 2017 +0100 PCI: Test INTx masking during enumeration, not at run-time The test for INTx masking via PCI_COMMAND_INTX_DISABLE performed in pci_intx_mask_supported() should be done before the device can be used. This is to avoid writing PCI_COMMAND while the driver owns the device, in case that has any effect on MSI/MSI-X interrupts. Move the content of pci_intx_mask_supported() to pci_intx_mask_broken() and call it from pci_setup_device(). The test result can be queried at any time later using the same pci_intx_mask_supported() interface as before (though with changed implementation), so callers (uio, vfio) should be unaffected. Signed-off-by: Piotr Gregor [bhelgaas: changelog, remove quirk check, remove locking, move dev->broken_intx_masking assignment to caller] Signed-off-by: Bjorn Helgaas Reviewed-by: Alex Williamson Acked-by: Michael S. Tsirkin diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index b01bd5bba8e6..7c4e1aa67c67 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -3708,46 +3708,6 @@ void pci_intx(struct pci_dev *pdev, int enable) } EXPORT_SYMBOL_GPL(pci_intx); -/** - * pci_intx_mask_supported - probe for INTx masking support - * @dev: the PCI device to operate on - * - * Check if the device dev support INTx masking via the config space - * command word. - */ -bool pci_intx_mask_supported(struct pci_dev *dev) -{ - bool mask_supported = false; - u16 orig, new; - - if (dev->broken_intx_masking) - return false; - - pci_cfg_access_lock(dev); - - pci_read_config_word(dev, PCI_COMMAND, &orig); - pci_write_config_word(dev, PCI_COMMAND, - orig ^ PCI_COMMAND_INTX_DISABLE); - pci_read_config_word(dev, PCI_COMMAND, &new); - - /* - * There's no way to protect against hardware bugs or detect them - * reliably, but as long as we know what the value should be, let's - * go ahead and check it. - */ - if ((new ^ orig) & ~PCI_COMMAND_INTX_DISABLE) { - dev_err(&dev->dev, "Command register changed from 0x%x to 0x%x: driver or hardware bug?\n", - orig, new); - } else if ((new ^ orig) & PCI_COMMAND_INTX_DISABLE) { - mask_supported = true; - pci_write_config_word(dev, PCI_COMMAND, orig); - } - - pci_cfg_access_unlock(dev); - return mask_supported; -} -EXPORT_SYMBOL_GPL(pci_intx_mask_supported); - static bool pci_check_and_set_intx_mask(struct pci_dev *dev, bool mask) { struct pci_bus *bus = dev->bus; @@ -3798,7 +3758,7 @@ static bool pci_check_and_set_intx_mask(struct pci_dev *dev, bool mask) * @dev: the PCI device to operate on * * Check if the device dev has its INTx line asserted, mask it and - * return true in that case. False is returned if not interrupt was + * return true in that case. False is returned if no interrupt was * pending. */ bool pci_check_and_mask_intx(struct pci_dev *dev) diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 19c8950c6c38..28c57b31261c 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1329,6 +1329,34 @@ static void pci_msi_setup_pci_dev(struct pci_dev *dev) pci_msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0); } +/** + * pci_intx_mask_broken - test PCI_COMMAND_INTX_DISABLE writability + * @dev: PCI device + * + * Test whether PCI_COMMAND_INTX_DISABLE is writable for @dev. Check this + * at enumeration-time to avoid modifying PCI_COMMAND at run-time. + */ +static int pci_intx_mask_broken(struct pci_dev *dev) +{ + u16 orig, toggle, new; + + pci_read_config_word(dev, PCI_COMMAND, &orig); + toggle = orig ^ PCI_COMMAND_INTX_DISABLE; + pci_write_config_word(dev, PCI_COMMAND, toggle); + pci_read_config_word(dev, PCI_COMMAND, &new); + + pci_write_config_word(dev, PCI_COMMAND, orig); + + /* + * PCI_COMMAND_INTX_DISABLE was reserved and read-only prior to PCI + * r2.3, so strictly speaking, a device is not *broken* if it's not + * writable. + */ + if (new == toggle) + return 1; + return 0; +} + /** * pci_setup_device - fill in class and map information of a device * @dev: the device structure to fill @@ -1399,6 +1427,8 @@ int pci_setup_device(struct pci_dev *dev) } } + dev->broken_intx_masking = pci_intx_mask_broken(dev); + switch (dev->hdr_type) { /* header type */ case PCI_HEADER_TYPE_NORMAL: /* standard header */ if (class == PCI_CLASS_BRIDGE_PCI) diff --git a/include/linux/pci.h b/include/linux/pci.h index 33c2b0b77429..4f0613d5d2d9 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -366,7 +366,7 @@ struct pci_dev { unsigned int is_thunderbolt:1; /* Thunderbolt controller */ unsigned int __aer_firmware_first_valid:1; unsigned int __aer_firmware_first:1; - unsigned int broken_intx_masking:1; + unsigned int broken_intx_masking:1; /* INTx masking can't be used */ unsigned int io_window_1k:1; /* Intel P2P bridge 1K I/O windows */ unsigned int irq_managed:1; unsigned int has_secondary_link:1; @@ -1003,6 +1003,15 @@ int __must_check pci_reenable_device(struct pci_dev *); int __must_check pcim_enable_device(struct pci_dev *pdev); void pcim_pin_device(struct pci_dev *pdev); +static inline bool pci_intx_mask_supported(struct pci_dev *pdev) +{ + /* + * INTx masking is supported if PCI_COMMAND_INTX_DISABLE is + * writable and no quirk has marked the feature broken. + */ + return !pdev->broken_intx_masking; +} + static inline int pci_is_enabled(struct pci_dev *pdev) { return (atomic_read(&pdev->enable_cnt) > 0); @@ -1026,7 +1035,6 @@ int __must_check pci_set_mwi(struct pci_dev *dev); int pci_try_set_mwi(struct pci_dev *dev); void pci_clear_mwi(struct pci_dev *dev); void pci_intx(struct pci_dev *dev, int enable); -bool pci_intx_mask_supported(struct pci_dev *dev); bool pci_check_and_mask_intx(struct pci_dev *dev); bool pci_check_and_unmask_intx(struct pci_dev *dev); int pci_wait_for_pending(struct pci_dev *dev, int pos, u16 mask);