Received: by 2002:ad5:474a:0:0:0:0:0 with SMTP id i10csp3698600imu; Sun, 11 Nov 2018 21:48:57 -0800 (PST) X-Google-Smtp-Source: AJdET5fh+yDDAeQ+mUdzXYXM1RCT0JgkIK2Zme94P8nmw/p2c1mgQ9byg4SoiXTFwumbVWGitFF3 X-Received: by 2002:a17:902:e101:: with SMTP id cc1-v6mr18927691plb.165.1542001737628; Sun, 11 Nov 2018 21:48:57 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1542001737; cv=none; d=google.com; s=arc-20160816; b=0jJgF/TK63ax8S7WOlS1Azicvza9EoYsrpah/ZrZte9Gj9IJ0xNMHnJTNFu89AcBxY PeDiaIAOpgQC6/YcNnZ2JDEpwJd3oVOE2+MRA42PxHQ7KcMFKurYP99a3FIXmWWj0a5t 6P1BXVLEpNvckTnc0dkRDFCMPPPDqpZg+ryI6cafGZc5gz+WGs0QciOOg5pwZjO8ecyT l9LgbSIS8UJkwxPXybtIkH2buFtB/9LdRLfjfdptI/MQFw0XkbUjpRdKWYX2lcUZ7ahg +OPQXVJmy6wdxEVU2+D7b/J6Gvhk4AHN08ENqMe/gosHbCyEKYAEyP054eU1IKHxgaCx 16MA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding:mime-version :references:in-reply-to:date:cc:to:from:subject:message-id :dkim-signature; bh=0rI7aHn7uedZuF8H8kWsTC16RnFu7xe9LB9UVgPSR1k=; b=X47luqMATVb5DgLlDL5R9h0IElmcd1pd+Ve35WTKkYedP5GKjL7dtJbMyw+Vhg+oSI FKP2M18ldAau/dETcyYJd5t9+XtImL3dJKbB0XzSsc9unZWvWZm6SHY43u1zTuVPYLtp Gk59ASMYQdM3vRKVBuJuZwwskezTOcM7xZr8XdkT4h+8KE0uI5Yyr3SFCispjTONPWXW IEqr16/X+CkYcGyEbO5rWov4QWl+jX3UHaSBqmDDzEhJYoI3fXigAX4p4nVHxeL0Abrt +i2tSbStOOJQK/O33b3WfqimFS4Eu1Gn36c7M/6u/sO/Cj5ssKn3EKYbFUCTS4TN/UzJ oUKg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=lRPo9aZ8; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id c24-v6si2896870plz.116.2018.11.11.21.48.42; Sun, 11 Nov 2018 21:48:57 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=lRPo9aZ8; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731213AbeKLPj4 (ORCPT + 99 others); Mon, 12 Nov 2018 10:39:56 -0500 Received: from mail-pf1-f196.google.com ([209.85.210.196]:40227 "EHLO mail-pf1-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728269AbeKLPj4 (ORCPT ); Mon, 12 Nov 2018 10:39:56 -0500 Received: by mail-pf1-f196.google.com with SMTP id x2-v6so3747970pfm.7; Sun, 11 Nov 2018 21:48:16 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=message-id:subject:from:to:cc:date:in-reply-to:references :mime-version:content-transfer-encoding; bh=0rI7aHn7uedZuF8H8kWsTC16RnFu7xe9LB9UVgPSR1k=; b=lRPo9aZ8otfc5qTuW8+H5mke4Zi9KAEINGuHkp5GtZ6EY/dIZjkdeHhHbvzb9M3+k2 4RkoDvl/84lIhlAQT/Z/urzvcDaiJ4jJ1ioeeVuNiGgMredXi2xgBuB6gG/ma2zIAJD2 v++zIGWuP7oIJknFZrS8+9LbM3H/6Qf44nrPNtMWJK5PAOc7D0HL5zYyAwqmWrk0Lrjp eS66sWkXq95njjOOO6W3UlQmqjIdNfWjbGn7GBPDpgwmvW9cDRy4SGH6u5+Y1VAYO4NG azkpYy7xEuX3PoSCfJ5qBGq+GIoD9IyIXxu6k0/zYuTO1zxz2L/7iR3fPX0SPr7hiUb1 U4+w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to :references:mime-version:content-transfer-encoding; bh=0rI7aHn7uedZuF8H8kWsTC16RnFu7xe9LB9UVgPSR1k=; b=rdKIFYTj1soEFb22i/eBDyDYrRg7CG0mI5qeromkMxNkll2AZXkORnoSh+L8hUaTjf dzhd43xJPwH21jf6qbDYz9AWHzqFZ9PTrytue/PmdinOTZYlIn6S0Nbt+RniGNlEJRv5 yxuPNgVRs8yd/1x7+DbwGp6URhbIEwfUzAQnRqQlEHqbY350ZQN5cVSlLq2KC6tzxmB/ 4KZabltFEPhZyWcByICCVs2uypC4iYfHzfQty4i4T1IKXJJMhDb1xIv4+P8R+c39ugb0 3n79u+DYTOfg6LMHZMySuPjC5B9X5gWuz0GR9iU+a3U0t88z9HRZ2/DHJhMrYtq/Erx7 xByw== X-Gm-Message-State: AGRZ1gI7g7LKkn8eQYkRI7mGy4UMSQ1cPNbkbfb+XXTsaIkCSq3VBQS2 tbF8XOzL5afoJ4eUKspcjF4= X-Received: by 2002:a62:1e42:: with SMTP id e63-v6mr18775808pfe.149.1542001695705; Sun, 11 Nov 2018 21:48:15 -0800 (PST) Received: from wafer.ozlabs.ibm.com ([122.99.82.10]) by smtp.googlemail.com with ESMTPSA id g27-v6sm33542299pfj.162.2018.11.11.21.48.10 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Sun, 11 Nov 2018 21:48:14 -0800 (PST) Message-ID: <9da5bbe3a76c6adf09804d88d0a9edc3ddedff4d.camel@gmail.com> Subject: Re: [PATCH v2] PCI/MSI: Don't touch MSI bits when the PCI device is disconnected From: Oliver O'Halloran To: Lukas Wunner , Bjorn Helgaas Cc: Alexandru Gagniuc , linux-pci@vger.kernel.org, keith.busch@intel.com, alex_gagniuc@dellteam.com, austin_bolen@dell.com, shyam_iyer@dell.com, linux-kernel@vger.kernel.org, Jonathan Derrick , Greg Kroah-Hartman , Russell Currey , Sam Bobroff , linuxppc-dev@lists.ozlabs.org Date: Mon, 12 Nov 2018 16:48:08 +1100 In-Reply-To: <20181109071139.uxa6gu7jwsvr7ve6@wunner.de> References: <20180918221501.13112-1-mr.nuke.me@gmail.com> <20181107234257.GC41183@google.com> <20181108200855.GE41183@google.com> <20181109071139.uxa6gu7jwsvr7ve6@wunner.de> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.28.5 (3.28.5-1.fc28) Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 2018-11-09 at 08:11 +0100, Lukas Wunner wrote: > On Thu, Nov 08, 2018 at 02:09:17PM -0600, Bjorn Helgaas wrote: > > + /* > > + * If an MMIO read from the device returns ~0 data, that data may > > + * be valid, or it may indicate a bus error. If config space is > > + * readable, assume it's valid data; otherwise, assume a bus error. > > + */ > > + if (val == ~0) { > > + pci_read_config_dword(dev, PCI_VENDOR_ID, &id); > > + if (id == ~0) > > + pci_dev_set_disconnected(dev, NULL); > > + } > > This isn't safe unfortunately because "all ones" may occur for other > reasons besides disconnectedness. E.g. on an Uncorrectable Error, > the device may likewise respond with all ones, but revert to valid > responses if the error can be recovered through a Secondary Bus Reset. > In such a case, marking the device disconnected would be inappropriate. I don't really see why we're trying to make a distinction between recoverable errors and disconnected devices at this stage. In either case we should assume the device is broken and shouldn't be accessed until we perform some kind of recovery action. Bjorn's MMIO wrappers are more-or-less an opt-in software emulation of the freeze-MMIO-on-error behaviour that the EEH mechanism provides on IBM hardware so I think it makes sense. It also has the nice side effect of giving driver writers an error injection mechanism so they might actually test how their drivers deal with errors. > Accessing a device in D3cold would be another example where all ones > is returned both from mmio and config space despite the device still > being present and future accesses having a chance to succeed. Is doing a MMIO to a device in D3cold (or hot) ever a valid thing to do? > In fact, in v2 of Keith's patches adding pci_dev_set_disconnected() > he attempted the same as what you're doing here and caused issues > for me with devices in D3cold: > > https://spinics.net/lists/linux-pci/msg54337.html > > > > One thing I'm uncomfortable with is that [...]. Another is that the > > only place we call pci_dev_set_disconnected() is in pciehp and acpiphp, > > so the only "disconnected" case we catch is if hotplug happens to be > > involved. > > Yes, that's because the hotplug drivers are the only ones who can > identify removal authoritatively and unambiguously. They *know* > when the device is gone and don't have to resort to heuristics > such as all ones. (ISTR that dpc also marks devices disconnected.) The herustics shouldn't be used to work out when the device is gone, rather they should be used to work out when we need to check on the device. One of the grosser bits of EEH support is a hook in readl() and friends that checks for a 0xFF response. If it finds one, it looks at the EEH state and will start the recovery process if the device is marked as frozen. (don't look at the code. you won't like what you find) > > sprinkling pci_dev_is_disconnected() around feels ad hoc > > instead of systematic, in the sense that I don't know how we convince > > ourselves that this (and only this) is the correct place to put it. > > We need to add documentation for driver authors how to deal with > surprise removal. Briefly: > > * If (pdev->error_state == pci_channel_io_perm_failure), the device > is definitely gone and any further device access can be skipped. > Otherwise presence of the device is likely, but not guaranteed. > > * If a device access can significantly delay device removal due to > Completion Timeouts, or can cause an infinite loop, MCE or crash, > do check pdev->error_state before carrying out the device access. > > * Always be prepared that a device access may fail due to surprise > removal, do not blindly trust mmio or config space reads or > assume success of writes. Completely agree. We really need better documentation of what drivers should be doing. Oliver