Re: powerpc/pci: [PATCH 1/1 V3] PCIE PHB reset

["Oliver O'Halloran" <oohall@gmail.com>]

On Tue, Jun 16, 2020 at 9:55 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> wenxiong@linux.vnet.ibm.com writes:
> > From: Wen Xiong <wenxiong@linux.vnet.ibm.com>
> >
> > 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. Wedging the two together doesn't make any
real sense IMO since this should be usable even with !CONFIG_EEH.

> That would isolate the code in the right place, and allow you to use the
> existing ibm_get_config_addr_info.
>
> You probably can't use pseries_eeh_get_pe_addr(), because you won't have
> a "pe" structure yet.
>
> Instead you should add a helper that does the core of that logic but
> accepts config_addr/buid as parameters, and then have your code and
> pseries_eeh_get_pe_addr() call that.

I have a patch in my next pile of eeh reworks which kills off
pseries_eeh_get_pe_addr() entirely. It's used to implement
eeh_ops->get_pe_addr for pseries, but the only caller of
->get_pe_addr() is in pseries EEH code and the powernv version is a
stub so I was going to drop it from EEH ops and fold it into the
caller. We could do that in this patch, but it's just going to create
a merge conflict for you to deal with. Up to you.

> *snip*
> > +             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.

It could be better explained I suppose, but you need to read
PAPR/LoPAPR to make sense of anything RTAS so what's it really buying
you?

> Oh I see, it's copied from pseries_eeh_reset().
> > +                             ret = rtas_call(ibm_set_slot_reset, 4, 1, NULL,
> > +                                     config_addr, BUID_HI(phb->buid),
> > +                                     BUID_LO(phb->buid), EEH_RESET_HOT);
> > +
> > +                     if (ret) {
> > +                             pr_err("%s: PHB#%x-PE# failed with rtas_call activate reset=%d\n",
>                                                       ^
>                                                       again missing PE number.
>
> > +                                     __func__, phb->global_number, ret);
> > +                             continue;
> > +                     }
> > +             }
> > +             msleep(EEH_PE_RST_SETTLE_TIME);
>
> So that loop is basically a copy of pseries_eeh_reset() but with the
> sleep hoisted out of the loop.
>
> I'd really prefer to see that refactored into a helper that takes the
> config_addr and buid and doesn't do the sleep.
>
> Then this loop could call that helper, and so could pseries_eeh_reset().

That's better so long as we're not requiring CONFIG_EEH being
selected. pseries_eeh_reset() uses the rtas token variables
initialised in pseries_eeh_init() which are static to the file so
they'd need to be initialised somewhere common.