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

[Michael Ellerman]

"Oliver O'Halloran" <oohall at gmail.com> writes:
> On Tue, Jun 16, 2020 at 9:55 PM Michael Ellerman <mpe at ellerman.id.au> wrote:
>> wenxiong at linux.vnet.ibm.com writes:
>> > From: Wen Xiong <wenxiong at 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.

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.

And if you could this patch wouldn't compile because it uses EEH
constants that are behind #ifdef CONFIG_EEH.

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.

So IMO this is EEH code, and should be with the other EEH code and
should be behind 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.

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.

As long as there's only a single usage then I don't mind it.

> 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?

Making the code easier for new folks to understand?

cheers