From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752456AbbIQP4Z (ORCPT ); Thu, 17 Sep 2015 11:56:25 -0400 Received: from out01.mta.xmission.com ([166.70.13.231]:46055 "EHLO out01.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752261AbbIQP4V (ORCPT ); Thu, 17 Sep 2015 11:56:21 -0400 From: ebiederm@xmission.com (Eric W. Biederman) To: Bjorn Helgaas Cc: "Michael S. Tsirkin" , linux-kernel@vger.kernel.org, Fam Zheng , Yinghai Lu , Ulrich Obergfell , Rusty Russell , linux-pci@vger.kernel.org, virtualization@lists.linux-foundation.org References: <1441553385-3810-1-git-send-email-mst@redhat.com> <20150917154424.GI25767@google.com> Date: Thu, 17 Sep 2015 10:49:06 -0500 In-Reply-To: <20150917154424.GI25767@google.com> (Bjorn Helgaas's message of "Thu, 17 Sep 2015 10:44:24 -0500") Message-ID: <874mitxczx.fsf@x220.int.ebiederm.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-AID: U2FsdGVkX18HRMykkrm39sCxFjJ9JXDUSaHs9iRrOFc= X-SA-Exim-Connect-IP: 67.3.201.231 X-SA-Exim-Mail-From: ebiederm@xmission.com X-Spam-Report: * -1.0 ALL_TRUSTED Passed through trusted hosts only via SMTP * 0.0 TVD_RCVD_IP Message was received from an IP address * 0.0 T_TM2_M_HEADER_IN_MSG BODY: No description available. * 0.8 BAYES_50 BODY: Bayes spam probability is 40 to 60% * [score: 0.5000] * -0.0 DCC_CHECK_NEGATIVE Not listed in DCC * [sa04 1397; Body=1 Fuz1=1 Fuz2=1] X-Spam-DCC: XMission; sa04 1397; Body=1 Fuz1=1 Fuz2=1 X-Spam-Combo: ;Bjorn Helgaas X-Spam-Relay-Country: X-Spam-Timing: total 1613 ms - load_scoreonly_sql: 0.03 (0.0%), signal_user_changed: 3.7 (0.2%), b_tie_ro: 2.5 (0.2%), parse: 0.73 (0.0%), extract_message_metadata: 16 (1.0%), get_uri_detail_list: 3.1 (0.2%), tests_pri_-1000: 8 (0.5%), tests_pri_-950: 0.99 (0.1%), tests_pri_-900: 0.81 (0.1%), tests_pri_-400: 41 (2.6%), check_bayes: 40 (2.5%), b_tokenize: 9 (0.6%), b_tok_get_all: 17 (1.1%), b_comp_prob: 3.0 (0.2%), b_tok_touch_all: 4.6 (0.3%), b_finish: 0.64 (0.0%), tests_pri_0: 676 (41.9%), tests_pri_500: 863 (53.5%), poll_dns_idle: 856 (53.1%), rewrite_mail: 0.00 (0.0%) Subject: Re: [PATCH v7] pci: quirk to skip msi disable on shutdown X-Spam-Flag: No X-SA-Exim-Version: 4.2.1 (built Wed, 24 Sep 2014 11:00:52 -0600) X-SA-Exim-Scanned: Yes (on in01.mta.xmission.com) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Bjorn Helgaas writes: > On Sun, Sep 06, 2015 at 06:32:35PM +0300, Michael S. Tsirkin wrote: >> On some hypervisors, virtio devices tend to generate spurious interrupts >> when switching between MSI and non-MSI mode. Normally, either MSI or >> non-MSI is used and all is well, but during shutdown, linux disables MSI >> which then causes an "irq %d: nobody cared" message, with irq being >> subsequently disabled. >> >> Since bus mastering is already disabled at this point, disabling MSI >> isn't actually useful for spec compliant devices: MSI interrupts are >> in-bus memory writes so disabling Bus Master (which is already done) >> disables these as well: after some research, it appears to be there for >> the benefit of devices that ignore the bus master bit. >> >> As it's not clear how common this kind of bug is, this patch simply adds >> a quirk, to be set by devices that wish to skip disabling msi on >> shutdown, relying on bus master instead. >> >> We set this quirk in virtio core. >> >> Reported-by: Fam Zheng >> Cc: Bjorn Helgaas >> Cc: Yinghai Lu >> Cc: Ulrich Obergfell >> Cc: Rusty Russell >> Cc: "Eric W. Biederman" >> Signed-off-by: Michael S. Tsirkin > > Eric, what do you think of this? You had many comments on previous > versions. I still think it is a hack to avoid actually fixing a driver. I think the hack is better as a quirk. I think the quirk would tend to be better if it was limited to whatever set of hypervisors where this is actually a problem. And of course given that on any sane configuration we have the irq watchdog I don't see what this is fixing in practice. Other than suggesting this hack become find a way to limit itself to the driver that is actually having problems I don't see a way to improve it. > Minor comment on a comment below. > >> --- >> >> >> changes from v6: >> limit changes to virtio only >> changes from v5: >> rebased on top of pci/msi >> fixed commit log, including comments by Bjorn >> and adding explanation to address comments/questions by Eric >> dropped stable Cc, this patch does not seem to qualify for stable >> changes from v4: >> Yijing Wang noted that >> early fixups rely on pci_msi_off. >> Split out the functionality and move off the >> required part to run early during pci_device_setup. >> Changes from v3: >> fix a copy-and-paste error in >> pci: drop some duplicate code >> other patches are unchanged >> drop Cc stable for now >> Changes from v2: >> move code from probe to device enumeration >> add patches to unexport pci_msi_off >> >> >> include/linux/pci.h | 2 ++ >> drivers/pci/pci-driver.c | 6 ++++-- >> drivers/virtio/virtio_pci_common.c | 4 ++++ >> 3 files changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/include/linux/pci.h b/include/linux/pci.h >> index 860c751..80f3494 100644 >> --- a/include/linux/pci.h >> +++ b/include/linux/pci.h >> @@ -180,6 +180,8 @@ enum pci_dev_flags { >> PCI_DEV_FLAGS_NO_BUS_RESET = (__force pci_dev_flags_t) (1 << 6), >> /* Do not use PM reset even if device advertises NoSoftRst- */ >> PCI_DEV_FLAGS_NO_PM_RESET = (__force pci_dev_flags_t) (1 << 7), >> + /* Do not disable MSI on shutdown, disable bus master instead */ > > This comment doesn't really match what the patch does. The patch merely > does "Do not disable MSI on shutdown." It doesn't "disable bus master > instead." > > Bus master may be disabled elsewhere, but that is independent of the > PCI_DEV_FLAGS_NO_MSI_SHUTDOWN flag. > >> + PCI_DEV_FLAGS_NO_MSI_SHUTDOWN = (__force pci_dev_flags_t) (1 << 8), >> }; >> >> enum pci_irq_reroute_variant { >> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c >> index 3cb2210..59d9e40 100644 >> --- a/drivers/pci/pci-driver.c >> +++ b/drivers/pci/pci-driver.c >> @@ -450,8 +450,10 @@ 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); >> + if (!(pci_dev->dev_flags & PCI_DEV_FLAGS_NO_MSI_SHUTDOWN)) { >> + pci_msi_shutdown(pci_dev); >> + pci_msix_shutdown(pci_dev); >> + } >> >> #ifdef CONFIG_KEXEC >> /* >> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c >> index 78f804a..26f46c3 100644 >> --- a/drivers/virtio/virtio_pci_common.c >> +++ b/drivers/virtio/virtio_pci_common.c >> @@ -528,6 +528,8 @@ static int virtio_pci_probe(struct pci_dev *pci_dev, >> if (rc) >> goto err_register; >> >> + pci_dev->dev_flags |= PCI_DEV_FLAGS_NO_MSI_SHUTDOWN; >> + >> return 0; >> >> err_register: >> @@ -546,6 +548,8 @@ static void virtio_pci_remove(struct pci_dev *pci_dev) >> { >> struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev); >> >> + pci_dev->dev_flags &= ~PCI_DEV_FLAGS_NO_MSI_SHUTDOWN; >> + >> unregister_virtio_device(&vp_dev->vdev); >> >> if (vp_dev->ioaddr) >> -- >> MST