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=-0.3 required=3.0 tests=DKIM_ADSP_CUSTOM_MED, DKIM_INVALID,DKIM_SIGNED,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS 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 54732C433DF for ; Fri, 19 Jun 2020 06:11:21 +0000 (UTC) Received: from lists.ozlabs.org (unknown [203.11.71.2]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id A536820786 for ; Fri, 19 Jun 2020 06:11:20 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="NgqVy5db" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org A536820786 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Received: from bilbo.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) by lists.ozlabs.org (Postfix) with ESMTP id 49p7jL2WRczDrP7 for ; Fri, 19 Jun 2020 16:11:18 +1000 (AEST) Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gmail.com (client-ip=2607:f8b0:4864:20::d42; helo=mail-io1-xd42.google.com; envelope-from=oohall@gmail.com; receiver=) Authentication-Results: lists.ozlabs.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: lists.ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.a=rsa-sha256 header.s=20161025 header.b=NgqVy5db; dkim-atps=neutral Received: from mail-io1-xd42.google.com (mail-io1-xd42.google.com [IPv6:2607:f8b0:4864:20::d42]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 49p7gG455WzDrND for ; Fri, 19 Jun 2020 16:09:30 +1000 (AEST) Received: by mail-io1-xd42.google.com with SMTP id f23so4643020iof.6 for ; Thu, 18 Jun 2020 23:09:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=QYxn2HuTRA3kCqcFXfYQRyqqNTAy3PwXnnRQDGWHA4U=; b=NgqVy5db7TBXpcCoWsiNcm81+r8wB7QoDkLtYA8FTiExi4WXjDYhZZiktk6PQx+XXi sNxsk+YR6+9p7fACijf3HzJYlxbgVbZzTjdaQrrhetJ/R4Msgq6290Oig1cg+jTZIq2f yl3EQph2BWnzfnIPbz9YFUN+YlB64zBSDACRy9ju9RnuM0+IE9NHqsVNFE0K645veJ5R gJajFl76B1M0FeuAE1cy/TjkYbJ0XHTuoi8R3H1e9ARnD+B5r4wLjsgRkphK1qVwu1E8 uP2NRtbMUBkLewVc4iw5Hm4i14DQpXnVaB3wz9nF5jl8RwMI5eB/VBwciS2v6XBnbmiK jTPA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=QYxn2HuTRA3kCqcFXfYQRyqqNTAy3PwXnnRQDGWHA4U=; b=LFYxCEq2oHI5HqUandWcxJzO976l8ssKbKNjf/KVpZ+txdVZK93g5JIsRa1b0DYQXS tENCr8TWnBFYl0wSgmAlm1upu0zDHQambUEH7o18MsU7Pt1IknoRfEgeLJC0rSGJL6MN dT+vhBiLocJawT4WggTA/WCbM7A/Z06r/h7VBwoen5BA9m/2/I4WY/JWkaaFeCtS2O/w Ra3Tq9sMjNef8T7D9F9Ytj6c9+YbBtBNRvppIQVaCFXCUUfF4pqBti0vjoguOqYvG2TF QOSFDc0LtPkcR3h3md5oo04Roawt5kJJ8qrPFJ2Z+Y2zQAiJkTpSsqnHG/rHxG8i7Gs7 fhmg== X-Gm-Message-State: AOAM533s+YAJw2tAhEUDxqOo5HFXxjQDpqqTN6iYnNPSNY9QCYf0xaum sRvT1pMDX7Yq/i6h/MzbYNZX1PyTU/MS+Y2urTk= X-Google-Smtp-Source: ABdhPJxY7eSgKH6dKZYnyajlEKqNF9Z9p1uErbaLgioSZZ78WjmRrjQY8zG6KEncBf0O3XFyxmsKLcD7ZiP7npCW3ok= X-Received: by 2002:a6b:780d:: with SMTP id j13mr2723403iom.66.1592546966717; Thu, 18 Jun 2020 23:09:26 -0700 (PDT) MIME-Version: 1.0 References: <1590499319-6472-1-git-send-email-wenxiong@linux.vnet.ibm.com> <87r1ufdy1x.fsf@mpe.ellerman.id.au> <87ftaudx1x.fsf@mpe.ellerman.id.au> In-Reply-To: <87ftaudx1x.fsf@mpe.ellerman.id.au> From: "Oliver O'Halloran" Date: Fri, 19 Jun 2020 16:09:15 +1000 Message-ID: Subject: Re: powerpc/pci: [PATCH 1/1 V3] PCIE PHB reset To: Michael Ellerman Content-Type: text/plain; charset="UTF-8" X-BeenThere: linuxppc-dev@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Brian King , Wen Xiong , linuxppc-dev , wenxiong@us.ibm.com Errors-To: linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Sender: "Linuxppc-dev" On Wed, Jun 17, 2020 at 4:29 PM Michael Ellerman wrote: > > "Oliver O'Halloran" writes: > > On Tue, Jun 16, 2020 at 9:55 PM Michael Ellerman wrote: > >> wenxiong@linux.vnet.ibm.com writes: > >> > From: Wen Xiong > >> > > >> > Several device drivers hit EEH(Extended Error handling) when triggering > >> > kdump on Pseries PowerVM. This patch implemented a reset of the PHBs > >> > in pci general code when triggering kdump. > >> > >> Actually it's in pseries specific PCI code, and the reset is done in the > >> 2nd kernel as it boots, not when triggering the kdump. > >> > >> You're doing it as a: > >> > >> machine_postcore_initcall(pseries, pseries_phb_reset); > >> > >> But we do the EEH initialisation in: > >> > >> core_initcall_sync(eeh_init); > >> > >> Which happens first. > >> > >> So it seems to me that this should be called from pseries_eeh_init(). > > > > This happens to use some of the same RTAS calls as EEH, but it's > > entirely orthogonal to it. > > I don't agree. I mean it's literally calling EEH_RESET_FUNDAMENTAL etc. > Those RTAS calls are all documented in the EEH section of PAPR. > > I guess you're saying it's orthogonal to the kernel handling an EEH and > doing the recovery process etc, which I can kind of see. > > > Wedging the two together doesn't make any real sense IMO since this > > should be usable even with !CONFIG_EEH. > > You can't turn CONFIG_EEH off for pseries or powernv. Not yet :) > And if you could this patch wouldn't compile because it uses EEH > constants that are behind #ifdef CONFIG_EEH. That's fixable. > If you could turn CONFIG_EEH off it would presumably be because you were > on a platform that didn't support EEH, in which case you wouldn't need > this code. I think there's an argument to be made for disabling EEH in some situations. A lot of drivers do a pretty poor job of recovering in the first place so it's conceivable that someone might want to disable it in say, a kdump kernel. That said, the real reason is mostly for the sake of code organisation. EEH is an optional platform feature but you wouldn't know it looking at the implementation and I'd like to stop it bleeding into odd places. Making it buildable without !CONFIG_EEH would probably help. > So IMO this is EEH code, and should be with the other EEH code and > should be behind CONFIG_EEH. *shrug* I wanted it to follow the model of the powernv implementation of the same feature which is done immediately after initialising the pci_controller and independent of all of the EEH setup. Although, looking at it again I see it calls pnv_eeh_phb_reset() which is in eeh_powernv.c so I guess that's pretty similar to what you're suggesting. > That sounds like a good cleanup. I'm not concerned about conflicts > within arch/powerpc, I can fix them up. > > >> > + list_for_each_entry(phb, &hose_list, list_node) { > >> > + config_addr = pseries_get_pdn_addr(phb); > >> > + if (config_addr == -1) > >> > + continue; > >> > + > >> > + ret = rtas_call(ibm_set_slot_reset, 4, 1, NULL, > >> > + config_addr, BUID_HI(phb->buid), > >> > + BUID_LO(phb->buid), EEH_RESET_FUNDAMENTAL); > >> > + > >> > + /* If fundamental-reset not supported, try hot-reset */ > >> > + if (ret == -8) > >> > >> Where does -8 come from? > > > > There's a comment right there. > > Yeah I guess. I was expecting it would map to some RTAS_ERROR_FOO value, > but it's just literally -8 in PAPR. Yeah, as far as I can tell the meaning of the return codes are specific to each RTAS call, it's a bit bad.