| Message ID | 20171025152754.25166-1-gpiccoli@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] powerpc/powernv: Add pci_reset_phbs parameter to issue a PHB reset | expand |
On 10/25/2017 01:27 PM, Guilherme G. Piccoli wrote: > During a kdump kernel boot in PowerPC, we request a reset of the PHBs > to the FW. It makes sense, since if we are booting a kdump kernel it > means we had some trouble before and we cannot rely in the adapters' > health; they could be in a bad state, hence the reset is needed. > > But this reset is useful not only in kdump - there are situations, > specially when debugging drivers, that we could break an adapter in > a way it requires such reset. One can tell to just go ahead and > reboot the machine, but happens that many times doing kexec is much > faster, and so preferable than a full power cycle. > > This patch adds the pci_reset_phbs parameter to perform such reset > when desired by the user. > > Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com> > --- > v2: changed name of the parameter [ben/mpe suggestion]. > > The patch was implemented against powerpc/next. Hi Michael/Ben...any thoughts about this one? Thanks in advance! > > arch/powerpc/platforms/powernv/pci-ioda.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c > index fb5cd7511189..6070e0d4a7f3 100644 > --- a/arch/powerpc/platforms/powernv/pci-ioda.c > +++ b/arch/powerpc/platforms/powernv/pci-ioda.c > @@ -89,6 +89,7 @@ void pe_level_printk(const struct pnv_ioda_pe *pe, const char *level, > } > > static bool pnv_iommu_bypass_disabled __read_mostly; > +static bool pci_reset_phbs __read_mostly; > > static int __init iommu_setup(char *str) > { > @@ -110,6 +111,14 @@ static int __init iommu_setup(char *str) > } > early_param("iommu", iommu_setup); > > +static int __init pci_reset_phbs_setup(char *str) > +{ > + pci_reset_phbs = true; > + return 0; > +} > + > +early_param("pci_reset_phbs", pci_reset_phbs_setup); > + > static inline bool pnv_pci_is_m64(struct pnv_phb *phb, struct resource *r) > { > /* > @@ -4014,9 +4023,10 @@ static void __init pnv_pci_init_ioda_phb(struct device_node *np, > * If we're running in kdump kernel, the previous kernel never > * shutdown PCI devices correctly. We already got IODA table > * cleaned out. So we have to issue PHB reset to stop all PCI > - * transactions from previous kernel. > + * transactions from previous kernel. The pci_reset_phbs > + * kernel parameter will force this reset too. > */ > - if (is_kdump_kernel()) { > + if (is_kdump_kernel() || pci_reset_phbs) { > pr_info(" Issue PHB reset ...\n"); > pnv_eeh_phb_reset(hose, EEH_RESET_FUNDAMENTAL); > pnv_eeh_phb_reset(hose, EEH_RESET_DEACTIVATE); >
"Guilherme G. Piccoli" <gpiccoli@linux.vnet.ibm.com> writes: > During a kdump kernel boot in PowerPC, we request a reset of the PHBs > to the FW. It makes sense, since if we are booting a kdump kernel it > means we had some trouble before and we cannot rely in the adapters' > health; they could be in a bad state, hence the reset is needed. > > But this reset is useful not only in kdump - there are situations, > specially when debugging drivers, that we could break an adapter in > a way it requires such reset. One can tell to just go ahead and > reboot the machine, but happens that many times doing kexec is much > faster, and so preferable than a full power cycle. > > This patch adds the pci_reset_phbs parameter to perform such reset > when desired by the user. > > Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com> > --- > v2: changed name of the parameter [ben/mpe suggestion]. > > The patch was implemented against powerpc/next. > > arch/powerpc/platforms/powernv/pci-ioda.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c > index fb5cd7511189..6070e0d4a7f3 100644 > --- a/arch/powerpc/platforms/powernv/pci-ioda.c > +++ b/arch/powerpc/platforms/powernv/pci-ioda.c > @@ -89,6 +89,7 @@ void pe_level_printk(const struct pnv_ioda_pe *pe, const char *level, > } > > static bool pnv_iommu_bypass_disabled __read_mostly; > +static bool pci_reset_phbs __read_mostly; > > static int __init iommu_setup(char *str) > { > @@ -110,6 +111,14 @@ static int __init iommu_setup(char *str) > } > early_param("iommu", iommu_setup); > > +static int __init pci_reset_phbs_setup(char *str) > +{ > + pci_reset_phbs = true; > + return 0; > +} > + > +early_param("pci_reset_phbs", pci_reset_phbs_setup); Is there a reason you didn't make it a module parameter? That is preferable IMHO because it is namespaced, which means we don't need to worry about polluting the top-level parameter namespace. cheers
On Thu, Oct 26, 2017 at 2:27 AM, Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com> wrote: > During a kdump kernel boot in PowerPC, we request a reset of the PHBs > to the FW. It makes sense, since if we are booting a kdump kernel it > means we had some trouble before and we cannot rely in the adapters' > health; they could be in a bad state, hence the reset is needed. > > But this reset is useful not only in kdump - there are situations, > specially when debugging drivers, that we could break an adapter in > a way it requires such reset. One can tell to just go ahead and > reboot the machine, but happens that many times doing kexec is much > faster, and so preferable than a full power cycle. > > This patch adds the pci_reset_phbs parameter to perform such reset > when desired by the user. > Do we care to reset specific phbs or all of them? I guess all based on your description. Balbir Singh.
On 11/16/2017 01:49 AM, Balbir Singh wrote: > On Thu, Oct 26, 2017 at 2:27 AM, Guilherme G. Piccoli > <gpiccoli@linux.vnet.ibm.com> wrote: >> During a kdump kernel boot in PowerPC, we request a reset of the PHBs >> to the FW. It makes sense, since if we are booting a kdump kernel it >> means we had some trouble before and we cannot rely in the adapters' >> health; they could be in a bad state, hence the reset is needed. >> >> But this reset is useful not only in kdump - there are situations, >> specially when debugging drivers, that we could break an adapter in >> a way it requires such reset. One can tell to just go ahead and >> reboot the machine, but happens that many times doing kexec is much >> faster, and so preferable than a full power cycle. >> >> This patch adds the pci_reset_phbs parameter to perform such reset >> when desired by the user. >> > > Do we care to reset specific phbs or all of them? I guess all based on > your description. Exactly Balbir, it does reset all of them. We could add such granularity, but I don't see much usability.. But if somebody feels it's useful, we can change... Thanks! > > Balbir Singh. >
On Thu, Nov 16, 2017 at 11:14 PM, Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com> wrote: > On 11/16/2017 01:49 AM, Balbir Singh wrote: >> On Thu, Oct 26, 2017 at 2:27 AM, Guilherme G. Piccoli >> <gpiccoli@linux.vnet.ibm.com> wrote: >>> During a kdump kernel boot in PowerPC, we request a reset of the PHBs >>> to the FW. It makes sense, since if we are booting a kdump kernel it >>> means we had some trouble before and we cannot rely in the adapters' >>> health; they could be in a bad state, hence the reset is needed. >>> >>> But this reset is useful not only in kdump - there are situations, >>> specially when debugging drivers, that we could break an adapter in >>> a way it requires such reset. One can tell to just go ahead and >>> reboot the machine, but happens that many times doing kexec is much >>> faster, and so preferable than a full power cycle. >>> >>> This patch adds the pci_reset_phbs parameter to perform such reset >>> when desired by the user. >>> >> >> Do we care to reset specific phbs or all of them? I guess all based on >> your description. > > Exactly Balbir, it does reset all of them. We could add such > granularity, but I don't see much usability.. > But if somebody feels it's useful, we can change... > OK.. makes sense, any reason why this can't be folded into reset_devices? I guess we want reset_phbs to be independent of reset_devices Balbir
On 11/21/2017 12:35 AM, Balbir Singh wrote: > On Thu, Nov 16, 2017 at 11:14 PM, Guilherme G. Piccoli > <gpiccoli@linux.vnet.ibm.com> wrote: >> On 11/16/2017 01:49 AM, Balbir Singh wrote: >>> On Thu, Oct 26, 2017 at 2:27 AM, Guilherme G. Piccoli >>> <gpiccoli@linux.vnet.ibm.com> wrote: >>>> During a kdump kernel boot in PowerPC, we request a reset of the PHBs >>>> to the FW. It makes sense, since if we are booting a kdump kernel it >>>> means we had some trouble before and we cannot rely in the adapters' >>>> health; they could be in a bad state, hence the reset is needed. >>>> >>>> But this reset is useful not only in kdump - there are situations, >>>> specially when debugging drivers, that we could break an adapter in >>>> a way it requires such reset. One can tell to just go ahead and >>>> reboot the machine, but happens that many times doing kexec is much >>>> faster, and so preferable than a full power cycle. >>>> >>>> This patch adds the pci_reset_phbs parameter to perform such reset >>>> when desired by the user. >>>> >>> >>> Do we care to reset specific phbs or all of them? I guess all based on >>> your description. >> >> Exactly Balbir, it does reset all of them. We could add such >> granularity, but I don't see much usability.. >> But if somebody feels it's useful, we can change... >> > > OK.. makes sense, any reason why this can't be folded into reset_devices? > I guess we want reset_phbs to be independent of reset_devices It was, in v1. But mpe asked it to be a powerpc specific parameter heheh Cheers, Guilherme > > Balbir >
"Guilherme G. Piccoli" <gpiccoli@linux.vnet.ibm.com> writes: > On 11/21/2017 12:35 AM, Balbir Singh wrote: >> On Thu, Nov 16, 2017 at 11:14 PM, Guilherme G. Piccoli >> <gpiccoli@linux.vnet.ibm.com> wrote: >>> On 11/16/2017 01:49 AM, Balbir Singh wrote: >>>> On Thu, Oct 26, 2017 at 2:27 AM, Guilherme G. Piccoli >>>> <gpiccoli@linux.vnet.ibm.com> wrote: >>>>> During a kdump kernel boot in PowerPC, we request a reset of the PHBs >>>>> to the FW. It makes sense, since if we are booting a kdump kernel it >>>>> means we had some trouble before and we cannot rely in the adapters' >>>>> health; they could be in a bad state, hence the reset is needed. >>>>> >>>>> But this reset is useful not only in kdump - there are situations, >>>>> specially when debugging drivers, that we could break an adapter in >>>>> a way it requires such reset. One can tell to just go ahead and >>>>> reboot the machine, but happens that many times doing kexec is much >>>>> faster, and so preferable than a full power cycle. >>>>> >>>>> This patch adds the pci_reset_phbs parameter to perform such reset >>>>> when desired by the user. >>>>> >>>> >>>> Do we care to reset specific phbs or all of them? I guess all based on >>>> your description. >>> >>> Exactly Balbir, it does reset all of them. We could add such >>> granularity, but I don't see much usability.. >>> But if somebody feels it's useful, we can change... >>> >> >> OK.. makes sense, any reason why this can't be folded into reset_devices? >> I guess we want reset_phbs to be independent of reset_devices > > It was, in v1. But mpe asked it to be a powerpc specific parameter heheh LOL. Turtles. I still feel like we shouldn't be creating a generically named parameter like this, though we have done it many times in the past. Can we call it "ppc_reset_phbs". And then I'll merge it without further quibbling, honest. cheers
On 12/08/2017 10:03 AM, Michael Ellerman wrote: > "Guilherme G. Piccoli" <gpiccoli@linux.vnet.ibm.com> writes: > >> On 11/21/2017 12:35 AM, Balbir Singh wrote: >>> On Thu, Nov 16, 2017 at 11:14 PM, Guilherme G. Piccoli >>> <gpiccoli@linux.vnet.ibm.com> wrote: >>>> On 11/16/2017 01:49 AM, Balbir Singh wrote: >>>>> On Thu, Oct 26, 2017 at 2:27 AM, Guilherme G. Piccoli >>>>> <gpiccoli@linux.vnet.ibm.com> wrote: >>>>>> During a kdump kernel boot in PowerPC, we request a reset of the PHBs >>>>>> to the FW. It makes sense, since if we are booting a kdump kernel it >>>>>> means we had some trouble before and we cannot rely in the adapters' >>>>>> health; they could be in a bad state, hence the reset is needed. >>>>>> >>>>>> But this reset is useful not only in kdump - there are situations, >>>>>> specially when debugging drivers, that we could break an adapter in >>>>>> a way it requires such reset. One can tell to just go ahead and >>>>>> reboot the machine, but happens that many times doing kexec is much >>>>>> faster, and so preferable than a full power cycle. >>>>>> >>>>>> This patch adds the pci_reset_phbs parameter to perform such reset >>>>>> when desired by the user. >>>>>> >>>>> >>>>> Do we care to reset specific phbs or all of them? I guess all based on >>>>> your description. >>>> >>>> Exactly Balbir, it does reset all of them. We could add such >>>> granularity, but I don't see much usability.. >>>> But if somebody feels it's useful, we can change... >>>> >>> >>> OK.. makes sense, any reason why this can't be folded into reset_devices? >>> I guess we want reset_phbs to be independent of reset_devices >> >> It was, in v1. But mpe asked it to be a powerpc specific parameter heheh > > LOL. Turtles. > > I still feel like we shouldn't be creating a generically named parameter > like this, though we have done it many times in the past. > > Can we call it "ppc_reset_phbs". And then I'll merge it without further > quibbling, honest. hahah sure! How about this one: https://patchwork.ozlabs.org/patch/839123 Let me know your thoughts! Cheers, Guilherme > > cheers >
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c index fb5cd7511189..6070e0d4a7f3 100644 --- a/arch/powerpc/platforms/powernv/pci-ioda.c +++ b/arch/powerpc/platforms/powernv/pci-ioda.c @@ -89,6 +89,7 @@ void pe_level_printk(const struct pnv_ioda_pe *pe, const char *level, } static bool pnv_iommu_bypass_disabled __read_mostly; +static bool pci_reset_phbs __read_mostly; static int __init iommu_setup(char *str) { @@ -110,6 +111,14 @@ static int __init iommu_setup(char *str) } early_param("iommu", iommu_setup); +static int __init pci_reset_phbs_setup(char *str) +{ + pci_reset_phbs = true; + return 0; +} + +early_param("pci_reset_phbs", pci_reset_phbs_setup); + static inline bool pnv_pci_is_m64(struct pnv_phb *phb, struct resource *r) { /* @@ -4014,9 +4023,10 @@ static void __init pnv_pci_init_ioda_phb(struct device_node *np, * If we're running in kdump kernel, the previous kernel never * shutdown PCI devices correctly. We already got IODA table * cleaned out. So we have to issue PHB reset to stop all PCI - * transactions from previous kernel. + * transactions from previous kernel. The pci_reset_phbs + * kernel parameter will force this reset too. */ - if (is_kdump_kernel()) { + if (is_kdump_kernel() || pci_reset_phbs) { pr_info(" Issue PHB reset ...\n"); pnv_eeh_phb_reset(hose, EEH_RESET_FUNDAMENTAL); pnv_eeh_phb_reset(hose, EEH_RESET_DEACTIVATE);
During a kdump kernel boot in PowerPC, we request a reset of the PHBs to the FW. It makes sense, since if we are booting a kdump kernel it means we had some trouble before and we cannot rely in the adapters' health; they could be in a bad state, hence the reset is needed. But this reset is useful not only in kdump - there are situations, specially when debugging drivers, that we could break an adapter in a way it requires such reset. One can tell to just go ahead and reboot the machine, but happens that many times doing kexec is much faster, and so preferable than a full power cycle. This patch adds the pci_reset_phbs parameter to perform such reset when desired by the user. Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com> --- v2: changed name of the parameter [ben/mpe suggestion]. The patch was implemented against powerpc/next. arch/powerpc/platforms/powernv/pci-ioda.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)