From patchwork Sat Jun 17 17:45:44 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Piotr Gregor X-Patchwork-Id: 777363 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 3wql903Hwmz9s76 for ; Sun, 18 Jun 2017 03:48:52 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=rsyncme-org.20150623.gappssmtp.com header.i=@rsyncme-org.20150623.gappssmtp.com header.b="nXJ9pq5z"; dkim-atps=neutral Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753167AbdFQRsu (ORCPT ); Sat, 17 Jun 2017 13:48:50 -0400 Received: from mail-wm0-f42.google.com ([74.125.82.42]:37385 "EHLO mail-wm0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752538AbdFQRss (ORCPT ); Sat, 17 Jun 2017 13:48:48 -0400 Received: by mail-wm0-f42.google.com with SMTP id d73so44789975wma.0 for ; Sat, 17 Jun 2017 10:48:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rsyncme-org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:signed-off-by:user-agent; bh=myPRWRaTRBDijLgYknYPIGOHegIf3roumtg+NOVDm5I=; b=nXJ9pq5zw2gWvrWIiVSAuH0nSJoO9fSZxiwJathyyId9nDa0MMfg5lY03aARsS6l80 mjP8aKwI9nSJtopbukD7XPR3F/7uaAWqWwxqphNqnj3RXKP28HfNFbo5akWGTh6JBgMe h7sWjM2dZ5JbNUwbUGYBQSWljEtTtjSC+xpcMuMNVTMTSTNSOEA1JTMvYP36cux2k++n iRS+6JB8Fwh8jfcvHcdvw3H3FZ4JplPj9l2qkgCHinFZjf6TrQIUrMrljrn1AbQvJWsw +BNOa2obFYGmSOOsSeTHALPli79bevpOSOYagRdVeJFdUf43ID+U97UKlgxY7BEq1OND sjKA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:signed-off-by :user-agent; bh=myPRWRaTRBDijLgYknYPIGOHegIf3roumtg+NOVDm5I=; b=FJpjHT3iPIrk287hvSLFOaCz1QjC7Lqq3wHt/nmbgIHOgaJospjjxfg8TNPhxbauJS 3o85Vt+ZHZaZsTatcer+NiSDvNGy06fGFEXr+0bBV3hc+XZheAqttkboq+q9cL/gIBSz O8U1jPQGgIx1zrPf7q72QpNXDKZhwxNwHPB/OxAX2EHrXJ40xPv9Vn5YTOE95Sqhguc8 zXQAA09t+Ti3LnhRWMfaKx/Vb7O8mdxyca8W4PlcfNeJMBvauL7enrUUoeAbUys/4aol Hoca5ZMxlcymTozYbVs+cCV8MDtdAmX95B8KlIH7zlrK1RLNdgvW47mfG/lOy4SlJizC f3ew== X-Gm-Message-State: AKS2vOwsspzRKnB+LRNr8ZyiSajdQOMkY4YdDtNqCIfoDphRpTAZPfAy nkJvHZ9ZqkGMMGu7 X-Received: by 10.28.128.198 with SMTP id b189mr10321227wmd.79.1497721726381; Sat, 17 Jun 2017 10:48:46 -0700 (PDT) Received: from westernst (host86-153-194-197.range86-153.btcentralplus.com. [86.153.194.197]) by smtp.gmail.com with ESMTPSA id y9sm7862995wry.32.2017.06.17.10.48.45 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 17 Jun 2017 10:48:45 -0700 (PDT) Date: Sat, 17 Jun 2017 18:45:44 +0100 From: Piotr Gregor To: Bjorn Helgaas 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: <20170617174543.GA1723@westernst> References: <20170526210222.GA22204@westernst> <20170616225446.GG11129@bhelgaas-glaptop.roam.corp.google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20170616225446.GG11129@bhelgaas-glaptop.roam.corp.google.com> Signed-off-by: Piotr Gregor User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org Hi Bjorn, The pci_cfg_access_lock is most likely not needed there. The assignment by return type is indeed preferred in this case. However, you have changed the meaning of returned boolean information by pci_intx_mask_broken leaving pci_intx_mask_supported unchanged. The test should be: if (new != toggle) /* the test failed */ return 1; return 0; I have attached patch (created on pci/enumeration branch, should I commit this to your repo too?). Regarding v2.3 - do you think it is worth to apply the check so we would have something like if ((new == toggle) || PCI_VERSION_PRIOR_TO_23) /* test OK or PCI prior to r2.3 */ return 0; return 1; cheers, Piotr On Fri, Jun 16, 2017 at 05:54:46PM -0500, Bjorn Helgaas wrote: > 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); From 6d814685b907980b4cae5c27c57fb4bdea13d5bb Mon Sep 17 00:00:00 2001 From: Piotr Gregor Date: Fri, 26 May 2017 22:02:25 +0100 Subject: [PATCH] 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 --- drivers/pci/pci.c | 42 +----------------------------------------- drivers/pci/probe.c | 30 ++++++++++++++++++++++++++++++ include/linux/pci.h | 12 ++++++++++-- 3 files changed, 41 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..e2c2650 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1330,6 +1330,34 @@ static void pci_msi_setup_pci_dev(struct pci_dev *dev) } /** + * 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 33c2b0b..4f0613d 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); -- 2.1.4