From: Nick Desaulniers <ndesaulniers@google.com> To: "Oliver O'Halloran" <oohall@gmail.com>, Daniel Axtens <dja@axtens.net>, Michael Ellerman <mpe@ellerman.id.au> Cc: Nathan Chancellor <nathan@kernel.org>, Joe Perches <joe@perches.com>, Benjamin Herrenschmidt <benh@kernel.crashing.org>, Paul Mackerras <paulus@samba.org>, Alexey Kardashevskiy <aik@ozlabs.ru>, linuxppc-dev <linuxppc-dev@lists.ozlabs.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org> Subject: Re: [PATCH] powerpc/powernv/pci: remove dead code from !CONFIG_EEH Date: Mon, 17 May 2021 17:16:55 -0700 [thread overview] Message-ID: <CAKwvOdmMugQkTRwC3HOEt2-em2zSfAoi7gpvJRkqfdzSDRMeEg@mail.gmail.com> (raw) In-Reply-To: <CAOSf1CGoN5R0LUrU=Y=UWho1Z_9SLgCX8s3SbFJXwJXc5BYz4A@mail.gmail.com> On Thu, Apr 22, 2021 at 6:13 PM Oliver O'Halloran <oohall@gmail.com> wrote: > > On Fri, Apr 23, 2021 at 9:09 AM Daniel Axtens <dja@axtens.net> wrote: > > > > Hi Nick, > > > > > While looking at -Wundef warnings, the #if CONFIG_EEH stood out as a > > > possible candidate to convert to #ifdef CONFIG_EEH, but it seems that > > > based on Kconfig dependencies it's not possible to build this file > > > without CONFIG_EEH enabled. > > > > This seemed odd to me, but I think you're right: > > > > arch/powerpc/platforms/Kconfig contains: > > > > config EEH > > bool > > depends on (PPC_POWERNV || PPC_PSERIES) && PCI > > default y > > > > It's not configurable from e.g. make menuconfig because there's no prompt. > > You can attempt to explicitly disable it with e.g. `scripts/config -d EEH` > > but then something like `make oldconfig` will silently re-enable it for > > you. > > > > It's been forced on since commit e49f7a9997c6 ("powerpc/pseries: Rivet > > CONFIG_EEH for pSeries platform") in 2012 which fixed it for > > pseries. That moved out from pseries to pseries + powernv later on. > > > > There are other cleanups in the same vein that could be made, from the > > Makefile (which has files only built with CONFIG_EEH) through to other > > source files. It looks like there's one `#ifdef CONFIG_EEH` in > > arch/powerpc/platforms/powernv/pci-ioda.c that could be pulled out, for > > example. > > > > I think it's probably worth trying to rip out all of those in one patch? > > The change in commit e49f7a9997c6 ("powerpc/pseries: Rivet CONFIG_EEH > for pSeries platform") never should have been made. I'll change my patch to keep the conditionals, but use #ifdef instead of #if then? > > There's no inherent reason why EEH needs to be enabled and forcing it > on is (IMO) a large part of why EEH support is the byzantine > clusterfuck that it is. One of the things I was working towards was > allowing pseries and powernv to be built with !CONFIG_EEH since that > would help define a clearer boundary between what is "eeh support" and > what is required to support PCI on the platform. Pseries is > particularly bad for this since PAPR says the RTAS calls needed to do > a PCI bus reset are part of the EEH extension, but there's non-EEH > reasons why you might want to use those RTAS calls. The PHB reset that > we do when entering a kdump kernel is a good example since that uses > the same RTAS calls, but it has nothing to do with the EEH recovery > machinery enabled by CONFIG_EEH. > > I was looking into that largely because people were considering using > OPAL for microwatt platforms. Breaking the assumption that > powernv==EEH support is one of the few bits of work required to enable > that, but even if you don't go down that road I think everyone would > be better off if you kept a degree of separation between the two. -- Thanks, ~Nick Desaulniers
WARNING: multiple messages have this Message-ID (diff)
From: Nick Desaulniers <ndesaulniers@google.com> To: "Oliver O'Halloran" <oohall@gmail.com>, Daniel Axtens <dja@axtens.net>, Michael Ellerman <mpe@ellerman.id.au> Cc: Alexey Kardashevskiy <aik@ozlabs.ru>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Nathan Chancellor <nathan@kernel.org>, Paul Mackerras <paulus@samba.org>, Joe Perches <joe@perches.com>, linuxppc-dev <linuxppc-dev@lists.ozlabs.org> Subject: Re: [PATCH] powerpc/powernv/pci: remove dead code from !CONFIG_EEH Date: Mon, 17 May 2021 17:16:55 -0700 [thread overview] Message-ID: <CAKwvOdmMugQkTRwC3HOEt2-em2zSfAoi7gpvJRkqfdzSDRMeEg@mail.gmail.com> (raw) In-Reply-To: <CAOSf1CGoN5R0LUrU=Y=UWho1Z_9SLgCX8s3SbFJXwJXc5BYz4A@mail.gmail.com> On Thu, Apr 22, 2021 at 6:13 PM Oliver O'Halloran <oohall@gmail.com> wrote: > > On Fri, Apr 23, 2021 at 9:09 AM Daniel Axtens <dja@axtens.net> wrote: > > > > Hi Nick, > > > > > While looking at -Wundef warnings, the #if CONFIG_EEH stood out as a > > > possible candidate to convert to #ifdef CONFIG_EEH, but it seems that > > > based on Kconfig dependencies it's not possible to build this file > > > without CONFIG_EEH enabled. > > > > This seemed odd to me, but I think you're right: > > > > arch/powerpc/platforms/Kconfig contains: > > > > config EEH > > bool > > depends on (PPC_POWERNV || PPC_PSERIES) && PCI > > default y > > > > It's not configurable from e.g. make menuconfig because there's no prompt. > > You can attempt to explicitly disable it with e.g. `scripts/config -d EEH` > > but then something like `make oldconfig` will silently re-enable it for > > you. > > > > It's been forced on since commit e49f7a9997c6 ("powerpc/pseries: Rivet > > CONFIG_EEH for pSeries platform") in 2012 which fixed it for > > pseries. That moved out from pseries to pseries + powernv later on. > > > > There are other cleanups in the same vein that could be made, from the > > Makefile (which has files only built with CONFIG_EEH) through to other > > source files. It looks like there's one `#ifdef CONFIG_EEH` in > > arch/powerpc/platforms/powernv/pci-ioda.c that could be pulled out, for > > example. > > > > I think it's probably worth trying to rip out all of those in one patch? > > The change in commit e49f7a9997c6 ("powerpc/pseries: Rivet CONFIG_EEH > for pSeries platform") never should have been made. I'll change my patch to keep the conditionals, but use #ifdef instead of #if then? > > There's no inherent reason why EEH needs to be enabled and forcing it > on is (IMO) a large part of why EEH support is the byzantine > clusterfuck that it is. One of the things I was working towards was > allowing pseries and powernv to be built with !CONFIG_EEH since that > would help define a clearer boundary between what is "eeh support" and > what is required to support PCI on the platform. Pseries is > particularly bad for this since PAPR says the RTAS calls needed to do > a PCI bus reset are part of the EEH extension, but there's non-EEH > reasons why you might want to use those RTAS calls. The PHB reset that > we do when entering a kdump kernel is a good example since that uses > the same RTAS calls, but it has nothing to do with the EEH recovery > machinery enabled by CONFIG_EEH. > > I was looking into that largely because people were considering using > OPAL for microwatt platforms. Breaking the assumption that > powernv==EEH support is one of the few bits of work required to enable > that, but even if you don't go down that road I think everyone would > be better off if you kept a degree of separation between the two. -- Thanks, ~Nick Desaulniers
next prev parent reply other threads:[~2021-05-18 0:17 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-04-22 19:54 [PATCH] powerpc/powernv/pci: remove dead code from !CONFIG_EEH Nick Desaulniers 2021-04-22 19:54 ` Nick Desaulniers 2021-04-22 23:09 ` Daniel Axtens 2021-04-22 23:09 ` Daniel Axtens 2021-04-23 1:13 ` Oliver O'Halloran 2021-04-23 1:13 ` Oliver O'Halloran 2021-05-18 0:16 ` Nick Desaulniers [this message] 2021-05-18 0:16 ` Nick Desaulniers 2021-05-18 6:13 ` Michael Ellerman 2021-05-18 6:13 ` Michael Ellerman 2021-05-18 20:40 ` [PATCH v2] powerpc/powernv/pci: fix header guard Nick Desaulniers 2021-05-18 20:40 ` Nick Desaulniers 2021-05-18 20:41 ` Nathan Chancellor 2021-05-18 20:41 ` Nathan Chancellor 2021-06-06 12:08 ` Michael Ellerman 2021-06-06 12:08 ` Michael Ellerman
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAKwvOdmMugQkTRwC3HOEt2-em2zSfAoi7gpvJRkqfdzSDRMeEg@mail.gmail.com \ --to=ndesaulniers@google.com \ --cc=aik@ozlabs.ru \ --cc=benh@kernel.crashing.org \ --cc=dja@axtens.net \ --cc=joe@perches.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=mpe@ellerman.id.au \ --cc=nathan@kernel.org \ --cc=oohall@gmail.com \ --cc=paulus@samba.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.