Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933465AbbELND5 (ORCPT ); Tue, 12 May 2015 09:03:57 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56393 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933444AbbELNDy (ORCPT ); Tue, 12 May 2015 09:03:54 -0400 Date: Tue, 12 May 2015 15:03:32 +0200 From: "Michael S. Tsirkin" To: linux-kernel@vger.kernel.org Cc: Bjorn Helgaas , linux-pci@vger.kernel.org, Fam Zheng , Yinghai Lu , Yijing Wang , "Eric W. Biederman" , Ulrich Obergfell , Rusty Russell Subject: [PATCH v6 1/2] PCI/MSI: Don't disable MSI/MSI-X at shutdown Message-ID: <1431431730-25164-2-git-send-email-mst@redhat.com> References: <1431431730-25164-1-git-send-email-mst@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1431431730-25164-1-git-send-email-mst@redhat.com> X-Mutt-Fcc: =sent Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3559 Lines: 80 d52877c7b1af ("pci/irq: let pci_device_shutdown to call pci_msi_shutdown v2") disabled MSI/MSI-X at device shutdown to address a kexec problem. The change made by the above commit is no longer necessary: it was superceded by commit d5dea7d95c48 ("PCI: msi: Disable msi interrupts when we initialize a pci device") which makes sure the original kexec problem is solved in the new kernel, and commit b566a22c23 ("PCI: disable Bus Master on PCI device shutdown") which makes sure device does not send MSI interrupts (MSI is a memory write and so is suppressed when bus master is cleared). On the contrary, disabling MSI makes it *more* likely that the device will cause mischief since unlike MSI, INT#x interrupts are not suppressed by clearing bus mastering. One example of such mischief is that after we disable MSI, the device may assert INT#x (remember, cleaning bus mastering does not disable INT#x interrupts), and if the driver hasn't registered an interrupt handler for it, the interrupt is never deasserted which causes an "irq %d: nobody cared" message, with irq being subsequently disabled. This is actually not hard to observe with virtio devices. Not a huge problem, but ugly, and might in theory cause other problems, e.g. if the irq being disabled is shared with another device that attempts to use it in its shutdown callback, or if irq debugging was explicitly disabled on the kernel command line. Another theoretical problem is that if the driver does not flush MSI interrupts in the shutdown callback, MSI interrupt handler will run at the same time as the INT#x handler, which doesn't normally happen outside the shutdown path; Depending on the driver, the two handlers might conflict. I did not go looking for such driver bugs but this seems plausible. virtio specifically has another issue: register functions change between msi enabled and disabled modes, so disabling MSI while driver is doing things (e.g. from a kthread) will make the device confused. Of course, some of the above specific issues can be solved by implementing a shutdown callback in the driver: this callback would have to suppress further activity from both the driver and the device, and flush outstanding handlers. This is a non-trivial amount of code that can trigger at any time, so needs careful thought to avoid race conditions causing bugs, and that's only running on shutdown, so isn't well tested. Instead, stop disabling MSIs in PCI core at shutdown: it's simple, safe, removes code instead of adding more code, and needs no driver support. Reported-by: Fam Zheng Tested-by: Fam Zheng Signed-off-by: Michael S. Tsirkin Signed-off-by: Bjorn Helgaas CC: Yinghai Lu CC: Ulrich Obergfell CC: Rusty Russell --- drivers/pci/pci-driver.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c index 3cb2210..38a602c 100644 --- a/drivers/pci/pci-driver.c +++ b/drivers/pci/pci-driver.c @@ -450,8 +450,6 @@ static void pci_device_shutdown(struct device *dev) if (drv && drv->shutdown) drv->shutdown(pci_dev); - pci_msi_shutdown(pci_dev); - pci_msix_shutdown(pci_dev); #ifdef CONFIG_KEXEC /* -- MST -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/