From patchwork Wed Jun 24 03:08:09 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hidetoshi Seto X-Patchwork-Id: 32075 Received: from vger.kernel.org (vger.kernel.org [209.132.176.167]) by demeter.kernel.org (8.14.2/8.14.2) with ESMTP id n5O38NEY013787 for ; Wed, 24 Jun 2009 03:08:23 GMT Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751220AbZFXDIT (ORCPT ); Tue, 23 Jun 2009 23:08:19 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751248AbZFXDIT (ORCPT ); Tue, 23 Jun 2009 23:08:19 -0400 Received: from fgwmail6.fujitsu.co.jp ([192.51.44.36]:34662 "EHLO fgwmail6.fujitsu.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751220AbZFXDIS (ORCPT ); Tue, 23 Jun 2009 23:08:18 -0400 Received: from m6.gw.fujitsu.co.jp ([10.0.50.76]) by fgwmail6.fujitsu.co.jp (Fujitsu Gateway) with ESMTP id n5O38K7p024600 for (envelope-from seto.hidetoshi@jp.fujitsu.com); Wed, 24 Jun 2009 12:08:20 +0900 Received: from smail (m6 [127.0.0.1]) by outgoing.m6.gw.fujitsu.co.jp (Postfix) with ESMTP id D27F445DE51 for ; Wed, 24 Jun 2009 12:08:19 +0900 (JST) Received: from s6.gw.fujitsu.co.jp (s6.gw.fujitsu.co.jp [10.0.50.96]) by m6.gw.fujitsu.co.jp (Postfix) with ESMTP id A41BB45DE4F for ; Wed, 24 Jun 2009 12:08:19 +0900 (JST) Received: from s6.gw.fujitsu.co.jp (localhost.localdomain [127.0.0.1]) by s6.gw.fujitsu.co.jp (Postfix) with ESMTP id 70801E08005 for ; Wed, 24 Jun 2009 12:08:19 +0900 (JST) Received: from m106.s.css.fujitsu.com (m106.s.css.fujitsu.com [10.249.87.106]) by s6.gw.fujitsu.co.jp (Postfix) with ESMTP id 172A61DB8038 for ; Wed, 24 Jun 2009 12:08:19 +0900 (JST) Received: from m106.css.fujitsu.com (m106 [127.0.0.1]) by m106.s.css.fujitsu.com (Postfix) with ESMTP id DC02B5B879C; Wed, 24 Jun 2009 12:08:18 +0900 (JST) Received: from [127.0.0.1] (unknown [10.124.100.141]) by m106.s.css.fujitsu.com (Postfix) with ESMTP id 703B55B8726; Wed, 24 Jun 2009 12:08:18 +0900 (JST) Message-ID: <4A419899.4000406@jp.fujitsu.com> Date: Wed, 24 Jun 2009 12:08:09 +0900 From: Hidetoshi Seto User-Agent: Thunderbird 2.0.0.22 (Windows/20090605) MIME-Version: 1.0 To: Matthew Wilcox CC: linux-pci@vger.kernel.org, Jesse Barnes , Michael Ellerman Subject: Re: [PATCH] pci, msi: Fix restoration of MSI/MSI-X mask states in suspend/resume References: <4A40948D.9070508@jp.fujitsu.com> <20090623115428.GA19977@parisc-linux.org> In-Reply-To: <20090623115428.GA19977@parisc-linux.org> Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org Matthew Wilcox wrote: > I don't like this 'save' boolean. How about we do it this way: Looks good! Thank you for providing the idea. Here is the updated version. How about this? Thanks, H.Seto Reviewed-by: Matthew Wilcox === [PATCH] pci, msi: Fix restoration of MSI/MSI-X mask states in suspend/resume v2 There are 2 problems on mask states in suspend/resume. [1]: It is better to restore the mask states of MSI/MSI-X to initial states (MSI is unmasked, MSI-X is masked) when we release the device. The pci_msi_shutdown() does the restoration of mask states for MSI, while the msi_free_irqs() does it for MSI-X. In other words, in the "disable" path both of MSI and MSI-X are cared, but in the "shutdown" path only MSI is cared. MSI: pci_disable_msi() => pci_msi_shutdown() [ mask states for MSI restored ] => msi_set_enable(dev, pos, 0); => msi_free_irqs() MSI-X: pci_disable_msix() => pci_msix_shutdown() => msix_set_enable(dev, 0); => msix_free_all_irqs => msi_free_irqs() [ mask states for MSI-X restored ] This patch moves the masking for MSI-X from msi_free_irqs() to pci_msix_shutdown(). This change has some positive side effects: - It prevents OS from touching mask states before reading preserved bits in the register, which can be happen if msi_free_irqs() is called from error path in msix_capability_init(). - It also prevents touching the register after turning off MSI-X in "disable" path, which can be a problem on some devices. [2]: We have cache of the mask state in msi_desc, which is automatically updated when msi/msix_mask_irq() is called. This cached states are used for the resume. But since what need to be restored in the resume is the states before the shutdown on the suspend, calling msi/msix_mask_irq() from pci_msi/msix_shutdown() is not appropriate. This patch introduces __msi/msix_mask_irq() that do mask as same as msi/msix_mask_irq() but does not update cached state, for use in pci_msi/msix_shutdown(). v2: get rid of msi/msix_mask_irq_nocache() (proposed by Matthew Wilcox) Signed-off-by: Hidetoshi Seto --- drivers/pci/msi.c | 35 ++++++++++++++++++++++++++++------- 1 files changed, 28 insertions(+), 7 deletions(-) diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c index d9f06fb..d069b5e 100644 --- a/drivers/pci/msi.c +++ b/drivers/pci/msi.c @@ -127,17 +127,23 @@ static inline __attribute_const__ u32 msi_enabled_mask(u16 control) * reliably as devices without an INTx disable bit will then generate a * level IRQ which will never be cleared. */ -static void msi_mask_irq(struct msi_desc *desc, u32 mask, u32 flag) +static u32 __msi_mask_irq(struct msi_desc *desc, u32 mask, u32 flag) { u32 mask_bits = desc->masked; if (!desc->msi_attrib.maskbit) - return; + return 0; mask_bits &= ~mask; mask_bits |= flag; pci_write_config_dword(desc->dev, desc->mask_pos, mask_bits); - desc->masked = mask_bits; + + return mask_bits; +} + +static void msi_mask_irq(struct msi_desc *desc, u32 mask, u32 flag) +{ + desc->masked = __msi_mask_irq(desc, mask, flag); } /* @@ -147,7 +153,7 @@ static void msi_mask_irq(struct msi_desc *desc, u32 mask, u32 flag) * file. This saves a few milliseconds when initialising devices with lots * of MSI-X interrupts. */ -static void msix_mask_irq(struct msi_desc *desc, u32 flag) +static u32 __msix_mask_irq(struct msi_desc *desc, u32 flag) { u32 mask_bits = desc->masked; unsigned offset = desc->msi_attrib.entry_nr * PCI_MSIX_ENTRY_SIZE + @@ -155,7 +161,13 @@ static void msix_mask_irq(struct msi_desc *desc, u32 flag) mask_bits &= ~1; mask_bits |= flag; writel(mask_bits, desc->mask_base + offset); - desc->masked = mask_bits; + + return mask_bits; +} + +static void msix_mask_irq(struct msi_desc *desc, u32 flag) +{ + desc->masked = __msix_mask_irq(desc, flag); } static void msi_set_mask_bit(unsigned irq, u32 flag) @@ -611,9 +623,11 @@ void pci_msi_shutdown(struct pci_dev *dev) pci_intx_for_msi(dev, 1); dev->msi_enabled = 0; + /* Return the device with MSI unmasked as initial states */ pci_read_config_word(dev, pos + PCI_MSI_FLAGS, &ctrl); mask = msi_capable_mask(ctrl); - msi_mask_irq(desc, mask, ~mask); + /* Keep cached state to be restored */ + __msi_mask_irq(desc, mask, ~mask); /* Restore dev->irq to its default pin-assertion irq */ dev->irq = desc->msi_attrib.default_irq; @@ -653,7 +667,6 @@ static int msi_free_irqs(struct pci_dev* dev) list_for_each_entry_safe(entry, tmp, &dev->msi_list, list) { if (entry->msi_attrib.is_msix) { - msix_mask_irq(entry, 1); if (list_is_last(&entry->list, &dev->msi_list)) iounmap(entry->mask_base); } @@ -741,9 +754,17 @@ static void msix_free_all_irqs(struct pci_dev *dev) void pci_msix_shutdown(struct pci_dev* dev) { + struct msi_desc *entry; + if (!pci_msi_enable || !dev || !dev->msix_enabled) return; + /* Return the device with MSI-X masked as initial states */ + list_for_each_entry(entry, &dev->msi_list, list) { + /* Keep cached states to be restored */ + __msix_mask_irq(entry, 1); + } + msix_set_enable(dev, 0); pci_intx_for_msi(dev, 1); dev->msix_enabled = 0;