From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 181EBCA9EAF for ; Thu, 24 Oct 2019 08:07:30 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id E578C20684 for ; Thu, 24 Oct 2019 08:07:29 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2438010AbfJXIH3 (ORCPT ); Thu, 24 Oct 2019 04:07:29 -0400 Received: from Galois.linutronix.de ([193.142.43.55]:52704 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725977AbfJXIH3 (ORCPT ); Thu, 24 Oct 2019 04:07:29 -0400 Received: from p5b06da22.dip0.t-ipconnect.de ([91.6.218.34] helo=nanos) by Galois.linutronix.de with esmtpsa (TLS1.2:DHE_RSA_AES_256_CBC_SHA256:256) (Exim 4.80) (envelope-from ) id 1iNY9U-0000HG-T3; Thu, 24 Oct 2019 10:07:25 +0200 Date: Thu, 24 Oct 2019 10:07:23 +0200 (CEST) From: Thomas Gleixner To: Matthew Wilcox cc: Bjorn Helgaas , Xiang Zheng , linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, mingo@redhat.com, peterz@infradead.org, alex.williamson@redhat.com, Wang Haibin , Guoheyi , yebiaoxiang Subject: Re: Kernel panic while doing vfio-pci hot-plug/unplug test In-Reply-To: <20191023163851.GA2963@bombadil.infradead.org> Message-ID: References: <2e7293dc-eb27-bce3-c209-e0ba15409f16@huawei.com> <20191023151540.GA168080@google.com> <20191023163851.GA2963@bombadil.infradead.org> User-Agent: Alpine 2.21 (DEB 202 2017-01-01) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Linutronix-Spam-Score: -1.0 X-Linutronix-Spam-Level: - X-Linutronix-Spam-Status: No , -1.0 points, 5.0 required, ALL_TRUSTED=-1,SHORTCIRCUIT=-0.0001 Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org On Wed, 23 Oct 2019, Matthew Wilcox wrote: > Some problems I see: > > 1. Commit df65c1bcd9b7b639177a5a15da1b8dc3bee4f5fa (tglx) says: > > x86/PCI: Select CONFIG_PCI_LOCKLESS_CONFIG > > All x86 PCI configuration space accessors have either their own > serialization or can operate completely lockless (ECAM). > > Disable the global lock in the generic PCI configuration space accessors. > > The concept behind this patch is broken. We still need to lock out > config space accesses when devices are undergoing D-state transitions. > I would suggest that for the contention case that tglx is concerned about, > we should have a pci_bus_read_config_unlocked_##size set of functions > which can be used for devices we know never go into D states. I don't think that it's broken. A D-state transition has to make sure that the rest of stuff which might be touching the config space is quiescent. pci_lock cannot provide that protection > > 2. Commit a2e27787f893621c5a6b865acf6b7766f8671328 (jan kiszka) > exports pci_lock. I think this is a mistake; at best there should be > accessors for the pci_lock. But I don't understand why it needs to > exclude PCI config space changes throughout pci_check_and_set_intx_mask(). > Why can it not do: > > - bus->ops->read(bus, dev->devfn, PCI_COMMAND, 4, &cmd_status_dword); > + pci_read_config_dword(dev, PCI_COMMAND, &cmd_status_dword); Hmm. Need to look closer on that. > 3. I don't understand why 511dd98ce8cf6dc4f8f2cb32a8af31ce9f4ba4a1 > changed pci_lock to be a raw spinlock. The patch description > essentially says "We need it for RT" which isn't terribly helpful. Yes, I could slap myself for writing such a useless changelog. The reason why it is a raw spinlock is that config space access happens from very low level contexts, which require to have interrupts disabled even on RT, e.g. from the guts of the interrupt code. > 4. Finally, getting back to the original problem report here, I wouldn't > write this code this way today. There's no reason not to use the > regular add_wait_queue etc. BUT! Why are we using this custom locking > mechanism? It pretty much screams to me of an rwsem (reads/writes > of config space take it for read; changes to config space accesses > (disabling and changing of accessor methods) take it for write. You cannot use a RWSEM as low level interrupt code needs to access the config space with interrupts disabled and raw spinlocks held, e.g. to fiddle with the interrupt and MSI stuff. Thanks, tglx